DEV: Defer button actions with layout change to the next frame paint (#27967)

User actions can trigger functions that render changes to the screen within the same cycle (e.g. pressing the reply button will cause the login modal to pop up), potentially impacting performance and causing some jank on slower devices.

This change inserts runAfterFramePaint where certain actions are triggered. Below are some screenshots indicating an improved INP for some of the buttons affected on controls with the highest INPs. The two places where this is added help with several actions, e.g. user + group cards, generic button action usage.
This commit is contained in:
Natalie Tay 2024-07-22 10:59:39 +08:00 committed by GitHub
parent f41716d532
commit 352d6f9dfb
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
4 changed files with 31 additions and 17 deletions

View File

@ -6,6 +6,7 @@ import { htmlSafe } from "@ember/template";
import { or } from "truth-helpers";
import GlimmerComponentWithDeprecatedParentView from "discourse/components/glimmer-component-with-deprecated-parent-view";
import concatClass from "discourse/helpers/concat-class";
import runAfterFramePaint from "discourse/lib/after-frame-paint";
import icon from "discourse-common/helpers/d-icon";
import deprecated from "discourse-common/lib/deprecated";
import I18n from "discourse-i18n";
@ -118,17 +119,17 @@ export default class DButton extends GlimmerComponentWithDeprecatedParentView {
);
}
} else if (typeof actionVal === "object" && actionVal.value) {
if (forwardEvent) {
actionVal.value(actionParam, event);
} else {
actionVal.value(actionParam);
}
runAfterFramePaint(() =>
forwardEvent
? actionVal.value(actionParam, event)
: actionVal.value(actionParam)
);
} else if (typeof actionVal === "function") {
if (forwardEvent) {
actionVal(actionParam, event);
} else {
actionVal(actionParam);
}
runAfterFramePaint(() =>
forwardEvent
? actionVal(actionParam, event)
: actionVal(actionParam)
);
}
} else if (route) {
this.router.transitionTo(route);

View File

@ -1,4 +1,4 @@
import DEBUG from "@glimmer/env";
import { DEBUG } from "@glimmer/env";
import { registerWaiter } from "@ember/test";
import { isTesting } from "discourse-common/config/environment";

View File

@ -2,6 +2,7 @@ import { alias, match } from "@ember/object/computed";
import Mixin from "@ember/object/mixin";
import { schedule, throttle } from "@ember/runloop";
import { service } from "@ember/service";
import runAfterFramePaint from "discourse/lib/after-frame-paint";
import { wantsNewWindow } from "discourse/lib/intercept-click";
import { headerOffset } from "discourse/lib/offset-calculator";
import DiscourseURL from "discourse/lib/url";
@ -86,9 +87,11 @@ export default Mixin.create({
document.querySelector(".card-cloak")?.classList.remove("hidden");
this.appEvents.trigger("user-card:show", { username });
this._positionCard(target, event);
this._showCallback(username).then((user) => {
this.appEvents.trigger("user-card:after-show", { user });
runAfterFramePaint(() => {
this._positionCard(target, event);
this._showCallback(username).then((user) => {
this.appEvents.trigger("user-card:after-show", { user });
});
});
// We bind scrolling on mobile after cards are shown to hide them if user scrolls

View File

@ -34,6 +34,10 @@ describe "Homepage", type: :system do
# Wait for the save to complete
find(".btn-primary.save-changes:not([disabled])", wait: 5)
try_until_success do
visit "/u/#{user.username}/preferences/interface"
homepage_picker.has_selected_name?("Top")
end
visit "/"
@ -89,9 +93,12 @@ describe "Homepage", type: :system do
homepage_picker.select_row_by_name("Top")
page.find(".btn-primary.save-changes").click
# Wait for the save to complete
# Make sure save is complete
find(".btn-primary.save-changes:not([disabled])", wait: 5)
expect(user.user_option.homepage_id).to eq(UserOption::HOMEPAGES.key("top"))
try_until_success do
visit "/u/#{user.username}/preferences/interface"
homepage_picker.has_selected_name?("Top")
end
find("#site-logo").click
expect(page).to have_css(".navigation-container .top.active", text: "Top")
@ -107,7 +114,10 @@ describe "Homepage", type: :system do
# Wait for the save to complete
find(".btn-primary.save-changes:not([disabled])", wait: 5)
expect(user.reload.user_option.homepage_id).to_not eq(UserOption::HOMEPAGES.key("top"))
try_until_success do
visit "/u/#{user.username}/preferences/interface"
homepage_picker.has_selected_name?("(default)")
end
find("#site-logo").click