Closed Bug 1364672 Opened 2 years ago Closed 2 years ago

Glitches in animation in photon hamburger panel

Categories

(Firefox :: Toolbars and Customization, defect, P1)

defect

Tracking

()

VERIFIED FIXED
Firefox 55
Iteration:
55.6 - May 29
Tracking Status
firefox55 --- verified

People

(Reporter: soeren.hentzschel, Assigned: mikedeboer)

References

(Blocks 1 open bug)

Details

(Whiteboard: [photon-structure])

Attachments

(2 files)

Attached video more-menu.mov
Bug 1354127 introduced a new "more menu" in the new static hamburger menu. But the animation is not shippable in this state. Please see the attached screencast. ;)
The horizontal jumping of the header text I assume will go away as a result of bug 1363753.

The jump in height after the 'more' view is first shown should go away after bug 1363756 (which, come to think of it, may be required for bug 1363756 as well).

(In reply to Sören Hentzschel from comment #0)
> But the animation is not shippable in this state.

Can you please be explicit about what specifically you mean in that screencast, especially if you're seeing something else you object to besides what I already noted in this comment?
Depends on: 1363753, 1363756
Flags: needinfo?(cadeyrn)
Summary: Weird animation in new more menu (Photon) → Glitches in animation in photon hamburger panel
Whiteboard: [photon-structure]
Flags: qe-verify+
Priority: -- → P2
QA Contact: gwimberly
Hi Gijs,

sorry, I was in a hurry. I think you described all the problems I saw. I'll test again when bug 1363753 and bug 1363756 are fixed. Thank you.
Flags: needinfo?(cadeyrn)
(In reply to :Gijs from comment #1)
> The horizontal jumping of the header text I assume will go away as a result
> of bug 1363753.

I don't see this anymore on m-c now that this has landed, so I think this is fixed.

Mike, I can reproduce the final vertical 'jump' at the end of the transition locally on OS X. Can you? Any idea what's responsible for that? Seems like we're underestimating the final height somehow, which might actually mean that bug 1363756 miiight not fix this. Not sure. Thoughts?
Flags: needinfo?(mdeboer)
(In reply to :Gijs from comment #3)
> Mike, I can reproduce the final vertical 'jump' at the end of the transition
> locally on OS X. Can you? Any idea what's responsible for that? Seems like
> we're underestimating the final height somehow, which might actually mean
> that bug 1363756 miiight not fix this. Not sure. Thoughts?

Indeed, let's keep this bug to fix that final glitch. It seems like we're getting that behavior _after_ I added the padding to the arrowpanel, to match the specs. Adding that padding to the `getBoundsWithoutFlushing()` result should get us back to where we need to be.
Assignee: nobody → mdeboer
Status: NEW → ASSIGNED
Flags: needinfo?(mdeboer)
Iteration: --- → 55.6 - May 29
Priority: P2 → P1
Comment on attachment 8868536 [details]
Bug 1364672 - take the padding of the views into account when calculating the height of each sub view.

https://reviewboard.mozilla.org/r/140148/#review143492

::: browser/components/customizableui/PanelMultiView.jsm:389
(Diff revision 1)
> -        if (this.panelViews && !this._mainViewWidth)
> +        if (this.panelViews && !this._mainViewWidth) {
>            this._mainViewWidth = previousRect.width;
> +          let cs = window.getComputedStyle(previousViewNode);
> +          this._viewPadding = parseInt(cs.getPropertyValue("padding-top"), 10) +
> +            parseInt(cs.getPropertyValue("padding-bottom"), 10);
> +        }

3 notes here:

- Can you add a comment that makes it clear this relies on the previousViewNode/previousRect to be the main view
- Can we call this `_viewVerticalPadding` for good measure?
- This is causing a sync style flush, right, because we're reading computed style? I don't know that I agree with the commit message that that doesn't affect perf... Instead, can we get the lazy bounds of the first and last child of the view node (which should be cheap and just use DOM + lazy bounds getting), and compare the diff between their top/bottom and the (lazy) height of the view rect? That should effectively get the padding (and border, but there isn't any), right?

::: browser/components/customizableui/PanelMultiView.jsm:515
(Diff revision 1)
>                // axis gets confused. I.e. it lies.
>                // So what we need to resort to here is count the height of each
>                // individual child element of the view.
>                viewRect.height = [viewNode.header, ...viewNode.children].reduce((acc, node) => {
>                  return acc + dwu.getBoundsWithoutFlushing(node).height;
> -              }, 0);
> +              }, 0) + this._viewPadding;

Nit: can also pass this as the base for the reduce() (instead of 0). Don't much care either way, but I figured I'd flag it up.
Attachment #8868536 - Flags: review?(gijskruitbosch+bugs)
Comment on attachment 8868536 [details]
Bug 1364672 - take the padding of the views into account when calculating the height of each sub view.

https://reviewboard.mozilla.org/r/140148/#review143554

I haven't tested this, but assuming it fixes things, r=me
Attachment #8868536 - Flags: review?(gijskruitbosch+bugs) → review+
Pushed by mdeboer@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/4dfebbea3897
take the padding of the views into account when calculating the height of each sub view. r=Gijs
https://hg.mozilla.org/mozilla-central/rev/4dfebbea3897
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
Verified on Windows, Mac, Ubuntu.
Status: RESOLVED → VERIFIED
Flags: qe-verify+
Blocks: 1387512
You need to log in before you can comment on or make changes to this bug.