Bug 233688 – [selectors] :focus-visible should stop matching after blur

• Status: RESOLVED
• Resolution: FIXED
• Component: CSS

[Manuel Rego Casasnovas]

I've been checking the failures on the last patch at https://bugs.webkit.org/show_bug.cgi?id=221925 and it looks like we introduced a bug in r286134 (https://bugs.webkit.org/show_bug.cgi?id=233178) and :focus-visible doesn't stop matching after blur.

[Manuel Rego Casasnovas]

Created attachment 445534 [details]
Patch

[EWS Watchlist]

This patch modifies the imported WPT tests. Please ensure that any changes on the tests (not coming from a WPT import) are exported to WPT. Please see https://trac.webkit.org/wiki/WPTExportProcess

[Antti Koivisto]

Comment on attachment 445534 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=445534&action=review

> Source/WebCore/style/ElementRuleCollector.cpp:161
> +    if (matchesFocusPseudoClass(element()) || matchesFocusVisiblePseudoClass(element()))

We should understand why matchesFocusPseudoClass is false while matchesFocusVisiblePseudoClass is true. It makes no logical sense, every element with visible focus should have focus.

[Manuel Rego Casasnovas]

Created attachment 445571 [details]
Patch

[Manuel Rego Casasnovas]

Thanks for the quick review.

(In reply to Antti Koivisto from comment #3)
> Comment on attachment 445534 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=445534&action=review
> 
> > Source/WebCore/style/ElementRuleCollector.cpp:161
> > +    if (matchesFocusPseudoClass(element()) || matchesFocusVisiblePseudoClass(element()))
> 
> We should understand why matchesFocusPseudoClass is false while
> matchesFocusVisiblePseudoClass is true. It makes no logical sense, every
> element with visible focus should have focus.

Yeah you're right, the actual problem was in Element::setFocus().
We are using there Style::PseudoClassChangeInvalidation for :focus, but that was used before focus-visible flag was set, and thus the flags were out of sync during invalidation.

The new version of the patch fixes that. PTAL.

[Antti Koivisto]

Comment on attachment 445571 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=445571&action=review

> Source/WebCore/dom/Element.cpp:-847
> -    Style::PseudoClassChangeInvalidation styleInvalidation(*this, CSSSelector::PseudoClassFocusVisible);

I see direct call to setHasFocusVisible in EventHandler::internalKeyEvent(). Would that miss invalidation now?

Does that case have a test?

[Manuel Rego Casasnovas]

Comment on attachment 445571 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=445571&action=review

>> Source/WebCore/dom/Element.cpp:-847
>> -    Style::PseudoClassChangeInvalidation styleInvalidation(*this, CSSSelector::PseudoClassFocusVisible);
> 
> I see direct call to setHasFocusVisible in EventHandler::internalKeyEvent(). Would that miss invalidation now?
> 
> Does that case have a test?

Yes there's a test and it actually failed due to that.

[Manuel Rego Casasnovas]

Created attachment 445577 [details]
Patch

[Antti Koivisto]

Comment on attachment 445577 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=445577&action=review

> Source/WebCore/dom/Element.cpp:855
> -    Style::PseudoClassChangeInvalidation styleInvalidation(*this, CSSSelector::PseudoClassFocusVisible);
> +    if (invalidation == Invalidation::PseudoClass)
> +        Style::PseudoClassChangeInvalidation styleInvalidation(*this, CSSSelector::PseudoClassFocusVisible);
>      document().userActionElements().setHasFocusVisible(*this, flag);

This won't work, PseudoClassChangeInvalidation needs to be scoped around the the mutation (setting focus visible flag) as invalidation needs to run in both the tree before and after the mutation (from constructor and destructor). Surprised this doesn't break tests.

I don't think you need to do this change though. The only change that is needed is to call setHasFocusVisible within the scope of focusStyleInvalidation.

[Antti Koivisto]

Well, I suppose you actually need to do something like this get consistent scopes.

Easiest thing to do is to just add PseudoClassChangeInvalidation to the caller of setHasFocusVisible.

[Manuel Rego Casasnovas]

Created attachment 445671 [details]
Patch

[Manuel Rego Casasnovas]

(In reply to Antti Koivisto from comment #10)
> Easiest thing to do is to just add PseudoClassChangeInvalidation to the
> caller of setHasFocusVisible.

Ok, I've done this in a new version of the patch. Thanks again for the review.

[EWS]

Committed r286415 (244761@main): <https://commits.webkit.org/244761@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 445671 [details].

[Radar WebKit Bug Importer]

<rdar://problem/85958613>