Closed
Bug 1383689
Opened 7 years ago
Closed 7 years ago
Remove uninterruptible reflow at setToolbarButtonHeightProperty@resource://gre/modules/BrowserUtils.jsm
Categories
(Firefox :: Toolbars and Customization, enhancement, P1)
Firefox
Toolbars and Customization
Tracking
()
Tracking | Status | |
---|---|---|
firefox57 | --- | fixed |
People
(Reporter: jaws, Assigned: jaws)
References
Details
(Whiteboard: [reserve-photon-animation])
Attachments
(1 file)
There is an uninterruptible reflow in setToolbarButtonHeightProperty when the `forceLayoutFlushIfNeeded` option is used.
With the patches from bug 1383367 we won't need to use this and can instead wait for the reflow before starting the animation.
Updated•7 years ago
|
Flags: qe-verify?
Priority: -- → P3
Assignee | ||
Updated•7 years ago
|
Assignee: nobody → jaws
Status: NEW → ASSIGNED
Updated•7 years ago
|
Iteration: --- → 56.4 - Aug 1
Priority: P3 → P1
Comment 1•7 years ago
|
||
(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #0)
> There is an uninterruptible reflow in setToolbarButtonHeightProperty when
> the `forceLayoutFlushIfNeeded` option is used.
>
> With the patches from bug 1383367 we won't need to use this and can instead
> wait for the reflow before starting the animation.
It might mean that the animation starts one frame later (see also bug 1385729). I'm not sure that's desirable. :/
Assignee | ||
Comment 2•7 years ago
|
||
(In reply to Dão Gottwald [::dao] from comment #1)
> (In reply to Jared Wein [:jaws] (please needinfo? me) from comment #0)
> > There is an uninterruptible reflow in setToolbarButtonHeightProperty when
> > the `forceLayoutFlushIfNeeded` option is used.
> >
> > With the patches from bug 1383367 we won't need to use this and can instead
> > wait for the reflow before starting the animation.
>
> It might mean that the animation starts one frame later (see also bug
> 1385729). I'm not sure that's desirable. :/
For this animation it will not be a problem. The pin-to-overflow animation shows the overflow arrow in its original position, then rotates the arrow down, then rotates the arrow back to the original position. Painting the arrow in its original position for the first frame will not cause any awkwardness with this animation, but we may want to reduce the animation by one frame at the beginning if we think it looks like it is too slow to begin moving.
Comment 3•7 years ago
|
||
I'm not concerned about some kind of awkwardness. It's a matter of perceived performance. Animations should start ASAP after the user interaction they respond to. If there are idle frames at the beginning, we should remove those regardless of this bug.
Assignee | ||
Comment 4•7 years ago
|
||
chrome://browser/skin/chevron-animation.svg is the animation, which at first glance has what appears to be roughly 3 idle frames at the beginning, though inspecting the SVG shows that there are minor pixel differences between each frame.
I believe we could remove the first frame of the animation as part of fixing this bug and there would be no issue.
Updated•7 years ago
|
Flags: qe-verify? → qe-verify-
Comment hidden (mozreview-request) |
Comment 6•7 years ago
|
||
mozreview-review |
Comment on attachment 8892580 [details]
Bug 1383689 - Remove uninterruptible reflow at setToolbarButtonHeightProperty@resource://gre/modules/BrowserUtils.jsm.
https://reviewboard.mozilla.org/r/163584/#review168938
::: browser/themes/shared/toolbarbutton-icons.inc.css:392
(Diff revision 1)
> }
>
> #nav-bar-overflow-button[animate] > .toolbarbutton-animatable-box > .toolbarbutton-animatable-image {
> animation-name: overflow-animation;
> - animation-timing-function: steps(70);
> - animation-duration: 1.1s;
> + animation-timing-function: steps(69);
> + animation-duration: 1104ms;
Why has this inreased when the number of frames has decreased?
::: toolkit/modules/BrowserUtils.jsm:437
(Diff revision 1)
> }
> let bounds = dwu.getBoundsWithoutFlushing(toolbarItem);
> - if (!bounds.height && options.forceLayoutFlushIfNeeded) {
> + if (!bounds.height) {
> + let document = element.ownerDocument;
> + await BrowserUtils.promiseLayoutFlushed(document, "layout", () => {
> - bounds = toolbarItem.getBoundingClientRect();
> + bounds = toolbarItem.getBoundingClientRect();
Why can't we use dwu again here?
Attachment #8892580 -
Flags: review?(gijskruitbosch+bugs)
Assignee | ||
Comment 7•7 years ago
|
||
mozreview-review-reply |
Comment on attachment 8892580 [details]
Bug 1383689 - Remove uninterruptible reflow at setToolbarButtonHeightProperty@resource://gre/modules/BrowserUtils.jsm.
https://reviewboard.mozilla.org/r/163584/#review168938
> Why has this inreased when the number of frames has decreased?
It should have been 1120ms before at 70 frames. This is now 1104ms at 69 frames. It is now slightly longer but it is more correct now than it was before.
> Why can't we use dwu again here?
Yeah, we can. I went back and forth on this and didn't think it would be so expensive to put in something that would guarantee we would get the value we wanted since we had just waited for layout flush. I'll change this to use dwu.
Comment hidden (mozreview-request) |
Comment 9•7 years ago
|
||
mozreview-review |
Comment on attachment 8892580 [details]
Bug 1383689 - Remove uninterruptible reflow at setToolbarButtonHeightProperty@resource://gre/modules/BrowserUtils.jsm.
https://reviewboard.mozilla.org/r/163584/#review168970
Attachment #8892580 -
Flags: review?(gijskruitbosch+bugs) → review+
Comment 10•7 years ago
|
||
Pushed by jwein@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/daca6e4033c7
Remove uninterruptible reflow at setToolbarButtonHeightProperty@resource://gre/modules/BrowserUtils.jsm. r=Gijs
Updated•7 years ago
|
Iteration: 56.4 - Aug 1 → 57.1 - Aug 15
Comment 11•7 years ago
|
||
bugherder |
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox57:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 57
You need to log in
before you can comment on or make changes to this bug.
Description
•