Closed Bug 1222747 Opened 4 years ago Closed 4 years ago

Forward button looks compressed on Developer Edition Theme

Categories

(Firefox :: Theme, defect)

defect
Not set

Tracking

()

VERIFIED FIXED
Firefox 46
Tracking Status
firefox43 - unaffected
firefox44 + wontfix
firefox45 + verified
firefox46 --- verified

People

(Reporter: arni2033, Assigned: Gijs)

References

Details

(Keywords: regression)

Attachments

(5 files, 1 obsolete file)

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-)
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.
Attachment #8688438 - Flags: review?(dao)
Assignee: nobody → gijskruitbosch+bugs
Status: NEW → ASSIGNED
Flags: needinfo?(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.
Attachment #8688438 - Flags: review?(dao)
(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.
Attachment #8688438 - Attachment is obsolete: true
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.
Attachment #8688942 - Flags: review?(dao) → review-
Tracked for FF44 as it's a trivial but very visible (kinda sloppy) regression. I also see this on 43.0b7.
(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?
Flags: needinfo?(rkothari)
(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.
Flags: needinfo?(rkothari)
Attached image forward arrow.jpg
The forward arrow being smaller than the back arrow is by design.
Too late for 43, we are shipping soon.
44 is in Beta cycle and this issue is only in DevEdition AFAIU.
Has STR: --- → yes
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.
Attachment #8703648 - Flags: review?(dao) → review+
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?
Attachment #8703649 - Flags: review?(dao)
(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...
Flags: needinfo?(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...
Flags: needinfo?(dao)
(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 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/
Attachment #8703648 - Flags: review+ → review?(dao)
Attachment #8703649 - Flags: review?(dao)
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 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...
Attachment #8703648 - Flags: review+
Attachment #8703648 - Flags: review?(dao)
Attachment #8703649 - Flags: review?(dao) → review+
https://hg.mozilla.org/mozilla-central/rev/897610807fb5
https://hg.mozilla.org/mozilla-central/rev/92cb9839960b
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 46
Gijs, do you plan to fill the uplift request to aurora? Thanks
Flags: needinfo?(gijskruitbosch+bugs)
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
Flags: needinfo?(gijskruitbosch+bugs)
Attachment #8703648 - Flags: approval-mozilla-aurora?
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 ;)
Attachment #8703648 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
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?
Flags: needinfo?(gijskruitbosch+bugs)
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
Flags: needinfo?(gijskruitbosch+bugs)
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
Status: RESOLVED → VERIFIED
Depends on: 1253500
You need to log in before you can comment on or make changes to this bug.