490665 - [HTML5][Patch] Make isindex magic in submission

Status: RESOLVED FIXED

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

Description

[Henri Sivonen (:hsivonen)]

To make sense for the DOM the HTML5 parser builds, the form submission code needs to do the IE-like magic when a text input is named isindex:
http://www.whatwg.org/specs/web-apps/current-work/multipage/forms.html#form-submission

'If the entry's name is "isindex", its type is "text", and this is the first entry in the form data set, then append the value to result and skip the rest of the substeps for this entry, moving on to the next entry, if any, or the next step in the overall algorithm otherwise.'

Comment 1

[Henri Sivonen (:hsivonen)]

sicking: this bug may be relevant to your form submission cleanup.

Comment 2

[Henri Sivonen (:hsivonen)]

Attachment: Implement HTML5 exception for isindex

Comment 3

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 429694 [details] [diff] [review]
Implement HTML5 exception for isindex

> nsresult
> nsFormData::AddNameValuePair(const nsAString& aName,
>-                             const nsAString& aValue)
>+                             const nsAString& aValue,
>+                             PRBool aInputTypeText)

I don't like fixing this by adding another argument to AddNameValuePair and thus moving some of the type logic into the submission, the code got quite nicer when I removed similar arguments in the recent cleanup IMHO.

An alternative approach would be to add something like the following functions to nsFormSubmission:


  virtual PRBool SupportsIsIndexSubmission()
  {
    return PR_FALSE;
  }

  virtual nsresult AddIsIndex(const nsAString& aValue)
  {
    NS_NOTREACHED("AddIsIndex called when not supported");
    return NS_ERROR_UNEXPECTED;
  }

And then override those in nsFSURLEncoded, and use them in nsHTMLInputElement::SubmitNamesValues.

Comment 4

[Henri Sivonen (:hsivonen)]

Attachment: Try again addressing review comments

I removed the last NS_FORM_INPUT_IMAGE check while I was at it, because it appears to be dead code considering the earlier NS_FORM_INPUT_IMAGE check.

Comment 5

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 431621 [details] [diff] [review]
Try again addressing review comments

>diff --git a/content/html/content/public/nsIFormSubmission.h b/content/html/content/public/nsIFormSubmission.h

>+  virtual PRBool SupportsIsIndexSubmission()
...
>+  virtual nsresult AddIsIndex(const nsAString& aValue)

Nit: Looking at it, I think casing it as "Isindex" rather than "IsIndex" is better. Treating it as one word makes it clearer that this is for <isindex> elements.

>diff --git a/content/html/content/src/nsHTMLInputElement.cpp b/content/html/content/src/nsHTMLInputElement.cpp
>+  else if (mType == NS_FORM_INPUT_TEXT &&
>+           aFormSubmission->SupportsIsIndexSubmission() &&
>+           name.EqualsLiteral("isindex")) {
>+    rv = aFormSubmission->AddIsIndex(value);

Nit: It's probably slightly faster to reverse the last two tests. Mostly because of the virtual call overhead, but also because the SupportsIsIndexSubmission test likely will test true much more often.

r=me with those nits fixed. Thanks for the patch!

Comment 6

[Henri Sivonen (:hsivonen)]

Thanks. Pushed with the nits fixed.

http://hg.mozilla.org/mozilla-central/rev/a0061217d338