Closed Bug 998157 Opened 6 years ago Closed 6 years ago

Forward button is offset by one pixel across Windows versions

Categories

(Firefox :: Theme, defect)

29 Branch
All
Windows 8.1
defect
Not set
Points:
5

Tracking

()

VERIFIED FIXED
Firefox 32
Iteration:
33.3
Tracking Status
firefox31 + verified
firefox32 --- verified

People

(Reporter: brandon.cheng, Assigned: mikedeboer)

References

Details

(Whiteboard: [Australis:P3+] [qa-])

Attachments

(4 files, 2 obsolete files)

Attached image Forward button position comparison (obsolete) —
User Agent: Mozilla/5.0 (Windows NT 6.3; WOW64; rv:29.0) Gecko/20100101 Firefox/29.0 (Beta/Release)
Build ID: 20140414143035

Steps to reproduce:

I believe this problem started occurring after my patch in bug 8393132 was pushed.


Actual results:

The forward button is offset by one pixel to the left on Windows versions below 8.

This causes two issues.
1. The urlbar to bleed into the back button and create a white arc on the right side of it.
2. The measurable width of the forward button to become 23px, when it should be 24px.


Expected results:

The forward button needs to be moved 1px to the right on Windows XP, 7, and probably Vista.
This bug has me very confused, and beyond the weird behavior on different Windows versions. I haven't done a full recompile, but the position offset still happens when the left margin and padding is set to the values it was before bug 893661 was pushed. I don't think bug 893661 directly caused this issue, but exposed it. (Don't take my word for it though.)

I apologize for not testing the original bug 893661 fully across all the Windows platforms and reporting this finding so close to Australis release.

I'm trying to find more on the cause, but this is all I know for now. If someone more experienced can look into this, thank you.
Attachment #8408707 - Attachment is obsolete: true
Status: UNCONFIRMED → NEW
Ever confirmed: true
Hardware: x86_64 → All
Whiteboard: [Australis:P3+]
Blocks: 893661
Depends on: 997131
Component: Untriaged → Theme
Attached patch Patch v1.patch (obsolete) — Splinter Review
Explanation for patch.

The back button is bigger on Windows 8 from the changes in bug 960517 (part 7.5). As a result, the Windows 8 back button pushes all elements to the right of it 1px more than below Windows versions. A condition was added for Windows versions below 8 to the forward button to have one less negative left margin to make this equal out.

When I added my changes in bug 893661, I changed left margin on the forward button but didn't change the below-8 condition, resulting in the discrepancy.

This patch reverts the left margin back to -7px and adds 1px of left padding. I've tested this on all Windows 8, 7, and XP.

For reference, this is the style condition I'm referring to: http://hg.mozilla.org/mozilla-central/file/c55dfb01a027/browser/themes/windows/browser.css#l895
Attachment #8408984 - Flags: review?(jaws)
(In reply to Brandon Cheng from comment #4)
> The back button is bigger on Windows 8 from the changes in bug 960517 (part
> 7.5). As a result, the Windows 8 back button pushes all elements to the
> right of it 1px more than below Windows versions. A condition was added for
> Windows versions below 8 to the forward button to have one less negative
> left margin to make this equal out.

This kind of inconsistency is asking for trouble. We shouldn't do this. We should make it so that the back button has a consistent size across Windows versions. Can we make this be the fix for this bug?

> For reference, this is the style condition I'm referring to:
> http://hg.mozilla.org/mozilla-central/file/c55dfb01a027/browser/themes/
> windows/browser.css#l895

I'm removing this in bug 997131.
Comment on attachment 8408984 [details] [diff] [review]
Patch v1.patch

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

Yeah, we should fix this inconsistency.
Attachment #8408984 - Flags: review?(jaws)
(In reply to Dão Gottwald [:dao] from comment #5)
> (In reply to Brandon Cheng from comment #4)
> > The back button is bigger on Windows 8 from the changes in bug 960517 (part
> > 7.5). As a result, the Windows 8 back button pushes all elements to the
> > right of it 1px more than below Windows versions. A condition was added for
> > Windows versions below 8 to the forward button to have one less negative
> > left margin to make this equal out.
> 
> This kind of inconsistency is asking for trouble. We shouldn't do this. We
> should make it so that the back button has a consistent size across Windows
> versions. Can we make this be the fix for this bug?
> 
> > For reference, this is the style condition I'm referring to:
> > http://hg.mozilla.org/mozilla-central/file/c55dfb01a027/browser/themes/
> > windows/browser.css#l895
> 
> I'm removing this in bug 997131.

Awesome. Even better.
Flags: firefox-backlog+
Brandon, since bug 997131 landed on m-c, this should be resolved. How does it look for you?
Status: NEW → RESOLVED
Closed: 6 years ago
Flags: needinfo?(bcheng.gt)
Resolution: --- → FIXED
Assignee: nobody → dao
Target Milestone: --- → Firefox 31
(In reply to Mike de Boer [:mikedeboer] from comment #8)
> Brandon, since bug 997131 landed on m-c, this should be resolved. How does
> it look for you?

Bug 997131 didn't change the back button size. It fixed this issue on Windows 7 but created it on Windows 8.
Status: RESOLVED → REOPENED
Flags: needinfo?(bcheng.gt)
Resolution: FIXED → ---
The back button size / layout still needs to be made consistent across Windows versions (i.e. Win 8 should probably just follow the others).
Assignee: dao → nobody
Status: REOPENED → NEW
Blocks: 960517
Brandon, thanks for looking at it again - super helpful!

I *think* I should be able to tackle this.
Assignee: nobody → mdeboer
Status: NEW → ASSIGNED
We should make sure this gets fixed for 31, as the original bug landed on that branch.
Dão, the back-button is the same size on all Windows versions atm (32x32), which is good.

I'll fix up Win8 margins for the conditional forward button.
(In reply to Mike de Boer [:mikedeboer] from comment #14)
> Dão, the back-button is the same size on all Windows versions atm (32x32),
> which is good.
> 
> I'll fix up Win8 margins for the conditional forward button.

s/fix up/remove/? Why specify Win8-specific margins in the first place?
(In reply to Dão Gottwald [:dao] from comment #15)
> s/fix up/remove/? Why specify Win8-specific margins in the first place?

ATM, there's no rule specifying a Win8-specific margin. The different offset is caused by the back button using a 1px border, whereas on other Windows versions it uses box-shadow, instead of a border.

I'll upload a patch soon using border-box to draw the back-button outline.
Attachment #8408984 - Attachment is obsolete: true
Attachment #8418746 - Flags: review?(dao)
(In reply to Mike de Boer [:mikedeboer] from comment #17)
> Created attachment 8418746 [details] [diff] [review]
> Patch v2: use box-shadow instead of border

Was taking advantage of the box-sizing CSS property considered?
(In reply to Brandon Cheng from comment #18)
> Was taking advantage of the box-sizing CSS property considered?

It's already border-box and changing it to padding-box didn't yield any positive effect for me.
In the end I didn't see an advantage pursuing that direction further as the patch I posted fixes the issue and unifies back-button styling with < Win8 styles.
Comment on attachment 8418746 [details] [diff] [review]
Patch v2: use box-shadow instead of border

>--- a/browser/themes/windows/browser.css
>+++ b/browser/themes/windows/browser.css
>@@ -929,44 +929,45 @@ toolbarbutton[sdk-button="true"][cui-are
>   margin-top: -1px !important;
> }
> 
> #back-button > .toolbarbutton-icon {
>   border-radius: 10000px !important;
>   background-clip: padding-box !important;
>   background-color: hsla(210,25%,98%,.08) !important;
>   padding: 6px !important;
>-  border-color: hsla(210,4%,10%,.25) !important;
>-  transition-property: background-color, border-color !important;
>+  border: none !important;

nit: border-style: none
Attachment #8418746 - Flags: review?(dao) → review+
Thanks!

Pushed to fx-team as: https://hg.mozilla.org/integration/fx-team/rev/3930dd3b0bfc
Whiteboard: [Australis:P3+] → [Australis:P3+][fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/3930dd3b0bfc
Status: ASSIGNED → RESOLVED
Closed: 6 years ago6 years ago
Resolution: --- → FIXED
Whiteboard: [Australis:P3+][fixed-in-fx-team] → [Australis:P3+]
Mike, I think we want that in beta too. Can we get an uplift request? Thanks
Flags: needinfo?(mdeboer)
Comment on attachment 8418746 [details] [diff] [review]
Patch v2: use box-shadow instead of border

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Australis
User impact if declined: The forward button position is influenced by the styles used by the back button on Windows 8. This patch fixes that discrepancy and establishes a consistent look across Windows versions.
Testing completed (on m-c, etc.): baked on m-c for a month.
Risk to taking this patch (and alternatives if risky): minor.
String or IDL/UUID changes made by this patch: n/a.
Attachment #8418746 - Flags: approval-mozilla-beta?
Flags: needinfo?(mdeboer)
Attachment #8418746 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Keywords: verifyme
Depends on: 1029652
Verified fixed on Windows 7 64bit, Windows 8.1 64bit and Windows XP 32bit using Firefox 31.0b7 and latest Aurora 32.0a2 (buildID: 20140706004001).
Status: RESOLVED → VERIFIED
Keywords: verifyme
Hi Mike, can you provide a point value for this bug.
Iteration: --- → 33.2
QA Whiteboard: [qa+]
Flags: needinfo?(mdeboer)
Iteration: 33.2 → 33.3
Points: --- → 5
Flags: needinfo?(mdeboer)
Whiteboard: [Australis:P3+] → [Australis:P3+] [qa-]
QA Whiteboard: [qa+] → [qa!]
You need to log in before you can comment on or make changes to this bug.