1384341 - Pocket and Bookmark animation for Library is cut off on the bottom in compact mode

Status: VERIFIED FIXED

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

Description

[Mike Conley (:mconley) (:⚙️)]

Attachment: pocket-compact.gif

STR:

0) Ensure you have a Pocket account set up and ready to use, and the icon in your toolbar.
1) Enter customize mode, and set the UI density to Compact
2) Click the Pocket icon in your toolbar.
3) Click it again the close the panel.

ER:

The animation should be just like in the Normal UI Density mode.

AR:

The bottom of the animation gets cut off. See attachment.

Comment 1

[Jared Wein [:jaws] (please needinfo? me)]

The clipping at the top of the animation only occurs of the selected tab is above the animation since `.tabbrowser-tab[visuallyselected="true"]` has z-index: 2;

The clipping at the bottom of the animation is because the value for --toolbarbutton-height is calculated at 32px but we need a minimum of 38px.

Comment 2

[Jared Wein [:jaws] (please needinfo? me)]

Attachment: Bug 1384341 - Set a minimum size for the animation area so animations that are larger than toolbar buttons don't get clipped.

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

Comment 3

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8890482 [details]
Bug 1384341 - Set a minimum size for the animation area so animations that are larger than toolbar buttons don't get clipped.

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

Comment 4

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8890482 [details]
Bug 1384341 - Set a minimum size for the animation area so animations that are larger than toolbar buttons don't get clipped.

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

Comment 5

[Dão Gottwald [:dao]]

Comment on attachment 8890482 [details]
Bug 1384341 - Set a minimum size for the animation area so animations that are larger than toolbar buttons don't get clipped.

https://reviewboard.mozilla.org/r/161618/#review166938

