Bug 90931 – Show or hide <dialog> depending on the open attribute

• Status: RESOLVED
• Resolution: FIXED
• Component: DOM

[Matt Falkenhagen]

The dialog element should be shown after show() is called and not shown after close() is called.

The spec says this can be implemented through CSS:
"A dialog element without an open attribute specified should not be shown to the user. This requirement may be implemented indirectly through the style layer. For example, user agents that support the suggested default rendering implement this requirement using the CSS rules described in the rendering section."

[Matt Falkenhagen]

Created attachment 151653 [details]
Patch

[Matt Falkenhagen]

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.

[WebKit Review Bot]

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

[WebKit Review Bot]

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

[Kent Tamura]

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().

[Matt Falkenhagen]

(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?

[Ojan Vafai]

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]).

[Matt Falkenhagen]

(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.

[Matt Falkenhagen]

Created attachment 153431 [details]
address review comments

[Matt Falkenhagen]

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

[Kent Tamura]

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.

[Matt Falkenhagen]

Created attachment 153435 [details]
address new review comments

[Kent Tamura]

Comment on attachment 153435 [details]
address new review comments

ok

[WebKit Review Bot]

Comment on attachment 153435 [details]
address new review comments

Clearing flags on attachment: 153435

Committed r123193: <http://trac.webkit.org/changeset/123193>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.