Closed Bug 862693 Opened 8 years ago Closed 8 years ago

Stop the :-moz-focusring pseudo-class from matching if an element is themed and the theme will display a visual indication of focus for the element

Categories

(Core :: CSS Parsing and Computation, defect)

defect
Not set
normal

Tracking

()

RESOLVED WONTFIX
mozilla23

People

(Reporter: jwatt, Assigned: jwatt)

References

Details

Attachments

(1 file)

Spinning this out from bug 851777, and landing part of the (r+'ed) patch from there under this separate bug. This is to give this change higher visibility, and is case there are regressions.

The part of the patch from bug 851777 that I'll be landing here stops the :-moz-focusring pseudo-class from matching elements if they are themed and the theme will take care of displaying a visual indication of focus for the element.
As noted in bug 851777, this won't check to see if the NS_EVENT_STATE_FOCUSRING state should be updated if the -moz-appearance property changes for a focused element. I think that's a pretty obscure case, and not worth the additional code complexity or time figuring out how to do that.
Component: SVG → CSS Parsing and Computation
https://hg.mozilla.org/integration/mozilla-inbound/rev/4363de95b961

Note that this commit also also updates dom/tests/mochitest/general/test_focusrings.xul to match the desired behavior.
Wait, so.... doesn't this make element state depend on the styles?  And those styles themselves depend on element state, right?

I don't see why this is the right approach here...
Or is that what comment 2 is basically about?  But are you sure that the case described in comment 2 is the only broken one?  Can an element get focused while it has no frame, for example?
https://hg.mozilla.org/mozilla-central/rev/23d89270390d
Status: NEW → RESOLVED
Closed: 8 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla23
(In reply to Boris Zbarsky (:bz) from comment #5)
> Wait, so.... doesn't this make element state depend on the styles?  And
> those styles themselves depend on element state, right?

It makes the NS_EVENT_STATE_FOCUSRING state depend on the value of -moz-appearance in so far as it will look at the -moz-appearance value at any time it would normally update the NS_EVENT_STATE_FOCUSRING state. No attempt is made to link the NS_EVENT_STATE_FOCUSRING state to changes in -moz-appearance though, so it doesn't depend on it in that sense. I'm also not aware of -moz-appearance depending on any element state, or specifically on NS_EVENT_STATE_FOCUSRING...the state code isn't something I'm familiar with though.

> I don't see why this is the right approach here...

Well...I guess my question is why is it the wrong approach? :) If style wasn't available/up-to-date at the time state computation is done then I could see a problem. But if that were the case then I'd also expect the patch would fail to fix bug 851777. It seems to work though...

It also seems like :-moz-focusring shouldn't apply to elements that have their own themed way of indicating focus.
> I'm also not aware of -moz-appearance depending on any element state

Say a page has:

  :-moz-focusring { -moz-appearance: none; }

then what?
Yes, that's a good point. I'm not even the right reviewer for this. Sorry :-(.

We should back this out.
One thing we should do is make :-moz-focusring only work in UA style sheets.

Another thing we should do is remove the aPresContext and aFrame parameters from nsITheme::ThemeDrawsFocusForWidget. The implementations don't use them.

I guess we need to go back to the approach of using custom logic in BuildDisplayList to check the appearance, call nsITheme::ThemeDrawsFocusForWidget and then decide whether to draw the focus ring or not.
(In reply to Boris Zbarsky (:bz) from comment #9)
> Say a page has:
> 
>   :-moz-focusring { -moz-appearance: none; }
> 
> then what?

Uff, right. I'll back this out tomorrow. I kinda just want to say "why on earth would anyone do that?", but I guess that isn't really the point. :)
Status: RESOLVED → REOPENED
Keywords: dev-doc-needed
Resolution: FIXED → ---
https://hg.mozilla.org/integration/mozilla-inbound/rev/1547d8556df7
Status: REOPENED → RESOLVED
Closed: 8 years ago8 years ago
Resolution: --- → WONTFIX
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #11)
> I guess we need to go back to the approach of using custom logic in
> BuildDisplayList to check the appearance, call
> nsITheme::ThemeDrawsFocusForWidget and then decide whether to draw the focus
> ring or not.

Filed bug 864120.
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #11)
> Another thing we should do is remove the aPresContext and aFrame parameters
> from nsITheme::ThemeDrawsFocusForWidget. The implementations don't use them.

Patched in bug 871264.
You need to log in before you can comment on or make changes to this bug.