::: browser/base/content/browser-places.js:140
(Diff revision 3)
>                       AppConstants.MOZ_PHOTON_ANIMATIONS &&
>                       (libraryButton = document.getElementById("library-button")) &&
>                       libraryButton.getAttribute("cui-areatype") != "menu-panel" &&
>                       libraryButton.getAttribute("overflowedItem") != "true" &&
>                       libraryButton.closest("#nav-bar")) {
> -            BrowserUtils.setToolbarButtonHeightProperty(libraryButton);
> +            BrowserUtils.setToolbarButtonHeightProperty(libraryButton, {minimumHeight: 38});

Why is this in the script? Can you set min-height in CSS instead? Can you also document how you arrived at this magic number?

Comment 6

[Dão Gottwald [:dao]]

Comment on attachment 8890482 [details]
Bug 1384341 - Set a minimum size for the animation area so animations that are larger than toolbar buttons don't get clipped.

https://reviewboard.mozilla.org/r/161618/#review166940

::: browser/themes/shared/tabs.inc.css:51
(Diff revision 3)
>    -moz-box-align: stretch;
>  }
>  
> -/* The selected tab should appear above adjacent tabs, .tabs-newtab-button and the highlight of #nav-bar */
> +%ifndef MOZ_PHOTON_ANIMATIONS
> +/* The selected tab should appear above adjacent tabs, .tabs-newtab-button and the highlight of #nav-bar.
> +   This is disabled in Photon builds since we won't have overlapping tabs and we need the #nav-bar

We still need the selected tab to overlay the shadow / border between the navigation toolbar and the tabs toolbar.

Comment 7

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8890482 [details]
Bug 1384341 - Set a minimum size for the animation area so animations that are larger than toolbar buttons don't get clipped.

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

Comment 8

[Jared Wein [:jaws] (please needinfo? me)]

Note that with this patch the animation is still clipped at the top if the selected tab is above the animation. This is because the selected tab is at a higher z-index (2) than the #nav-bar (1). I don't see a way to fix this without removing the nav-bar shadow/highlight.

@shorlander, is this something that we need to keep?

@dao, do you have any ideas of a fix for this that keeps the nav-bar shadow/highlight?

Comment 9

[Dão Gottwald [:dao]]

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #8)
> Note that with this patch the animation is still clipped at the top if the
> selected tab is above the animation. This is because the selected tab is at
> a higher z-index (2) than the #nav-bar (1). I don't see a way to fix this
> without removing the nav-bar shadow/highlight.
> 
> @shorlander, is this something that we need to keep?

We need to keep the shadow / border, not the highlight. You can peek at the WIP patch in bug 1349555 that I just posted, although I only tested it on Linux.

> @dao, do you have any ideas of a fix for this that keeps the nav-bar
> shadow/highlight?

Can you set a higher z-index on the toolbarbutton-animatable-box thingy?

Comment 10

[Jared Wein [:jaws] (please needinfo? me)]

(In reply to Dão Gottwald [::dao] from comment #9)
> (In reply to Jared Wein [:jaws] (please needinfo? me) from comment #8)
> > Note that with this patch the animation is still clipped at the top if the
> > selected tab is above the animation. This is because the selected tab is at
> > a higher z-index (2) than the #nav-bar (1). I don't see a way to fix this
> > without removing the nav-bar shadow/highlight.
> > 
> > @shorlander, is this something that we need to keep?
> 
> We need to keep the shadow / border, not the highlight. You can peek at the
> WIP patch in bug 1349555 that I just posted, although I only tested it on
> Linux.
> 
> > @dao, do you have any ideas of a fix for this that keeps the nav-bar
> > shadow/highlight?
> 
> Can you set a higher z-index on the toolbarbutton-animatable-box thingy?

Setting a higher z-index on the animatable-box will not work (I tried the max value of ). This doesn't work because of how the stacking context is set up. It currently looks like this:

<toolbar id=TabsToolbar>
  ...
    <tab selected=true style="z-index: 2; position: relative;"/>
</toolbar>
<toolbar id=nav-bar style="z-index: 1; position: relative;">
  ...
    <toolbarbutton>
      <hbox class="toolbarbutton-animatable-box">
        <image class="toolbarbutton-animatable-image"/>
      </hbox>
    </toolbarbutton>
</toolbar>

Both the selected tab and the nav-bar share the same stacking context, the root element. Since the selected tab has a higher z-index it will always be painted after the nav-bar.

Would you be OK with disabling the shadow / border during animations?

Comment 11

[Jared Wein [:jaws] (please needinfo? me)]

Regarding comment #10 ^

Comment 12

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8890482 [details]
Bug 1384341 - Set a minimum size for the animation area so animations that are larger than toolbar buttons don't get clipped.

https://reviewboard.mozilla.org/r/161618/#review168914

Thanks! I can confirm that this fixes the problem for me - however, Dao's issues seem pretty reasonable, so I'm r-'ing for now.

Comment 13

[Mike Conley (:mconley) (:⚙️)]

(In reply to Mike Conley (:mconley) - Buried in needinfo / review backlog, eating my way out from comment #12)
> Thanks! I can confirm that this fixes the problem for me - however, Dao's
> issues seem pretty reasonable, so I'm r-'ing for now.

Heh, and now I'm seeing the conversation in here after Dao's issues were raised. I guess let's see what shorlander says.

Comment 14

[Jared Wein [:jaws] (please needinfo? me)]

I'm going to narrow this bug down to the clipping at the bottom of the animation. The part of the animation that is getting clipped by the active tab can get fixed in bug 1384953 since that bug is already on file and the solution isn't so related to how this bug will get fixed.

Comment 15

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8890482 [details]
Bug 1384341 - Set a minimum size for the animation area so animations that are larger than toolbar buttons don't get clipped.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/161618/diff/4-5/

Comment 16

[Mike Conley (:mconley) (:⚙️)]

Comment on attachment 8890482 [details]
Bug 1384341 - Set a minimum size for the animation area so animations that are larger than toolbar buttons don't get clipped.

https://reviewboard.mozilla.org/r/161618/#review169962

Thanks! Fixes the issue, at least for the bookmarking case. Can't test the Pocket case, since for some reason that animation doesn't seem to occur anymore since the Pocket icon moved into the URL bar. Not sure if that's a known bug or not.

Comment 17

[Jared Wein [:jaws] (please needinfo? me)]

Yeah, the Pocket animation is being tracked by bug 1387077.

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/b98350e98fec
Set a minimum size for the animation area so animations that are larger than toolbar buttons don't get clipped. r=mconley

Comment 19

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/b98350e98fec

Comment 20

[Azmina]

I have reproduced this bug with Nightly 56.0a1 (2017-07-25) on Windows 8.1, 64 Bit!

This bug's fix is verified on Latest Nightly 57.0a1.

Build ID : 20170817100132
User Agent : Mozilla/5.0 (Windows NT 6.3; Win64; x64; rv:57.0) Gecko/20100101 Firefox/57.0