921504 - Implement the HTML inert subtrees

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, enhancement, P3)

Description

[steve faulkner]

the inert attribute
http://www.w3.org/html/wg/drafts/html/master/editing.html#the-inert-attribute
inert subtrees
http://www.w3.org/html/wg/drafts/html/master/editing.html#inert-subtrees

Comment 1

[Michael[tm] Smith [:sideshowbarker]]

As far as I know, this isn't an issue for the HTML parser. The spec doesn't require any special parsing behavior for the "inert" attribute. But I don't know what other component would be appropriate instead.

Comment 2

[Michael[tm] Smith [:sideshowbarker]]

Sorry for the noise, I see the component has already been changed. I'll shut up now...

Comment 3

[Anne (:annevk)]

There's no such attribute. If we implement this concept it'll be as part of <dialog>. There's no need for this bug as far as I can tell.

Comment 4

[Michael[tm] Smith [:sideshowbarker]]

Yeah, for the record here, the attribute was dropped from the spec more than year ago https://html5.org/r/8536

Comment 5

[Marcos Caceres [:marcosc]]

so, Chrome is bringing back the "inert" attribute: 

https://github.com/WICG/inert

Their intent to implemement:
https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/N--HhuYFJQI

Should we reconsider? Seems useful.

Comment 6

[David Bolter [:davidb] (NeedInfo me for attention)]

Cc'ing :ntim based on bug 1322939 comment 25.

Comment 7

[David Bolter [:davidb] (NeedInfo me for attention)]

Further to Chrome's intent to implement, it is shipping behind a flag now: https://twitter.com/rob_dodson/status/868296999375847424
Webkit bug is: https://bugs.webkit.org/show_bug.cgi?id=165279

Comment 8

[Marcos Caceres [:marcosc]]

The little video in the tweet shows how tremendously useful this will be (beyond `dialog`).

[1] https://twitter.com/rob_dodson/status/868296999375847424

Comment 9

[aboxhall]

The video in the tweet illustrates one use case which is easy to show visually, but you could reasonably make the argument that a slide-in menu is <dialog> like.

However, a related, important case is when that slide-in menu is offscreen. You want to keep it laid out to avoid paying that cost each time it slides in and out, but if it's laid out then the focusable items in the menu are in the tab order. `inert` allows you to temporarily remove them from both the tab order and from assistive technology while not forcing the offscreen content to re-render each time you show it.

https://www.youtube.com/watch?v=fGLp_gfMMGU goes into this case in more detail.

Comment 10

[Carolyn MacLeod]

Another video showing the modal dialog use case: https://www.youtube.com/watch?v=JS68faEUduk

Comment 11

[Carolyn MacLeod]

Note that this bug has now been opened for Edge: https://wpdev.uservoice.com/forums/257854-microsoft-edge-developer/suggestions/31606522-support-the-inert-attribute

Comment 12

[Tim Nguyen :ntim]

Unassigning to reflect real status.

Comment 13

[Olli Pettay [:smaug][[e-mail]]]

There has been some interested on inert, but it is still unclear how inert should work with shadow DOM.
See comments on https://github.com/whatwg/html/pull/4288

Comment 14

[Tim Nguyen :ntim]

I'd consider this as an enhancement on top of <dialog> (and depending on its implementation, since it will make this bug easier).

Comment 15

[Frédéric Wang (:fredw)]

