Closed Bug 1173738 Opened 5 years ago Closed 4 years ago

Update URL and search bar borders on Windows 10

Categories

(Firefox :: Theme, defect, P1)

Unspecified
Windows 10
defect

Tracking

()

VERIFIED FIXED
Firefox 42
Tracking Status
firefox40 --- verified
firefox41 --- verified
firefox42 --- verified

People

(Reporter: dao, Assigned: dao)

References

(Blocks 2 open bugs)

Details

Attachments

(1 file, 1 obsolete file)

Assignee: nobody → ntim.bugs
Status: NEW → ASSIGNED
Tim, are you working on this? If not, I'd like to take over. We need to get this done soon.
Flags: needinfo?(ntim.bugs)
Feel free to take over, I haven't started working on this.
Flags: needinfo?(ntim.bugs)
Assignee: ntim.bugs → dao
Attached patch patch (obsolete) — Splinter Review
Attachment #8628241 - Attachment description: WIP → patch
Attachment #8628241 - Flags: review?(gijskruitbosch+bugs)
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?)
Attachment #8628241 - Flags: review?(gijskruitbosch+bugs)
(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.
Attachment #8628241 - Flags: review?(gijskruitbosch+bugs)
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?
Attachment #8628241 - Flags: review?(gijskruitbosch+bugs) → review+
Attached patch patchSplinter 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
Attachment #8628241 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/5d74c3c273f6
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 42
Flags: qe-verify+
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.
Status: RESOLVED → VERIFIED
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
Attachment #8628510 - Flags: approval-mozilla-beta?
Attachment #8628510 - Flags: approval-mozilla-aurora?
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.
Attachment #8628510 - Flags: approval-mozilla-beta?
Attachment #8628510 - Flags: approval-mozilla-beta+
Attachment #8628510 - Flags: approval-mozilla-aurora?
Attachment #8628510 - Flags: approval-mozilla-aurora+
This is also fixed using Firefox 40 beta 6 (build ID: 20150720220238) and latest Aurora (build ID: 20150721004010) on Windows 10 64-bit.
You need to log in before you can comment on or make changes to this bug.