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

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

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

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

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

(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

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

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