Bug 97425 - Skeleton implementation of dialog.showModal()
Summary: Skeleton implementation of dialog.showModal()
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: 84796
  Show dependency treegraph
 
Reported: 2012-09-23 22:30 PDT by Matt Falkenhagen
Modified: 2012-09-24 00:40 PDT (History)
4 users (show)

See Also:


Attachments
Patch (6.30 KB, patch)
2012-09-23 22:47 PDT, Matt Falkenhagen
no flags Details | Formatted Diff | Diff
Patch (6.51 KB, patch)
2012-09-24 00:03 PDT, Matt Falkenhagen
no flags Details | Formatted Diff | Diff
Patch (6.87 KB, patch)
2012-09-24 00:17 PDT, 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 2012-09-23 22:30:19 PDT
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.
Comment 1 Matt Falkenhagen 2012-09-23 22:47:43 PDT
Created attachment 165312 [details]
Patch
Comment 2 Kent Tamura 2012-09-23 23:03:19 PDT
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()?
Comment 3 Matt Falkenhagen 2012-09-24 00:00:51 PDT
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.
Comment 4 Matt Falkenhagen 2012-09-24 00:03:24 PDT
Created attachment 165317 [details]
Patch
Comment 5 Kent Tamura 2012-09-24 00:09:31 PDT
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.
Comment 6 Matt Falkenhagen 2012-09-24 00:17:34 PDT
Created attachment 165319 [details]
Patch
Comment 7 Kent Tamura 2012-09-24 00:18:29 PDT
Comment on attachment 165319 [details]
Patch

ok
Comment 8 WebKit Review Bot 2012-09-24 00:40:45 PDT
Comment on attachment 165319 [details]
Patch

Clearing flags on attachment: 165319

Committed r129330: <http://trac.webkit.org/changeset/129330>
Comment 9 WebKit Review Bot 2012-09-24 00:40:49 PDT
All reviewed patches have been landed.  Closing bug.