787095 - Update formMethod reflection to have the empty string as default value (and 'get' as invalid value)

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Mounir Lamouri (:mounir)]

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)]

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]

i am interested in working on it. can it be assigned to me?

Comment 3

[Mounir Lamouri (:mounir)]

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

Attachment: 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]

(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)]

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]

Attachment: 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)]

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]

(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)]

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

Attachment: 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]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/840023db4d12

Comment 13

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/840023db4d12

Comment 14

[Mounir Lamouri (:mounir)]

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)]

And thanks again for your contribution :)

Comment 16

[Benedict Singer]

Cool. Definitely makes more sense this way!