-> P3, and a candidate for being in a UI polish clusterbug.

Looks like there's actually no gradient in the back button's background.

(I won't be able to review this until I get back to my regular windows machine, so that'll be Monday. Feel free to forward if that is too slow. Sorry!)

Monday is fine, thanks.

Comment on attachment 8669370 [details] [diff] [review] patch v2 Review of attachment 8669370 [details] [diff] [review]: ----------------------------------------------------------------- This makes the back button border color on win8 (no lwt) seem lighter (I can see it without comparing pixels) than the urlbar border. It's also just objectively different when I do compare the pixels. That seems wrong. On win10, the border color looks better but there is overlap between the back button border and the forward/urlbar border, and because both are semi-transparent it looks like darker-grey blobs at the intersection of the two lines. Making the overlap 8px instead of 9px works better in my testing. Also on win10, when there is no back history but there *is* forward history, and you're not hovering/focusing the location bar, the forward button's border is very dark in comparison with the other (very faint) borders - but its border with the back button (which is the back button's border, really) is 'open' because it is so faint, which looks broken. I think in this case the forward button shouldn't have such a strong border. Finally, on win10 with a dark (brighttext) lightweight theme, the forward button is too large compared to the location bar, and I can't see the borders at all. ::: browser/themes/windows/browser.css @@ +938,3 @@ > padding-right: 3px !important; > +% icon width + border + horizontal padding without --backbutton-urlbar-overlap > +%define forwardButtonWidth 25 This should have a unit (px, presumably). But why not make this a CSS variable? Also, this remains constant even though the overlap variable increases on win10, while I expect the total width of the button is constant between pre-win10 and win10. That doesn't make sense, so is my assumption wrong, or am I misreading the CSS?

