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

VERIFIED FIXED in Firefox 57

Status

()

P1
normal
VERIFIED FIXED
a year ago
a year ago

People

(Reporter: mconley, Assigned: jaws)

Tracking

unspecified
Firefox 57
Points:
---

Firefox Tracking Flags

(firefox57 fixed)

Details

(Whiteboard: [reserve-photon-animation])

Attachments

(2 attachments)

(Reporter)

Description

a year ago
Created attachment 8890130 [details]
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.
(Reporter)

Updated

a year ago
Blocks: 1355922
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.
Assignee: nobody → jaws
Status: NEW → ASSIGNED
Whiteboard: [reserve-photon-animation]
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)

Comment 5

a year ago
mozreview-review
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

a year ago
mozreview-review
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.
Attachment #8890482 - Attachment description: Bug 1384341 - Set a minimum size for the animation area so animations that are larger than default-sized (non-compact) toolbar buttons don't get clipped. Also, don't apply z-index:2 to the selected tab in photon builds since the tabs won't be overlapping → Bug 1384341 - Set a minimum size for the animation area so animations that are larger than default-sized (non-compact) toolbar buttons don't get clipped. Also, don't apply z-index:2 to the selected tab in photon builds since the tabs won't be overlapping
Attachment #8890482 - Flags: review?(mconley)
Flags: qe-verify+
Summary: Pocket animation for Library is cut off on the bottom in compact mode → Pocket and Bookmark animation for Library is cut off on the bottom in compact mode
Comment hidden (mozreview-request)
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?
Flags: needinfo?(shorlander)
Flags: needinfo?(dao+bmo)
(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?
Flags: needinfo?(shorlander)
Flags: needinfo?(dao+bmo)
(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?
Flags: needinfo?(dao+bmo)
Regarding comment #10 ^
Flags: needinfo?(shorlander)
(Reporter)

Comment 12

a year ago
mozreview-review
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.
Attachment #8890482 - Flags: review?(mconley) → review-
(Reporter)

Comment 13

a year ago
(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.
Iteration: --- → 57.1 - Aug 15
Priority: -- → P1
QA Contact: jwilliams
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.
Flags: needinfo?(shorlander)
Flags: needinfo?(dao+bmo)
Comment hidden (mozreview-request)
(Reporter)

Comment 16

a year ago
mozreview-review
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.
Attachment #8890482 - Flags: review?(mconley) → review+
Yeah, the Pocket animation is being tracked by bug 1387077.

Comment 18

a year ago
Pushed by jwein@mozilla.com:
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
https://hg.mozilla.org/mozilla-central/rev/b98350e98fec
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
status-firefox57: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 57

Comment 20

a year ago
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
QA Whiteboard: [bugday-20170816]
Status: RESOLVED → VERIFIED
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.