1381993 - The Stop/Reload animation is placed in the wrong position when using a non-default font-size

Status: VERIFIED FIXED

(Firefox :: Theme, defect, P1)

Description

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

+++ This bug was initially created as a clone of Bug #1381991 +++

STR:
Open the Browser Toolbox
Inspect the <window>
Set the font-size on the window to font-size:1.4em;
Load mozilla.org

Expected result:
The stop/reload animation should run, vertically centered and aligned with the other toolbar icons

Actual result:
The stop/reload animation is shifted down and not in line with the other toolbar icons.

Comment 1

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

Attachment: Bug 1381993 - Position the stop/reload animation using absolute positioning to make sure that the animation remains vertically centered even when non-default font sizes are used.

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

Comment 2

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

The patch on this bug must be applied on top of the patch from bug 1381957.

Comment 3

[:Gijs (he/him)]

Comment on attachment 8887747 [details]
Bug 1381993 - Position the stop/reload animation using absolute positioning to make sure that the animation remains vertically centered even when non-default font sizes are used.

https://reviewboard.mozilla.org/r/158668/#review164066

If I use:

```
document.documentElement.style.fontSize = "2.5em"
```
on OS X, the animation is still half-cut off, appearing in the wrong place.

Comment 4

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

Comment on attachment 8887747 [details]
Bug 1381993 - Position the stop/reload animation using absolute positioning to make sure that the animation remains vertically centered even when non-default font sizes are used.

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

Comment 5

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

I'll try to move this functionality to BrowserUtils.jsm while fixing bug 1355922.

Comment 6

[:Gijs (he/him)]

Comment on attachment 8887747 [details]
Bug 1381993 - Position the stop/reload animation using absolute positioning to make sure that the animation remains vertically centered even when non-default font sizes are used.

https://reviewboard.mozilla.org/r/158668/#review164390

::: browser/base/content/browser.js:4999
(Diff revision 2)
> +  /* This function is necessary to correctly vertically center the animation
> +     within the toolbar, which uses -moz-pack-align:stretch; and thus a height
> +     which is dependant on the font-size. */
> +  setAnimationImageHeightRelativeToToolbarButtonHeight() {
> +    let dwu = window.getInterface(Ci.nsIDOMWindowUtils);
> +    let toolbarItem = this.reload.closest(".customization-target > toolbaritem");

Why can't this just select the parent toolbaritem? I'm worried this will break after bug 1363485 if the item moves into the panel (either dynamic overflow or permanently moved), say, because there's no `.customization-target` node.


Maybe this can just use this.stopReloadContainer ?

::: browser/base/content/browser.js:5002
(Diff revision 2)
> +  setAnimationImageHeightRelativeToToolbarButtonHeight() {
> +    let dwu = window.getInterface(Ci.nsIDOMWindowUtils);
> +    let toolbarItem = this.reload.closest(".customization-target > toolbaritem");
> +    let bounds = dwu.getBoundsWithoutFlushing(toolbarItem);
> +    let toolbar = this.reload.closest("toolbar");
> +    toolbar.style.setProperty("--toolbarbutton-button-height", bounds.height + "px");

Couldn't we set this directly on the node itself (ie on this.stopReloadContainer) ? "toolbarbutton-button-height" sounds like it applies to all toolbarbuttons, and I don't know if that's really the case here.

Comment 7

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

Comment on attachment 8887747 [details]
Bug 1381993 - Position the stop/reload animation using absolute positioning to make sure that the animation remains vertically centered even when non-default font sizes are used.

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

Comment 8

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

Comment on attachment 8887747 [details]
Bug 1381993 - Position the stop/reload animation using absolute positioning to make sure that the animation remains vertically centered even when non-default font sizes are used.

https://reviewboard.mozilla.org/r/158668/#review164390

> Why can't this just select the parent toolbaritem? I'm worried this will break after bug 1363485 if the item moves into the panel (either dynamic overflow or permanently moved), say, because there's no `.customization-target` node.
> 
> 
> Maybe this can just use this.stopReloadContainer ?

`this.reload.closest(".customization-target > toolbaritem");` wasn't selecting this.stopReloadContainer, in fact it is selecting #url-bar. Changing this to be based off of stopReloadContainer gets us the wrong value (10px shorter than it should be in the 2.5em font-size case).

Based off of discussions in bug 1355922 we are disabling animations outside of the navbar due to other complications, so we may be able to just disable this animation if the reload button ends up in the overflow panel.

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/83347c6ab45e
Position the stop/reload animation using absolute positioning to make sure that the animation remains vertically centered even when non-default font sizes are used. r=Gijs

Comment 10

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

https://hg.mozilla.org/mozilla-central/rev/83347c6ab45e

Comment 11

[Stefan [:StefanG_QA]]

Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:57.0) Gecko/20100101 Firefox/57.0 (20170925150345)

This issue is verified as fixed with the latest 57.0b3 build on 9/26/2017 and on 56.0a1 from 20170721030204.