Brandon Cheng Reporter
	Description • 10 years ago

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.

	Brandon Cheng Reporter
	Comment 1 • 10 years ago

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.

	Brandon Cheng Reporter
	Comment 4 • 10 years ago

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

	Dão Gottwald [:dao]
	Comment 5 • 10 years ago

(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.

	Jared Wein [:jaws] (please needinfo? me)
	Comment 6 • 10 years ago

Comment on attachment 8408984 [details] [diff] [review]
Patch v1.patch

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

Yeah, we should fix this inconsistency.

	Brandon Cheng Reporter
	Comment 7 • 10 years ago

(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.

	Mike de Boer [:mikedeboer] Assignee
	Comment 8 • 10 years ago

Brandon, since bug 997131 landed on m-c, this should be resolved. How does it look for you?

	Brandon Cheng Reporter
	Comment 9 • 10 years ago

(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.

	Dão Gottwald [:dao]
	Comment 11 • 10 years ago

The back button size / layout still needs to be made consistent across Windows versions (i.e. Win 8 should probably just follow the others).

	Mike de Boer [:mikedeboer] Assignee
	Comment 12 • 10 years ago

Brandon, thanks for looking at it again - super helpful!

I *think* I should be able to tackle this.

	:Gijs (he/him)
	Comment 13 • 10 years ago

We should make sure this gets fixed for 31, as the original bug landed on that branch.

	Mike de Boer [:mikedeboer] Assignee
	Comment 14 • 10 years ago

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.

	Dão Gottwald [:dao]
	Comment 15 • 10 years ago

(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?

	Mike de Boer [:mikedeboer] Assignee
	Comment 16 • 10 years ago

(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.

	Brandon Cheng Reporter
	Comment 18 • 10 years ago

(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?

	Mike de Boer [:mikedeboer] Assignee
	Comment 19 • 10 years ago

(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.

	Dão Gottwald [:dao]
	Comment 20 • 10 years ago

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

	Mike de Boer [:mikedeboer] Assignee
	Comment 21 • 10 years ago

Thanks!

Pushed to fx-team as: https://hg.mozilla.org/integration/fx-team/rev/3930dd3b0bfc

	Ryan VanderMeulen [:RyanVM]
	Comment 22 • 10 years ago

https://hg.mozilla.org/mozilla-central/rev/3930dd3b0bfc

	Sylvestre Ledru [:Sylvestre]
	Comment 23 • 10 years ago

Mike, I think we want that in beta too. Can we get an uplift request? Thanks

	Mike de Boer [:mikedeboer] Assignee
	Comment 24 • 10 years ago

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.

	Ryan VanderMeulen [:RyanVM]
	Comment 25 • 10 years ago

https://hg.mozilla.org/releases/mozilla-beta/rev/5b08134446f4

	Camelia Badau [:cbadau], Desktop QA
	Comment 26 • 10 years ago

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).

	Marco Mucci [:MarcoM]
	Comment 27 • 10 years ago

Hi Mike, can you provide a point value for this bug.