Closed Bug 1389550 Opened 7 years ago Closed 7 years ago

Sync reflow at init_dynamic_padding in preferences.js

Categories

(Firefox :: Settings UI, enhancement, P3)

Product:
Firefox
Component:
Settings UI
Type:
enhancement

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Firefox 58
Tracking Flags:
Tracking Status
firefox57 --- wontfix
firefox58 --- fixed

People

(Reporter: florian, Assigned: rickychien)

References

Details

(Whiteboard: [photon-preference])

Attachments

(1 file, 1 obsolete file)

Bug 1389550 - Move help button into category and remove init_dynamic_padding in preferences.js
59 bytes, text/x-review-board-request
jaws
: review+
Details 
Here is a profile of opening the preferences on a slow netbook: https://perfht.ml/2uNCVUH

The biggest causes of slowness are:
- the familiar _rebuildFonts method (bug 1375978),

- the sync style flush in gotoPref
This used to be a whole layout flush before bug 1364360... but maybe we could skip completely the "mainContent.scrollTop = 0" line when it's the first time we open the preferences rather than a navigation between categories?
http://searchfox.org/mozilla-central/rev/4b79f3b23aebb4080ea85e94351fd4046116a957/browser/components/preferences/in-content-new/preferences.js#213

- a sync layout flush in init_dynamic_padding: http://searchfox.org/mozilla-central/rev/4b79f3b23aebb4080ea85e94351fd4046116a957/browser/components/preferences/in-content-new/preferences.js#99
Is the whole function still valid with our new visual design? See bug 1126278.

http://searchfox.org/mozilla-central/rev/4b79f3b23aebb4080ea85e94351fd4046116a957/browser/components/preferences/in-content-new/preferences.js#96-127
Flags: needinfo?(evan)
Whiteboard: [photon-preference][triage]
We still need the `init_dynamic_padding` function to layout the category list and the "Firefox Support" link on low window height situation.
Flags: needinfo?(evan)
Priority: -- → P3
Here is my suggestion solution for getting rid of init_dynamic_padding call.

