Summary: | Implement inert attribute (affects keyboard, events, and accessibility) | ||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | James Craig <jcraig> | ||||||||||||||||||||||
Component: | Accessibility | Assignee: | Tim Nguyen (:ntim) <ntim> | ||||||||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||||||||
Severity: | Normal | CC: | 709922234, agibson, bugzilla, carolynmacleod4, Carolyn_MacLeod, cdumez, changseok, cigitia, clopez, craig+webkit, emilio, esprehn+autocc, ews-watchlist, fred.wang, fweber, gyuyoung.kim, herr.ernst, hidde, info, kangil.han, kondapallykalyan, michaelcpuckett, ntim, rniwa, robdodson, sam, simon.fraser, webkit-bug-importer, webkit, youennf | ||||||||||||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||||||||||||
Version: | WebKit Nightly Build | ||||||||||||||||||||||||
Hardware: | All | ||||||||||||||||||||||||
OS: | All | ||||||||||||||||||||||||
See Also: | https://bugzilla.mozilla.org/show_bug.cgi?id=921504 | ||||||||||||||||||||||||
Bug Depends on: | 240009, 110952, 229330, 229410, 229728, 230474, 230680, 230845, 231741, 235668, 235836, 236512, 237033, 237244, 239017, 239831, 241022 | ||||||||||||||||||||||||
Bug Blocks: | |||||||||||||||||||||||||
Attachments: |
|
Description
James Craig
2016-12-01 14:31:39 PST
There are obvious benefits for accessibility. Are there concerns from WebKit implementors about the functionality change or implementability of for events handling and keyboard/focus behavior? Also discussion re: naming conflict with template: https://github.com/WICG/inert/issues/39 *** Bug 159748 has been marked as a duplicate of this bug. *** See also: https://github.com/whatwg/html/pull/1474 Google this shipping behind a flag now. Enable: chrome://flags#enable-experimental-web-platform-features Explainer: https://github.com/WICG/inert/blob/master/README.md Chrome "Intent to Implement" from February: https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/N--HhuYFJQI FF bug was reopened in May/17: https://bugzilla.mozilla.org/show_bug.cgi?id=921504 Edge bug opened in Sept/17 (already has 187 votes): https://wpdev.uservoice.com/forums/257854-microsoft-edge-developer/suggestions/31606522-support-the-inert-attribute Please consider adding to the feature status list here: https://webkit.org/status Created attachment 396647 [details]
testcase
Created attachment 396649 [details]
WIP Patch
Just a quick proof-of-concept patch. It seems to at least work for links. Probably there are several cases to check and things to verify, but that gives a starting point.
Comment on attachment 396649 [details] WIP Patch View in context: https://bugs.webkit.org/attachment.cgi?id=396649&action=review > Source/WebCore/dom/Element.h:473 > + virtual bool isDisabledFormControl() const { return isInert(); } Obviously, this should also be done in derived class... it seems they don't call their parent. Nudge. :) Chrome/Edge/Opera and Firefox all have this ready and waiting behind a flag: https://caniuse.com/mdn-api_htmlelement_inert The HTML spec has an approved pull request waiting: https://github.com/whatwg/html/pull/4288 So it seems everybody is waiting on webkit/Safari. Hmmm, I guess people are also waiting on https://github.com/whatwg/html/issues/5650 and https://github.com/w3c/html-aam/issues/295. If you can help move those conversations forward from a webkit point of view, please do. I'm going to start implementing inert subtrees in bug 110952 in the next weeks, though only the minimum needed for <dialog> element. I suspect it won't be hard to get the attribute working after that's properly done. Created attachment 435977 [details]
Patch
Created attachment 435994 [details]
Patch
Comment on attachment 435994 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=435994&action=review > Source/WebCore/html/HTMLElement.idl:60 > + // Experimental: https://github.com/WICG/inert/blob/master/README.md > + [CEReactions, Reflect, EnabledBySetting=InertAttribute] attribute boolean inert; I would put this above the Non-standard attributes (and below the attachInternals commented out attribute). Comment on attachment 435994 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=435994&action=review > Source/WebCore/dom/Node.cpp:2640 > + const Element* element = is<Element>(this) ? downcast<Element>(this) : parentElement(); > + while (element) { > + if (is<HTMLElement>(element) && element->hasAttribute(HTMLNames::inertAttr)) > + return true; > + element = element->parentElement(); Doing an ancestor tree walk every time this is called doesn't seem great for perf. (In reply to Simon Fraser (smfr) from comment #19) > Comment on attachment 435994 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=435994&action=review > > > Source/WebCore/dom/Node.cpp:2640 > > + const Element* element = is<Element>(this) ? downcast<Element>(this) : parentElement(); > > + while (element) { > > + if (is<HTMLElement>(element) && element->hasAttribute(HTMLNames::inertAttr)) > > + return true; > > + element = element->parentElement(); > > Doing an ancestor tree walk every time this is called doesn't seem great for > perf. Simon suggested adding a bit on Document that is set anytime one of the elements get the inert attribute, and only doing the ancestor walk in that case. Created attachment 436191 [details]
Patch
Created attachment 436192 [details]
Patch
Comment on attachment 436192 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=436192&action=review > Source/WebCore/dom/Document.h:1635 > + bool hasInertAttributeBeenUsed() { return m_hasInertAttributeBeenUsed; } should be const. > Source/WebCore/dom/Node.cpp:2636 > + const Element* element = is<Element>(this) ? downcast<Element>(this) : parentElement(); Seems like you should be using lineageOfType: for (auto& element : lineageOfType<HTMLElement>(*this)) { if (element->hasAttribute(HTMLNames::inertAttr)) return true; } > Source/WebCore/html/HTMLElement.cpp:486 > + document().setHasInertAttributeBeenUsed(); How does that work when an element gets moved from one document to another? Created attachment 436200 [details]
Patch
Comment on attachment 436200 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=436200&action=review r=me > Source/WebCore/dom/Node.cpp:2636 > + auto* element = is<Element>(this) ? downcast<Element>(this) : parentElement(); is<Element>(*this) to avoid an unnecessary null-check. Created attachment 436203 [details]
Patch
Found 30 new test failures: accessibility/accessibility-crash-focused-element-change.html, accessibility/accessibility-crash-setattribute.html, accessibility/accessibility-node-reparent.html, accessibility/mac/abbr-acronym-tags.html, accessibility/mac/accessibility-make-first-responder.html, accessibility/mac/accesskey.html, accessibility/mac/attributed-string/attributed-string-for-range-with-options.html, accessibility/mac/attributed-string/attributed-string-for-range.html, accessibility/mac/search-text/search-text.html, accessibility/mac/select-text/select-text-1.html ... Comment on attachment 436203 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=436203&action=review > Source/WebCore/dom/Node.cpp:2636 > + auto* element = is<Element>(*this) ? downcast<Element>(this) : parentElement(); Please use RefPtr. Created attachment 436228 [details]
Patch
Comment on attachment 436228 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=436228&action=review > Source/WebCore/dom/Node.cpp:2637 > + auto* element = is<Element>(*this) ? downcast<Element>(this) : parentElement(); > + if (element) { Please use RefPtr. Also declare the variable inside if like so: if (RefPtr element = ~) Comment on attachment 436228 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=436228&action=review > Source/WebCore/dom/Node.cpp:2639 > + if (is<HTMLElement>(ancestor) && ancestor.hasAttribute(HTMLNames::inertAttr)) Shouldn't this use the flat tree? Created attachment 436265 [details]
Patch
Committed r281490 (240864@main): <https://commits.webkit.org/240864@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 436265 [details]. |