1364672 - Glitches in animation in photon hamburger panel

Status: VERIFIED FIXED

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

Description

[Sören Hentzschel]

Attachment: 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. ;)

Comment 1

[:Gijs (he/him)]

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?

Comment 2

[Sören Hentzschel]

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.

Comment 3

[:Gijs (he/him)]

(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?

Comment 4

[Mike de Boer [:mikedeboer]]

(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.

Comment 5

[Mike de Boer [:mikedeboer]]

Attachment: Bug 1364672 - take the padding of the views into account when calculating the height of each sub view.

It's ok to cache the padding size of the main view and re-use it in this case,
because the padding is maintained for each view consistently.
The performance characteristics are therefore maintained.

Review commit: https://reviewboard.mozilla.org/r/140148/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/140148/

Comment 6

[:Gijs (he/him)]

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.

Comment 7

[Mike de Boer [:mikedeboer]]

Comment on attachment 8868536 [details]
Bug 1364672 - take the padding of the views into account when calculating the height of each sub view.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/140148/diff/1-2/

Comment 8

[Mike de Boer [:mikedeboer]]

Comment on attachment 8868536 [details]
Bug 1364672 - take the padding of the views into account when calculating the height of each sub view.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/140148/diff/2-3/

Comment 9

[:Gijs (he/him)]

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

Comment 10

[Pulsebot]

Pushed by [e-mail]:
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

Comment 11

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/4dfebbea3897

Comment 12

[u554753]

Verified on Windows, Mac, Ubuntu.