The HTML spec defines the concept of inert subtrees. When a Document is blocked by a modal <dialog>, every node not an ascendant or descendant of the dialog is considered inert: http://www.whatwg.org/specs/web-apps/current-work/multipage/editing.html#inert-subtrees
Created attachment 190459 [details] WIP patch
I uploaded an early WIP patch. Comments are welcome. My idea is to reuse disabled for inert, so we automatically get the behavior for event targeting and focus that we want. My first attempt to was reuse the Fullscreen implementation, but it turns out it doesn't actually implement modality. You can use tab and space to click a button behind the fullscreened element. It may be problematic to merge inertness and disabled though. With this patch, an inert button is automatically grayed out, but the spec says there should be no UI indication of inertness from the UA. Also, we must support the "inert" attribute which is separate from "disabled".
Comment on attachment 190459 [details] WIP patch Attachment 190459 [details] did not pass chromium-ews (chromium-xvfb): Output: http://webkit-commit-queue.appspot.com/results/16663197 New failing tests: fast/dom/HTMLDialogElement/modal-dialog-blocks-mouse-events.html
Comment on attachment 190459 [details] WIP patch Attachment 190459 [details] did not pass chromium-ews (chromium-xvfb): Output: http://webkit-commit-queue.appspot.com/results/16818139 New failing tests: fast/dom/HTMLDialogElement/modal-dialog-blocks-mouse-events.html
I think there is another tangible difference between disabled and inert. A disabled button swallows a mouse click, but the spec seems to dictate that a mouse events be dispatched to a non-inert node rendered underneath an inert node, possibly the <body>. So I think the hit testing code would have to be modified to ignore inert nodes. Does that sound right? From the spec: "For example, consider a page that consists of just a single inert paragraph positioned in the middle of a body. If a user moves their pointing device from the body over to the inert paragraph and clicks on the paragraph, no mouseover event would be fired, and the mousemove and click events would be fired on the body element rather than the paragraph."
I think I can get the desired hit testing by modifying RenderObject::visibleToHitTesting(). As a first step, I'd like to try to land this patch as is. Then the hit test behavior and lack of appearance change for inert nodes can be added in later patches.
Comment on attachment 190459 [details] WIP patch Is it enough to cover only form controls? How about <a>, or even JS-based interactivity?
Ah, they are covered by Node::disabled().
(In reply to comment #8) > Ah, they are covered by Node::disabled(). Yes, it covers at least <a> and calling click() programatically. I can add more tests, though I'm not sure how to best structure them (we probably don't want a test for every possible element...).
Comment on attachment 190459 [details] WIP patch Could you add tests for - non-modal dialogs and - non-active dialogs to cover negative cases?
Created attachment 192181 [details] add test cases for non-modal and non-open dialogs
Comment on attachment 192181 [details] add test cases for non-modal and non-open dialogs I'm a bit worrying about the perf implication of this change since this makes disabled() slower. But I'm not sure if there are good test to cover such scenarios. disabled() might not called that much, after all. Let's land this and see how graphs react.
Comment on attachment 192181 [details] add test cases for non-modal and non-open dialogs Clearing flags on attachment: 192181 Committed r145340: <http://trac.webkit.org/changeset/145340>
All reviewed patches have been landed. Closing bug.
Comment on attachment 192181 [details] add test cases for non-modal and non-open dialogs View in context: https://bugs.webkit.org/attachment.cgi?id=192181&action=review > Source/WebCore/dom/Node.cpp:2467 > bool Node::disabled() const > { > +#if ENABLE(DIALOG_ELEMENT) > + if (isInert()) > + return true; > +#endif > return false; > } The code added here to Node::disabled should instead go into HTMLFormControlElement::disabled. Node is too low a level to hook this in. In fact, we should investigate removing or renaming Node::disabled because I don’t think HTMLStyleElement::disabled and SVGStyleElement::disabled are intending to override this function! I think it’s only in the Node class to support the way it hooks into the style system. We don’t want arbitrary nodes like text nodes or arbitrary elements to return true for disabled and match disabled pseudo-style rules. Just form controls. > Source/WebCore/dom/Node.h:418 > + bool isInert() const; This should also go into HTMLFormConrolElement.
Comment on attachment 192181 [details] add test cases for non-modal and non-open dialogs View in context: https://bugs.webkit.org/attachment.cgi?id=192181&action=review >> Source/WebCore/dom/Node.h:418 >> + bool isInert() const; > > This should also go into HTMLFormConrolElement. Maybe not. It might be OK for this to stay in Node, or perhaps move to Element. I am not sure where else we’d want to call isInert. Generally, though, the answer to “Should I add something to the Node class?” is “no!”.
(In reply to comment #16) > (From update of attachment 192181 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=192181&action=review > > >> Source/WebCore/dom/Node.h:418 > >> + bool isInert() const; > > > > This should also go into HTMLFormConrolElement. > > Maybe not. It might be OK for this to stay in Node, or perhaps move to Element. I am not sure where else we’d want to call isInert. > > Generally, though, the answer to “Should I add something to the Node class?” is “no!”. Agreed. Moving things on Node down to Element is good way to go in general. And doing it would be a good refactoring. Filed Bug 112085.
(In reply to comment #17) > Agreed. Moving things on Node down to Element is good way to go in general. > And doing it would be a good refactoring. Filed Bug 112085. Yes, it’s OK to move isInert to Element. But the disabled logic should be moved to HTMLFormControlElement::disabled. We don’t want inert non-form-control elements to match :disabled style rules. We also should have test cases covering that.
This was all rolled out in bug 120467.
This should re-land combined with bug 112085
<rdar://problem/80013802>
Created attachment 435961 [details] Patch
Created attachment 435967 [details] Patch
Committed r281309 (240729@main): <https://commits.webkit.org/240729@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 435967 [details].