Bug 110952 - Implement inert subtrees needed for modal <dialog>
Summary: Implement inert subtrees needed for modal <dialog>
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Matt Falkenhagen
URL:
Keywords:
Depends on:
Blocks: 84635 159748
  Show dependency treegraph
 
Reported: 2013-02-27 00:04 PST by Matt Falkenhagen
Modified: 2016-07-13 16:52 PDT (History)
12 users (show)

See Also:


Attachments
WIP patch (8.44 KB, patch)
2013-02-27 00:40 PST, Matt Falkenhagen
no flags Details | Formatted Diff | Diff
add test cases for non-modal and non-open dialogs (14.49 KB, patch)
2013-03-08 02:12 PST, Matt Falkenhagen
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Matt Falkenhagen 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
Comment 1 Matt Falkenhagen 2013-02-27 00:40:56 PST
Created attachment 190459 [details]
WIP patch
Comment 2 Matt Falkenhagen 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".
Comment 3 WebKit Review Bot 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
Comment 4 WebKit Review Bot 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
Comment 5 Matt Falkenhagen 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."
Comment 6 Matt Falkenhagen 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.
Comment 7 Hajime Morrita 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?
Comment 8 Hajime Morrita 2013-03-07 22:37:49 PST
Ah, they are covered by Node::disabled().
Comment 9 Matt Falkenhagen 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...).
Comment 10 Hajime Morrita 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?
Comment 11 Matt Falkenhagen 2013-03-08 02:12:51 PST
Created attachment 192181 [details]
add test cases for non-modal and non-open dialogs
Comment 12 Hajime Morrita 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.
Comment 13 WebKit Review Bot 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>
Comment 14 WebKit Review Bot 2013-03-10 23:15:22 PDT
All reviewed patches have been landed.  Closing bug.
Comment 15 Darin Adler 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.
Comment 16 Darin Adler 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!”.
Comment 17 Hajime Morrita 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.
Comment 18 Darin Adler 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.