1. Change "Firefox Support" link (html:a) to a XUL label.
2. Wrap "Firefox Support" link (label) into a new vbox element with align="end" within #category richlistbox.
3. Remove position: fixed style for .help-button in preferences.inc.css.
4. We can also use list-style-image instead of background-image in .help-button for fixing high contrast mode issue.
5. Remove init_dynamic_padding call to avoid sync reflow.
Flags: qe-verify-
Whiteboard: [photon-preference][triage] → [photon-preference]
The patch I attached offered an another, quicker solution than comment 3. It's definitely not "better" but we can make a decision on whether or not we should land this and get this off our back for now.
requestAnimationFrame is the right place to do DOM modifications, but it doesn't magically make sync reflows disappear. Using getComputedStyle inside a requestAnimationFrame callback will cause a sync reflow.
(In reply to Florian Quèze [:florian] [:flo] from comment #6)
> requestAnimationFrame is the right place to do DOM modifications, but it
> doesn't magically make sync reflows disappear. Using getComputedStyle inside
> a requestAnimationFrame callback will cause a sync reflow.

Point taken. My intention was to move the sync reflow away from DOMContentLoaded, as the commit message suggests. comment 3 is indeed worth trying to implement a sync reflow free version of bug 1126278, but I am not entirely sure how we could reimplement bug 1018066 without sync reflow...
(In reply to Tim Guan-tin Chien [:timdream] (please needinfo) from comment #7)
> (In reply to Florian Quèze [:florian] [:flo] from comment #6)
> > requestAnimationFrame is the right place to do DOM modifications, but it
> > doesn't magically make sync reflows disappear. Using getComputedStyle inside
> > a requestAnimationFrame callback will cause a sync reflow.
> 
> Point taken. My intention was to move the sync reflow away from
> DOMContentLoaded, as the commit message suggests.

I'm not sure if you are hoping the requestAnimationFrame callback will be called before the preferences are painted on screen (in this case, I don't see how moving the sync reflow away from DOMContentLoaded changes anything from a user point of view) or after (in which case there'll be one frame painted before the update, which will cause visible flickering). Either way, I'm afraid this isn't an improvement.
Comment on attachment 8898104 [details]
Bug 1389550 - Remove sync reflow during about:preferences loading and schedule them with animationend event,

Tim asked me in #fx-team to cancel the review so he can verify the patch first.
Attachment #8898104 - Flags: review?(jaws)
(In reply to Florian Quèze [:florian] [:flo] from comment #8)
> I'm not sure if you are hoping the requestAnimationFrame callback will be
> called before the preferences are painted on screen (in this case, I don't
> see how moving the sync reflow away from DOMContentLoaded changes anything
> from a user point of view) or after (in which case there'll be one frame
> painted before the update, which will cause visible flickering). Either way,
> I'm afraid this isn't an improvement.

I am looking at my own profile to figure out when rAF callback is called. Acccording to what Florian said here:

http://logs.glob.uno/?c=mozilla%23fx-team&s=17+Aug+2017&e=17+Aug+2017&h=timdream#c301226

It looks like rAF callback gets run before first paint, so we are not getting time-to-first-paint any faster here.

Also, while my patch removed sync reflow from init_all(), it triggered a sync reflow indirectly at 

http://searchfox.org/mozilla-central/rev/e8c36327cd8c9432c69e5e1383156a74330f11f2/browser/components/preferences/in-content-new/findInPage.js#24

by calling FocusManager.

This approach indeed needs more work.
I've asked around in my timezone and realized there isn't a method available for me to hook a callback right after first paint. I misunderstood rAF callbacks -- they fire at every frame, even if the frame paints nothing yet.

Back in the Firefox OS days, I am told the first MozAfterEvent does reflect first paint. It's not relevant here unfortunately because about:preferences has no access to that event. 

I guess if bug 1377251 is accessible for content, we could use that -- run an rAF loop and only execute the callback after performance.timing.timeToNonBlankPaint returns a timestamp.

Does all my assumption valid here? If so it sounds like there is no way to get rid of this sync reflow with a "temporal" solution, and we must go back and look at "spacial" (not "special") solution like comment 3 (which get rid of this function but not FocusManager nor scrollTop found during my investigation).
(In reply to Tim Guan-tin Chien [:timdream] (please needinfo) from comment #11)

> Does all my assumption valid here? If so it sounds like there is no way to
> get rid of this sync reflow with a "temporal" solution, and we must go back
> and look at "spacial" (not "special") solution like comment 3 (which get rid
> of this function but not FocusManager nor scrollTop found during my
> investigation).

I don't think 'temporal' hacks would work there, as either we trigger a sync reflow, or cause flickering; so comment 3 looks like a more interesting solution.

For scrollTop, do we really need it when opening the preferences? Or is it only needed when switching between preference panes?
(In reply to Florian Quèze [:florian] [:flo] from comment #12)
> I don't think 'temporal' hacks would work there, as either we trigger a sync
> reflow, or cause flickering; so comment 3 looks like a more interesting
> solution.

I don't see any sync reflow after wrapping calls into rAF. My recollection is that as long as there isn't another rAF call writing into the DOM, a read of the layout shouldn't trigger sync reflow (ref FastDOM).

As of the flickering, I believe it's acceptable if we actively manage what shows up first. it's not optimal but it will be better than looking at a blank page for a long time.

> For scrollTop, do we really need it when opening the preferences? Or is it
> only needed when switching between preference panes?

Yes, we could get rid of it there. I however not entirely sure how to do that in a maintainable way (or one more argument to the function? That can pile up very quickly...)
(In reply to Tim Guan-tin Chien [:timdream] (please needinfo) from comment #13)
> (In reply to Florian Quèze [:florian] [:flo] from comment #12)
> > I don't think 'temporal' hacks would work there, as either we trigger a sync
> > reflow, or cause flickering; so comment 3 looks like a more interesting
> > solution.
> 
> I don't see any sync reflow after wrapping calls into rAF. My recollection
> is that as long as there isn't another rAF call writing into the DOM, a read
> of the layout shouldn't trigger sync reflow (ref FastDOM).

It'll definitely cause a sync reflow if another random piece of code used rAF for the same frame, and wrote into the DOM right before your rAF callback gets called. rAF is designed to write DOM changes, not to read layout data.

> As of the flickering, I believe it's acceptable if we actively manage what
> shows up first. it's not optimal but it will be better than looking at a
> blank page for a long time.

Having something that appears on screen slightly later than the rest of the page is OK. Having things that appear in the wrong place and move after one frame is worse than delaying the whole display by one frame.

> > For scrollTop, do we really need it when opening the preferences? Or is it
> > only needed when switching between preference panes?
> 
> Yes, we could get rid of it there. I however not entirely sure how to do
> that in a maintainable way (or one more argument to the function? That can
> pile up very quickly...)

It'll be a hack, but I think it's worth it.
On my walk way home, I realize a way to make the "temporal" hack work. I realized we could reliably get a callback right after first paint (right before second paint to be precise), by scheduling a 0s no-op CSS animation, and listen to its animationend event.

Here is a profile on MacBook Pro demonstrate what this patch does. Noted that this is record with Cmd+, and with Preferences page already loaded once (to workaround the effect of bug 1375978): http://bit.ly/2vdkrlH

Previously, rAF won't work because there will be multiple refresh ticks between DOMContentLoaded and the real first paint.

If we have access in the DOM to determine first paint in content (either with bug 1377251 or any of the proposed APIs in WICG), we could employ an rAF loop and probe that property.

Florian, Jaws, let me know what you think about this hack.

I can profile this patch on the Acer next week if we need to.
(In reply to Tim Guan-tin Chien [:timdream] (please needinfo) from comment #16)
> On my walk way home, I realize a way to make the "temporal" hack work. I
> realized we could reliably get a callback right after first paint (right
> before second paint to be precise), by scheduling a 0s no-op CSS animation,
> and listen to its animationend event.

(quick nit on the statement above; animationend happens after 2nd paint in the profile -- it's not exactly right after first paint but it's good enough IMHO -- the page now first shows in ~300ms from the first keydown)
Assignee: nobody → timdream
Status: NEW → ASSIGNED
Comment on attachment 8898104 [details]
Bug 1389550 - Remove sync reflow during about:preferences loading and schedule them with animationend event,

https://reviewboard.mozilla.org/r/169430/#review179564

::: browser/components/preferences/in-content-new/preferences.xul:126
(Diff revision 2)
> -    <richlistbox id="categories">
> +    <richlistbox id="categories" class="visually-hidden">
>        <richlistitem id="category-general"

Will this cause a flicker of the categories list? The first two frames will be blank and then these will appear? That doesn't seem like an improvement to me.

Without this change, the user may not notice the jank if they're not interacting with the page or browser during the first couple frames. With this change, the user can visually see pieces of the UI appearing one at a time.

::: browser/themes/shared/incontentprefs/preferences.inc.css:23
(Diff revision 2)
> +.main-content {
> +}

This seems unnecessary.

::: browser/themes/shared/incontentprefs/preferences.inc.css:26
(Diff revision 2)
> +@keyframes noop {
>  }

Is this guaranteed to fire animationend by the spec? Could your use of this be removed later by some optimization?
Attachment #8898104 - Flags: review?(jaws) → review-
Comment on attachment 8898104 [details]
Bug 1389550 - Remove sync reflow during about:preferences loading and schedule them with animationend event,

Comment 18 says all I had to say here :-).
Attachment #8898104 - Flags: review?(florian)
I don't have time to work on alternative solution.
Assignee: timdream → nobody
Status: ASSIGNED → NEW
init_dynamic_padding() will be removed after help button rewrite with XUL.

See also https://bugzilla.mozilla.org/show_bug.cgi?id=1393415#c2.
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → DUPLICATE
Whiteboard: [photon-preference]
Status: RESOLVED → REOPENED
Resolution: DUPLICATE → ---
Move this XUL layout patch from bug 1393415 to here. The perf enhancement patch will only be landed in fx58 in order to reduce the risk for uplifting 57.
Assignee: nobody → rchien
Status: REOPENED → ASSIGNED
status-firefox57: affected → wontfix
Attachment #8914238 - Flags: review?(jaws)
Comment on attachment 8914238 [details]
Bug 1389550 - Move help button into category and remove init_dynamic_padding in preferences.js

https://reviewboard.mozilla.org/r/185556/#review190894

::: browser/themes/shared/incontentprefs/preferences.inc.css:620
(Diff revision 3)
> -  background-size: 16px;
> -  padding-inline-start: 38px;
>    margin-inline-start: 34px;
> +  margin-bottom: 36px;
> +  height: 36px;
> +  padding: 1px; /* Adding padding around children in order to make entire keyboard focusing outline visible */

Would this be the same as using the border that was present before? Why switch to using outline?

::: browser/themes/shared/incontentprefs/preferences.inc.css:648
(Diff revision 3)
>    fill: currentColor;
> +  width: 16px;
> +  height: 16px;
> +  margin-inline-start: 10px;
> +  margin-inline-end: 14px;
> +  vertical-align: sub;

Is this doing anything here? I would only expect this to work in a display:table-cell element.

::: browser/themes/shared/incontentprefs/preferences.inc.css:655
(Diff revision 3)
>  
> -.help-button:link,
> -.help-button:visited {
> +.help-icon:hover {
> +  fill: currentColor !important;
> +}
> +
> +.help-button:hover:active:not([disabled="true"]) {

.help-button:hover:active:not([disabled])

::: npm-shrinkwrap.json:1007
(Diff revision 3)
>      "sprintf-js": {
>        "version": "1.0.3",
>        "resolved": "https://registry.npmjs.org/sprintf-js/-/sprintf-js-1.0.3.tgz",
>        "integrity": "sha1-BOaSb2YolTVPPdAVIDYzuFcpfiw="
>      },
> +    "string_decoder": {

Please undo the changes to this file. Please also file a bug to revert the change to this file that was done by the patch that landed in bug 1349689 (it shouldn't have touched this file).

I've seen other people put patches up for review that are trying to make this same change and I think it is all fall-out from the patch that landed in bug 1349689.
Attachment #8914238 - Flags: review?(jaws) → review-
Comment on attachment 8914238 [details]
Bug 1389550 - Move help button into category and remove init_dynamic_padding in preferences.js

https://reviewboard.mozilla.org/r/185556/#review190894

> Would this be the same as using the border that was present before? Why switch to using outline?

Using border isn't an ideal solution in our case. It will introduce 1px border around richlistiem when hovering, we can see that icon & text of category shift slightly while keyboard navigating between categories.

We can still introduce a transparent 1px border for richlistitem but those transparent borders will always appear in high contrast mode, it turns out that outline could be the best solution.

> Is this doing anything here? I would only expect this to work in a display:table-cell element.

It will take effect on inline element as well. See also https://developer.mozilla.org/zh-TW/docs/Web/CSS/vertical-align
(In reply to Ricky Chien [:rickychien] from comment #27)
> It will take effect on inline element as well. See also
> https://developer.mozilla.org/zh-TW/docs/Web/CSS/vertical-align

Ah, XUL image and label might not be inline elements but I'm sure that `vertical-align` takes effect in this case.
Whiteboard: [photon-preference]
Comment on attachment 8914238 [details]
Bug 1389550 - Move help button into category and remove init_dynamic_padding in preferences.js

https://reviewboard.mozilla.org/r/185556/#review192018

Conditional r+ depending on the answer about the cursor. We should be using the same cursor as the category buttons. If you disagree then please reqeust review again.

::: browser/themes/shared/incontentprefs/preferences.inc.css:646
(Diff revisions 3 - 7)
>    outline: var(--in-content-category-outline-focus);
>  }
>  
>  .help-icon {
>    list-style-image: url("chrome://global/skin/icons/help.svg");
> -  -moz-context-properties: fill, fill-opacity;
> +  -moz-context-properties: fill;

Why is fill-opacity being removed?

::: browser/themes/shared/incontentprefs/preferences.inc.css:661
(Diff revisions 3 - 7)
>    fill: currentColor !important;
>  }
>  
> -.help-button:hover:active:not([disabled="true"]) {
> -  background-color: var(--in-content-category-background-active);
> +.help-label {
> +  margin: 0 4px;
> +  cursor: unset;

Why do we need to unset the cursor? Can we use the same cursor as the category icons?
Attachment #8914238 - Flags: review?(jaws) → review+
Pushed by rchien@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/0aeea10bdcbe
Move help button into category and remove init_dynamic_padding in preferences.js r=jaws
https://hg.mozilla.org/mozilla-central/rev/0aeea10bdcbe
Status: ASSIGNED → RESOLVED
Closed: 7 years ago → 7 years ago
status-firefox58: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 58
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: