Do not compress forward-icon in the default theme on Windows 10

VERIFIED FIXED in Firefox 46

Status

()

Firefox
Theme
VERIFIED FIXED
2 years ago
2 years ago

People

(Reporter: magicp, Assigned: Gijs)

Tracking

(Depends on: 1 bug)

45 Branch
Firefox 47
All
Windows 10
Points:
---
Dependency tree / graph
Bug Flags:
qe-verify +

Firefox Tracking Flags

(firefox44 unaffected, firefox45 wontfix, firefox46 verified, firefox47 verified, firefox-esr45 fixed)

Details

Attachments

(3 attachments, 2 obsolete attachments)

(Reporter)

Description

2 years ago
Created attachment 8726565 [details]
forward-icon.png

User Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:47.0) Gecko/20100101 Firefox/47.0
Build ID: 20160303030253

Steps to reproduce:

1. Start Firefox 45 or later version on Windows 10
2. Shows Forward button
3. Compare forward icon with Toolbar.png


Actual results:

Forward-icon is compressed.

Regression range:
https://hg.mozilla.org/integration/fx-team/pushloghtml?fromchange=f26c050a39a17cf1f4a88d8cde00916abb3763b1&tochange=92cb9839960be04e1c31633dd0efc289d2a500a1


Expected results:

Forward-icon is not compressed.
(Reporter)

Updated

2 years ago
Has Regression Range: --- → yes
Has STR: --- → yes
status-firefox44: --- → unaffected
status-firefox45: --- → affected
status-firefox46: --- → affected
status-firefox47: --- → affected
Component: Untriaged → Theme
OS: Unspecified → Windows 10
Hardware: Unspecified → All
(Reporter)

Updated

2 years ago
Flags: needinfo?(gijskruitbosch+bugs)
(Assignee)

Updated

2 years ago
Blocks: 1222747
Flags: needinfo?(gijskruitbosch+bugs)
(Assignee)

Comment 1

2 years ago
Created attachment 8726663 [details] [diff] [review]
fix forwardbutton width on win10 where padding is different because of a different backbutton url bar overlap,

MozReview-Commit-ID: 4I6mC0OnQlF
Attachment #8726663 - Flags: review?(dao)
(Assignee)

Updated

2 years ago
Assignee: nobody → gijskruitbosch+bugs
Status: NEW → ASSIGNED
(Assignee)

Comment 2

2 years ago
Sylvestre, what are our options here for 45? From the calendar it seems this was reported too late to go into 45, which is a pain because this is a primary UI regression for windows 10 that we would then ship for the next year as part of ESR. :-\
Flags: needinfo?(sledru)
Comment on attachment 8726663 [details] [diff] [review]
fix forwardbutton width on win10 where padding is different because of a different backbutton url bar overlap,

This would have just worked if https://hg.mozilla.org/integration/fx-team/rev/897610807fb5 hadn't included --backbutton-urlbar-overlap in --forwardbutton-width. I think we should make it so at least on Windows.
Attachment #8726663 - Flags: review?(dao) → review-
(Assignee)

Comment 4

