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.
Created attachment 445534 [details] Patch
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
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.
Created attachment 445571 [details] Patch
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.
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?
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.
Created attachment 445577 [details] Patch
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.
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.
Created attachment 445671 [details] Patch
(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.
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].
<rdar://problem/85958613>