(In reply to Tim Nguyen :ntim from comment #14)

I'd consider this as an enhancement on top of <dialog> (and depending on its implementation, since it will make this bug easier).

@Tim: Can you please explain the rationale behind inverting thi dependency? I'm told by my colleague Brian Kardell that one use case of the inert attribute is to allow a polyfill for the <dialog> element, so from the user point of view it seems it will be good to have the inert attribute before the <dialog> element. Do you think it would be possible to implement the inert attribute without finishing bug 840640 first and in any case how difficult it would be to implement the internal logic specific to inert only (i.e. ignoring nodes for UI purpose). It's not clear to me if bug 840640 comment 36 says it's really non-trivial or if that's only referring only to the "top layer stack".

Comment 16

[Tim Nguyen :ntim]

Can you please explain the rationale behind inverting thi dependency?

The inert attribute and inert subtrees are different (but related) concepts. Both the inert attribute and the dialog element require inert subtrees to be supported, but the inert attribute isn't officially part of the spec (and the chromium implementation has been removed), so it makes sense that <dialog> is worked on first.

Do you think it would be possible to implement the inert attribute without finishing bug 840640 first and in any case how difficult it would be to implement the internal logic specific to inert only (i.e. ignoring nodes for UI purpose).

I think the inert logic (inert subtrees) is quite a chunk of work, but it is needed to implement dialog anyway, bug 1200896 will cover that.

There's someone currently working on dialog, so it shouldn't be too much time until this is done.

Comment 17

[alexander :surkov (:asurkov)]

(In reply to Tim Nguyen :ntim from comment #16)

Can you please explain the rationale behind inverting thi dependency?

The inert attribute and inert subtrees are different (but related) concepts. Both the inert attribute and the dialog element require inert subtrees to be supported, but the inert attribute isn't officially part of the spec (and the chromium implementation has been removed), so it makes sense that <dialog> is worked on first.

there's ongoing work to readd inert into the spec https://github.com/whatwg/html/pull/4288

Comment 18

[Frédéric Wang (:fredw)]

Thanks (In reply to Tim Nguyen :ntim from comment #16)

I think the inert logic (inert subtrees) is quite a chunk of work, but it is needed to implement dialog anyway, bug 1200896 will cover that.

There's someone currently working on dialog, so it shouldn't be too much time until this is done.

Thanks for the explanation Tim! So if the dialog / inert subtrees will be done soon, it seems it makes sense to wait a bit and supporting the inert attribute would be more or less straightforward since the need logic will already be in place.

Comment 19

[Sean Feng [:sefeng]]

I'd like to know more about the status of the inert attribute, is it going to be actively prototyped soon? Are there any blockers preventing us from prototyping it?

fredw, asurkov, I assume one of you will be prototyping it?

Comment 20

[alexander :surkov (:asurkov)]

(In reply to Sean Feng [:sefeng] from comment #19)

I'd like to know more about the status of the inert attribute, is it going to be actively prototyped soon? Are there any blockers preventing us from prototyping it?

Just sent intent to prototype, https://groups.google.com/forum/#!topic/mozilla.dev.platform/DS9qGnqc_3Q. Emilio suggested to prototype it via -moz-inert pseudo class which is defined as

:moz-inert {
user-select: none !important;
pointer-events: none !important;
cursor: default !important;
}

which seems reasonable to me. So I'm not aware of any blockers.

fredw, asurkov, I assume one of you will be prototyping it?

I'm in I think.

Comment 21

[Tim Nguyen :ntim]

(In reply to alexander :surkov (:asurkov) from comment #20)

(In reply to Sean Feng [:sefeng] from comment #19)

I'd like to know more about the status of the inert attribute, is it going to be actively prototyped soon? Are there any blockers preventing us from prototyping it?

Just sent intent to prototype, https://groups.google.com/forum/#!topic/mozilla.dev.platform/DS9qGnqc_3Q. Emilio suggested to prototype it via -moz-inert pseudo class which is defined as

:moz-inert {
user-select: none !important;
pointer-events: none !important;
cursor: default !important;
}

which seems reasonable to me. So I'm not aware of any blockers.

FWIW, I tried something similar in https://hg.mozilla.org/mozreview/gecko/rev/93a1a5f4a9c19531d45d0cc20f76746cb8cd2b77#index_header , but IIRC it didn't work super well, as I think I could still tab through things.

Comment 22

[Emilio Cobos Álvarez (:emilio)]

That patch above is wrong, what takes care of calling UpdateState on the DOM so that the right bits end up there?

A potentially simpler alternative is adding an internal -moz-inert property, rather than pseudo-class, and do something like:

:inert {
  -moz-inert: inert;
}

dialog:-moz-modal-dialog {
  -moz-inert: none;
}

And either check for -moz-inert in the relevant places, or make StyleAdjuster set user-select / etc based on -moz-inert. Then the modal dialog code would just do OwnerDoc()->GetRootElement()->AddState(NS_EVENT_STATE_INERT) or such to make the whole document inert, except the modal dialog.

Not sure which of the two approaches would end up simpler off-hand.

Comment 23

[alexander :surkov (:asurkov)]

Attachment: Bug 921504 - implement -moz-inert CSS property

Comment 24

[alexander :surkov (:asurkov)]

Attachment: Bug 921504 - add HTMLElement::inert

Depends on D81701

Comment 25

[alexander :surkov (:asurkov)]

Having those two patches applied I have these wpt failures left:

• inert-node-is-uneditable.tentative.html because send_keys not implemented yet (not inert related)
• inert-node-is-unselectable.tentative.html because document.execCommand('SelectAll') seems has no effect at all (not inert related)
• inert-retargeting-iframe.tentative.html and inert-retargeting.tentative.html - event retargeting errors, for example, this test https://searchfox.org/mozilla-central/source/testing/web-platform/tests/inert/inert-retargeting-iframe.tentative.html#221-243

Emilio, could you take a look please at event retargeting tests, is it something related with pointer-events:none thing we used to implement inert subtrees?

Comment 26

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/eb53880e5f0e
implement HTML:inert r=emilio,heycam
https://hg.mozilla.org/integration/autoland/rev/e87edffd46b9
add HTMLElement::inert r=emilio

Comment 27

[Narcis Beleuzu [:NarcisB]]

Backed out for mochitest failures on test_animation-type-longhand.html

Backout link: https://hg.mozilla.org/integration/autoland/rev/3c7eb39d39d1903e3bc3ef7091f8325bb1af8275
Log link: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=310387679&repo=autoland&lineNumber=2795

Comment 28

[Narcis Beleuzu [:NarcisB]]

Please also check:

1. Mochitest failures on test_property_database.html -> https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=310388326&repo=autoland&lineNumber=2493
2. wpt failures on inert-in-shadow-dom.tentative.html -> https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=310390291&repo=autoland&lineNumber=7639

Comment 29

[alexander :surkov (:asurkov)]

(In reply to Narcis Beleuzu [:NarcisB] from comment #27)

Backed out for mochitest failures on test_animation-type-longhand.html

this one fails because -moz-inert is not listed in devtools longhands [1], I suppose adding -moz-inert into that file will be a fix for the issue

(In reply to Narcis Beleuzu [:NarcisB] from comment #28)

1. Mochitest failures on test_property_database.html -> https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=310388326&repo=autoland&lineNumber=2493

this one fails because -moz-inert is not listed in gCSSProperties [2], when I add an entry for it, I run into other errors though. So if I set domProp: "MozInert" for the entry then I get this failure: FAIL '-moz-inert' listed with correct DOM property name - got "MozInert", expected null. If set it to null, then I get another one: "DOM property for -moz-inert - got null, expected "MozInert" at [3]. The question is whether MozInert should be exposed via DOM or not. If not, then I suppose I should adjust "Test that DOM properties match the expected rules" to filter it out.

2. wpt failures on inert-in-shadow-dom.tentative.html -> https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=310390291&repo=autoland&lineNumber=7639

This is a weird one because it doesn't fail locally, apparently the problem is an element.focus() may focus the element. Any ideas where to look?

Emilio, could you please take a look at these findings and comment them?

[1] https://searchfox.org/mozilla-central/source/devtools/server/actors/animation-type-longhand.js
[2] https://searchfox.org/mozilla-central/source/layout/style/test/property_database.js#1336
[3] https://searchfox.org/mozilla-central/source/layout/style/test/test_property_database.html#175

Comment 30

[Emilio Cobos Álvarez (:emilio)]

(In reply to alexander :surkov (:asurkov) from comment #29)

(In reply to Narcis Beleuzu [:NarcisB] from comment #27)

Backed out for mochitest failures on test_animation-type-longhand.html

this one fails because -moz-inert is not listed in devtools longhands [1], I suppose adding -moz-inert into that file will be a fix for the issue

Run ./mach devtools-css-db.

(In reply to Narcis Beleuzu [:NarcisB] from comment #28)

1. Mochitest failures on test_property_database.html -> https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=310388326&repo=autoland&lineNumber=2493

this one fails because -moz-inert is not listed in gCSSProperties [2], when I add an entry for it, I run into other errors though. So if I set domProp: "MozInert" for the entry then I get this failure: FAIL '-moz-inert' listed with correct DOM property name - got "MozInert", expected null. If set it to null, then I get another one: "DOM property for -moz-inert - got null, expected "MozInert" at [3]. The question is whether MozInert should be exposed via DOM or not. If not, then I suppose I should adjust "Test that DOM properties match the expected rules" to filter it out.

Add -moz-inert to this list.

2. wpt failures on inert-in-shadow-dom.tentative.html -> https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=310390291&repo=autoland&lineNumber=7639

This is a weird one because it doesn't fail locally, apparently the problem is an element.focus() may focus the element. Any ideas where to look?

For this one I don't have a better suggestion than logging something inside nsIFrame::IsFocusable / Element::IsFocusable and so on. Though I don't have a great idea of how that could possibly happen given nsFocusManager::FlushAndCheckIfFocusable well, flushes properly

Comment 31

[alexander :surkov (:asurkov)]

(In reply to Emilio Cobos Álvarez (:emilio) from comment #30)

2. wpt failures on inert-in-shadow-dom.tentative.html -> https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=310390291&repo=autoland&lineNumber=7639

This is a weird one because it doesn't fail locally, apparently the problem is an element.focus() may focus the element. Any ideas where to look?

For this one I don't have a better suggestion than logging something inside nsIFrame::IsFocusable / Element::IsFocusable and so on. Though I don't have a great idea of how that could possibly happen given nsFocusManager::FlushAndCheckIfFocusable well, flushes properly

it turns out that inert pref is off when the test is running, i.e. StaticPrefs::html5_inert_enabled() returns false when nsIFrame::IsFocusable is called. Curious if dir.ini is ignored by any chance?

Comment 32

[alexander :surkov (:asurkov)]

(In reply to alexander :surkov (:asurkov) from comment #31)

it turns out that inert pref is off when the test is running, i.e. StaticPrefs::html5_inert_enabled() returns false when nsIFrame::IsFocusable is called. Curious if dir.ini is ignored by any chance?

because there's no dir.ini I suppose :)

Comment 33

[alexander :surkov (:asurkov)]

Emilio, while you are needinfoed, could you please take a look at comment #25?

Comment 34

[Emilio Cobos Álvarez (:emilio)]

Note that the file should be __dir__.ini. Re coment 25, what is the exact error? It's hard to know the cause otherwise. But yeah it seems like the odd magical difference between inert and pointer events.

Comment 35

[alexander :surkov (:asurkov)]

(In reply to Emilio Cobos Álvarez (:emilio) from comment #34)

Re coment 25, what is the exact error? It's hard to know the cause otherwise. But yeah it seems like the odd magical difference between inert and pointer events.

Ok, I will file a follow up on this one if it's ok.

Comment 36

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a56b2d354613
implement HTML:inert r=emilio,heycam
https://hg.mozilla.org/integration/autoland/rev/8b75cd744e80
add HTMLElement::inert r=emilio

Comment 37

[Narcis Beleuzu [:NarcisB]]

Backed out for wpt failures on inert-retargeting-iframe.tentative.html

Backout link: https://hg.mozilla.org/integration/autoland/rev/87b2cdd65128b015280c7a5429f9ef26bdb83d15
Log link: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=310756140&repo=autoland&lineNumber=2988

Comment 38

[alexander :surkov (:asurkov)]

(In reply to Narcis Beleuzu [:NarcisB] from comment #37)

Backed out for wpt failures on inert-retargeting-iframe.tentative.html

Backout link: https://hg.mozilla.org/integration/autoland/rev/87b2cdd65128b015280c7a5429f9ef26bdb83d15
Log link: https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=310756140&repo=autoland&lineNumber=2988

weird that ./mach try auto doesn't handle that, I suppose it will be better to split the patches into multiple chunks: -moz-inert property, HTML:inert attribute, HTML:inert idl and land them independently: might be easier to get them stick on m-c.

Comment 39

[alexander :surkov (:asurkov)]

Attachment: Bug 921504 - implement -moz-inert CSS property HTML:dialog and HTML:inert

-moz-inert CSS property reflect inert subtrees concept and can be used to implement HTML:dialog element and HTML:inert attribute

Comment 40

[alexander :surkov (:asurkov)]

Attachment: Bug 921504 - implement HTML:inert attribute

Depends on D84614

Comment 41

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/beb139133417
implement -moz-inert CSS property r=emilio

Comment 42

[Alexandru Michis [:malexandru]]

https://hg.mozilla.org/mozilla-central/rev/beb139133417

Comment 43

[alexander :surkov (:asurkov)]

few patches to land are left

Comment 44

[Matthew N. [:MattN]]

(In reply to alexander :surkov (:asurkov) from comment #43)

few patches to land are left

You shouldn't land some patches in a bug unless you know the rest will make it in the same release. That first commit landed in Fx80 but the future ones will be Fx81+ and that will confuse web developers, QA, relman, etc. Can we instead re-summarize this bug and take the remaining patches to a new bug?

Comment 45

[alexander :surkov (:asurkov)]

(In reply to Matthew N. [:MattN] from comment #44)

(In reply to alexander :surkov (:asurkov) from comment #43)

few patches to land are left

You shouldn't land some patches in a bug unless you know the rest will make it in the same release. That first commit landed in Fx80 but the future ones will be Fx81+ and that will confuse web developers, QA, relman, etc.

the landed part is not exposed to the web, so technically shouldn't confuse webdevs and perhaps QA but

Can we instead re-summarize this bug and take the remaining patches to a new bug?

I think I can do it for sure.

Comment 46

[alexander :surkov (:asurkov)]

filed bug for HTML:inert, bug 1655722 and keeping this one for HTML inert subtrees implemented as -moz-inert CSS property

Comment 47

[Matthew N. [:MattN]]

(In reply to alexander :surkov (:asurkov) from comment #45)

(In reply to Matthew N. [:MattN] from comment #44)

(In reply to alexander :surkov (:asurkov) from comment #43)

few patches to land are left

You shouldn't land some patches in a bug unless you know the rest will make it in the same release. That first commit landed in Fx80 but the future ones will be Fx81+ and that will confuse web developers, QA, relman, etc.

the landed part is not exposed to the web, so technically shouldn't confuse webdevs and perhaps QA but

Sure, but if one of your next patches causes issues people will think they affect Fx80 since this bug has that as the target milestone and first :fixed status flag version.

Can we instead re-summarize this bug and take the remaining patches to a new bug?

I think I can do it for sure.

Thanks. Note that you can edit the bug number field (and commit message bug #) in Phabricator to have the patches moved to the new bug to preserve the review comments and Differential ID.

Comment 48

[Phabricator Automation]

Comment on attachment 9165556 [details]
Bug 921504 - implement HTML:inert attribute

Revision D84615 was moved to bug 1655722. Setting attachment 9165556 [details] to obsolete.

Comment 49

[Phabricator Automation]

Comment on attachment 9162533 [details]
Bug 921504 - add HTMLElement::inert

Revision D82943 was moved to bug 1655722. Setting attachment 9162533 [details] to obsolete.

Comment 50

[alexander :surkov (:asurkov)]

(In reply to Matthew N. [:MattN] from comment #47)

(In reply to alexander :surkov (:asurkov) from comment #45)

(In reply to Matthew N. [:MattN] from comment #44)

(In reply to alexander :surkov (:asurkov) from comment #43)

few patches to land are left

You shouldn't land some patches in a bug unless you know the rest will make it in the same release. That first commit landed in Fx80 but the future ones will be Fx81+ and that will confuse web developers, QA, relman, etc.

the landed part is not exposed to the web, so technically shouldn't confuse webdevs and perhaps QA but

Sure, but if one of your next patches causes issues people will think they affect Fx80 since this bug has that as the target milestone and first :fixed status flag version.

fair enough

Can we instead re-summarize this bug and take the remaining patches to a new bug?

I think I can do it for sure.

Thanks. Note that you can edit the bug number field (and commit message bug #) in Phabricator to have the patches moved to the new bug to preserve the review comments and Differential ID.

yep, thanks!