2 years ago
(In reply to Dão Gottwald [:dao] from comment #3)
> Comment on attachment 8726663 [details] [diff] [review]
> fix forwardbutton width on win10 where padding is different because of a
> different backbutton url bar overlap,
> 
> This would have just worked if
> https://hg.mozilla.org/integration/fx-team/rev/897610807fb5 hadn't included
> --backbutton-urlbar-overlap in --forwardbutton-width. I think we should make
> it so at least on Windows.

I don't think a change like that is upliftable to 45 at this point - it's too risky in case we're missing something else like we did in the blocking chain of this bug. Would you be OK with the attached patch going into 45?
Flags: needinfo?(dao)
It's basically what we did before bug 1222747. I think it's actually less risky than keeping the current setup.
Flags: needinfo?(dao)
(Assignee)

Comment 6

2 years ago
(In reply to Dão Gottwald [:dao] from comment #5)
> It's basically what we did before bug 1222747.

That doesn't really make sense to me; we had a %define before that bug and its width was ~32px and therefore included the overlap. What am I missing?

> I think it's actually less
> risky than keeping the current setup.

I disagree, but I guess another option is just backing out 1222747 on beta/release, as it isn't like devedition is generally usable there.
Flags: needinfo?(dao)
(Assignee)

Comment 7

2 years ago
(In reply to :Gijs Kruitbosch from comment #6)
> (In reply to Dão Gottwald [:dao] from comment #5)
> > It's basically what we did before bug 1222747.
> 
> That doesn't really make sense to me; we had a %define before that bug and
> its width was ~32px and therefore included the overlap. What am I missing?

Oh, so the define behaved differently on Windows compared to everywhere else, is what I'm missing. Sigh. I'm not sure that's better. Leaving needinfo for the backout question.
If it can be backed out cleanly from beta and release, that sounds like a fine plan.
Flags: needinfo?(dao)
This is too late for 45 but we can take a patch in the first 45 esr.
Flags: needinfo?(sledru)
(Assignee)

Comment 10

2 years ago
Created attachment 8726739 [details] [diff] [review]
change forwardbutton-width to stop including the backbutton-urlbar-overlap so as to fix the forward button apperance on the default theme in Windows 10,

MozReview-Commit-ID: 4I6mC0OnQlF
(Assignee)

Updated

2 years ago
Attachment #8726663 - Attachment is obsolete: true
(Assignee)

Comment 11

2 years ago
Created attachment 8726740 [details] [diff] [review]
change forwardbutton-width to stop including the backbutton-urlbar-overlap so as to fix the forward button apperance on the default theme in Windows 10,

MozReview-Commit-ID: 4I6mC0OnQlF
Attachment #8726740 - Flags: review?(dao)
(Assignee)

Updated

2 years ago
Attachment #8726739 - Attachment is obsolete: true
(Assignee)

Comment 12

2 years ago
Comment on attachment 8726740 [details] [diff] [review]
change forwardbutton-width to stop including the backbutton-urlbar-overlap so as to fix the forward button apperance on the default theme in Windows 10,

Review of attachment 8726740 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/themes/osx/devedition.css
@@ +4,5 @@
>  
>  %include ../shared/devedition.inc.css
>  
> +:root {
> +  --forwardbutton-width: 32px;

The Windows and Linux devedition themes already have this set to 29px specifically (they have 2px less padding than the default theme, meaning they get 29 rather than 31 as the total width). On OS X the default value of 32 used to be OK for the devedition theme, but now that we're reducing it because it no longer includes the back button overlap, it needs updating.
Comment on attachment 8726740 [details] [diff] [review]
change forwardbutton-width to stop including the backbutton-urlbar-overlap so as to fix the forward button apperance on the default theme in Windows 10,

(In reply to :Gijs Kruitbosch from comment #12)
> ::: browser/themes/osx/devedition.css
> @@ +4,5 @@
> >  
> >  %include ../shared/devedition.inc.css
> >  
> > +:root {
> > +  --forwardbutton-width: 32px;
> 
> The Windows and Linux devedition themes already have this set to 29px
> specifically (they have 2px less padding than the default theme, meaning
> they get 29 rather than 31 as the total width). On OS X the default value of
> 32 used to be OK for the devedition theme, but now that we're reducing it
> because it no longer includes the back button overlap, it needs updating.

Why aren't these numbers the same across platforms for devedition? Shouldn't this just be set to 29px in shared/devedition.inc.css?
Attachment #8726740 - Flags: review?(dao) → review+
(Assignee)

Comment 14

2 years ago
(In reply to Dão Gottwald [:dao] from comment #13)
> Comment on attachment 8726740 [details] [diff] [review]
> change forwardbutton-width to stop including the backbutton-urlbar-overlap
> so as to fix the forward button apperance on the default theme in Windows 10,
> 
> (In reply to :Gijs Kruitbosch from comment #12)
> > ::: browser/themes/osx/devedition.css
> > @@ +4,5 @@
> > >  
> > >  %include ../shared/devedition.inc.css
> > >  
> > > +:root {
> > > +  --forwardbutton-width: 32px;
> > 
> > The Windows and Linux devedition themes already have this set to 29px
> > specifically (they have 2px less padding than the default theme, meaning
> > they get 29 rather than 31 as the total width). On OS X the default value of
> > 32 used to be OK for the devedition theme, but now that we're reducing it
> > because it no longer includes the back button overlap, it needs updating.
> 
> Why aren't these numbers the same across platforms for devedition? Shouldn't
> this just be set to 29px in shared/devedition.inc.css?

Padding is different on different OSes, as is the size of the back button. I expect that all it's currently doing is making the back and forward buttons match in terms of width. I agree the width should be unified across platforms, but I don't want to do that in this bug. I'll file a followup.
(Assignee)

Updated

2 years ago
Depends on: 1253760

Comment 16

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/2d7b6a0a54d2
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox47: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 47
(Assignee)

Comment 17

2 years ago
Comment on attachment 8726740 [details] [diff] [review]
change forwardbutton-width to stop including the backbutton-urlbar-overlap so as to fix the forward button apperance on the default theme in Windows 10,

Approval Request Comment
[Feature/regressing bug #]: bug 1222747
[User impact if declined]: forward button looks busted on windows 10 default theme
[Describe test coverage new/current, TreeHerder]: no, appearance only
[Risks and why]: medium-low. It's super-early-beta right now, so I think that there's enough time to figure out issues with the patch. It's also baked on Nightly for a while now with no regressions reported that I know of.
[String/UUID change made/needed]: no.
Attachment #8726740 - Flags: approval-mozilla-beta?
(Assignee)

Comment 18

2 years ago
Created attachment 8728987 [details] [diff] [review]
Patch for ESR45
(Assignee)

Updated

2 years ago
Attachment #8726740 - Attachment is obsolete: true
Attachment #8726740 - Flags: approval-mozilla-beta?
(Assignee)

Comment 19

2 years ago
Comment on attachment 8726740 [details] [diff] [review]
change forwardbutton-width to stop including the backbutton-urlbar-overlap so as to fix the forward button apperance on the default theme in Windows 10,

Ugh, thanks bzexport.

Approval Request Comment
[Feature/regressing bug #]: bug 1222747
[User impact if declined]: forward button looks busted on windows 10 default theme
[Describe test coverage new/current, TreeHerder]: no, appearance only
[Risks and why]: medium-low. It's super-early-beta right now, so I think that there's enough time to figure out issues with the patch. It's also baked on Nightly for a while now with no regressions reported that I know of.
[String/UUID change made/needed]: no.
Attachment #8726740 - Attachment is obsolete: false
Attachment #8726740 - Flags: approval-mozilla-beta?
(Assignee)

Comment 20

2 years ago
Comment on attachment 8728987 [details] [diff] [review]
Patch for ESR45

[Approval Request Comment]
If this is not a sec:{high,crit} bug, please state case for ESR consideration: visible primary UI regression on Windows 10, which especially on something like ESR will only grow in share as the ESR cycle lengthens, and if we don't correct now we'll have to live with that for a year
User impact if declined: wonky forward button on win10 default Firefox theme;
Fix Landed on Version: 47, but this backout hasn't landed anywhere else;
Risk to taking this patch (and alternatives if risky): this is a backout of something that was uplifted to 45, so in that sense it's pretty safe. We'd regress the devedition theme, but that's not available on release/esr anyway.
String or UUID changes made by this patch: no.

See https://wiki.mozilla.org/Release_Management/ESR_Landing_Process for more info.
Attachment #8728987 - Attachment description: backout changesets 5031dae2d2af and 4a1745d4c58a (bug 1222747) for causing default theme bustage on win10 → Patch for ESR45
Attachment #8728987 - Attachment filename: . → esr45-win10-fix
Attachment #8728987 - Flags: approval-mozilla-esr45?
Comment on attachment 8726740 [details] [diff] [review]
change forwardbutton-width to stop including the backbutton-urlbar-overlap so as to fix the forward button apperance on the default theme in Windows 10,

Icon fix so our UI looks right on win 10. 
This should make it into beta 2.
Attachment #8726740 - Flags: approval-mozilla-beta? → approval-mozilla-beta+

Comment 22

2 years ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-beta/rev/4590bb590728
status-firefox46: affected → fixed
Flags: qe-verify+

Comment 23

2 years ago
[bugday-20160323]

Status: RESOLVED,FIXED -> VERIFIED

Comments:
Test not Successful

Component: 
Name			Firefox
Version			46.0b9
Build ID		20160322075646
Update Channel          beta
User Agent		Mozilla/5.0 (Windows NT 6.1; WOW64; rv:46.0) Gecko/20100101 Firefox/46.0
OS			Windows 7 SP1 x86_64

Expected Results: 
No forward button is still smaller than backward button

Actual Results: 
As expected
Reproduced the initial issue on Firefox 45.0.2.

Verified as fixed on Windows 10 x64 using:
- latest 47.0a2 Aurora, build ID: 20160410004056;
- Fx 46.0b9, build ID:  20160407053945.
Status: RESOLVED → VERIFIED
status-firefox46: fixed → verified
status-firefox47: fixed → verified
Flags: qe-verify+
QA Contact: cornel.ionce
status-firefox45: affected → wontfix
status-firefox-esr45: --- → affected
Comment on attachment 8728987 [details] [diff] [review]
Patch for ESR45

Polish it, taking it.
Should be in 45.1.0
Attachment #8728987 - Flags: approval-mozilla-esr45? → approval-mozilla-esr45+

Comment 26

2 years ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-esr45/rev/c41e2bae3678
status-firefox-esr45: affected → fixed
Setting the flag back for verification on 45.1.0esr.
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.