Add site-specific quirk to make anchor elements mouse-focusable on thesaurus.com and dictionary.com#63082
Conversation
swpatters
added
the
UI Events
|
EWS run on previous version of this PR (hash 69a12bd) Details |
swpatters
requested review from
Ahmad-S792,
aprotyas,
hortont424,
karlcow,
pxlcoder and
rr-codes
and removed request for
cdumez and
rniwa
webkit-ews-buildbot
added
the
merging-blocked
swpatters
force-pushed
the
eng/HTMLAnchorElement-isMouseFocusable-returns-false-for-a-href-links-without-explicit-tabindex
branch
from
69a12bd to
8962543
Compare
|
EWS run on previous version of this PR (hash 8962543) Details |
🛠 tv-sim
swpatters
force-pushed
the
eng/HTMLAnchorElement-isMouseFocusable-returns-false-for-a-href-links-without-explicit-tabindex
branch
from
8962543 to
b38dbe4
Compare
|
EWS run on previous version of this PR (hash b38dbe4) Details |
aprotyas
left a comment
aprotyas
left a comment
There was a problem hiding this comment.
The technical approach is sound, and it is a real web compat problem that you're trying to fix, but this touches a deliberately-maintained platform behavior with extensive history (https://bugs.webkit.org/show_bug.cgi?id=22261). We'd need broader discussion before landing.
Also, worth considering: HTMLFormControlElement::isMouseFocusable() has a site-specific quirk to work around this as a stop-gap. Maybe we can employ something like that for anchors, which would allow us to fix thesaurus.com without committing to a blanket behavior change right now.
karlcow
left a comment
karlcow
left a comment
There was a problem hiding this comment.
for form elements. some quirks have been put in place to make some websites working.
WebKit/Source/WebCore/page/Quirks.cpp
Lines 253 to 262 in 367f77e
WebKit/Source/WebCore/html/HTMLFormControlElement.cpp
Lines 230 to 241 in 367f77e
Just for information.
That's fair. You're right. The original blocker — no way to distinguish mouse vs keyboard focus — is now solved. Support for :focus-visible shipped and is implemented properly. The original reason for the Mac restriction no longer applies. But, yes, there should be more discussion. Maybe a quirk is the route to take. |
|
@swpatters Do you want me to create the quirk or would you like to give a stab at it? |
swpatters
force-pushed
the
eng/HTMLAnchorElement-isMouseFocusable-returns-false-for-a-href-links-without-explicit-tabindex
branch
from
b38dbe4 to
f5a77cf
Compare
|
EWS run on previous version of this PR (hash f5a77cf) Details |
Whatever works. I'm inclined to wait for more conversation about it overall. @karlcow If you think a quirk is better suited here, I can change it to a quirk for thesaurus.com. |
|
Why are you making quirks targeting specific websites? We all want a working browser, what's so special about thesaurus? |
Speaking as someone who was around back when it was implemented: visible focus ring wasn't the original reason for the Mac mouse and keyboard focus behavior. The original intent was to be consistent with the Mac platform and HI guidelines. On Mac, in native apps, only controls that the user can type in such as text fields can receive keyboard and mouse focus by default. The system setting "Keyboard Navigation" overrides this and makes all controls focusable. Note that even if no focus ring is drawn, focusing a link or button changes where the focus will go next when the user hits TAB. We realize that this longstanding behavior is at odds with web developer expectations, since other browsers (even on Mac) implement Windows-like behavior by default, where any control can be focused by clicking. We have been discussing whether we need to entirely give up on the historical desire for consistency with Mac native apps, or if we can come up with some partway measure that preserves both Mac-like behavior and web compatibility. Possibilities include focusing but hiding the focus ring (imperfect), setting focus temporarily but then unsetting (unclear if this will be compatible enough) or hiding the focus ring and reseting focus to top level when the user performs keyboard focus navigation (probably compatible but kind of complicated). An additional complication is accessibility. VoiceOver users are notified of focus changes. If a user uses VoiceOver without Keyboard Navigation, telling them about a focus change that will be reset as soon as they hit TAB could be confusing. Sorry for the longwinded explanation but I wanted to make sure folks proceed based on correct information about the intent of this behavior and the state of discussions about changing it. |
All of these will end up being Lovecraftian level horrors for the web devs trying to create a simple dropdown menu that should close when you tab away to the next element. What do you mean focusing but not showing the focus ring being imperfect? This is literally expected behaviour that has been in place everywhere but in Safari for 20+ years |
swpatters
force-pushed
the
eng/HTMLAnchorElement-isMouseFocusable-returns-false-for-a-href-links-without-explicit-tabindex
branch
from
8592e8c to
d8fbb21
Compare
|
EWS run on previous version of this PR (hash d8fbb21) Details |
swpatters
force-pushed
the
eng/HTMLAnchorElement-isMouseFocusable-returns-false-for-a-href-links-without-explicit-tabindex
branch
from
d8fbb21 to
aa33cda
Compare
|
EWS run on previous version of this PR (hash aa33cda) Details |
|
@othermaciej by "everywhere" I mean every OS and every browser in the world, except Safari. If you adopt this behavior it wouldn't even change anything visually for the person, I don't understand what is the big deal here, aside from maybe accessibility tools. But then again, if the whole world works this way + Safari can also work this way if you manually add tabindex="0" to everything – then I'm pretty sure a11y will also be alright.
Here's a very common case where Safari makes web platform jump through notorious hoops: 
When we navigate using keyboard and we hit tab after "Button" – we get "Dropdown" focused. Then we hit enter or arrow down which opens the dropdown. Then if we hit tab again, focus moves to "Another button" and dropdown must be closed, because focus is no longer ON its host and neither it is INSIDE dropdown content. So to know when to close the dropdown we must watch focus. Now imagine after we opened the dropdown we click on a radio button inside the dropdown. In Safari focus goes nowhere and we must figure out ourselves with overcomplicated machinery whether this focus loss warrants dropdown closure or if it's just Safari acting up. |
swpatters
removed
the
merging-blocked
brentfulgham
left a comment
brentfulgham
left a comment
There was a problem hiding this comment.
Looks good. Thank you for resolving this! r=me
| @@ -110,7 +111,7 @@ bool HTMLAnchorElement::isMouseFocusable() const | |||
| { | |||
| #if !(PLATFORM(GTK) || PLATFORM(WPE)) | |||
| // Only allow links with tabIndex or contentEditable to be mouse focusable. | |||
| if (isLink()) | |||
| if (isLink() && !protect(document())->quirks().needsAnchorToBeMouseFocusable()) | |||
There was a problem hiding this comment.
It's a shame we have to ref document every time isMouseFocusable() gets called. It would be nice if this could be cached so that it only fired the first time (since it is true for the life of the WCP). But probably only worth doing if it shows up in a perf trace.
There was a problem hiding this comment.
100 anchors x 3 iterations
With quirk (thesaurus.com): 861, 834, 1258 ms avg=984 ms
Without quirk (example.com): 832, 1173, 857 ms avg=954 ms
No meaningful difference — ~984ms vs ~954ms, well within the noise (note the 1258ms and 1173ms outliers swapping between runs). The protect(document()) overhead is not measurable here.
There was a problem hiding this comment.
But I'll keep an eye out. Thank you.
There was a problem hiding this comment.
I think Brent was referring to more systematic tracing with our perf infra.
There was a problem hiding this comment.
I ran some local perf tests to see if could see anything ahead of infrastructure. But yes, Brent's concern is still valid and we'll wait and see.
There was a problem hiding this comment.
The quirk makes sense to me modulo having it be effective on other Cocoa platforms too.
Can you add test coverage? We have an established pattern for testing quirk behavior using window.internals.setTopDocumentURLForQuirks(). See LayoutTests/fast/events/mac/mouse-down-can-start-selection-quirk.html (Spotify) or LayoutTests/fast/css/expedia-group-animation-quirk.html (Expedia) for examples.
I'm imagining something like this:
// <a id="link" href="#">Click me</a>
window.internals.setTopDocumentURLForQuirks("https://www.thesaurus.com");
eventSender.mouseMoveTo({ anchor coordinates });
eventSender.mouseDown();
eventSender.mouseUp();
shouldBe("document.activeElement", "link");
swpatters
force-pushed
the
eng/HTMLAnchorElement-isMouseFocusable-returns-false-for-a-href-links-without-explicit-tabindex
branch
from
aa33cda to
ac21e17
Compare
|
EWS run on previous version of this PR (hash ac21e17) Details |
swpatters
force-pushed
the
eng/HTMLAnchorElement-isMouseFocusable-returns-false-for-a-href-links-without-explicit-tabindex
branch
from
ac21e17 to
c16cc57
Compare
|
EWS run on previous version of this PR (hash c16cc57) Details |
| @@ -0,0 +1,43 @@ | |||
| <!DOCTYPE html> | |||
| <!-- Test for the anchor mouse-focusability quirk on thesaurus.com and dictionary.com. | |||
| https://bugs.webkit.org/show_bug.cgi?id=312692 rdar://174959285 | |||
There was a problem hiding this comment.
I would either link to both tracking bugs or neither. (I'm leaning towards the latter)
| <script> | ||
| description("Tests that the anchor mouse-focusability quirk works on thesaurus.com and dictionary.com, but not on unrelated sites."); | ||
|
|
||
| var link = document.getElementById("link"); |
There was a problem hiding this comment.
| var link = document.getElementById("link"); | |
| let link = document.getElementById("link"); |
There was a problem hiding this comment.
var link kept as var — needed because shouldBe() uses eval() which can't access let variables from a different script scope.
| var link = document.getElementById("link"); | ||
|
|
||
| function clickLink() { | ||
| var rect = link.getBoundingClientRect(); |
There was a problem hiding this comment.
| var rect = link.getBoundingClientRect(); | |
| let rect = link.getBoundingClientRect(); |
| // thesaurus.com https://bugs.webkit.org/show_bug.cgi?id=312692 rdar://174959285 | ||
| // dictionary.com https://bugs.webkit.org/show_bug.cgi?id=312692 rdar://174959285 |
There was a problem hiding this comment.
These both point to the thesaurus.com bug, I would adjust the bug trackers for dictionary.com
| QUIRKS_EARLY_RETURN_IF_NOT_DOMAIN("thesaurus.com"_s); | ||
|
|
||
| quirksData.isThesaurus = true; | ||
| quirksData.enableQuirk(QuirksData::SiteSpecificQuirk::NeedsScriptToEvaluateBeforeRunningScriptFromURLQuirk); |
There was a problem hiding this comment.
Shouldn't these quirks only be enabled for #if ENABLE(DESKTOP_CONTENT_MODE_QUIRKS) configs?
There was a problem hiding this comment.
(previously, these were only enabled in the #if PLATFORM(IOS_FAMILY) && ENABLE(DESKTOP_CONTENT_MODE_QUIRKS) path.)
There was a problem hiding this comment.
I wrapped it with #if PLATFORM(IOS_FAMILY) to match the original behavior — and that the original code was only IOS_FAMILY-guarded (not DESKTOP_CONTENT_MODE_QUIRKS), see lines 3775-3776 and 3853-3854.
There was a problem hiding this comment.
Sounds good, I don't feel strongly about this in either direction. There's a latent risk that somewhere else in newer code we may end up reading SiteSpecificQuirk::NeedsScriptToEvaluateBeforeRunningScriptFromURLQuirk as enabled on macOS, but not a big deal.
| QUIRKS_EARLY_RETURN_IF_NOT_DOMAIN("dictionary.com"_s); | ||
|
|
||
| quirksData.isDictionary = true; | ||
| quirksData.enableQuirk(QuirksData::SiteSpecificQuirk::NeedsScriptToEvaluateBeforeRunningScriptFromURLQuirk); |
There was a problem hiding this comment.
Same comment here about DESKTOP_CONTENT_MODE_QUIRKS
There was a problem hiding this comment.
Similar issue as above.
aprotyas
left a comment
aprotyas
left a comment
There was a problem hiding this comment.
Thanks for iterating. Two action items for using the correct bug numbers and considering the scope of the NeedsScriptToEvaluateBeforeRunningScriptFromURLQuirk quirk.
swpatters
force-pushed
the
eng/HTMLAnchorElement-isMouseFocusable-returns-false-for-a-href-links-without-explicit-tabindex
branch
from
c16cc57 to
8407f6d
Compare
|
EWS run on previous version of this PR (hash 8407f6d) Details |
swpatters
force-pushed
the
eng/HTMLAnchorElement-isMouseFocusable-returns-false-for-a-href-links-without-explicit-tabindex
branch
from
8407f6d to
eca26e0
Compare
|
EWS run on previous version of this PR (hash eca26e0) Details |
…esaurus.com and dictionary.com https://bugs.webkit.org/show_bug.cgi?id=312692 rdar://174959285 Clicking links on thesaurus.com and dictionary.com causes blur events with a null relatedTarget, breaking dropdown menus that rely on relatedTarget to stay open. Rather than changing the default behavior for all sites, add a site-specific quirk following the existing needsFormControlToBeMouseFocusable pattern. The broader behavior discussion is tracked in bug 22261. Widen platform guards from PLATFORM(MAC) to PLATFORM(COCOA) so the quirk is also effective on iPad desktop-class browsing and visionOS. * LayoutTests/fast/events/anchor-mouse-focusable-quirk-expected.txt: Added. * LayoutTests/fast/events/anchor-mouse-focusable-quirk.html: Added. * Source/WebCore/html/HTMLAnchorElement.cpp: (WebCore::HTMLAnchorElement::isMouseFocusable): * Source/WebCore/page/Quirks.cpp: (WebCore::Quirks::needsAnchorToBeMouseFocusable): (WebCore::handleThesaurusQuirks): (WebCore::handleDictionaryQuirks): (WebCore::Quirks::determineRelevantQuirks): * Source/WebCore/page/Quirks.h: * Source/WebCore/page/QuirksData.h:
swpatters
force-pushed
the
eng/HTMLAnchorElement-isMouseFocusable-returns-false-for-a-href-links-without-explicit-tabindex
branch
from
eca26e0 to
867436f
Compare
|
EWS run on current version of this PR (hash 867436f) Details |
webkit-ews-buildbot
added
the
merging-blocked
9 participants
69a12bd
867436f
🛠 playstation