The default bug view has changed. See this FAQ.

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

RESOLVED FIXED in mozilla21

Status

()

Core
DOM: Core & HTML
RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: mounir, Assigned: Benedict Singer)

Tracking

Trunk
mozilla21
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [mentor=mounir][lang=c++], URL)

Attachments

(1 attachment, 2 obsolete attachments)

(Reporter)

Description

5 years ago
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

5 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

5 years ago
i am interested in working on it. can it be assigned to me?
(Reporter)

Comment 3

5 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

4 years ago
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.
Assignee: nobody → singerb.dev
Status: NEW → ASSIGNED
Attachment #701524 - Flags: review?(mounir)
(Assignee)

Comment 5

4 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

4 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

4 years ago
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.
Attachment #701524 - Attachment is obsolete: true
Attachment #702855 - Flags: feedback?(mounir)
(Reporter)

Comment 8

4 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

4 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

4 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

4 years ago
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.
Attachment #702855 - Attachment is obsolete: true
(Assignee)

Updated

4 years ago
Keywords: checkin-needed
https://hg.mozilla.org/integration/mozilla-inbound/rev/840023db4d12
Flags: in-testsuite+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/840023db4d12
Status: ASSIGNED → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla21
(Reporter)

Comment 14

4 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

4 years ago
And thanks again for your contribution :)
(Assignee)

Comment 16

4 years ago
Cool. Definitely makes more sense this way!

Updated

4 years ago
Depends on: 839171
You need to log in before you can comment on or make changes to this bug.