Bug 165279 - Implement inert attribute (affects keyboard, events, and accessibility)
Summary: Implement inert attribute (affects keyboard, events, and accessibility)
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Accessibility (show other bugs)
Version: WebKit Nightly Build
Hardware: All All
: P2 Normal
Assignee: Tim Nguyen (:ntim)
URL:
Keywords: InRadar
: 159748 (view as bug list)
Depends on: 239017 240009 110952 229330 229410 229728 230474 230680 230845 231741 235668 235836 236512 237033 237244 239831
Blocks:
  Show dependency treegraph
 
Reported: 2016-12-01 14:31 PST by James Craig
Modified: 2022-05-03 07:31 PDT (History)
30 users (show)

See Also:


Attachments
testcase (5.02 KB, text/html)
2020-04-16 07:43 PDT, Frédéric Wang (:fredw)
no flags Details
WIP Patch (4.57 KB, patch)
2020-04-16 07:47 PDT, Frédéric Wang (:fredw)
no flags Details | Formatted Diff | Diff
Patch (62.43 KB, patch)
2021-08-20 05:48 PDT, Tim Nguyen (:ntim)
no flags Details | Formatted Diff | Diff
Patch (67.34 KB, patch)
2021-08-20 08:56 PDT, Tim Nguyen (:ntim)
no flags Details | Formatted Diff | Diff
Patch (69.46 KB, patch)
2021-08-23 07:08 PDT, Tim Nguyen (:ntim)
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (69.27 KB, patch)
2021-08-23 07:14 PDT, Tim Nguyen (:ntim)
no flags Details | Formatted Diff | Diff
Patch (68.02 KB, patch)
2021-08-23 09:04 PDT, Tim Nguyen (:ntim)
no flags Details | Formatted Diff | Diff
Patch (68.01 KB, patch)
2021-08-23 09:28 PDT, Tim Nguyen (:ntim)
no flags Details | Formatted Diff | Diff
Patch (68.08 KB, patch)
2021-08-23 13:15 PDT, Tim Nguyen (:ntim)
no flags Details | Formatted Diff | Diff
Patch (67.91 KB, patch)
2021-08-24 00:29 PDT, Tim Nguyen (:ntim)
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description James Craig 2016-12-01 14:31:39 PST
WICG discussion happening here:

https://discourse.wicg.io/t/inert-attribute/1838
Comment 1 Radar WebKit Bug Importer 2016-12-01 14:32:16 PST
<rdar://problem/29467435>
Comment 2 James Craig 2016-12-01 15:28:40 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?
Comment 3 James Craig 2016-12-01 15:52:47 PST
Also discussion re: naming conflict with template: https://github.com/WICG/inert/issues/39
Comment 4 James Craig 2016-12-08 18:54:45 PST
*** Bug 159748 has been marked as a duplicate of this bug. ***
Comment 5 James Craig 2016-12-08 18:59:03 PST
Relating to <dialog> bug 84635
Comment 6 Chris Rebert 2016-12-08 19:50:48 PST
See also: https://github.com/whatwg/html/pull/1474
Comment 7 James Craig 2017-05-27 00:50:26 PDT
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
Comment 8 Carolyn MacLeod 2017-10-06 19:11:24 PDT
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
Comment 9 Michael Puckett 2018-09-03 05:44:42 PDT
Please consider adding to the feature status list here:

https://webkit.org/status
Comment 10 Frédéric Wang (:fredw) 2020-04-16 07:43:38 PDT
Created attachment 396647 [details]
testcase
Comment 11 Frédéric Wang (:fredw) 2020-04-16 07:47:18 PDT
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 12 Frédéric Wang (:fredw) 2020-04-16 07:52:27 PDT
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.
Comment 13 Carolyn MacLeod 2021-04-09 05:33:24 PDT
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.
Comment 14 Carolyn MacLeod 2021-04-09 05:55:46 PDT
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.
Comment 15 Tim Nguyen (:ntim) 2021-07-17 15:32:09 PDT
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.
Comment 16 Tim Nguyen (:ntim) 2021-08-20 05:48:34 PDT
Created attachment 435977 [details]
Patch
Comment 17 Tim Nguyen (:ntim) 2021-08-20 08:56:08 PDT
Created attachment 435994 [details]
Patch
Comment 18 Sam Weinig 2021-08-20 11:54:26 PDT
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 19 Simon Fraser (smfr) 2021-08-20 12:04:00 PDT
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.
Comment 20 Tim Nguyen (:ntim) 2021-08-20 13:53:35 PDT
(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.
Comment 21 Tim Nguyen (:ntim) 2021-08-23 07:08:10 PDT
Created attachment 436191 [details]
Patch
Comment 22 Tim Nguyen (:ntim) 2021-08-23 07:14:02 PDT
Created attachment 436192 [details]
Patch
Comment 23 Chris Dumez 2021-08-23 08:08:40 PDT
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?
Comment 24 Tim Nguyen (:ntim) 2021-08-23 09:04:08 PDT
Created attachment 436200 [details]
Patch
Comment 25 Chris Dumez 2021-08-23 09:15:13 PDT
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.
Comment 26 Tim Nguyen (:ntim) 2021-08-23 09:28:14 PDT
Created attachment 436203 [details]
Patch
Comment 27 EWS 2021-08-23 11:20:09 PDT
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 28 Ryosuke Niwa 2021-08-23 13:15:20 PDT
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.
Comment 29 Tim Nguyen (:ntim) 2021-08-23 13:15:32 PDT
Created attachment 436228 [details]
Patch
Comment 30 Ryosuke Niwa 2021-08-23 13:16:15 PDT
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 31 Emilio Cobos Álvarez (:emilio) 2021-08-23 16:47:03 PDT
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?
Comment 32 Tim Nguyen (:ntim) 2021-08-24 00:29:12 PDT
Created attachment 436265 [details]
Patch
Comment 33 EWS 2021-08-24 01:18:01 PDT
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].