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

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

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

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.