Summary: | Elements that become unfocusable do not lose focus | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Darin Adler <darin> | ||||||||
Component: | UI Events | Assignee: | Darin Adler <darin> | ||||||||
Status: | RESOLVED DUPLICATE | ||||||||||
Severity: | Normal | CC: | aboxhall, apinheiro, cdumez, cfleizach, dmazzoni, emilio, esprehn+autocc, ews-watchlist, jcraig, jdiggs, kangil.han, koivisto, rniwa, samuel_white, simon.fraser, wenson_hsieh | ||||||||
Priority: | P2 | ||||||||||
Version: | WebKit Nightly Build | ||||||||||
Hardware: | All | ||||||||||
OS: | All | ||||||||||
See Also: | https://bugs.webkit.org/show_bug.cgi?id=216366 | ||||||||||
Attachments: |
|
Description
Darin Adler
2020-09-07 12:23:39 PDT
Created attachment 408194 [details]
Patch
Created attachment 408301 [details]
Patch
Created attachment 408412 [details]
Patch
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 *** This bug has been marked as a duplicate of bug 29241 *** (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? (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. (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. |