Bug 27458

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?

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

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

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

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.

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

Created attachment 34593 [details] Patch v1

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

Created attachment 34684 [details] Patch v1

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

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

Created attachment 34699 [details] Patch v1

Fixed in r47155.