Bug 27458 - Support :default HTML5 CSS selector
: Support :default HTML5 CSS selector
Status: RESOLVED FIXED
Product: WebKit
Classification: Unclassified
Component: CSS
: 528+ (Nightly build)
: All All
: P2 Enhancement
Assigned To: Michelangelo De Simone
http://www.whatwg.org/specs/web-apps/...
:
Depends on:
Blocks: HTML5Forms
  Show dependency treegraph
 
Reported: 2009-07-20 12:32 PDT by Peter Kasting
Modified: 2009-08-12 15:21 PDT (History)
4 users (show)

See Also:


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

Note You need to log in before you can comment on or make changes to this bug.
Description Peter Kasting 2009-07-20 12:32:22 PDT
See spec section 4.13.

Basically, :default matches form buttons that are the default buttons for their forms.
Comment 1 Michelangelo De Simone 2009-08-07 15:04:30 PDT
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 Michelangelo De Simone 2009-08-07 17:57:48 PDT
Created attachment 34350 [details]
Simple test with multiple submit buttons
Comment 3 Michelangelo De Simone 2009-08-07 17:59:16 PDT
Created attachment 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 Michelangelo De Simone 2009-08-11 10:09:35 PDT
CC'ed Adele and Darin, perhaps they've some good suggestions.
Comment 5 Darin Adler 2009-08-11 10:13:56 PDT
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 Michelangelo De Simone 2009-08-11 13:35:59 PDT
Ok, everything's ok now. A clean patch is underway.
Comment 7 Michelangelo De Simone 2009-08-11 14:26:52 PDT
Created attachment 34593 [details]
Patch v1
Comment 8 Peter Kasting 2009-08-11 14:30:16 PDT
(In reply to comment #7)
> Created an attachment (id=34593) [details]
> Patch v1

Might be good to have a negative test, where no controls match :default.
Comment 9 Eric Seidel 2009-08-11 22:42:51 PDT
Comment on attachment 34593 [details]
Patch v1

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 Michelangelo De Simone 2009-08-12 13:28:27 PDT
Created attachment 34684 [details]
Patch v1
Comment 11 Darin Adler 2009-08-12 15:05:11 PDT
Comment on attachment 34684 [details]
Patch v1

> +    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 Michelangelo De Simone 2009-08-12 15:08:03 PDT
(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 Michelangelo De Simone 2009-08-12 15:12:53 PDT
Created attachment 34699 [details]
Patch v1
Comment 14 Peter Kasting 2009-08-12 15:21:20 PDT
Fixed in r47155.