Support :default HTML5 CSS selector

• Component: CSS
• Status: RESOLVED FIXED

[Peter Kasting]

See spec section 4.13.

Basically, :default matches form buttons that are the default buttons for their forms.

[Michelangelo De Simone]

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?

[Michelangelo De Simone]

Created attachment 34350 [details]
Simple test with multiple submit buttons

[Michelangelo De Simone]

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

[Michelangelo De Simone]

CC'ed Adele and Darin, perhaps they've some good suggestions.

[Darin Adler]

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.

[Michelangelo De Simone]

Ok, everything's ok now. A clean patch is underway.

[Michelangelo De Simone]

Created attachment 34593 [details]
Patch v1

[Peter Kasting]

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

[Eric Seidel (no email)]

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.

[Michelangelo De Simone]

Created attachment 34684 [details]
Patch v1

[Darin Adler]

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

[Michelangelo De Simone]

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

[Michelangelo De Simone]

Created attachment 34699 [details]
Patch v1

[Peter Kasting]

Fixed in r47155.