Bug 97425 – Skeleton implementation of dialog.showModal()

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Matt Falkenhagen]

Add a simple implementation of dialog.showModal() so it can later be used to test the top layer (not yet implemented, bug 84796). Modal dialogs should be in the top layer.

[Matt Falkenhagen]

Created attachment 165312 [details]
Patch

[Kent Tamura]

Comment on attachment 165312 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=165312&action=review

> Source/WebCore/html/HTMLDialogElement.cpp:70
> +    if (fastHasAttribute(openAttr) || !inDocument()) {
> +        ec = INVALID_STATE_ERR;
> +        return;
> +    }

What should happen if the document is not attached to any pages?  We can make such documents by DOMImplementation::createHTMLDocument(), etc.

> LayoutTests/fast/dom/HTMLDialogElement/dialog-show-modal.html:17
> +shouldBe("computedStyle.getPropertyValue('display')", "'none'");

We had better use shouldBeEqualToString() to avoid nested quotation marks.

> LayoutTests/fast/dom/HTMLDialogElement/dialog-show-modal.html:21
> +dialog.showModal();
> +computedStyle = window.getComputedStyle(dialog, null);
> +shouldBe("computedStyle.getPropertyValue('display')", "'block'");

Will this work when we implement modal behavior for showModal()?

[Matt Falkenhagen]

Comment on attachment 165312 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=165312&action=review

>> Source/WebCore/html/HTMLDialogElement.cpp:70
>> +    }
> 
> What should happen if the document is not attached to any pages?  We can make such documents by DOMImplementation::createHTMLDocument(), etc.

It looks like the spec doesn't say anything about this case. I think showModal() can execute as normal. When there is no document, it can't proceed because there is no "pending dialog stack".

>> LayoutTests/fast/dom/HTMLDialogElement/dialog-show-modal.html:21
>> +shouldBe("computedStyle.getPropertyValue('display')", "'block'");
> 
> Will this work when we implement modal behavior for showModal()?

Yes, I believe modal doesn't block JavaScript execution. It just blocks user interaction with the rest of the document.

[Matt Falkenhagen]

Created attachment 165317 [details]
Patch

[Kent Tamura]

Comment on attachment 165317 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=165317&action=review

Almost ok.
Some nits.

> LayoutTests/fast/dom/HTMLDialogElement/dialog-show-modal-expected.txt:12
> +PASS computedStyle.getPropertyValue('display') is "none"
> +PASS computedStyle.getPropertyValue('display') is "block"
> +PASS dialog.showModal(); threw exception Error: INVALID_STATE_ERR: DOM Exception 11.
> +PASS computedStyle.getPropertyValue('display') is "none"
> +PASS dialog.showModal(); threw exception Error: INVALID_STATE_ERR: DOM Exception 11.
> +PASS dialog.open is false
> +PASS dialog.open is true

The readability of test result is not good.  Please print some explanations about what we are testing.

> LayoutTests/fast/dom/HTMLDialogElement/dialog-show-modal.html:23
> +// "If dialog already has an open attribute, then throw an InvalidStateError exception."

This sentence should be printed with debug().

> LayoutTests/fast/dom/HTMLDialogElement/dialog-show-modal.html:30
> +// "If dialog is not in a Document, then throw an InvalidStateError exception."

ditto.

[Matt Falkenhagen]

Created attachment 165319 [details]
Patch

[Kent Tamura]

Comment on attachment 165319 [details]
Patch

ok

[WebKit Review Bot]

Comment on attachment 165319 [details]
Patch

Clearing flags on attachment: 165319

Committed r129330: <http://trac.webkit.org/changeset/129330>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.