1234758 - non-default focus rings incorrectly set in preference style sheet

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Cameron McCormack (:heycam)]

In bug 1169514 when I moved the preference style sheet to be built in the nsLayoutStylesheetCache, I made two mistakes:

* I left off the px unit on the border-width value:

https://hg.mozilla.org/integration/mozilla-inbound/rev/8f69f20370b7#l3.378
https://hg.mozilla.org/integration/mozilla-inbound/rev/8f69f20370b7#l5.152

* I looked at focusRingWidth instead of focusRingStyle:

https://hg.mozilla.org/integration/mozilla-inbound/rev/8f69f20370b7#l3.379
https://hg.mozilla.org/integration/mozilla-inbound/rev/8f69f20370b7#l5.159

Comment 1

[Cameron McCormack (:heycam)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=02d5e4964db3

Comment 2

[Cameron McCormack (:heycam)]

Attachment: patch

Comment 3

[Cameron McCormack (:heycam)]

https://treeherder.mozilla.org/#/jobs?repo=try&author=cmccormack%40mozilla.com

Comment 4

[Daniel Holbert [:dholbert]]

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

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

Thanks for the hg.m.o links in comment 0 -- very helpful in demonstrating the problem here!  r=me, with questions below addressed as you see fit.

First question: is it possible to test this? (I'm actually not clear on how/where these styles get used; nonetheless I'm still happy r+'ing this targeted change, based on your comment 0 links.)  If so, please add a test or at least file a followup on adding tests so that we can catch things like this in the future...

::: layout/style/nsLayoutStylesheetCache.cpp
@@ +893,2 @@
>            focusRingWidth,
> +          focusRingStyle == 0 ? (const char*) "solid" : (const char*) "dotted");

Second question: do you actually need the (const char*) casts here?  I don't see why you would, and I can remove them locally & still build just fine.  So, maybe ditch these casts while you're here, unless there's a reason we need them?

Comment 5

[Cameron McCormack (:heycam)]

(In reply to Daniel Holbert [:dholbert] from comment #4)
> First question: is it possible to test this? (I'm actually not clear on
> how/where these styles get used; nonetheless I'm still happy r+'ing this
> targeted change, based on your comment 0 links.)  If so, please add a test
> or at least file a followup on adding tests so that we can catch things like
> this in the future...

We can test this specific mistake by setting browser.display.focus_ring_width to 2, showing a -moz-appearance:none button, and noticing that the focus ring still has the default 1px width.  I'll add a reftest for this.

(I'm not actually sure what the difference between using solid and dotted is given this is a transparent border, incidentally.)

> ::: layout/style/nsLayoutStylesheetCache.cpp
> @@ +893,2 @@
> >            focusRingWidth,
> > +          focusRingStyle == 0 ? (const char*) "solid" : (const char*) "dotted");
> 
> Second question: do you actually need the (const char*) casts here?  I don't
> see why you would, and I can remove them locally & still build just fine. 
> So, maybe ditch these casts while you're here, unless there's a reason we
> need them?

I remember running into compiler errors before for the two expressions being different types (e.g. const char[6] and const char[7]) and needing to cast to get around this.  I'll do a try run to see if I can leave them out.

Comment 6

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d7ea13af4441

Comment 7

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/6aedf1738475

Comment 8

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/d7ea13af4441
https://hg.mozilla.org/mozilla-central/rev/6aedf1738475

Comment 9

[Cameron McCormack (:heycam)]

Having awful trouble writing a reftest with a button that gets focused in script (for some reason I need to call .focus() late enough after page load to have an effect, but I can't work out what the exact conditions are, despite it working as expected when loading in a browser tab).  So I'm declaring this not worth it and moving on.

Comment 10

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/11b5a873a5c9

Comment 11

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/11b5a873a5c9