Bug 27458 - Support :default HTML5 CSS selector
: Support :default HTML5 CSS selector
Status: RESOLVED FIXED
: WebKit
CSS
: 528+ (Nightly build)
: All All
: P2 Enhancement
Assigned To:
: http://www.whatwg.org/specs/web-apps/...
:
:
: 19264
  Show dependency treegraph
 
Reported: 2009-07-20 12:32 PST by
Modified: 2009-08-12 15:21 PST (History)


Attachments
Simple test with multiple submit buttons (403 bytes, text/html)
2009-08-07 17:57 PST, Michelangelo De Simone
no flags Details
Preliminary patch (5.00 KB, patch)
2009-08-07 17:59 PST, Michelangelo De Simone
no flags Review Patch | Details | Formatted Diff | Diff
Patch v1 (13.15 KB, patch)
2009-08-11 14:26 PST, Michelangelo De Simone
no flags Review Patch | Details | Formatted Diff | Diff
Patch v1 (15.96 KB, patch)
2009-08-12 13:28 PST, Michelangelo De Simone
no flags Review Patch | Details | Formatted Diff | Diff
Patch v1 (15.94 KB, patch)
2009-08-12 15:12 PST, Michelangelo De Simone
darin: review+
Review Patch | Details | Formatted Diff | Diff


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2009-07-20 12:32:22 PST
See spec section 4.13.

Basically, :default matches form buttons that are the default buttons for their forms.
------- Comment #1 From 2009-08-07 15:04:30 PST -------
There's a glitch in the code I've been testing for this bug. I'm looking for the best way to compare two HTMLFormControlElement to check whether they're the same element or not.

Any clue?
------- Comment #2 From 2009-08-07 17:57:48 PST -------
Created an attachment (id=34350) [details]
Simple test with multiple submit buttons
------- Comment #3 From 2009-08-07 17:59:16 PST -------
Created an attachment (id=34352) [details]
Preliminary patch

This is the piece of code which I need some help about:

bool HTMLFormControlElement::isDefaultButtonForForm() const
{
    if (!isSuccessfulSubmitButton() || !m_form)
        return false;

    return isSameNode(m_form->defaultButton()); // equivalent to m_form->defaultButton() == this;
}

This method is called by CSSStyleSelector to match element's style but, for reasons I can't quite understand, it always returns true on this test:

<input name="victim" type="submit" value="Submit" /> <- It should match only this one
<input name="victim" type="submit" value="Submit"/>
<input name="victim" type="submit" value="Submit"/>

However, the right style is matched if one of the two "not default" submit buttons is kept pressed: try to apply the patch and keep pressed  one of the last two buttons in the test.

So, the question is quite simple: there should be a way to check univocally one element's against another. Node::isEqualNode() isn't the answer for sure.
I tought that this issue can depend on something related to the attachment thread but I'm not certain about it: any kind of help would be much appreciated.

m_form->defaultButton() always returns the first submit button, take it as functioning.:)
------- Comment #4 From 2009-08-11 10:09:35 PST -------
CC'ed Adele and Darin, perhaps they've some good suggestions.
------- Comment #5 From 2009-08-11 10:13:56 PST -------
You should never use isSameNode in C++ code. It's only there because it's part of the DOM standard. To check if two nodes are the same you should just use ==. I am almost certain the problem you're having is not due to the "==" check -- you can verify that with some printf debugging.

I believe you're having trouble because of style sharing. I don't have time to explain it right now, but two identical elements will share style, and the style sharing mechanism doesn't know that these elements can't share style.
------- Comment #6 From 2009-08-11 13:35:59 PST -------
Ok, everything's ok now. A clean patch is underway.
------- Comment #7 From 2009-08-11 14:26:52 PST -------
Created an attachment (id=34593) [details]
Patch v1
------- Comment #8 From 2009-08-11 14:30:16 PST -------
(In reply to comment #7)
> Created an attachment (id=34593) [details] [details]
> Patch v1

Might be good to have a negative test, where no controls match :default.
------- Comment #9 From 2009-08-11 22:42:51 PST -------
(From update of attachment 34593 [details])
These tests would be better as modern fast/js style tests.  see make-js-test-wrappers and fast/js/resources/  and TEMPLATE.html files.
------- Comment #10 From 2009-08-12 13:28:27 PST -------
Created an attachment (id=34684) [details]
Patch v1
------- Comment #11 From 2009-08-12 15:05:11 PST -------
(From update of attachment 34684 [details])
> +    DEFINE_STATIC_LOCAL(AtomicString, defaultStr, ("default"));

Do we really need the abbreviation "Str" here? I know we can't use default because it's a reserved word, but how about defaultString or defaultAtomicString?

> +bool HTMLFormControlElement::isDefaultButtonForForm() const
> +{
> +    if (!isSuccessfulSubmitButton() || !m_form)
> +        return false;
> +
> +    return m_form->defaultButton() == this;
> +}

I think this would read better in a single line:

    return isSuccessfulSubmitButton() && m_form && m_form->defaultButton() == this;

Easier to read!

r=me
------- Comment #12 From 2009-08-12 15:08:03 PST -------
(In reply to comment #11)

> > +    DEFINE_STATIC_LOCAL(AtomicString, defaultStr, ("default"));
> Do we really need the abbreviation "Str" here? I know we can't use default
> because it's a reserved word, but how about defaultString or
> defaultAtomicString?

Ok.

> I think this would read better in a single line:
>     return isSuccessfulSubmitButton() && m_form && m_form->defaultButton() ==
> this;
> Easier to read!

Ok.:)
------- Comment #13 From 2009-08-12 15:12:53 PST -------
Created an attachment (id=34699) [details]
Patch v1
------- Comment #14 From 2009-08-12 15:21:20 PST -------
Fixed in r47155.