Bug 216248 – Elements that become unfocusable do not lose focus

• Component: UI Events
• Status: RESOLVED DUPLICATE

[Darin Adler]

Elements that become unfocusable do not lose focus

[Darin Adler]

Created attachment 408194 [details]
Patch

[Darin Adler]

Created attachment 408301 [details]
Patch

[Darin Adler]

Created attachment 408412 [details]
Patch

[Ryosuke Niwa]

Comment on attachment 408412 [details]
Patch

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

r- because this previously broke Facebook.

See https://bugs.webkit.org/show_bug.cgi?id=29241#c31

> Source/WebCore/dom/Document.cpp:2056
> +    if (m_focusedElement && !m_focusedElement->isFocusable())
> +        setFocusedElement(nullptr, FocusDirectionNone, FocusRemovalEventsMode::DoNotDispatch);

We shouldn't be updating this state during resolveStyle as this would make style resolution timing visible to scripts.
Namely, document.activeElement discloses when we resolved the style.
See discsuiosn on https://github.com/w3c/uievents/issues/236

[Darin Adler]

*** This bug has been marked as a duplicate of bug 29241 ***

[Darin Adler]

(In reply to Ryosuke Niwa from comment #4)
> Comment on attachment 408412 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=408412&action=review
> 
> r- because this previously broke Facebook.
> 
> See https://bugs.webkit.org/show_bug.cgi?id=29241#c31

We need to put some comments in test expectations or source code about this so this doesn’t keep happening. Maybe the best way is to create a test case that matches the Facebook requirement and add it to WebKit tests, or, better, to WPT.  Also we need to get web platform tests fixed! And figure out the long term behavior cross-browser.

Seems like this problem might need to someday need be fixed by changing Facebook to not rely on this. It’s likely become an iOS vs. desktop difference, because I assume desktop websites are more likely to adapt to the behavior of Chrome and Firefox assuming they both agree and WebKit is the odd one out.

> > Source/WebCore/dom/Document.cpp:2056
> > +    if (m_focusedElement && !m_focusedElement->isFocusable())
> > +        setFocusedElement(nullptr, FocusDirectionNone, FocusRemovalEventsMode::DoNotDispatch);
> 
> We shouldn't be updating this state during resolveStyle as this would make
> style resolution timing visible to scripts.
> Namely, document.activeElement discloses when we resolved the style.
> See discsuiosn on https://github.com/w3c/uievents/issues/236

Doesn’t this also mean we have the same problem with the hover code above that I mention in the comment?

[Darin Adler]

(In reply to Ryosuke Niwa from comment #4)
> We shouldn't be updating this state during resolveStyle as this would make
> style resolution timing visible to scripts.
> Namely, document.activeElement discloses when we resolved the style.

This argument doesn’t sound correct to me. We could prevent this by changing document.activeElement to resolve style as needed; offsetLeft has to do something similar for the same reason.

[Ryosuke Niwa]

(In reply to Darin Adler from comment #7)
> (In reply to Ryosuke Niwa from comment #4)
> > We shouldn't be updating this state during resolveStyle as this would make
> > style resolution timing visible to scripts.
> > Namely, document.activeElement discloses when we resolved the style.
> 
> This argument doesn’t sound correct to me. We could prevent this by changing
> document.activeElement to resolve style as needed; offsetLeft has to do
> something similar for the same reason.

That's a good point but it applies to all other APIs that ever use the focused element. All of those also need to update the layout / style before getting the focused element.