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

Status: RESOLVED WONTFIX

(Core :: CSS Parsing and Computation, defect)

Description

[Jonathan Watt [:jwatt]]

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.

Comment 1

[Jonathan Watt [:jwatt]]

Attachment: :-moz-focusring parts of patch from bug 851777, as r+'ed by roc

Comment 2

[Jonathan Watt [:jwatt]]

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.

Comment 3

[Jonathan Watt [:jwatt]]

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.

Comment 4

[Jonathan Watt [:jwatt]]

Wrong link. The correct link:

https://hg.mozilla.org/integration/mozilla-inbound/rev/23d89270390d

Comment 5

[Boris Zbarsky [:bzbarsky]]

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

Comment 6

[Boris Zbarsky [:bzbarsky]]

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?

Comment 7

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/23d89270390d

Comment 8

[Jonathan Watt [:jwatt]]

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

Comment 9

[Boris Zbarsky [:bzbarsky]]

> I'm also not aware of -moz-appearance depending on any element state

Say a page has:

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

then what?

Comment 10

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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

We should back this out.

Comment 11

[Robert O'Callahan (:roc) (email my personal email if necessary)]

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.

Comment 12

[Jonathan Watt [:jwatt]]

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

Comment 13

[Jonathan Watt [:jwatt]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1547d8556df7

Comment 14

[Jonathan Watt [:jwatt]]

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

Comment 15

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/1547d8556df7

Comment 16

[Jonathan Watt [:jwatt]]

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