At the moment, button elements uses 'submit' has a default type value when initializing but if the type attribute is changed dynamically, the type attribute doesn't follow the rules of enumerated attributes [1] which say the default value should be used when the value used is empty or invalid. At the moment, if the value is empty or invalid, the type value is not changed. [1] http://dev.w3.org/html5/spec/infrastructure.html#enumerated-attribute

So should just set mType if !res in ParseAttribute, right? Or is this really about the _attribute_ as opposed to the behavior of the button (seems off if so)? A testcase that has clear expected behavior would be good here.

Actually, as I am working on this, I quickly wrote this test. The button behavior and the attribute are not correct.

Oups, there was a typo in the previous test. The patch is nearly ready.

Where exactly is the behavior for .type documented for unknown @type values? The spec seems to say it should reflect the @type, no? That is, why are the expected results of that test correct?

This test is testing more stuff

Ah, looks like http://dev.w3.org/html5/spec/infrastructure.html#reflect has a special case for enumerated attrs: "If a reflecting IDL attribute is a DOMString whose content attribute is an enumerated attribute, and the IDL attribute is limited to only known values, then, on getting, the IDL attribute must return the conforming value associated with the state the attribute is in (in its canonical case), or the empty string if the attribute is in a state that has no associated keyword value; and on setting, if the new value is an ASCII case-insensitive match for one of the keywords given for that attribute, then the content attribute must be set to the conforming value associated with the state that the attribute would be in if set to the given new value, otherwise, if the new value is the empty string, then the content attribute must be removed, otherwise, the setter must raise a SYNTAX_ERR exception." I'm going to raise that setter issue; it doesn't make sense.

This patch is fixing the bug. Maybe we can add a macro and a Parse*Value function ?

The "patch" attachment looks just like the testcase.

Oups, sorry about that.

Ah, ok. Yes, you want a macro; we have lots of other enumerated attributes around, no?

This patch is creating macros, specific parse functions and refactoring between button and input code. Boris, could you review it ?

