Bug 90931 - Show or hide <dialog> depending on the open attribute
Summary: Show or hide <dialog> depending on the open attribute
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Matt Falkenhagen
URL:
Keywords:
Depends on: 91058 90934
Blocks: dialog-element
  Show dependency treegraph
 
Reported: 2012-07-10 20:17 PDT by Matt Falkenhagen
Modified: 2012-07-20 01:28 PDT (History)
9 users (show)

See Also:


Attachments
Patch (6.93 KB, patch)
2012-07-11 02:14 PDT, Matt Falkenhagen
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from gce-cr-linux-03 (523.93 KB, application/zip)
2012-07-11 02:55 PDT, WebKit Review Bot
no flags Details
address review comments (7.31 KB, patch)
2012-07-20 00:21 PDT, Matt Falkenhagen
no flags Details | Formatted Diff | Diff
address new review comments (7.32 KB, patch)
2012-07-20 00:32 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-07-10 20:17:36 PDT
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."
Comment 1 Matt Falkenhagen 2012-07-11 02:14:08 PDT
Created attachment 151653 [details]
Patch
Comment 2 Matt Falkenhagen 2012-07-11 02:18:32 PDT
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 3 WebKit Review Bot 2012-07-11 02:55:47 PDT
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
Comment 4 WebKit Review Bot 2012-07-11 02:55:51 PDT
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 5 Kent Tamura 2012-07-11 18:56:50 PDT
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().
Comment 6 Matt Falkenhagen 2012-07-11 19:30:34 PDT
(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 7 Ojan Vafai 2012-07-11 23:23:42 PDT
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]).
Comment 8 Matt Falkenhagen 2012-07-12 01:35:05 PDT
(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.
Comment 9 Matt Falkenhagen 2012-07-20 00:21:38 PDT
Created attachment 153431 [details]
address review comments
Comment 10 Matt Falkenhagen 2012-07-20 00:25:08 PDT
bug 91058 may be difficult to fix. I've added a new patch using isPresentationAttribute as a workaround.
Comment 11 Kent Tamura 2012-07-20 00:27:33 PDT
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.
Comment 12 Matt Falkenhagen 2012-07-20 00:32:34 PDT
Created attachment 153435 [details]
address new review comments
Comment 13 Kent Tamura 2012-07-20 00:37:07 PDT
Comment on attachment 153435 [details]
address new review comments

ok
Comment 14 WebKit Review Bot 2012-07-20 01:28:52 PDT
Comment on attachment 153435 [details]
address new review comments

Clearing flags on attachment: 153435

Committed r123193: <http://trac.webkit.org/changeset/123193>
Comment 15 WebKit Review Bot 2012-07-20 01:28:59 PDT
All reviewed patches have been landed.  Closing bug.