(I've not looked at Linux)

(In reply to :Gijs Kruitbosch from comment #9) > Comment on attachment 8669370 [details] [diff] [review] > patch v2 > > Review of attachment 8669370 [details] [diff] [review]: > ----------------------------------------------------------------- > > This makes the back button border color on win8 (no lwt) seem lighter (I can > see it without comparing pixels) than the urlbar border. It's also just > objectively different when I do compare the pixels. That seems wrong. Why does that seem wrong? The border colors aren't supposed to match. On Windows 10 the difference is even stronger. > On win10, the border color looks better but there is overlap between the > back button border and the forward/urlbar border, and because both are > semi-transparent it looks like darker-grey blobs at the intersection of the > two lines. Making the overlap 8px instead of 9px works better in my testing. IIRC 8px is worse because the location bar border doesn't reach the back button border, leaving a gap and the location bar corners visible. > Also on win10, when there is no back history but there *is* forward history, > and you're not hovering/focusing the location bar, the forward button's > border is very dark in comparison with the other (very faint) borders - but > its border with the back button (which is the back button's border, really) > is 'open' because it is so faint, which looks broken. I think in this case > the forward button shouldn't have such a strong border. Not sure what you're proposing here. That we set a different border color just for this specific case? Seems rather odd. Looks like the mockups didn't cover this case, how about we file a separate bug on it? Don't want this to be held up further with a UX feedback cycle for what seems like a minor issue. > Finally, on win10 with a dark (brighttext) lightweight theme, the forward > button is too large compared to the location bar, and I can't see the > borders at all. It's probably the other way around, you can't see the border and therefore the button seems too large. I'll have a look, but this is already broken in the non-lwt case (bug 1206087) and the patch fixes that, so I'm inclined to spin this off to a new bug as well. > ::: browser/themes/windows/browser.css > @@ +938,3 @@ > > padding-right: 3px !important; > > +% icon width + border + horizontal padding without --backbutton-urlbar-overlap > > +%define forwardButtonWidth 25 > > This should have a unit (px, presumably). As with conditionalForwardWithUrlbarWidth (which I just renamed and moved to a place with better contetxt), the unit gets added where the define is used. > But why not make this a CSS variable? Why would that be better? > Also, this remains constant even though the overlap variable increases on > win10, while I expect the total width of the button is constant between > pre-win10 and win10. That doesn't make sense, so is my assumption wrong, or > am I misreading the CSS? This is the button width without the overlap. As you note it remains constant. The overlap gets added to that and isn't constant.

(In reply to Dão Gottwald [:dao] from comment #11) > (In reply to :Gijs Kruitbosch from comment #9) > > Comment on attachment 8669370 [details] [diff] [review] > > patch v2 > > > > Review of attachment 8669370 [details] [diff] [review]: > > ----------------------------------------------------------------- > > > > This makes the back button border color on win8 (no lwt) seem lighter (I can > > see it without comparing pixels) than the urlbar border. It's also just > > objectively different when I do compare the pixels. That seems wrong. > > Why does that seem wrong? The border colors aren't supposed to match. The border colors of the location bar and buttons match on OSX and Win8 and Win10 today. Your patch changes that, and it looks worse to me. Where is the design that stipulated this? > On Windows 10 the difference is even stronger. Er, when I apply your patch, on win10 the border-color is continuous when hovered, and I see no difference in color at all. What is the difference supposed to be? > > On win10, the border color looks better but there is overlap between the > > back button border and the forward/urlbar border, and because both are > > semi-transparent it looks like darker-grey blobs at the intersection of the > > two lines. Making the overlap 8px instead of 9px works better in my testing. > > IIRC 8px is worse because the location bar border doesn't reach the back > button border, leaving a gap and the location bar corners visible. That's not what happens for me when I change the property to be 8px instead of 9. > > Also on win10, when there is no back history but there *is* forward history, > > and you're not hovering/focusing the location bar, the forward button's > > border is very dark in comparison with the other (very faint) borders - but > > its border with the back button (which is the back button's border, really) > > is 'open' because it is so faint, which looks broken. I think in this case > > the forward button shouldn't have such a strong border. > > Not sure what you're proposing here. That we set a different border color > just for this specific case? It seems to me like the forward button should use the same border-color CSS as the location bar if the back button is disabled, yes. > Seems rather odd. Looks like the mockups didn't > cover this case, how about we file a separate bug on it? Don't want this to > be held up further with a UX feedback cycle for what seems like a minor > issue. This would be a regression compared to the current state of things. This bug was originally just about the back button, and you morphed it to also be about the forward button, without explanation, and duped two other issues to it that were not originally duplicates. I'm surprised that you then say you don't want to fix this problem, while you're introducing it in this bug. > > Finally, on win10 with a dark (brighttext) lightweight theme, the forward > > button is too large compared to the location bar, and I can't see the > > borders at all. > > It's probably the other way around, you can't see the border and therefore > the button seems too large. I'll have a look, but this is already broken in > the non-lwt case (bug 1206087) and the patch fixes that, so I'm inclined to > spin this off to a new bug as well. Well no, the size looks fine when not hovered, without this patch. bug 1206087 was about the lack of distinction between the background color and the border on hover, which could have been fixed by just changing the background or border color for the non-lwt case. Your patch changes a lot more. > > ::: browser/themes/windows/browser.css > > @@ +938,3 @@ > > > padding-right: 3px !important; > > > +% icon width + border + horizontal padding without --backbutton-urlbar-overlap > > > +%define forwardButtonWidth 25 > > > > This should have a unit (px, presumably). > > As with conditionalForwardWithUrlbarWidth (which I just renamed and moved to > a place with better contetxt), the unit gets added where the define is used. > > > But why not make this a CSS variable? > > Why would that be better? Because the preprocessing wastes build time, breaks syntax highlighting and linting tools, and we should get rid of it now that there's a good, standards-compliant substitute (ie CSS variables). > > Also, this remains constant even though the overlap variable increases on > > win10, while I expect the total width of the button is constant between > > pre-win10 and win10. That doesn't make sense, so is my assumption wrong, or > > am I misreading the CSS? > > This is the button width without the overlap. As you note it remains > constant. The overlap gets added to that and isn't constant. I don't understand. Let me rephrase the question: is the total width of the button (incl. overlap) supposed to be different between win8 and win10? Why? It doesn't make sense to me that it is different, and therefore the fact that the code currently makes it different (because of the different overlap which gets added) is surprising to me. If the overlap really needs to be different (which I'm not sure about, see above) then this define/variable shouldn't be constant.

(In reply to :Gijs Kruitbosch from comment #12) > (In reply to Dão Gottwald [:dao] from comment #11) > > (In reply to :Gijs Kruitbosch from comment #9) > > > Comment on attachment 8669370 [details] [diff] [review] > > > patch v2 > > > > > > Review of attachment 8669370 [details] [diff] [review]: > > > ----------------------------------------------------------------- > > > > > > This makes the back button border color on win8 (no lwt) seem lighter (I can > > > see it without comparing pixels) than the urlbar border. It's also just > > > objectively different when I do compare the pixels. That seems wrong. > > > > Why does that seem wrong? The border colors aren't supposed to match. > > The border colors of the location bar and buttons match on OSX and Win8 and > Win10 today. Your patch changes that, and it looks worse to me. Where is the > design that stipulated this? http://people.mozilla.org/~shorlander/mockups/Windows-10/Windows-10.html > > On Windows 10 the difference is even stronger. > > Er, when I apply your patch, on win10 the border-color is continuous when > hovered, and I see no difference in color at all. What is the difference > supposed to be? You're saying "when hovered" now, which is quite different. So you're saying on hover the buttons should use the location bar border color instead of the toolbarbutton-1 border color? > > IIRC 8px is worse because the location bar border doesn't reach the back > > button border, leaving a gap and the location bar corners visible. > > That's not what happens for me when I change the property to be 8px instead > of 9. Will check again. > > > Also on win10, when there is no back history but there *is* forward history, > > > and you're not hovering/focusing the location bar, the forward button's > > > border is very dark in comparison with the other (very faint) borders - but > > > its border with the back button (which is the back button's border, really) > > > is 'open' because it is so faint, which looks broken. I think in this case > > > the forward button shouldn't have such a strong border. > > > > Not sure what you're proposing here. That we set a different border color > > just for this specific case? > > It seems to me like the forward button should use the same border-color CSS > as the location bar if the back button is disabled, yes. This doesn't make sense to me. The forward button's border color shouldn't change depending on the back button's disabled state. > > Seems rather odd. Looks like the mockups didn't > > cover this case, how about we file a separate bug on it? Don't want this to > > be held up further with a UX feedback cycle for what seems like a minor > > issue. > > This would be a regression compared to the current state of things. This bug > was originally just about the back button, and you morphed it to also be > about the forward button, without explanation, and duped two other issues to > it that were not originally duplicates. The explanation is that the back and forward buttons and their different states are related. Implicitly this bug has always been about the forward button as well. Furthermore, the current styling is such a mess to the point where seemingly innocent changes like "make this a two px border" give you headaches, a refactoring is now overdue. > I'm surprised that you then say you > don't want to fix this problem, while you're introducing it in this bug. Not what I said. I don't want to fix it in this patch. > > > Finally, on win10 with a dark (brighttext) lightweight theme, the forward > > > button is too large compared to the location bar, and I can't see the > > > borders at all. > > > > It's probably the other way around, you can't see the border and therefore > > the button seems too large. I'll have a look, but this is already broken in > > the non-lwt case (bug 1206087) and the patch fixes that, so I'm inclined to > > spin this off to a new bug as well. > > Well no, the size looks fine when not hovered, without this patch. bug > 1206087 was about the lack of distinction between the background color and > the border on hover, which could have been fixed by just changing the > background or border color for the non-lwt case. Your patch changes a lot > more. I can double-check, but the patch shouldn't be changing the height. The forward button and the location bar are in the same container with -moz-box-aling:stretch, so I'm not even sure how I'd make the forward button taller even if I wanted to. > > > This should have a unit (px, presumably). > > > > As with conditionalForwardWithUrlbarWidth (which I just renamed and moved to > > a place with better contetxt), the unit gets added where the define is used. > > > > > But why not make this a CSS variable? > > > > Why would that be better? > > Because the preprocessing wastes build time, breaks syntax highlighting and > linting tools, and we should get rid of it now that there's a good, > standards-compliant substitute (ie CSS variables). It doesn't waste build time, it uses it. CSS variables cost runtime overhead which I think is the better thing to optimize for here. > > > Also, this remains constant even though the overlap variable increases on > > > win10, while I expect the total width of the button is constant between > > > pre-win10 and win10. That doesn't make sense, so is my assumption wrong, or > > > am I misreading the CSS? > > > > This is the button width without the overlap. As you note it remains > > constant. The overlap gets added to that and isn't constant. > > I don't understand. Let me rephrase the question: is the total width of the > button (incl. overlap) supposed to be different between win8 and win10? Why? Yes, because the location bar is taller. > It doesn't make sense to me that it is different, and therefore the fact > that the code currently makes it different (because of the different overlap > which gets added) is surprising to me. If the overlap really needs to be > different (which I'm not sure about, see above) then this define/variable > shouldn't be constant. I don't follow. The define does not include the overlap. The thing it defines is and should be constant.

(In reply to Dão Gottwald [:dao] from comment #13) > > > IIRC 8px is worse because the location bar border doesn't reach the back > > > button border, leaving a gap and the location bar corners visible. > > > > That's not what happens for me when I change the property to be 8px instead > > of 9. > > Will check again. I still see it. It's more visible when using a dark theme. > > > > Finally, on win10 with a dark (brighttext) lightweight theme, the forward > > > > button is too large compared to the location bar, and I can't see the > > > > borders at all. > > > > > > It's probably the other way around, you can't see the border and therefore > > > the button seems too large. I'll have a look, but this is already broken in > > > the non-lwt case (bug 1206087) and the patch fixes that, so I'm inclined to > > > spin this off to a new bug as well. > > > > Well no, the size looks fine when not hovered, without this patch. bug > > 1206087 was about the lack of distinction between the background color and > > the border on hover, which could have been fixed by just changing the > > background or border color for the non-lwt case. Your patch changes a lot > > more. > > I can double-check, but the patch shouldn't be changing the height. The > forward button and the location bar are in the same container with > -moz-box-aling:stretch, so I'm not even sure how I'd make the forward button > taller even if I wanted to. The size is as expected. It's like I said, it appears visually taller because the border fades into the background. I'm working on a patch using the location bar's hover border color for the back / forward buttons, which should address that issue too.

Now using the location bar's hover border color. That color doesn't integrate as well with the toolbarbutton-1 colors set for #nav-bar[brighttext], so I removed those. Note that I improvised those colors anyway, they weren't part of the mockups.

forgot to remove the navbarTextboxCustomBorder define

... aaand I forgot to use -moz-windows-default-theme for the border color definitions :/

(In reply to Dão Gottwald [:dao] from comment #15) > Created attachment 8670176 [details] [diff] [review] > patch v3 > > Now using the location bar's hover border color. That color doesn't > integrate as well with the toolbarbutton-1 colors set for > #nav-bar[brighttext], so I removed those. Note that I improvised those > colors anyway, they weren't part of the mockups. OK. Can we still file a followup bug to get slightly more visible things for brighttext? The current borders are almost invisible on some themes (like the space fantasy thing we ship). Ideally we should do better. It might be worth changing the location bar border for those themes, too.

(In reply to :Gijs Kruitbosch from comment #18) > (In reply to Dão Gottwald [:dao] from comment #15) > > Created attachment 8670176 [details] [diff] [review] > > patch v3 > > > > Now using the location bar's hover border color. That color doesn't > > integrate as well with the toolbarbutton-1 colors set for > > #nav-bar[brighttext], so I removed those. Note that I improvised those > > colors anyway, they weren't part of the mockups. > > OK. Can we still file a followup bug to get slightly more visible things for > brighttext? The current borders are almost invisible on some themes (like > the space fantasy thing we ship). Ideally we should do better. It might be > worth changing the location bar border for those themes, too. Sure

Comment on attachment 8670185 [details] [diff] [review] patch v3 Review of attachment 8670185 [details] [diff] [review]: ----------------------------------------------------------------- For bonus points, I believe this fixes bug 1121782. ::: browser/themes/shared/notification-icons.inc.css @@ +102,5 @@ > -moz-margin-end: -8px; > } > > @conditionalForwardWithUrlbar@ > #forward-button[disabled] + #urlbar > #notification-popup-box { > + padding-left: calc(var(--backbutton-urlbar-overlap) + 3px); This increases this padding to 9px on OSX. Actually looks better that way, IMO, but there we go. Unrelated bug I noticed: 1. new tab 2. open html5demos.org/geo 3. append "#foo" to the URL and hit enter 4. click the back button 5. click the forward button and keep the mouse there Note how the left padding has now increased significantly. This issue predates your patch. I'm not really sure how hard fixing this would be, but there we are. Depending on how you feel, maybe you want to file a followup? (I imagine the noticeability/severity of the bug depends on how high the backbutton-urlbar-overlap is...) ::: browser/themes/windows/browser.css @@ +928,3 @@ > padding-right: 3px !important; > +% icon width + border + horizontal padding without --backbutton-urlbar-overlap > +%define forwardButtonWidth 25 Personally, I find the @foo@px notation a bit strange and error-prone (even if we had it before), and, if you don't want to change this to use a CSS variable, I would still prefer it if this had the unit, and we fixed the .01px variant to just subtract an additional 0.01px separately.

Comment on attachment 8670185 [details] [diff] [review] patch v3 Approval Request Comment [Feature/regressing bug #]: latecomer to Windows 10 theme initiative, and it would be nice to have this in the same release as bug 1173743 [User impact if declined]: [Describe test coverage new/current, TreeHerder]: [Risks and why]: not a trivial patch, as there's some code cleanup and refactoring in here, which happens to fix a couple of older bugs. [String/UUID change made/needed]:

I worry a little that this is non-trivial, has some refactoring, and doesn't have (new) tests. Dao if you are willing to keep an eye on this for any regressions it might cause (which we may not catch till early beta) then we could uplift this to aurora anyway. What do you think? Worth the risk of breaking something?

(In reply to Liz Henry (:lizzard) (needinfo? me) from comment #24) > I worry a little that this is non-trivial, has some refactoring, and doesn't > have (new) tests. Dao if you are willing to keep an eye on this for any > regressions it might cause (which we may not catch till early beta) then we > could uplift this to aurora anyway. > > What do you think? Worth the risk of breaking something? The refactoring is mostly simplification. It's pretty unlikely that this would cause regressions that are worse than the bugs fixed here. Otherwise we'd probably already have discovered them. I will certainly keep an eye on this, though, and fix regressions or even do a backout if needed.

Comment on attachment 8670185 [details] [diff] [review] patch v3 Theme fixes for Win10. Approved for uplift to aurora.

Thanks Dao, makes sense.

Mozilla/5.0 (Windows NT 10.0; rv:44.0) Gecko/20100101 Firefox/44.0 The back and forward button styling was properly updated. Verified on Windows 10 64-bit using latest Nightly, build ID: 20151015030233.

I'm also confirming this fix of latest Aurora, buildID 20151022004116. Mozilla/5.0 (Windows NT 10.0; WOW64; rv:43.0) Gecko/20100101 Firefox/43.0