1222747 - Forward button looks compressed on Developer Edition Theme

Status: VERIFIED FIXED

(Firefox :: Theme, defect)

Description

[arni2033]

Attachment: screenshot 1 - Forward button looks compressed on Developer Edition Theme.png

STR:   (Win7_64, Nightly 45, 32bit, ID 20151107030439, new profile)
0. Set Dev Edition theme in firefox
1. Open new tab, type "about:" in urlbar, press enter to visit page "about:"
2. Select all text in urlbar, type "about:about" instead of that text, press Enter
3. Press Alt+Left to go back to "about:"

Result:
 Forward button looks compressed on Developer Edition Theme due to max-width:25px

Expectations:
 Forward button should have normal width, like in previous versions (~ 43-)

Comment 1

[Ryan VanderMeulen [:RyanVM]]

Regression range: https://hg.mozilla.org/integration/fx-team/pushloghtml?fromchange=9fce08491c5326a6b862622a8b8da6107c262a16&tochange=4d1bdb825e4c3ecc5a2069af64bb1ee9af0b9513

Regression from bug 1208715.

Comment 2

[:Gijs (he/him)]

Attachment: fix width of forward button on devedition theme,

This is because the padding is different in devedition and the number we have in browser.css hardcodes the padding as part of the width it computes. Because devedition's theme changes the padding, we should also change the max-width.

Comment 3

[Dão Gottwald [:dao]]

Comment on attachment 8688438 [details] [diff] [review]
fix width of forward button on devedition theme,

We'll probably need this on Linux too. Also, there's another #forward-button > .toolbarbutton-icon rule which this one should be merged with.

Comment 4

[:Gijs (he/him)]

