Closed
Bug 787095
Opened 12 years ago
Closed 11 years ago
Update formMethod reflection to have the empty string as default value (and 'get' as invalid value)
Categories
(Core :: DOM: Core & HTML, defect)
Core
DOM: Core & HTML
Tracking
()
RESOLVED
FIXED
mozilla21
People
(Reporter: mounir, Assigned: singerb.dev)
References
()
Details
(Whiteboard: [mentor=mounir][lang=c++])
Attachments
(1 file, 2 obsolete files)
13.52 KB,
patch
|
Details | Diff | Splinter Review |
See $URL for the HTML spec change. See https://www.w3.org/Bugs/Public/show_bug.cgi?id=17185 for the reasons. I think it makes sense.
Reporter | ||
Comment 1•12 years ago
|
||
If you are interested to fix that, you will need to add a method like GetEnumAttr() but taking different values for the invalid default and missing default. Actually GetEnumAttr() will have to call that one. You will also need to have a new macro like NS_IMPL_ENUM_ATTR_DEFAULT_VALUE which will have to take two default values too. Those things live in content/html/content/src/nsGenericHTMLElement.h Finally, you will have to change content/html/content/src/nsHTMLInputElement.cpp: NS_IMPL_ENUM_ATTR_DEFAULT_VALUE(nsHTMLInputElement, FormMethod, formmethod, kFormDefaultMethod->tag) should be replaced by the new macro and take kFormDefaultMethod->tag and the EmptyString() as default values.
Whiteboard: [mentor=mounir][lang=c++]
Comment 2•12 years ago
|
||
i am interested in working on it. can it be assigned to me?
Reporter | ||
Comment 3•12 years ago
|
||
(In reply to abhishek rajput from comment #2) > i am interested in working on it. can it be assigned to me? I prefer to assign bugs when a first patch has been attached.
Assignee | ||
Comment 4•12 years ago
|
||
Here's a patch implementing the approach described above. Per the spec, (http://www.whatwg.org/specs/web-apps/current-work/#form-submission-0) both formMethod/formEnctype on input and method/enctype on form follow the new rules for missing and invalid defaults. Testcases for these attributes already existed, but the test framework needed updating to handle 2 different default values. The attached patch updates the reflect.js test framework as well as the 5 tests affected by this spec change.
Assignee | ||
Comment 5•12 years ago
|
||
(In reply to Benedict Singer from comment #4) > Created attachment 701524 [details] [diff] [review] > Initial patch + test updates. > > Here's a patch implementing the approach described above. Per the spec, > (http://www.whatwg.org/specs/web-apps/current-work/#form-submission-0) both > formMethod/formEnctype on input and method/enctype on form follow the new > rules for missing and invalid defaults. Testcases for these attributes > already existed, but the test framework needed updating to handle 2 > different default values. The attached patch updates the reflect.js test > framework as well as the 5 tests affected by this spec change. Forgot to add, here's a green Try run except for what looks to be a random orange: https://tbpl.mozilla.org/?tree=Try&rev=d9be48365038
Reporter | ||
Comment 6•12 years ago
|
||
Comment on attachment 701524 [details] [diff] [review] Initial patch + test updates. Review of attachment 701524 [details] [diff] [review]: ----------------------------------------------------------------- r=me with the macro naming issue solved, the nit fixed and reflect.js changes modified to take into account my comments. ::: content/html/content/src/nsGenericHTMLElement.cpp @@ +2043,5 @@ > + if (!attrVal && aDefaultMissing) { > + AppendASCIItoUTF16(nsDependentCString(aDefaultMissing), aResult); > + } else if (attrVal && attrVal->Type() != nsAttrValue::eEnum && aDefaultInvalid) { > + AppendASCIItoUTF16(nsDependentCString(aDefaultInvalid), aResult); > + } else if (attrVal && attrVal->Type() == nsAttrValue::eEnum) { This is easier for me to understand: if (!attrVal) { if (aDefaultMissing) { AppendASCIItoUTF16(nsDependentCString(aDefaultMissing), aResult); } return; } if (attrVal->Type() == nsAttrValue::eEnum) { attrVal->GetEnumString(aResult, true); return; } if (aDefaultInvalid) { AppendASCIItoUTF16(nsDependentCString(aDefaultInvalid), aResult); } ... mostly because I don't have to try to guess if all cases are taken into account. Feel free to keep your current code if you prefer it though. ::: content/html/content/src/nsGenericHTMLElement.h @@ +919,5 @@ > const char* aDefault, > nsAString& aResult) const; > > /** > + * Helper method for NS_IMPL_ENUM_ATTR_DEFAULT_VALUE. nit: _VALUES. @@ +1383,5 @@ > + * A macro to implement the getter and setter for a given content > + * property that needs to set an enumerated string. The method > + * uses a specific GetEnumAttr and the generic SetAttrHelper methods. > + */ > +#define NS_IMPL_ENUM_ATTR_DEFAULT_VALUES(_class, _method, _atom, _defaultMissing, _defaultInvalid) \ I'm not a big fan of having NS_IMPL_ENUM_ATTR_DEFAULT_VALUE and NS_IMPL_ENUM_DEFAULT_VALUES. The confusion would be too simple to happen. I think the best thing to do would be to convert all NS_IMPL_ENUM_ATTR_DEFAULT_VALUE to use NS_IMPL_ENUM_DEFAULT_VALUES. That way, we have only one method. Otherwise, we should find better names. If you want to go with using only NS_IMPL_ENUM_DEFAULT_VALUES, feel free to do that in a follow-up. If you want to rename, please do that in this bug. ::: content/html/content/test/reflect.js @@ +253,5 @@ > + * - defaultValue String [optional] default value when no valid value is set > + * OR > + * defaultValueInvalid String [optional] default value when an invalid value is set; if not set, defaultValue is used > + * defaultValueMissing String [optional] default value when no value is set; if not set, defaultValue is used > + * - unsupportedValues Array [optional] valid values we do not support I would highly prefer to have defaultValue being: * - defaultValue String [optional] default value when the empty string or no valid value is set. * OR * - defaultValue Object [optional] object containing two attributes, 'invalid' and 'missing'. A bit like |attribute| a few lines above |defaultValue|.
Attachment #701524 -
Flags: review?(mounir) → review+
Assignee | ||
Comment 7•12 years ago
|
||
The reflect.js changes are much nicer that way, thanks. I decided to rename the new macro, rather than change everything else; additionally, I tweaked the if/else structure in GetEnumAttr to make it more clear what's happening. Asking for feedback on those 2 changes.
Attachment #701524 -
Attachment is obsolete: true
Attachment #702855 -
Flags: feedback?(mounir)
Reporter | ||
Comment 8•12 years ago
|
||
Comment on attachment 702855 [details] [diff] [review] Patch with nits fixed. Review of attachment 702855 [details] [diff] [review]: ----------------------------------------------------------------- I re-read the specifications and I think Hixie did a mistake by changing form.{method,enctype,encoding} reflection behaviour. The change requested by Simon regarding formMethod and formEnctype reflection is sensible but changing the form element reflection can't follow the same logic. I think we should not change the form element attribute reflection and only change input element's attributes. r=me with that change. ::: content/html/content/src/nsGenericHTMLElement.h @@ +921,5 @@ > > /** > + * Helper method for NS_IMPL_ENUM_ATTR_DEFAULT_MISSING_INVALID_VALUES. > + * Gets the enum value string of an attribute and using a default value if > + * the attribute is missing or the string is an invalid enum value. nit: this description isn't very clear. There are two default values, not only one. @@ +1380,5 @@ > } > > /** > + * A macro to implement the getter and setter for a given content > + * property that needs to set an enumerated string. The method nit: could you mention the fact that this is like NS_IMPL_ENUM_ATTR_DEFAULT but with two default values? ::: content/html/content/src/nsHTMLInputElement.cpp @@ +917,5 @@ > NS_IMPL_ACTION_ATTR(nsHTMLInputElement, FormAction, formaction) > +NS_IMPL_ENUM_ATTR_DEFAULT_MISSING_INVALID_VALUES(nsHTMLInputElement, FormEnctype, formenctype, > + "", kFormDefaultEnctype->tag) > +NS_IMPL_ENUM_ATTR_DEFAULT_MISSING_INVALID_VALUES(nsHTMLInputElement, FormMethod, formmethod, > + "", kFormDefaultMethod->tag) nit: could you please fix alignment/indentation? ::: content/html/content/test/reflect.js @@ +265,5 @@ > var validValues = aParameters.validValues; > var invalidValues = aParameters.invalidValues; > var defaultValue = aParameters.defaultValue !== undefined > ? aParameters.defaultValue : ""; > + var defaultValueInvalid = aParameters.defaultValue === undefined nit: trailing whitespace
Attachment #702855 -
Flags: feedback?(mounir) → review+
Assignee | ||
Comment 9•11 years ago
|
||
(In reply to Mounir Lamouri (:mounir) from comment #8) > Comment on attachment 702855 [details] [diff] [review] > Patch with nits fixed. > > Review of attachment 702855 [details] [diff] [review]: > ----------------------------------------------------------------- > > I re-read the specifications and I think Hixie did a mistake by changing > form.{method,enctype,encoding} reflection behaviour. The change requested by > Simon regarding formMethod and formEnctype reflection is sensible but > changing the form element reflection can't follow the same logic. I think we > should not change the form element attribute reflection and only change > input element's attributes. > > r=me with that change. Is it worth filing something on the W3C bug tracker suggesting that this behavior be changed in the spec? > ::: content/html/content/src/nsGenericHTMLElement.h > @@ +921,5 @@ > > > > /** > > + * Helper method for NS_IMPL_ENUM_ATTR_DEFAULT_MISSING_INVALID_VALUES. > > + * Gets the enum value string of an attribute and using a default value if > > + * the attribute is missing or the string is an invalid enum value. > > nit: this description isn't very clear. There are two default values, not > only one. Done. > @@ +1380,5 @@ > > } > > > > /** > > + * A macro to implement the getter and setter for a given content > > + * property that needs to set an enumerated string. The method > > nit: could you mention the fact that this is like NS_IMPL_ENUM_ATTR_DEFAULT > but with two default values? Done. > ::: content/html/content/src/nsHTMLInputElement.cpp > @@ +917,5 @@ > > NS_IMPL_ACTION_ATTR(nsHTMLInputElement, FormAction, formaction) > > +NS_IMPL_ENUM_ATTR_DEFAULT_MISSING_INVALID_VALUES(nsHTMLInputElement, FormEnctype, formenctype, > > + "", kFormDefaultEnctype->tag) > > +NS_IMPL_ENUM_ATTR_DEFAULT_MISSING_INVALID_VALUES(nsHTMLInputElement, FormMethod, formmethod, > > + "", kFormDefaultMethod->tag) > > nit: could you please fix alignment/indentation? What would you like for indentation here? Currently it's the same as Inputmode on line 924. > ::: content/html/content/test/reflect.js > @@ +265,5 @@ > > var validValues = aParameters.validValues; > > var invalidValues = aParameters.invalidValues; > > var defaultValue = aParameters.defaultValue !== undefined > > ? aParameters.defaultValue : ""; > > + var defaultValueInvalid = aParameters.defaultValue === undefined > > nit: trailing whitespace Done.
Reporter | ||
Comment 10•11 years ago
|
||
(In reply to Benedict Singer from comment #9) > (In reply to Mounir Lamouri (:mounir) from comment #8) > > Comment on attachment 702855 [details] [diff] [review] > > Patch with nits fixed. > > > > Review of attachment 702855 [details] [diff] [review]: > > ----------------------------------------------------------------- > > > > I re-read the specifications and I think Hixie did a mistake by changing > > form.{method,enctype,encoding} reflection behaviour. The change requested by > > Simon regarding formMethod and formEnctype reflection is sensible but > > changing the form element reflection can't follow the same logic. I think we > > should not change the form element attribute reflection and only change > > input element's attributes. > > > > r=me with that change. > > Is it worth filing something on the W3C bug tracker suggesting that this > behavior be changed in the spec? I already re-opened the original W3C bug (see comment 0 for the link). > > ::: content/html/content/src/nsHTMLInputElement.cpp > > @@ +917,5 @@ > > > NS_IMPL_ACTION_ATTR(nsHTMLInputElement, FormAction, formaction) > > > +NS_IMPL_ENUM_ATTR_DEFAULT_MISSING_INVALID_VALUES(nsHTMLInputElement, FormEnctype, formenctype, > > > + "", kFormDefaultEnctype->tag) > > > +NS_IMPL_ENUM_ATTR_DEFAULT_MISSING_INVALID_VALUES(nsHTMLInputElement, FormMethod, formmethod, > > > + "", kFormDefaultMethod->tag) > > > > nit: could you please fix alignment/indentation? > > What would you like for indentation here? Currently it's the same as > Inputmode on line 924. Just align the second line with the first character after "NS_IMPL_ENUM_ATTR_DEFAULT_MISSING_INVALID_VALUES(".
Assignee | ||
Comment 11•11 years ago
|
||
Fixed the indentation. All other nits fixed, and form.{method, etc} changes reverted. Should be good to go.
Attachment #702855 -
Attachment is obsolete: true
Assignee | ||
Updated•11 years ago
|
Keywords: checkin-needed
Comment 12•11 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/840023db4d12
Flags: in-testsuite+
Keywords: checkin-needed
Comment 13•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/840023db4d12
Status: ASSIGNED → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla21
Reporter | ||
Comment 14•11 years ago
|
||
Benedict, for your information, the spec has been changed to match the initial bug report. We do follow the specs now ;)
Reporter | ||
Comment 15•11 years ago
|
||
And thanks again for your contribution :)
Assignee | ||
Comment 16•11 years ago
|
||
Cool. Definitely makes more sense this way!
You need to log in
before you can comment on or make changes to this bug.
Description
•