Bug 110952

Summary: Initial implementation of inert subtrees needed for modal <dialog>
Product: WebKit Reporter: Matt Falkenhagen <falken>
Component: Layout and RenderingAssignee: Tim Nguyen (:ntim) <ntim>
Status: RESOLVED FIXED    
Severity: Normal CC: aboxhall, andresg_22, apinheiro, cdumez, cfleizach, cmarcelo, darin, dglazkov, dmazzoni, eoconnor, esprehn+autocc, ews-watchlist, jchaffraix, jcraig, jdiggs, kangil.han, koivisto, mifenton, morrita, ntim, ojan.autocc, ojan, samuel_white, tkent, webkit-bug-importer, webkit.review.bot
Priority: P2 Keywords: InRadar
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
See Also: https://bugs.webkit.org/show_bug.cgi?id=112085
Bug Depends on: 227800    
Bug Blocks: 84635, 165279    
Attachments:
Description Flags
WIP patch
none
add test cases for non-modal and non-open dialogs
none
Patch
none
Patch none

Matt Falkenhagen
Reported 2013-02-27 00:04:12 PST
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
Attachments
WIP patch (8.44 KB, patch)
2013-02-27 00:40 PST, Matt Falkenhagen 		no flags
DetailsFormatted DiffDiff
add test cases for non-modal and non-open dialogs (14.49 KB, patch)
2013-03-08 02:12 PST, Matt Falkenhagen 		no flags
DetailsFormatted DiffDiff
Patch (7.98 KB, patch)
2021-08-20 02:38 PDT, Tim Nguyen (:ntim) 		no flags
DetailsFormatted DiffDiff
Patch (7.49 KB, patch)
2021-08-20 03:48 PDT, Tim Nguyen (:ntim) 		no flags
DetailsFormatted DiffDiff
Show Obsolete (3) View All Add attachment proposed patch, testcase, etc.
Matt Falkenhagen
Comment 1 2013-02-27 00:40:56 PST
Created attachment 190459 [details] WIP patch
Matt Falkenhagen
Comment 2 2013-02-27 00:46:48 PST
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".
WebKit Review Bot
Comment 3 2013-02-27 02:22:17 PST
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
WebKit Review Bot
Comment 4 2013-02-27 03:27:38 PST
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
Matt Falkenhagen
Comment 5 2013-03-04 02:50:58 PST
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."
Matt Falkenhagen
Comment 6 2013-03-07 21:16:48 PST
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.
Hajime Morrita
Comment 7 2013-03-07 22:36:45 PST
Comment on attachment 190459 [details] WIP patch Is it enough to cover only form controls? How about <a>, or even JS-based interactivity?
Hajime Morrita
Comment 8 2013-03-07 22:37:49 PST
Ah, they are covered by Node::disabled().
Matt Falkenhagen
Comment 9 2013-03-07 22:43:27 PST
(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...).
Hajime Morrita
Comment 10 2013-03-07 22:46:12 PST
Comment on attachment 190459 [details] WIP patch Could you add tests for - non-modal dialogs and - non-active dialogs to cover negative cases?
Matt Falkenhagen
Comment 11 2013-03-08 02:12:51 PST
Created attachment 192181 [details] add test cases for non-modal and non-open dialogs
Hajime Morrita
Comment 12 2013-03-10 20:51:33 PDT
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.
WebKit Review Bot
Comment 13 2013-03-10 23:15:18 PDT
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>
WebKit Review Bot
Comment 14 2013-03-10 23:15:22 PDT
All reviewed patches have been landed. Closing bug.
Darin Adler
Comment 15 2013-03-11 10:08:27 PDT
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.
Darin Adler
Comment 16 2013-03-11 10:10:03 PDT
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!”.
Hajime Morrita
Comment 17 2013-03-11 18:00:54 PDT
(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.
Darin Adler
Comment 18 2013-03-12 19:28:55 PDT
(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.
Tim Nguyen (:ntim)
Comment 19 2021-05-24 07:25:32 PDT
This was all rolled out in bug 120467.
Tim Nguyen (:ntim)
Comment 20 2021-05-24 07:27:50 PDT
This should re-land combined with bug 112085
Radar WebKit Bug Importer
Comment 21 2021-07-01 02:48:46 PDT
<rdar://problem/80013802>
Tim Nguyen (:ntim)
Comment 22 2021-08-20 02:38:52 PDT
Created attachment 435961 [details] Patch
Tim Nguyen (:ntim)
Comment 23 2021-08-20 03:48:22 PDT
Created attachment 435967 [details] Patch
EWS
Comment 24 2021-08-20 05:56:23 PDT
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].
Note You need to log in before you can comment on or make changes to this bug.