Summary: | Skeleton implementation of dialog.showModal() | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Matt Falkenhagen <falken> | ||||||||
Component: | Layout and Rendering | Assignee: | Matt Falkenhagen <falken> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | abarth, ojan, tkent, webkit.review.bot | ||||||||
Priority: | P2 | ||||||||||
Version: | 528+ (Nightly build) | ||||||||||
Hardware: | Unspecified | ||||||||||
OS: | Unspecified | ||||||||||
Bug Depends on: | |||||||||||
Bug Blocks: | 84796 | ||||||||||
Attachments: |
|
Description
Matt Falkenhagen
2012-09-23 22:30:19 PDT
Created attachment 165312 [details]
Patch
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 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. Created attachment 165317 [details]
Patch
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. Created attachment 165319 [details]
Patch
Comment on attachment 165319 [details]
Patch
ok
Comment on attachment 165319 [details] Patch Clearing flags on attachment: 165319 Committed r129330: <http://trac.webkit.org/changeset/129330> All reviewed patches have been landed. Closing bug. |