Bug 90931

Created attachment 151653 [details] Patch

Added a patch. Before committing, the layout test must be changed to enable the dialog feature through internal settings. That can be done after https://bugs.webkit.org/show_bug.cgi?id=90934 is resolved.

Comment on attachment 151653 [details] Patch Attachment 151653 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/13166751 New failing tests: fast/dom/HTMLDialogElement/dialog-open.html

Created attachment 151663 [details] Archive of layout-test-results from gce-cr-linux-03 The attached test failures were seen while running run-webkit-tests on the chromium-ews. Bot: gce-cr-linux-03 Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'> Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid

Comment on attachment 151653 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=151653&action=review > Source/WebCore/html/HTMLDialogElement.cpp:68 > +void HTMLDialogElement::attributeChanged(const Attribute& attribute) > +{ > + if (attribute.name() == openAttr) > + setNeedsStyleRecalc(); > + HTMLElement::attributeChanged(attribute); Is this needed? I thought we automatically do style-recalc if a stylesheet has [attribute] selectors. If we need to do something for it, we should override isPresentationAttribute().

(In reply to comment #5) > (From update of attachment 151653 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=151653&action=review > > > Source/WebCore/html/HTMLDialogElement.cpp:68 > > +void HTMLDialogElement::attributeChanged(const Attribute& attribute) > > +{ > > + if (attribute.name() == openAttr) > > + setNeedsStyleRecalc(); > > + HTMLElement::attributeChanged(attribute); > > Is this needed? > I thought we automatically do style-recalc if a stylesheet has [attribute] selectors. If we need to do something for it, we should override isPresentationAttribute(). Without this, the dialog sometimes wouldn't be displayed after a call to show(). Element::attributeChanged does a check like this: styleResolver->hasSelectorForAttribute(attribute.name().localName())) But for some reason the check comes out to be false for 'open'. In fact, hasSelectorForAttribute does a lookup in m_features.attrsInRules, but attrsInRules is empty. Maybe somehow the styleResolver for the document isn't affected by html.css?

Comment on attachment 151653 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=151653&action=review > Source/WebCore/ChangeLog:10 > + * css/html.css: Add recommended CSS for dialog from HTML5 spec. Can you add a link to the relevant part of the spec and a comment saying that you copy-pasted it directly without modification? >>> Source/WebCore/html/HTMLDialogElement.cpp:68 >>> + HTMLElement::attributeChanged(attribute); >> >> Is this needed? >> I thought we automatically do style-recalc if a stylesheet has [attribute] selectors. If we need to do something for it, we should override isPresentationAttribute(). > > Without this, the dialog sometimes wouldn't be displayed after a call to show(). > > Element::attributeChanged does a check like this: > > styleResolver->hasSelectorForAttribute(attribute.name().localName())) > > But for some reason the check comes out to be false for 'open'. In fact, hasSelectorForAttribute does a lookup in m_features.attrsInRules, but attrsInRules is empty. > > Maybe somehow the styleResolver for the document isn't affected by html.css? If that's true, then it's a bug that should be addressed. We have lots of other attributes in html.css (e.g. iframe[seamless]).

(In reply to comment #7) > If that's true, then it's a bug that should be addressed. We have lots of other attributes in html.css (e.g. iframe[seamless]). I filed bug 91058.

Created attachment 153431 [details] address review comments

bug 91058 may be difficult to fix. I've added a new patch using isPresentationAttribute as a workaround.

Comment on attachment 153431 [details] address review comments View in context: https://bugs.webkit.org/attachment.cgi?id=153431&action=review > Source/WebCore/html/HTMLDialogElement.cpp:67 > + // Workaround for <https://bugs.webkit.org/show_bug.cgi?id=91058>: modifying an attribute for which there is an attribute selector > + // in html.css sometimes does not trigger a style recalc. Please add a prefix "FIXME:" > LayoutTests/fast/dom/HTMLDialogElement/dialog-open.html:7 > +if (window.internals) > + internals.settings.setDialogElementEnabled(true); We usually use 4-space indentation.

Created attachment 153435 [details] address new review comments

Comment on attachment 153435 [details] address new review comments ok

Comment on attachment 153435 [details] address new review comments Clearing flags on attachment: 153435 Committed r123193: <http://trac.webkit.org/changeset/123193>

All reviewed patches have been landed. Closing bug.