Last Comment Bug 787095 - Update formMethod reflection to have the empty string as default value (and 'get' as invalid value)
: Update formMethod reflection to have the empty string as default value (and '...
Status: RESOLVED FIXED
[mentor=mounir][lang=c++]
:
Product: Core
Classification: Components
Component: DOM: Core & HTML (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla21
Assigned To: Benedict Singer
:
: Andrew Overholt [:overholt]
Mentors:
http://html5.org/tools/web-apps-track...
Depends on: 839171
Blocks: 582412
  Show dependency treegraph
 
Reported: 2012-08-30 08:51 PDT by Mounir Lamouri (:mounir)
Modified: 2013-02-07 11:16 PST (History)
4 users (show)
ryanvm: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Initial patch + test updates. (15.99 KB, patch)
2013-01-12 17:43 PST, Benedict Singer
mounir: review+
Details | Diff | Splinter Review
Patch with nits fixed. (16.34 KB, patch)
2013-01-16 08:47 PST, Benedict Singer
mounir: review+
Details | Diff | Splinter Review
Final patch. (13.52 KB, patch)
2013-01-22 17:46 PST, Benedict Singer
no flags Details | Diff | Splinter Review

Description Mounir Lamouri (:mounir) 2012-08-30 08:51:54 PDT
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.
Comment 1 Mounir Lamouri (:mounir) 2012-08-30 09:07:39 PDT
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.
Comment 2 abhishek rajput 2012-08-31 02:19:09 PDT
i am interested in working on it. can it be assigned to me?
Comment 3 Mounir Lamouri (:mounir) 2012-08-31 05:52:07 PDT
(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.
Comment 4 Benedict Singer 2013-01-12 17:43:20 PST
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.
Comment 5 Benedict Singer 2013-01-12 17:44:18 PST
(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
Comment 6 Mounir Lamouri (:mounir) 2013-01-15 07:53:43 PST
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|.
Comment 7 Benedict Singer 2013-01-16 08:47:08 PST
Created attachment 702855 [details] [diff] [review]
Patch with nits fixed.

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.
Comment 8 Mounir Lamouri (:mounir) 2013-01-21 09:05:00 PST
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
Comment 9 Benedict Singer 2013-01-21 16:08:39 PST
(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.
Comment 10 Mounir Lamouri (:mounir) 2013-01-22 09:43:17 PST
(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(".
Comment 11 Benedict Singer 2013-01-22 17:46:17 PST
Created attachment 705182 [details] [diff] [review]
Final patch.

Fixed the indentation. All other nits fixed, and form.{method, etc} changes reverted. Should be good to go.
Comment 12 Ryan VanderMeulen [:RyanVM] 2013-01-23 04:31:01 PST
https://hg.mozilla.org/integration/mozilla-inbound/rev/840023db4d12
Comment 13 Ryan VanderMeulen [:RyanVM] 2013-01-23 08:33:21 PST
https://hg.mozilla.org/mozilla-central/rev/840023db4d12
Comment 14 Mounir Lamouri (:mounir) 2013-01-27 02:25:48 PST
Benedict, for your information, the spec has been changed to match the initial bug report. We do follow the specs now ;)
Comment 15 Mounir Lamouri (:mounir) 2013-01-27 02:26:19 PST
And thanks again for your contribution :)
Comment 16 Benedict Singer 2013-01-27 07:08:11 PST
Cool. Definitely makes more sense this way!

Note You need to log in before you can comment on or make changes to this bug.