(In reply to Dão Gottwald [:dao] from comment #3)
> Comment on attachment 8688438 [details] [diff] [review]
> fix width of forward button on devedition theme,
> 
> We'll probably need this on Linux too.

Well, we don't strictly "need" it because:

https://dxr.mozilla.org/mozilla-central/source/browser/themes/linux/browser.css#620

uses 32px and overrides the forward-button specific rule there, and either way the one for the forward button produces 31px:

https://dxr.mozilla.org/mozilla-central/source/browser/themes/linux/browser.css#19
https://dxr.mozilla.org/mozilla-central/source/browser/themes/linux/browser.css#752

which with 18px icon size + 2 * 5px padding + 1px border = 29px is 2px too large. But because it's max-width and we don't support hidpi on Linux for Toolbar.png, I don't think this causes issues in practice. Am I missing something? Should we update the lot of this (maybe in a separate bug) to make it futureproof if/when we do switch to using hidpi Toolbar.png on Linux?

> Also, there's another #forward-button
> > .toolbarbutton-icon rule which this one should be merged with.

I will update the patch with this.

Comment 5

[:Gijs (he/him)]

Attachment: fix width of forward button on devedition theme,

Comment 6

[Dão Gottwald [:dao]]

Comment on attachment 8688942 [details] [diff] [review]
fix width of forward button on devedition theme,

http://hg.mozilla.org/mozilla-central/annotate/f8b569906e4c/browser/themes/windows/browser.css#l947

Seems like if we change the button's width, we'll also need to change the negative margin that hides the button. Probably best to convert @forwardButtonWidth@ to a CSS variable now.

Comment 7

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Tracked for FF44 as it's a trivial but very visible (kinda sloppy) regression. I also see this on 43.0b7.

Comment 8

[Dão Gottwald [:dao]]

(In reply to Ritu Kothari (:ritu) from comment #7)
> Tracked for FF44 as it's a trivial but very visible (kinda sloppy)
> regression. I also see this on 43.0b7.

This was filed as a dev edition bug. What are your steps to reproduce on 43.0b7?

Comment 9

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

(In reply to Dão Gottwald [:dao] from comment #8)
> (In reply to Ritu Kothari (:ritu) from comment #7)
> > Tracked for FF44 as it's a trivial but very visible (kinda sloppy)
> > regression. I also see this on 43.0b7.
> 
> This was filed as a dev edition bug. What are your steps to reproduce on
> 43.0b7?

Dao, I might be completely wrong here but I just read the bug title and on any given tab in DevEd44 or Beta43, I just compared the size of the "page back" and "page forward" button and to me the forward (right-arrow) looked smaller or compressed. That was my basis for saying this also happens on 43.0b7. Is this by design? I've attached a screen snapshot too.

Comment 10

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Attachment: forward arrow.jpg

Comment 11

[Dão Gottwald [:dao]]

The forward arrow being smaller than the back arrow is by design.

Comment 12

[Liz Henry (:lizzard) (relman/hg->git project)]

Too late for 43, we are shipping soon.

Comment 13

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

44 is in Beta cycle and this issue is only in DevEdition AFAIU.

Comment 14

[:Gijs (he/him)]

Attachment: MozReview Request: Bug 1222747 - fix width of forward button on devedition theme by making it a CSS variable, r?dao

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

Comment 15

[:Gijs (he/him)]

Attachment: MozReview Request: Bug 1222747 - fix alignment of forward button on OSX devedition theme, r?dao

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

Comment 16

[:Gijs (he/him)]

This moves the define to a CSS variable. I'm not changing its value on OS X, and the changing of the value on devedition does not currently, AFAIK, "matter", but I figured the latter was good futureproofing. While testing, I noticed that the forward-button icon was badly aligned on OS X because of this rule: https://dxr.mozilla.org/mozilla-central/rev/d7a0ad85d9fb77916f9d77d62697b852f3dc63e6/browser/themes/osx/browser.css#1226-1230

and fixed that in the second patch.

Comment 17

[Dão Gottwald [:dao]]

Comment on attachment 8703649 [details]
MozReview Request: Bug 1222747 - fix alignment of forward button on OSX devedition theme, r?dao

Can you please document what you're doing here?

I assume this is correct for RTL?

Comment 18

[:Gijs (he/him)]

(In reply to Dão Gottwald [:dao] from comment #17)
> Comment on attachment 8703649 [details]
> MozReview Request: Bug 1222747 - fix alignment of forward button on OSX
> devedition theme, r?dao
> 
> Can you please document what you're doing here?
> 
> I assume this is correct for RTL?

The rule this is essentially "dealing with" is linked in comment #17 . I assumed that was correct for RTL. Maybe it isn't? Do we treat back/fwd button specially for RTL? If not, I expect neither my change for devedition nor the original rule are correct for RTL...

Comment 19

[Dão Gottwald [:dao]]

(In reply to :Gijs Kruitbosch from comment #18)
> (In reply to Dão Gottwald [:dao] from comment #17)
> > Comment on attachment 8703649 [details]
> > MozReview Request: Bug 1222747 - fix alignment of forward button on OSX
> > devedition theme, r?dao
> > 
> > Can you please document what you're doing here?
> > 
> > I assume this is correct for RTL?
> 
> The rule this is essentially "dealing with" is linked in comment #17 . I
> assumed that was correct for RTL. Maybe it isn't?

Could be.

> Do we treat back/fwd button specially for RTL?

We mirror them, and there's a good chance that it's not done consistently across platforms...

Comment 20

[:Gijs (he/him)]

(In reply to Dão Gottwald [:dao] from comment #19)
> (In reply to :Gijs Kruitbosch from comment #18)
> > (In reply to Dão Gottwald [:dao] from comment #17)
> > > Comment on attachment 8703649 [details]
> > > MozReview Request: Bug 1222747 - fix alignment of forward button on OSX
> > > devedition theme, r?dao
> > > 
> > > Can you please document what you're doing here?
> > > 
> > > I assume this is correct for RTL?
> > 
> > The rule this is essentially "dealing with" is linked in comment #17 . I
> > assumed that was correct for RTL. Maybe it isn't?
> 
> Could be.
> 
> > Do we treat back/fwd button specially for RTL?
> 
> We mirror them, and there's a good chance that it's not done consistently
> across platforms...

So oddly, it seems the margin/padding-left here seems to work and ends up on the right-hand side of the element. I actually don't understand why that is the case. But well, at least it works. I'll resubmit that patch with a comment in the code.

Comment 21

[:Gijs (he/him)]

Comment on attachment 8703648 [details]
MozReview Request: Bug 1222747 - fix width of forward button on devedition theme by making it a CSS variable, r?dao

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

Comment 22

[:Gijs (he/him)]

Comment on attachment 8703649 [details]
MozReview Request: Bug 1222747 - fix alignment of forward button on OSX devedition theme, r?dao

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

Comment 23

[:Gijs (he/him)]

Comment on attachment 8703648 [details]
MozReview Request: Bug 1222747 - fix width of forward button on devedition theme by making it a CSS variable, r?dao

https://reviewboard.mozilla.org/r/29415/#review26681

Carrying forward this review...

Comment 24

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/897610807fb5
https://hg.mozilla.org/integration/fx-team/rev/92cb9839960b

Comment 25

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/897610807fb5
https://hg.mozilla.org/mozilla-central/rev/92cb9839960b

Comment 26

[Sylvestre Ledru [:Sylvestre]]

Gijs, do you plan to fill the uplift request to aurora? Thanks

Comment 27

[:Gijs (he/him)]

Comment on attachment 8703648 [details]
MozReview Request: Bug 1222747 - fix width of forward button on devedition theme by making it a CSS variable, r?dao

Approval Request Comment
[Feature/regressing bug #]: bug 1208715
[User impact if declined]: forward button looks warped/compressed when the devedition theme is in use.
[Describe test coverage new/current, TreeHerder]: no, appearance matter only
[Risks and why]: low, CSS changes only, has baked on trunk for a while with no regressions reported
[String/UUID change made/needed]: no

Comment 28

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8703648 [details]
MozReview Request: Bug 1222747 - fix width of forward button on devedition theme by making it a CSS variable, r?dao

Thanks for this css-only patch ;)

Comment 29

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

I'm having conflicts uplifting this to aurora. Seems like the devedition.css file isn't similar enough. Can we get a rebased patch for this?

Comment 30

[:Gijs (he/him)]

Ugh, accidentally pushed in two parts, but anyway:

remote:   https://hg.mozilla.org/releases/mozilla-aurora/rev/4a1745d4c58a
remote:   https://hg.mozilla.org/releases/mozilla-aurora/rev/5031dae2d2af

Comment 31

[Md. Ehsanul Hassan]

I have reproduced this bug in 45.0a1 (2015-11-07) on windows 8.1 (64 bit)

Bug is now verified as fixed latest Firefox Developer edition 45.0a2 (2016-01-15)
Build Id:20160115004002
User agent:Mozilla/5.0 (Windows NT 6.3; WOW64; rv:45.0) Gecko/20100101 Firefox/45.0