Bug 97425

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.