Bug 216248 - Elements that become unfocusable do not lose focus
Summary: Elements that become unfocusable do not lose focus
Status: RESOLVED DUPLICATE of bug 29241
Alias: None
Product: WebKit
Classification: Unclassified
Component: UI Events (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Darin Adler
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2020-09-07 12:23 PDT by Darin Adler
Modified: 2020-09-10 19:48 PDT (History)
16 users (show)

See Also:


Attachments
Patch (4.35 KB, patch)
2020-09-07 13:14 PDT, Darin Adler
no flags Details | Formatted Diff | Diff
Patch (13.59 KB, patch)
2020-09-08 19:08 PDT, Darin Adler
no flags Details | Formatted Diff | Diff
Patch (15.74 KB, patch)
2020-09-09 22:13 PDT, Darin Adler
rniwa: review-
ews-feeder: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Darin Adler 2020-09-07 12:23:39 PDT
Elements that become unfocusable do not lose focus
Comment 1 Darin Adler 2020-09-07 13:14:10 PDT Comment hidden (obsolete)
Comment 2 Darin Adler 2020-09-08 19:08:28 PDT Comment hidden (obsolete)
Comment 3 Darin Adler 2020-09-09 22:13:22 PDT
Created attachment 408412 [details]
Patch
Comment 4 Ryosuke Niwa 2020-09-10 01:03:36 PDT
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
Comment 5 Darin Adler 2020-09-10 04:13:24 PDT

*** This bug has been marked as a duplicate of bug 29241 ***
Comment 6 Darin Adler 2020-09-10 04:20:20 PDT
(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?
Comment 7 Darin Adler 2020-09-10 06:47:31 PDT
(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.
Comment 8 Ryosuke Niwa 2020-09-10 19:48:20 PDT
(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.