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

RESOLVED FIXED in Firefox 56

Status

()

Firefox
Theme
P1
normal
RESOLVED FIXED
a month ago
17 hours ago

People

(Reporter: jaws, Assigned: jaws)

Tracking

unspecified
Firefox 56
Points:
---
Dependency tree / graph
Bug Flags:
qe-verify +

Firefox Tracking Flags

(firefox56 fixed)

Details

(Whiteboard: [reserve-photon-animation])

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(1 attachment)

+++ 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.
See Also: → bug 1381991
Whiteboard: [photon-animation][triage]
Assignee: nobody → jaws
Status: NEW → ASSIGNED
Comment hidden (mozreview-request)
The patch on this bug must be applied on top of the patch from bug 1381957.
Depends on: 1381957

Updated

a month ago
Iteration: --- → 56.3 - Jul 24
Flags: qe-verify+
Priority: -- → P1
QA Contact: jwilliams
Whiteboard: [photon-animation][triage] → [reserve-photon-animation]

Comment 3

a month ago
mozreview-review
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.
Attachment #8887747 - Flags: review?(gijskruitbosch+bugs) → review-
Comment hidden (mozreview-request)
I'll try to move this functionality to BrowserUtils.jsm while fixing bug 1355922.
Blocks: 1355922

Comment 6

a month ago
mozreview-review
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.
Attachment #8887747 - Flags: review?(gijskruitbosch+bugs) → review+
Comment hidden (mozreview-request)
(Assignee)

Comment 8

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

a month ago
Pushed by jwein@mozilla.com:
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
https://hg.mozilla.org/mozilla-central/rev/83347c6ab45e
Status: ASSIGNED → RESOLVED
Last Resolved: 29 days ago
status-firefox56: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 56
Depends on: 1383879
You need to log in before you can comment on or make changes to this bug.