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

RESOLVED FIXED in mozilla30

Status

()

RESOLVED FIXED
6 years ago
3 years ago

People

(Reporter: jwatt, Assigned: jwatt)

Tracking

(Blocks: 1 bug)

Trunk
mozilla30
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

6 years ago
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.
(Assignee)

Comment 1

5 years ago
FxOS needs a fix here for bug 973514.
Blocks: 973514
(Assignee)

Comment 2

5 years ago
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.
How is this different from the situation buttons are in?  What do we do there?
(Assignee)

Comment 4

5 years ago
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.
(Assignee)

Comment 5

5 years ago
Attachment #8378033 - Flags: review?(bzbarsky)
(Assignee)

Comment 6

5 years ago
Attachment #8378033 - Attachment is obsolete: true
Attachment #8378033 - Flags: review?(bzbarsky)
Attachment #8378036 - Flags: review?(bzbarsky)
(Assignee)

Comment 7

5 years ago
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 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
Attachment #8378036 - Flags: review?(bzbarsky) → review+
(Assignee)

Comment 9

5 years ago
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.
https://hg.mozilla.org/mozilla-central/rev/cd1bb1422d60
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla30

Comment 11

3 years ago
this method doesn't work!
https://jsfiddle.net/CoolCmd/41av01Lg/
outer black border disappear. inner white border is not.
windows 7, ff 43
You need to log in before you can comment on or make changes to this bug.