1173738 - Update URL and search bar borders on Windows 10

Status: VERIFIED FIXED

(Firefox :: Theme, defect, P1)

Description

[Dão Gottwald [:dao]]

Spec is at the bottom of http://people.mozilla.org/~shorlander/mockups/Windows-10/Windows-10.html

Comment 1

[Dão Gottwald [:dao]]

Tim, are you working on this? If not, I'd like to take over. We need to get this done soon.

Comment 2

[Tim Nguyen :ntim]

Feel free to take over, I haven't started working on this.

Comment 3

[Dão Gottwald [:dao]]

Attachment: patch

Comment 4

[:Gijs (out for now; he/him)]

Comment on attachment 8628241 [details] [diff] [review]
patch

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

Out of curiosity, why implement with inset box-shadow rather than changing the border-width? This is generally less efficiently rendered because of the overlapping that happens, so all else being equal I would have leaned towards the latter. I presume all else is not equal, but I'd like to hear more about that from you. :-)

(Assuming agreement the approach taken, this is essentially r+ on implementing that approach, but I do have enough questions both about the approach and below on some implementation details, that I chose not to do that - please either submit something else depending on what you think answers to the questions are, or answer questions and re-request review on the same patch, whichever ends up making the most sense to you)

::: browser/themes/windows/browser.css
@@ +1193,5 @@
>  
> +@media (-moz-windows-default-theme) {
> +  #urlbar,
> +  .searchbar-textbox {
> +    @navbarTextboxCustomBorder@

So effectively, this is only applying on lwthemes, and on Windows XP, right - and then it will apply again if we ever see versions of Windows that don't match windows-win10 ? Should that be a concern?

@@ +1230,5 @@
> +
> +  @media not all and (-moz-os-version: windows-xp) {
> +    #urlbar:not(:-moz-lwtheme)[focused],
> +    .searchbar-textbox:not(:-moz-lwtheme)[focused] {
> +      border-color: Highlight;

Is there a CSS-order-of-rules reason for not having this nearer the top of the default-theme media query? That would make it easier to read these rules in a top-to-bottom oldest-to-newest way.

@@ +1262,5 @@
>  }
>  
>  @conditionalForwardWithUrlbar@ > #urlbar {
>    -moz-border-start: none;
> +  -moz-padding-start: 0 !important;

Please add a comment as to why this needs to be !important. (I think mostly DRY for the other CSS in this patch?)

Comment 5

[Dão Gottwald [:dao]]

(In reply to :Gijs Kruitbosch from comment #4)
> Out of curiosity, why implement with inset box-shadow rather than changing
> the border-width? This is generally less efficiently rendered because of the
> overlapping that happens, so all else being equal I would have leaned
> towards the latter. I presume all else is not equal, but I'd like to hear
> more about that from you. :-)

Changing the border-width changes layout, box-shadow doesn't do that. The former may be rendered more efficiently, but pure render time isn't the only performance factor here.

> ::: browser/themes/windows/browser.css
> @@ +1193,5 @@
> >  
> > +@media (-moz-windows-default-theme) {
> > +  #urlbar,
> > +  .searchbar-textbox {
> > +    @navbarTextboxCustomBorder@
> 
> So effectively, this is only applying on lwthemes, and on Windows XP, right
> - and then it will apply again if we ever see versions of Windows that don't
> match windows-win10 ? Should that be a concern?

No, should be fine.

> > +  @media not all and (-moz-os-version: windows-xp) {
> > +    #urlbar:not(:-moz-lwtheme)[focused],
> > +    .searchbar-textbox:not(:-moz-lwtheme)[focused] {
> > +      border-color: Highlight;
> 
> Is there a CSS-order-of-rules reason for not having this nearer the top of
> the default-theme media query? That would make it easier to read these rules
> in a top-to-bottom oldest-to-newest way.

There's no single true oldest-to-newest order. The above applies to anything but XP, including future versions of Windows. It's at the bottom because it's supposed to override other rules without being concerned too much with selector specificity.

> >  @conditionalForwardWithUrlbar@ > #urlbar {
> >    -moz-border-start: none;
> > +  -moz-padding-start: 0 !important;
> 
> Please add a comment as to why this needs to be !important. (I think mostly
> DRY for the other CSS in this patch?)

It needs to override the padding set in #urlbar:not(:-moz-lwtheme). But I think this selector takes precedence, so I can probably remove !important.

Comment 6

[:Gijs (out for now; he/him)]

Comment on attachment 8628241 [details] [diff] [review]
patch

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

Alright, wfm. I take it removing !important didn't work? Or were you planning on doing that post-review?

Comment 7

[Dão Gottwald [:dao]]

Attachment: patch

(In reply to :Gijs Kruitbosch from comment #6)
> Comment on attachment 8628241 [details] [diff] [review]
> patch
> 
> Review of attachment 8628241 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Alright, wfm. I take it removing !important didn't work? Or were you
> planning on doing that post-review?

I've removed it

Comment 8

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/5d74c3c273f6

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/5d74c3c273f6

Comment 10

[Cornel Ionce [:noni] [Hubs QA]]

Verified fixed using Latest Nightly 42.0a1, build ID: 20150712030212 on Windows 10 64-bit.
Ran into bug 1180260 on a MS Pro 2 device (HiDPI), which will be tracked separately.

Comment 11

[Dão Gottwald [:dao]]

Comment on attachment 8628510 [details] [diff] [review]
patch

Approval Request Comment
[Feature/regressing bug #]: windows 10
[User impact if declined]: n/a
[Describe test coverage new/current, TreeHerder]: n/a
[Risks and why]: low-risk CSS patch. just need to take care of bug 1180260.
[String/UUID change made/needed]: none

Comment 12

[Sylvestre Ledru [:Sylvestre]]

Comment on attachment 8628510 [details] [diff] [review]
patch

Having a polish version of Fx for Windows 10 in part of our objectives for the 40 release. Taking it.

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/ba188c7b76e7

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/189d7a1f736e

Comment 15

[Cornel Ionce [:noni] [Hubs QA]]

This is also fixed using Firefox 40 beta 6 (build ID: 20150720220238) and latest Aurora (build ID: 20150721004010) on Windows 10 64-bit.