932410 - Allow users to get rid of the focus ring from -moz-appearance:none styled <input type=range>

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Jonathan Watt [:jwatt]]

Someone has pointed out on my blog that it's not possible to get rid of the focus ring from -moz-appearance:none styled <input type=range>. We should allow that. I think it should be possible by getting the :-moz-focusring context and checking if outline is user-set. If so we should try to use that, but that's probably hard given the current architecture. We could at least check for the "none" value though and skip creating it.

Comment 1

[Jonathan Watt [:jwatt]]

FxOS needs a fix here for bug 973514.

Comment 2

[Jonathan Watt [:jwatt]]

Some background:

Originally I added a focus ring for range using a user-agent style sheet rule along the lines of:

  input[type="range"]:-moz-focusring {
    outline: 1px dotted;
  }

When it came to implementing native theming, this was an issue because we don't want that outline to show if the theme provides its own visual indication of focus. (We don't want two focus rings.)

To handle this I tried in bug 864120 to ignore the -moz-focusring pseudo-class styling (prevent the selector from matching) if native theming was applied. However, as pointed out by bz (in bug 862693 comment 5) that made element state depend on the styles, when styles depend on element state, which isn't right.

Instead, in bug 864120, I removed the -moz-focusring from the UA style sheet and created a new display list item (nsDisplayRangeFocusRing) to paint the focus ring when ranges are not natively styled. The problem with this approach is that the focus ring can't be removed using style, causing this bug and bug 973514.

One way around this bug would be to avoid using nsDisplayRangeFocusRing if 'outline' has been set using something along the lines of HasAuthorSpecifiedRules, but more a HasAnySpecifiedRules (so FxOS can get rid of the outline using a rule in its UA style sheet), for the outline property. dbaron doesn't want to use HasAuthorSpecifiedRules, and especially doesn't want to introduce a HasAnySpecifiedRules extension to that method, though. Instead he would prefer that we kill off nsDisplayRangeFocusRing, add back the original user-agent style sheet rule at the beginning at this comment, and just ignore the 'outline' property for nsRangeFrame if it is natively themed.

For the record I'm not in love with dbaron's approach since it assumes setting of 'outline' equates to -moz-focusring styling. Probably this is fine for virtually all content authors, but I need to think about this some more.

Comment 3

[Boris Zbarsky [:bzbarsky]]

How is this different from the situation buttons are in?  What do we do there?

Comment 4

[Jonathan Watt [:jwatt]]

Button rendering isn't simple (it being done via nsButtonFrameRenderer and involving -moz-focus-inner and -moz-focus-outer pseudo-elements used purely to specify style that nsButtonFrameRenderer will use to paint focus rings) but the short of it is that nsDisplayButtonForeground::Paint only calls nsButtonFrameRenderer::PaintOutlineAndFocusBorders if IsThemed or ThemeDrawsFocusForWidget returns false. So I guess that's essentially doing what dbaron feels we should do for range, except that the user of pseudo-elements means that content can still set an outline independent of the UA style sheet focus rings, which makes me happy.

Comment 5

[Jonathan Watt [:jwatt]]

Attachment: patch to use the same strategy as nsButtonControlFrame

Comment 6

[Jonathan Watt [:jwatt]]

Attachment: patch to use the same strategy as nsButtonControlFrame

Comment 7

[Jonathan Watt [:jwatt]]

With this patch content and UA style sheets would be able to get rid of the UA focus ring by using:

input[type=range]::-moz-focus-outer {
  border: 0;
}

or otherwise change the style of the focus ring by changing the border values for that pseudo-element.

Comment 8

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8378036 [details] [diff] [review]
patch to use the same strategy as nsButtonControlFrame

We'll probably need a different approach for standardizing this, but I'm not sure what that should be.  :(

>+nsDisplayRangeFocusRing::GetBounds(nsDisplayListBuilder* aBuilder, bool* aSnap)

Should assert in here that styleContext is not null, with a pointer to why (that is, that this display item only exists if it's not null).

>+  if (aBuilder->IsForPainting() ||
>+      !IsVisibleForPainting(aBuilder)) {
>+    // we don't want the focus ring item for hit-testing or if the item isn't
>+    // in the area being [re]painted

Is the IsForPainting() test backwards?  Missing a '!'?

>+    // no ::-moz-outer-focus specified border (how style specifies a focus

::-moz-focus-outer, right?

>+  // called if style changes that would change the -moz-outer-focus

-moz-focus-outer?

r=me with those nits

Comment 9

[Jonathan Watt [:jwatt]]

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

Thanks!

(In reply to Boris Zbarsky [:bz] from comment #8)
> We'll probably need a different approach for standardizing this, but I'm not
> sure what that should be.  :(

Me neither. :(

> Is the IsForPainting() test backwards?  Missing a '!'?

Thanks for catching that - I accidentally removed the check just prior to my last qref, and then screwed up re-adding it when I noticed that reviewing the original patch I attached.

Comment 10

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/cd1bb1422d60

Comment 11

[CoolCmd]

this method doesn't work!
https://jsfiddle.net/CoolCmd/41av01Lg/
outer black border disappear. inner white border is not.
windows 7, ff 43