Comment on attachment 432014 [details] [diff] [review] Patch v0.2 >+nsAttrValue::SetEnumValue(const EnumTable& aTable, Given that this is a protected method with only two callers, please make aCaseSensitive a required argument. >+ PRBool aCaseSensitive /*=PR_FALSE*/) Spaces after /* and before */ ? >+nsAttrValue::ParseEnumValueWithMissingDefault(const nsAString& aValue, I'm not that happy with that method name. Seems like it might be better to call this ParseEnumWithDefaultValue or something? >+ PRInt16 aDefaultMissing) That should just be aDefaultValue, right? >+ nsAutoString tag; >+ tag.AssignASCII(aTable->tag); >+ SetEnumValue(*aTable, tag); That seems like a significant change. In particular, if I have: <input type="foopy"> and I then getAttribute("type"), this code will make it return "text" as far as I can tell. That seems undesirable. There are other issues in this method too, but not worth worrying about for now until this fundamental problem is resolved. >+++ b/content/base/src/nsAttrValue.h Fri Mar 12 01:20:22 2010 +0100 >+nsGenericHTMLElement::GetEnumAttr(PRInt16 aType, const nsAttrValue::EnumTable* aTable, nsAString& aResult) >+ if (aType == aTable->value) >+ { >+ CopyUTF8toUTF16(aTable->tag, aResult); Isn't aTable->tag always ASCII? Please CopyASCII... here. >+++ b/content/html/content/src/nsGenericHTMLElement.h Fri Mar 12 01:20:22 2010 +0100 >+ * @return PR_TRUE if the string has been found, PR_FALSE otherwise. >+ */ >+ NS_HIDDEN_(nsresult) GetEnumAttr(PRInt16 aType, const nsAttrValue::EnumTable* aTable, nsAString& aResult); That comment doesn't match the code. And please wrap to 80 chars? >+++ b/content/html/content/src/nsHTMLButtonElement.cpp Fri Mar 12 01:20:22 2010 +0100 > PRBool > nsHTMLButtonElement::ParseAttribute(PRInt32 aNamespaceID, ... >+ return NS_OK; I don't see how that makes sense. >+++ b/content/html/content/src/nsHTMLInputElement.cpp Fri Mar 12 01:20:22 2010 +0100 > //NS_IMPL_STRING_ATTR_DEFAULT_VALUE(nsHTMLInputElement, Type, type, "text") That should be able to go away, right? > PRBool > nsHTMLInputElement::ParseAttribute(PRInt32 aNamespaceID, ... >+ return NS_OK; Once again, this makes no sense. <input> tests needed here too, no?

(In reply to comment #12) > (From update of attachment 432014 [details] [diff] [review]) > >+nsAttrValue::SetEnumValue(const EnumTable& aTable, > > Given that this is a protected method with only two callers, please make > aCaseSensitive a required argument. > > >+ PRBool aCaseSensitive /*=PR_FALSE*/) Ok. > Spaces after /* and before */ ? Ok. > >+nsAttrValue::ParseEnumValueWithMissingDefault(const nsAString& aValue, > > I'm not that happy with that method name. Seems like it might be better to > call this ParseEnumWithDefaultValue or something? Actually, I began with that name but there are three cases in the enum attribute parsing according to the specs: 1. no default value, missing and invalid value should be ignored 2. a missing default value and no invalid default value. The missing default value is used in case of the parsed value is empty or invalid. 3. a missing default value and a invalid default value I thought it would be better to really underline this function is for case 2. I can create a function for case 3 with aInvalidDefaultValue optional but I think the default value may not be really easy to choose. > > >+ PRInt16 aDefaultMissing) > > That should just be aDefaultValue, right? See previous above. > >+ nsAutoString tag; > >+ tag.AssignASCII(aTable->tag); > >+ SetEnumValue(*aTable, tag); > > That seems like a significant change. In particular, if I have: > > <input type="foopy"> > > and I then getAttribute("type"), this code will make it return "text" as far as > I can tell. That seems undesirable. It is not. The last test is testing that. > There are other issues in this method too, but not worth worrying about for now > until this fundamental problem is resolved. Shoot :) > >+++ b/content/base/src/nsAttrValue.h Fri Mar 12 01:20:22 2010 +0100 > >+nsGenericHTMLElement::GetEnumAttr(PRInt16 aType, const nsAttrValue::EnumTable* aTable, nsAString& aResult) > >+ if (aType == aTable->value) > >+ { > >+ CopyUTF8toUTF16(aTable->tag, aResult); > > Isn't aTable->tag always ASCII? Please CopyASCII... here. Indeed. It cames from the older function that I moved. > >+++ b/content/html/content/src/nsGenericHTMLElement.h Fri Mar 12 01:20:22 2010 +0100 > > >+ * @return PR_TRUE if the string has been found, PR_FALSE otherwise. > >+ */ > >+ NS_HIDDEN_(nsresult) GetEnumAttr(PRInt16 aType, const nsAttrValue::EnumTable* aTable, nsAString& aResult); > > That comment doesn't match the code. And please wrap to 80 chars? Indeed. By the way, is the 80 wraps a convention in the tree ? I don't had this feeling that why I'm not wrapping to 80 chars. > >+++ b/content/html/content/src/nsHTMLButtonElement.cpp Fri Mar 12 01:20:22 2010 +0100 > > PRBool > > nsHTMLButtonElement::ParseAttribute(PRInt32 aNamespaceID, > ... > >+ return NS_OK; > > I don't see how that makes sense. > > >+++ b/content/html/content/src/nsHTMLInputElement.cpp Fri Mar 12 01:20:22 2010 +0100 > > //NS_IMPL_STRING_ATTR_DEFAULT_VALUE(nsHTMLInputElement, Type, type, "text") > > That should be able to go away, right? > > > PRBool > > nsHTMLInputElement::ParseAttribute(PRInt32 aNamespaceID, > ... > >+ return NS_OK; > > Once again, this makes no sense. Should be PR_TRUE. > <input> tests needed here too, no? I did not change the behavior for the input element, it already has the right behavior. I only refactor the existing code so, as far as I know, the tests for input element should exist and they will have to pass.

(In reply to comment #13) > > That seems like a significant change. In particular, if I have: > > > > <input type="foopy"> > > > > and I then getAttribute("type"), this code will make it return "text" as far as > > I can tell. That seems undesirable. > > It is not. The last test is testing that. Is that test still passing once you switch from NS_OK (which is == PR_FALSE) to PR_TRUE in your ParseAttribute implementations? > I did not change the behavior for the input element, it already has the right > behavior. I only refactor the existing code so, as far as I know, the tests for > input element should exist and they will have to pass. I rather doubt they exist. That code almost certainly predates our test infrastructure.

This patch should be better I hope.

80 is a general convention, but a few areas use larger limits; assume 80 unless you know otherwise.

Comment on attachment 432560 [details] [diff] [review] Patch v0.3 Changing the review to Olli because Boris is out until 22/03.

Comment on attachment 432560 [details] [diff] [review] Patch v0.3 >+nsAttrValue::ParseEnumValueWithMissingDefault(const nsAString& aValue, >+ const EnumTable* aTable, >+ const EnumTable* aDefaultMissing) The "Missing" in the method name? Why not just ParseEnumValueWithDefault >+{ >+ // If aValue is empty or invalid, we use aDefaultMissing. >+ // aValue is known to be invalid when ParseEnumValue returns PR_FALSE. >+ if (aValue.IsEmpty() || !ParseEnumValue(aValue, aTable)) >+ { >+ PRInt32 tableValue; >+ if (GetEnumTableGlobalValue(aDefaultMissing, tableValue)) >+ { >+ SetIntValueAndType(tableValue, eEnum, &aValue); >+ } >+ } The coding style should be if (expr) { statement; } So '{' is in the same line as 'if' >+ void ParseEnumValueWithMissingDefault(const nsAString& aValue, >+ const EnumTable* aTable, >+ const EnumTable* aDefaultMissing); This could return PRBool, though I'm not sure if anyone really cares about the PR_FALSE case. >+ /** >+ * Get the 'global' value of an EnumTable. >+ * That means the value of the enum table using the index as an offset. >+ * >+ * @param aTable the EnumTable to get the value of. >+ * @param aResult the global value of the EnumTable. >+ * @return whether the value has been found. >+ */ >+ PRBool GetEnumTableGlobalValue(const EnumTable* aTable, PRInt32& aResult); Perhaps GetIndexOfEnumTable(...)? >+nsGenericHTMLElement::GetEnumAttr(PRInt16 aType, const nsAttrValue::EnumTable* aTable, nsAString& aResult) >+{ >+ for(; aTable->tag; ++aTable) >+ { for (...) { ... } >+ if (aType == aTable->value) >+ { if (...) { >+++ b/content/html/content/test/test_bug551670.html Mon Mar 15 15:53:39 2010 +0100 Could you test all the possible types of input and button elements.

Doesn't that last patch bloat the table-index list with the various default values?

(In reply to comment #19) > Doesn't that last patch bloat the table-index list with the various default > values? I do not understand how. Can you explain what do you have in mind ? (In reply to comment #18) > (From update of attachment 432560 [details] [diff] [review]) > >+nsAttrValue::ParseEnumValueWithMissingDefault(const nsAString& aValue, > >+ const EnumTable* aTable, > >+ const EnumTable* aDefaultMissing) > The "Missing" in the method name? Why not just ParseEnumValueWithDefault As I said in comment 13, there are 2 types of default values : missing and invalid. I prefer to add 'Missing' so when the invalid default value will be used, a new function with 'Invalid' could be added. What would you suggest ? > >+ void ParseEnumValueWithMissingDefault(const nsAString& aValue, > >+ const EnumTable* aTable, > >+ const EnumTable* aDefaultMissing); > This could return PRBool, though I'm not sure if anyone really cares about the > PR_FALSE case. I will add that. > >+ /** > >+ * Get the 'global' value of an EnumTable. > >+ * That means the value of the enum table using the index as an offset. > >+ * > >+ * @param aTable the EnumTable to get the value of. > >+ * @param aResult the global value of the EnumTable. > >+ * @return whether the value has been found. > >+ */ > >+ PRBool GetEnumTableGlobalValue(const EnumTable* aTable, PRInt32& aResult); > Perhaps GetIndexOfEnumTable(...)? Ok. > >+++ b/content/html/content/test/test_bug551670.html Mon Mar 15 15:53:39 2010 +0100 > Could you test all the possible types of input and button elements. I am not sure that is really needed as the code is mainly checking the default type is used as a fallback. 'otherType' is only used to check the parsing is case insensitive. If the tests are failing with a type and not with another that would mean the 'otherType' is not recognized... I will check the coding style issues.

I didn't change the name of 'ParseEnumValueWithMissingDefault' and I didn't add tests you suggested for reasons I've mentioned in comment 20. Let me know if you think my reasons are not right.

(In reply to comment #20) > As I said in comment 13, there are 2 types of default values : missing and > invalid. I prefer to add 'Missing' so when the invalid default value will be > used, a new function with 'Invalid' could be added. Could you explain you'd need too different methods to set default values?

Comment on attachment 433826 [details] [diff] [review] Patch v0.4 Per IRC this will change.

Comment on attachment 433859 [details] [diff] [review] Patch v0.5 > /** >+ * Parse into an enum value using a default value if is invalid. >+ * >+ * @param aValue the string to find the value for. >+ * @param aTable the enumeration to map with. >+ * @param aDefaultMissing the default value if aValue is invalid. >+ */ Nit, could you align the parameter comments: * @param aValue the string to find the value for. * @param aTable the enumeration to map with. * @param aDefaultMissing the default value if aValue is invalid. > nsHTMLButtonElement::ParseAttribute(PRInt32 aNamespaceID, > nsIAtom* aAttribute, > const nsAString& aValue, > nsAttrValue& aResult) > { > if (aAttribute == nsGkAtoms::type && kNameSpaceID_None == aNamespaceID) { > // XXX ARG!! This is major evilness. ParseAttribute > // shouldn't set members. Override SetAttr instead >- PRBool res = aResult.ParseEnumValue(aValue, kButtonTypeTable); >- if (res) { >+ PRBool success; >+ success = aResult.ParseEnumValueWithDefault(aValue, kButtonTypeTable, >+ kButtonDefaultType); kButtonDefaultType should be aligned under aValue. >@@ -760,16 +777,17 @@ NS_IMPL_STRING_ATTR(nsHTMLInputElement, > NS_IMPL_BOOL_ATTR(nsHTMLInputElement, ReadOnly, readonly) > NS_IMPL_URI_ATTR(nsHTMLInputElement, Src, src) > NS_IMPL_INT_ATTR_DEFAULT_VALUE(nsHTMLInputElement, TabIndex, tabindex, 0) > NS_IMPL_STRING_ATTR(nsHTMLInputElement, UseMap, usemap) > //NS_IMPL_STRING_ATTR(nsHTMLInputElement, Value, value) > //NS_IMPL_INT_ATTR_DEFAULT_VALUE(nsHTMLInputElement, Size, size, 0) > //NS_IMPL_STRING_ATTR_DEFAULT_VALUE(nsHTMLInputElement, Type, type, "text") Could you remove this, since you're implementing type few lines under this. > NS_IMPL_STRING_ATTR(nsHTMLInputElement, Placeholder, placeholder) >+NS_IMPL_ENUM_ATTR(nsHTMLInputElement, Type, type, mType, kInputTypeTable) > nsHTMLInputElement::ParseAttribute(PRInt32 aNamespaceID, > nsIAtom* aAttribute, > const nsAString& aValue, > nsAttrValue& aResult) > { > if (aNamespaceID == kNameSpaceID_None) { > if (aAttribute == nsGkAtoms::type) { > // XXX ARG!! This is major evilness. ParseAttribute > // shouldn't set members. Override SetAttr instead > PRInt32 newType; > PRBool success; >- if ((success = aResult.ParseEnumValue(aValue, kInputTypeTable))) { >+ success = aResult.ParseEnumValueWithDefault(aValue, kInputTypeTable, >+ kInputDefaultType); Align kInputDefaultType properly. >+https://bugzilla.mozilla.org/show_bug.cgi?id=551670 >+--> >+<head> >+ <title>Test for Bug 551670</title> >+ <script type="application/javascript" src="/MochiKit/packed.js"></script> >+ <script type="application/javascript" src="/tests/SimpleTest/SimpleTest.js"></script> >+ <link rel="stylesheet" type="text/css" href="/tests/SimpleTest/test.css"/> >+</head> >+<body> >+<a target="_blank" href="https://bugzilla.mozilla.org/show_bug.cgi?id=551670">Mozilla Bug 551670</a> >+<p id="display"></p> >+<div id="content" style="display: none"> >+ <button type='badtype' id='b1'></button> >+ <button id='b2'></button> >+ <input type='badtype' id='i1'> >+ <input id='i2'> >+</div> >+<pre id="test"> >+<script type="application/javascript"> >+ >+/** Test for Bug 551670 **/ >+ >+function checkType(e1, e2, defaultType, otherType) >+{ >+ // When accessing to the type attribute from the IDL, >+ // it should reflect the state of the button. >+ >+ defaultType = defaultType.toLowerCase(); >+ otherType = otherType.toLowerCase(); >+ >+ is(e1.type, defaultType, >+ "When the initial type attribute value is not valid, the state should be '" + >+ defaultType + "'"); >+ is(e2.type, defaultType, >+ "When the type attribute is not set, the state should be '" + >+ defaultType + "'"); >+ >+ e1.type = otherType; >+ e1.setAttribute('type', ''); >+ is(e1.type, defaultType, >+ "When type attribute is set to an empty string, the state should be '" + >+ defaultType + "'"); >+ >+ e1.type = otherType; >+ e1.type = ''; >+ is(e1.type, defaultType, >+ "When type attribute is set to an empty string, the state should be '" + >+ defaultType + "'"); >+ >+ e1.type = otherType; >+ e1.setAttribute('type', 'foo'); >+ is(e1.type, defaultType, >+ "When type attribute is set to an invalid value, the state should be '" + >+ defaultType + "'"); >+ >+ e1.type = otherType; >+ e1.type = 'foo'; >+ is(e1.type, defaultType, >+ "When type attribute is set to an invalid value, the state should be '" + >+ defaultType + "'"); >+ >+ is(e1.getAttribute('type'), 'foo', "Type attribute content should not be changed"); >+ >+ e1.type = otherType.toUpperCase(); >+ is(e1.type, otherType, "Type attribute should be case insensitive"); >+ >+ e1.removeAttribute('type'); >+ is(e1.type, defaultType, >+ "When type attribute is set to an empty string, the state should be '" + >+ defaultType + "'"); >+} >+ >+checkType(document.getElementById('b1'), document.getElementById('b2'), 'submit', 'reset'); >+checkType(document.getElementById('i1'), document.getElementById('i2'), 'text', 'password'); Could you still test all the input element types. Just to add more tests to prevent future regressions.

Olli, I let you re-read the patches and commit them as said on IRC.

> I do not understand how. Can you explain what do you have in mind ? Every call to nsAttrValue::GetEnumTableIndex does a linear search for the given const EnumTable* in sEnumTableArray. If not found, it adds the given const EnumTable* to the array. Your new code calls GetEnumTableIndex with your aDefault values as the const EnumTable*. Which means they will now get added to the array if used, which will slow down all GetEnumTableIndex callers. Then again, maybe this code was already completely broken and just added all sorts of random values into this array? Based on how this is set up, I'd expect each enum table to end up in the array once (so should be doing the IndexOf on the original aTable value passed to ParseEnumValue, not on the modified aTable that we're iterating through), but it looks like we put each enum _value_ in the array once or something.... Maybe this should all be a followup bug, but if that was the intent then passing around const EnumTable* pointing into the middle of tables is a step in the wrong direction. ccing sicking in case he remembers how this stuff is intended to work.

Oh, and the documentation in that patch for GetEnumTableIndex is confusing at best, but that might be due to the issues mentioned in comment 28.

A few more comments: 1) The code that changes mType in nsHTMLButtonElement::UnsetAttr is almost certainly wrong in that it won't trigger the right style changes. Changing mType changes states, and you need to notify on that change. 2) While the out-of-band mType is possibly useful for the element impl, it doesn't seem to be needed for GetEnumAttr, since the nsAttrValue in question should have the right enum value already. I guess an exception is when the attribute is not set... in which case you need to tell GetEnumAttr what the default value should be. Not sure whether the loop you have in GetEnumAttr is better or worse than the relevant GetAttr call (probably no worse), and whether other consumers that need to use this code too store the values out-of-band as well (almost certainly not). 3) We could remove the need for more EnumTable*s floating around if we made the default values just come first in the tables, no? I guess that could be more complicated if at some point we have multiple different defaults. Sorry for the lag here; I was sort of off all last week.... Feel free to turn any of the issues above into followup bugs as desired.

(In reply to comment #28) > > I do not understand how. Can you explain what do you have in mind ? > > Every call to nsAttrValue::GetEnumTableIndex does a linear search for the given > const EnumTable* in sEnumTableArray. If not found, it adds the given const > EnumTable* to the array. > > Your new code calls GetEnumTableIndex with your aDefault values as the const > EnumTable*. Which means they will now get added to the array if used, which > will slow down all GetEnumTableIndex callers. But aDefault is a valid value. That means it is a value from the k{Input,Button}TypeTable. So, it will not be added randomly. > Then again, maybe this code was already completely broken and just added all > sorts of random values into this array? Based on how this is set up, I'd > expect each enum table to end up in the array once (so should be doing the > IndexOf on the original aTable value passed to ParseEnumValue, not on the > modified aTable that we're iterating through), but it looks like we put each > enum _value_ in the array once or something.... As far as I understand it, sEnumTableArray is saving the list of EnumTable elements we have found. Elements of sEnumTableArray are never considered to be a table but only an EnumTable element. By the way, I did not changed ParseEnumValue() to call GetEnumTableIndex() during the iteration of aTable, I've only moved some code to create a new function. It was already behaving like that. > Maybe this should all be a followup bug, but if that was the intent then > passing around const EnumTable* pointing into the middle of tables is a step in > the wrong direction. > > ccing sicking in case he remembers how this stuff is intended to work.

> Elements of sEnumTableArray are never considered to be a table but only an > EnumTable element. Then why are we storing both the value from that element _and_ its index in the enum table array in the nsAttrValue int? Seems like just the latter would be enough to recover any information we might want. > By the way, I did not changed ParseEnumValue() to call GetEnumTableIndex() > during the iteration of aTable, Yes, I realize that. I'm just saying that your new code looked wrong to me, then I realized that the old code looks wrong in a very similar way. That doesn't necessarily mean your code (or the old code) is right, but does mean that your patch is at least not making things immediately worse and that following up on this issue in a separate bug is ok by me....

(In reply to comment #30) > A few more comments: > > 1) The code that changes mType in nsHTMLButtonElement::UnsetAttr is almost > certainly wrong in that it won't trigger the right style changes. Changing > mType changes states, and you need to notify on that change. I could be very wrong here, but I don't actually know any cases where changing <button>'s type need to notify *state* changes. (The attribute removal itself notifies the usual UPDATE_CONTENT_MODEL when needed)

Ah, ok, perhaps :default handling needs the state change.

> I don't actually know any cases where changing <button>'s type need to notify > *state* changes. Going from submit to one of the other types (or the reverse) can change the NS_EVENT_STATE_DEFAULT state of the button. Note that we have code in BeforeSetAttr and AfterSetAttr to send the right notifications, last I checked, but there are also assertions in nsGenericHTMLFormElement::IntrinsicState that assume that the type matches what IsDefaultSubmitElement is doing, etc. The whole thing is somewhat tangled up; the safe thing to do is whatever nsHTMLInputElement is doing.

Though, seems like there are many cases when :default isn't updated properly when there are dynamic changes.

(In reply to comment #36) > Though, seems like there are many cases when :default isn't updated properly > when there are dynamic changes. Or not, Input element seems to notify lots of extra changes.

> Though, seems like there are many cases when :default isn't updated properly > when there are dynamic changes. We have some pretty extensive tests for :default with <input>, iirc. > Or not, Input element seems to notify lots of extra changes. Yep.

Ah, nsGenericHTMLFormElement::AfterSetAttr does actually notify about NS_EVENT_STATE_DEFAULT, even with <button>

Mounir, could you add some test for :default handling. I think changing button's type should update :default correctly.

Argh, my mistake nsHTMLButtonElement::UnsetAttr certainly shouldn't call nsGenericHTMLElement::UnsetAttr, but nsGenericHTMLFormElement::UnsetAttr. And probably better to not use UnsetAttr, but the existing nsHTMLButtonElement::BeforeSetAttr.

According to your comments, this patch should fix the issues #1. About the issue #2 and #3, they are not linked to my patch (I used existing cod) so I think a follow up would be fine. However, I think the issue #2 is not really an issue because we do not need a default value. mType should never be set to something not in the kButtonTypeTable so the exception should never be throw. In addition, my tests make sure every 'not set' situations are covered so we should never got into the exception. I will write more tests to cover the points you have pointed later today.

With that last patch, mType is updated in BeforeSetAttr, right? I guess that works because nsGenericHTMLFormElement::BeforeSetAttr removes the node as a default submit as needed... But it really would be safer to make this work the way nsHTMLInputElement does (that is, change mType in AfterSetAttr). At least that way the behavior will be consistent if there are bugs either way.

Blame me for using BeforeSetAttr. I proposed that since based on how the code works atm it should be ok. But sure using AfterSetAttr would good.

I hope this patch is gonna make it :)

Comment on attachment 434890 [details] [diff] [review] Patch v0.7 Olli, as you've already reviewed an older version of this patch, I will no problem if you cancel this review.

I don't understand why we couldn't do for button elements what we do for input elements today. We're dealing with falling back to a default there. That said, it does indeed look like the enum code is borked. It was intended that sEnumTableArray would contain a pointer to the first item in the enum table, but that isn't what is happening here clearly. I'm surprised this is working without running out of indexes into the table. I'd definitely like to review the final patch here no matter what.

> I don't understand why we couldn't do for button elements what we do for input > elements today Jonas, what were doing for input today is having a custom GetType method. We _could_ do that for buttons too, but since we actually need such custom methods for a all enumerated attributes it seems worthwhile to make it very easy to create them using our getter/setter macros, which is what this patch aims for. If you plan to review the final patch, then I probably don't need to, right? So you can just take over the sr request?

Two comments on that last patch, based on quick skim. 1) The indentation on the nsHTMLButtonElement::AfterSetAttr declaration is broken. 2) ParseAttribute should reset mType to the right thing even if ParseEnumValueWithDefault fails.

Comment on attachment 434890 [details] [diff] [review] Patch v0.7 Yup, I'm happy to take the sr here. Makes sense regarding the custom GetType method. Not sure yet which approach I like better, will have to review the patch to determine that.

This patch should fix every issues mentioned. It is no longer using the default value when parsing to prevent the issues mentioned in bug 551846 and bug 555711. The NS_IMPL_ENUM_ATTR_* macro is now specific to this case (with a default missing value). Others have not been implemented because they are not needed at the moment.

Comment on attachment 436622 [details] [diff] [review] Patch v0.8 >+void >+nsAttrValue::GetEnumString(nsAString& aResult) const >+{ >+ NS_PRECONDITION(Type() == eEnum, "wrong type"); >+ >+ PRInt16 val = GetEnumValue(); >+ PRUint32 allEnumBits = >+ (BaseType() == eIntegerBase) ? static_cast<PRUint32>(GetIntInternal()) >+ : GetMiscContainer()->mEnumValue; >+ const EnumTable* table = sEnumTableArray-> >+ ElementAt(allEnumBits & NS_ATTRVALUE_ENUMTABLEINDEX_MASK); >+ while (table->tag) { >+ if (table->value == val) { >+ aResult.AssignASCII(table->tag); >+ return; >+ } >+ table++; >+ } >+ >+ NS_NOTREACHED("couldn't find value in EnumTable"); >+} >+ Why you need this? You could just use nsAttrValue::ToString This code is anyway copied from ToString. >+nsGenericHTMLElement::GetEnumAttrWithDefaultMissing(nsIAtom* aAttr, >+ const nsAString& aDefaultMissing, >+ nsAString& aResult) Strange method name "*WithDefaultMissing". Perhaps GetEnumAttrWithDefaultValue, or just GetEnumAttr(nsIAtom* aAttr, const nsAString& aDefaultValue, nsAString& aResult) >+#define NS_IMPL_ENUM_ATTR_DEFAULT_MISSING_VALUE(_class, _method, _atom, _default) \ NS_IMPL_ENUM_ATTR_WITH_DEFAULT_VALUE With those, r=me

And please file a followup bug to sort out how we handle enum attrs.

(In reply to comment #52) > (From update of attachment 436622 [details] [diff] [review]) > > >+void > >+nsAttrValue::GetEnumString(nsAString& aResult) const > >+{ > >+ NS_PRECONDITION(Type() == eEnum, "wrong type"); > >+ > >+ PRInt16 val = GetEnumValue(); > >+ PRUint32 allEnumBits = > >+ (BaseType() == eIntegerBase) ? static_cast<PRUint32>(GetIntInternal()) > >+ : GetMiscContainer()->mEnumValue; > >+ const EnumTable* table = sEnumTableArray-> > >+ ElementAt(allEnumBits & NS_ATTRVALUE_ENUMTABLEINDEX_MASK); > >+ while (table->tag) { > >+ if (table->value == val) { > >+ aResult.AssignASCII(table->tag); > >+ return; > >+ } > >+ table++; > >+ } > >+ > >+ NS_NOTREACHED("couldn't find value in EnumTable"); > >+} > >+ > Why you need this? You could just use nsAttrValue::ToString > This code is anyway copied from ToString. Because ToString does not call this code if the case wasn't the same. So, if I set type="SuBmIt", ToString() will return "SuBmIt" but I want to have "submit". > >+nsGenericHTMLElement::GetEnumAttrWithDefaultMissing(nsIAtom* aAttr, > >+ const nsAString& aDefaultMissing, > >+ nsAString& aResult) > Strange method name "*WithDefaultMissing". > Perhaps GetEnumAttrWithDefaultValue, or just > GetEnumAttr(nsIAtom* aAttr, const nsAString& aDefaultValue, nsAString& aResult) > > > >+#define NS_IMPL_ENUM_ATTR_DEFAULT_MISSING_VALUE(_class, _method, _atom, _default) \ > NS_IMPL_ENUM_ATTR_WITH_DEFAULT_VALUE Because at the moment, I do: + if (attrVal && attrVal->Type() == nsAttrValue::eEnum) { + attrVal->GetEnumString(aResult); + } else { + aResult.Append(aDefaultMissing); + } But according to the specs, two other situations are possible: + if (!attrVal) { + aResult.Append(aDefaultMissing); + } else if (attrVal->Type() != nsAttrValue::eEnum) { + aResult.Append(aDefaultInvalid); + } else { + attrVal->GetEnumString(aResult); + } and + if (!attrVal) { + aResult.Truncate(); + } else if (attrVal->Type() != nsAttrValue::eEnum) { + aResult.Append(aDefaultInvalid); + } else { + attrVal->GetEnumString(aResult); + } and + if (attrVal && attrVal->Type() == nsAttrValue::eEnum) { + attrVal->GetEnumString(aResult); + } else { + aResult.Truncate(); + } If you think my naming is not good, I can use a WithDefaults macro which will need two default values and if only one default value is needed, the other one will have to be "". Do you think it is better ? > > With those, r=me

(In reply to comment #54) > Because ToString does not call this code if the case wasn't the same. So, if I > set type="SuBmIt", ToString() will return "SuBmIt" but I want to have "submit". Ok, but could you still make ToString code to use the method you added. To do that you need to add support for NS_ATTRVALUE_ENUMTABLE_VALUE_NEEDS_TO_UPPER handling to your method. We don't want almost the same code to be duplicated in several places. > Because at the moment, I do: > + if (attrVal && attrVal->Type() == nsAttrValue::eEnum) { > + attrVal->GetEnumString(aResult); > + } else { > + aResult.Append(aDefaultMissing); > + } Uh, this should be Assign, not Append. When a method name ends with WithDefaultMissing I read is somehow so that default value is actually missing, but that is not quite the case here, since the default value is passed as a parameter. So I prefer not using *DefaultMissing In which cases aDefaultMissing value and aDefaultInvalid value are actually different per the HTML5 draft? Is the draft doing anything sane in those cases or should it be changed? In any case, needing 2 default values in the method isn't a problem. And there could be two different macros, one which take 1 and one which takes 2 default values.

r=smaug I had to add a boolean attribute for GetEnumString() because ToString needs ToUpper() but not the original need of GetEnumString().

This bug is going to block other bugs which have r+. Jonas, do you think you can sr it in the next few days ?

Comment on attachment 437019 [details] [diff] [review] Patch v8.1 >+nsAttrValue::GetEnumString(nsAString& aResult, PRBool aRealTag) const >+{ >+ NS_PRECONDITION(Type() == eEnum, "wrong type"); >+ >+ PRInt16 val = GetEnumValue(); >+ PRUint32 allEnumBits = >+ (BaseType() == eIntegerBase) ? static_cast<PRUint32>(GetIntInternal()) >+ : GetMiscContainer()->mEnumValue; You could save a bit of code by doing: PRInt16 val = allEnumBits >> NS_ATTRVALUE_ENUMTABLEINDEX_BITS (I know you copied this from the old code) >+ const EnumTable* table = sEnumTableArray-> >+ ElementAt(allEnumBits & NS_ATTRVALUE_ENUMTABLEINDEX_MASK); Hmm.. This looks wierd. This argument passed to ElementAt will always be a multiple of 16384 or 16 or something like that. I think. This bit twiddling is too complex and too spread out. I take full responsibility for this fact. Would you mind centralizing and cleaning it up? Something like the following defines: #define NS_ATTRVALUE_ENUM_TABLEINDEX_BITS (32 - 16 - 1 - NS_ATTRVALUE_INTEGERTYPE_BITS) #define NS_ATTRVALUE_ENUM_TABLEINDEX_OFFSET NS_ATTRVALUE_INTEGERTYPE_BITS #define NS_ATTRVALUE_ENUM_TABLEINDEX_MASK \ (PR_BITMASK(NS_ATTRVALUE_ENUM_TABLEINDEX_BITS) << NS_ATTRVALUE_ENUM_TABLEINDEX_OFFSET) #define NS_ATTRVALUE_ENUM_GET_TABLEINDEX(_bits) \ (((_bits) & NS_ATTRVALUE_ENUM_TABLEINDEX_MASK) >> NS_ATTRVALUE_ENUM_TABLEINDEX_OFFSET) #define NS_ATTRVALUE_ENUM_VALUE_NEEDS_TO_UPPER_BITS 1 #define NS_ATTRVALUE_ENUM_VALUE_NEEDS_TO_UPPER_OFFSET \ (NS_ATTRVALUE_ENUM_TABLEINDEX_OFFSET + NS_ATTRVALUE_ENUM_TABLEINDEX_BITS) #define NS_ATTRVALUE_ENUM_VALUE_NEEDS_TO_UPPER_MASK \ (PR_BITMASK(NS_ATTRVALUE_ENUM_VALUE_NEEDS_TO_UPPER_BITS) << NS_ATTRVALUE_ENUM_VALUE_NEEDS_TO_UPPER_OFFSET) #define NS_ATTRVALUE_ENUM_VALUE_NEEDS_TO_UPPER(_bits) \ ((_bits) & NS_ATTRVALUE_ENUM_VALUE_NEEDS_TO_UPPER_MASK) #define NS_ATTRVALUE_ENUM_VALUE_BITS 16 #define NS_ATTRVALUE_ENUM_VALUE_OFFSET \ (NS_ATTRVALUE_ENUM_TABLEINDEX_OFFSET + NS_ATTRVALUE_ENUM_TABLEINDEX_BITS) #define NS_ATTRVALUE_ENUM_VALUE_MASK \ (PR_BITMASK(NS_ATTRVALUE_ENUM_VALUE_BITS) << NS_ATTRVALUE_ENUM_VALUE_OFFSET) #define NS_ATTRVALUE_ENUM_GET_VALUE(_bits) \ (((_bits) & NS_ATTRVALUE_ENUM_VALUE_MASK) >> NS_ATTRVALUE_ENUM_VALUE_OFFSET) And then use these defines throughout. Feel free to simplify these macros. Might actually be more readable to calculate the values by hand and #define to those values directly. > nsAttrValue::ParseEnumValue(const nsAString& aValue, > const EnumTable* aTable, >- PRBool aCaseSensitive) >+ PRBool aCaseSensitive /* = PR_FALSE */) > { > ResetIfSet(); >+ const EnumTable* firstTableEntry = aTable; Nit: I think it would be cleaner to call this 'tableEntry' and use this variable to iterate over the table. And leave aTable untouched. In general I don't like modifying passed in arguments as it can easily lead to bugs just like the one you're fixing :) Feel free to go either way though. >diff -r c0bac5309fe2 content/html/content/src/nsGenericHTMLElement.cpp >+nsGenericHTMLElement::GetEnumAttrWithDefault(nsIAtom* aAttr, >+ const nsAString& aDefault, >+ nsAString& aResult) I think you can drop the 'WithDefault" as there is no enum getter without it. Also, making the default be a char* instead of an nsAString will make things nicer on the calling side. As it is now we always do the string conversion, weather needed or not. >diff -r c0bac5309fe2 content/html/content/src/nsHTMLButtonElement.cpp >+static const nsAttrValue::EnumTable kButtonTypeTable[] = { >+ { "button", NS_FORM_BUTTON_BUTTON }, >+ { "reset", NS_FORM_BUTTON_RESET }, >+ { "submit", NS_FORM_BUTTON_SUBMIT }, >+ { 0 } >+}; >+ >+// Default type is 'submit'. >+static const nsAttrValue::EnumTable* kButtonDefaultType = &kButtonTypeTable[2]; I'm not really convinced that kButtonDefaultType is needed. It's very unlikely to ever change. I'm fine either way though. Same in nsHTMLInputElement. >+nsHTMLButtonElement::AfterSetAttr(PRInt32 aNameSpaceID, nsIAtom* aName, >+ const nsAString* aValue, PRBool aNotify) >+{ >+ if (aNameSpaceID == kNameSpaceID_None) { >+ if (aName == nsGkAtoms::type && !aValue) { >+ mType = kButtonDefaultType->value; >+ } >+ } >+ >+ return nsGenericHTMLFormElement::AfterSetAttr(aNameSpaceID, aName, >+ aValue, aNotify); >+} Please collapse the two if-statements into one. Nit: If you put the !aValue test first then that will likely short-cut things most of the time as setAttribute is much more common than removeAttribute. >diff -r c0bac5309fe2 content/html/content/src/nsHTMLInputElement.cpp > nsHTMLInputElement::ParseAttribute(PRInt32 aNamespaceID, > nsIAtom* aAttribute, > const nsAString& aValue, > nsAttrValue& aResult) > { > if (aNamespaceID == kNameSpaceID_None) { > if (aAttribute == nsGkAtoms::type) { > // XXX ARG!! This is major evilness. ParseAttribute > // shouldn't set members. Override SetAttr instead > PRInt32 newType; > PRBool success; >- if ((success = aResult.ParseEnumValue(aValue, kInputTypeTable))) { >+ success = aResult.ParseEnumValue(aValue, kInputTypeTable, PR_FALSE); Please combine the line declaring 'success' with the one setting its initial value. P.S. if you are removing all places where we're not specifying the optional argument in the call to ParseEnumValue then it would rock if you make the argument required. I'm not a big fan of optional arguments as a rule of thumb :) Thanks for a great patch!

(In reply to comment #58) > >+ const EnumTable* table = sEnumTableArray-> > >+ ElementAt(allEnumBits & NS_ATTRVALUE_ENUMTABLEINDEX_MASK); > > Hmm.. This looks wierd. This argument passed to ElementAt will always be a > multiple of 16384 or 16 or something like that. I think. > > This bit twiddling is too complex and too spread out. I take full > responsibility for this fact. > > Would you mind centralizing and cleaning it up? Something like the following > defines: > > #define NS_ATTRVALUE_ENUM_TABLEINDEX_BITS (32 - 16 - 1 - > NS_ATTRVALUE_INTEGERTYPE_BITS) > #define NS_ATTRVALUE_ENUM_TABLEINDEX_OFFSET NS_ATTRVALUE_INTEGERTYPE_BITS > #define NS_ATTRVALUE_ENUM_TABLEINDEX_MASK \ > (PR_BITMASK(NS_ATTRVALUE_ENUM_TABLEINDEX_BITS) << > NS_ATTRVALUE_ENUM_TABLEINDEX_OFFSET) > #define NS_ATTRVALUE_ENUM_GET_TABLEINDEX(_bits) \ > (((_bits) & NS_ATTRVALUE_ENUM_TABLEINDEX_MASK) >> > NS_ATTRVALUE_ENUM_TABLEINDEX_OFFSET) > > #define NS_ATTRVALUE_ENUM_VALUE_NEEDS_TO_UPPER_BITS 1 > #define NS_ATTRVALUE_ENUM_VALUE_NEEDS_TO_UPPER_OFFSET \ > (NS_ATTRVALUE_ENUM_TABLEINDEX_OFFSET + NS_ATTRVALUE_ENUM_TABLEINDEX_BITS) > #define NS_ATTRVALUE_ENUM_VALUE_NEEDS_TO_UPPER_MASK \ > (PR_BITMASK(NS_ATTRVALUE_ENUM_VALUE_NEEDS_TO_UPPER_BITS) << > NS_ATTRVALUE_ENUM_VALUE_NEEDS_TO_UPPER_OFFSET) > #define NS_ATTRVALUE_ENUM_VALUE_NEEDS_TO_UPPER(_bits) \ > ((_bits) & NS_ATTRVALUE_ENUM_VALUE_NEEDS_TO_UPPER_MASK) > > #define NS_ATTRVALUE_ENUM_VALUE_BITS 16 > #define NS_ATTRVALUE_ENUM_VALUE_OFFSET \ > (NS_ATTRVALUE_ENUM_TABLEINDEX_OFFSET + NS_ATTRVALUE_ENUM_TABLEINDEX_BITS) > #define NS_ATTRVALUE_ENUM_VALUE_MASK \ > (PR_BITMASK(NS_ATTRVALUE_ENUM_VALUE_BITS) << NS_ATTRVALUE_ENUM_VALUE_OFFSET) > #define NS_ATTRVALUE_ENUM_GET_VALUE(_bits) \ > (((_bits) & NS_ATTRVALUE_ENUM_VALUE_MASK) >> NS_ATTRVALUE_ENUM_VALUE_OFFSET) > > And then use these defines throughout. Feel free to simplify these macros. > Might actually be more readable to calculate the values by hand and #define to > those values directly. I would prefer to do that in a follow-up because I think that's quite sensitive so it would be better to have a separate review for this. > >diff -r c0bac5309fe2 content/html/content/src/nsGenericHTMLElement.cpp > >+nsGenericHTMLElement::GetEnumAttrWithDefault(nsIAtom* aAttr, > >+ const nsAString& aDefault, > >+ nsAString& aResult) > > I think you can drop the 'WithDefault" as there is no enum getter without it. > Also, making the default be a char* instead of an nsAString will make things > nicer on the calling side. As it is now we always do the string conversion, > weather needed or not. In general, I prefer to do all the conversions before calling a function. Now, if we have a nsAString, char* will have to be passed to the function then it will be changed to nsAString again. Anyway, I did the fix because we only use char* as default arguments and I don't think it's going to change in a near future. > >diff -r c0bac5309fe2 content/html/content/src/nsHTMLButtonElement.cpp > > >+static const nsAttrValue::EnumTable kButtonTypeTable[] = { > >+ { "button", NS_FORM_BUTTON_BUTTON }, > >+ { "reset", NS_FORM_BUTTON_RESET }, > >+ { "submit", NS_FORM_BUTTON_SUBMIT }, > >+ { 0 } > >+}; > >+ > >+// Default type is 'submit'. > >+static const nsAttrValue::EnumTable* kButtonDefaultType = &kButtonTypeTable[2]; > > I'm not really convinced that kButtonDefaultType is needed. It's very unlikely > to ever change. I'm fine either way though. Same in nsHTMLInputElement. Using NS_FORM_INPUT_TEXT/NS_FORM_BUTTON_SUBMIT was fine but we need the tag string for the default value now so I prefer to use this static var. > P.S. if you are removing all places where we're not specifying the optional > argument in the call to ParseEnumValue then it would rock if you make the > argument required. I'm not a big fan of optional arguments as a rule of thumb > :) The new patch will have everything else (even the PS).

r=smaug, sr=jonas

sorry, r=smaug, sr=sicking