1347640 - HTMLInputElement shouldn't try to cancel image request on all the type changes

Status: RESOLVED FIXED

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

Description

[Olli Pettay [:smaug][[e-mail]]]

http://searchfox.org/mozilla-central/rev/ca7015fa45b30b29176fbaa70ba0a36fe9263c38/dom/html/HTMLInputElement.cpp#1462-1465

Comment 1

[Boris Zbarsky [:bzbarsky]]

General plan:

1)  Fix bug 656197 (patches coming up there).
2)  Move the change of mType from ParseAttribute to AfterSetAttr.
3)  Then we know what our type change is and can handle it accordingly

Comment 2

[Boris Zbarsky [:bzbarsky]]

Attachment: part 1. Move the changing of HTMLInputElement's mType from ParseAttribute to AfterSetAttr

This will make the timing of the change more consistent between SetAttr and
UnsetAttr, and ensure that we have both the old and new type available in
AfterSetAttr.

MozReview-Commit-ID: Gsrxkkve7BC

Comment 3

[Boris Zbarsky [:bzbarsky]]

Attachment: part 2. Move a few more things from AfterSetAttr to HandleTypeChange

We also stop doing some unnecessary work (e.g. canceling image requests if our old type wasn't image).

MozReview-Commit-ID: GLPl1McLL9N

Comment 4

[Boris Zbarsky [:bzbarsky]]

Bug 656197 comment 7 applies here too.  Please redirect the reviews as needed.

Comment 5

[Boris Zbarsky [:bzbarsky]]

Oh, I forgot to explain why bug 656197 is relevant at all: once that's done, there isn't much of interest between ParseAttribute and AfterSetAttr, except the actual setting of the attribute in the nsAttrAndChildArray.  So moving the type change from one to the other is pretty straightforward, I think.

Comment 6

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 8847955 [details] [diff] [review]
part 1.  Move the changing of HTMLInputElement's mType from ParseAttribute to AfterSetAttr

># HG changeset patch
># User Boris Zbarsky <[e-mail]>
># Date 1489646004 14400
>#      Thu Mar 16 02:33:24 2017 -0400
># Node ID c2fd694966f2437814db2071ff5157c5c3082df8
># Parent  c979bd214031e726c19a4b3a9bf678e16c49c9af
>Bug 1347640 part 1.  Move the changing of HTMLInputElement's mType from ParseAttribute to AfterSetAttr.  r=smaug
>
>This will make the timing of the change more consistent between SetAttr and
>UnsetAttr, and ensure that we have both the old and new type available in
>AfterSetAttr.
>
>MozReview-Commit-ID: Gsrxkkve7BC
>
>diff --git a/dom/html/HTMLInputElement.cpp b/dom/html/HTMLInputElement.cpp
>--- a/dom/html/HTMLInputElement.cpp
>+++ b/dom/html/HTMLInputElement.cpp
>@@ -173,25 +173,28 @@ static const nsAttrValue::EnumTable kInp
>   { "month", NS_FORM_INPUT_MONTH },
>   { "number", NS_FORM_INPUT_NUMBER },
>   { "password", NS_FORM_INPUT_PASSWORD },
>   { "radio", NS_FORM_INPUT_RADIO },
>   { "range", NS_FORM_INPUT_RANGE },
>   { "search", NS_FORM_INPUT_SEARCH },
>   { "submit", NS_FORM_INPUT_SUBMIT },
>   { "tel", NS_FORM_INPUT_TEL },
>-  { "text", NS_FORM_INPUT_TEXT },
>   { "time", NS_FORM_INPUT_TIME },
>   { "url", NS_FORM_INPUT_URL },
>   { "week", NS_FORM_INPUT_WEEK },
>+  // "text" must be last for ParseAttribute to work right.  If you add things
>+  // before it, please update kInputDefaultType.
Why?

>+  { "text", NS_FORM_INPUT_TEXT },
>   { nullptr, 0 }
> };
> 
> // Default type is 'text'.
>-static const nsAttrValue::EnumTable* kInputDefaultType = &kInputTypeTable[18];
>+static const nsAttrValue::EnumTable* kInputDefaultType =
>+  &kInputTypeTable[ArrayLength(kInputTypeTable)-2];
...since here you anyway take the second last item.

Nit space before and after -

> HTMLInputElement::HandleTypeChange(uint8_t aNewType)
> {
>-  if (mType == NS_FORM_INPUT_RANGE && mIsDraggingRange) {
>+  uint8_t oldType = mType;
>+  if (aNewType == NS_FORM_INPUT_FILE || oldType == NS_FORM_INPUT_FILE) {
>+    // This call isn't strictly needed any more since we'll never
>+    // confuse values and filenames. However it's there for backwards
>+    // compat.
>+    ClearFiles(false);
>+  }
Is the comment right even in case one does
<input type=file>, select some files and then
var f = input.files[0];
input.type = "text";
input.type = "file";
input.files[0] == f; // This should be false

>+      aResult.ParseEnumValue(aValue, kInputTypeTable, false, kInputDefaultType);
>+      int32_t newType = aResult.GetEnumValue();
>+      if ((IsExperimentalMobileType(newType) &&
>+           !IsExperimentalFormsEnabled()) ||
>+          (newType == NS_FORM_INPUT_NUMBER && !IsInputNumberEnabled()) ||
>+          (newType == NS_FORM_INPUT_COLOR && !IsInputColorEnabled()) ||
>+          (IsDateTimeInputType(newType) &&
>+           !IsDateTimeTypeSupported(newType))) {
>+        // There's no public way to set an nsAttrValue to an enum value, but we
>+        // can just re-parse with a table that doesn't have any types other than
>+        // "text" in it.
>+        aResult.ParseEnumValue(aValue, kInputDefaultType, false, kInputDefaultType);
interesting way to handle this

Comment 7

[Boris Zbarsky [:bzbarsky]]

> Nit space before and after -

Done.

> Is the comment right even in case one does

Sort of.  We could clear on going _to_ or _from_ the file state, but not both, and have that invariant still hold.  Updated the comment to say:

    // Strictly speaking, we only need to clear files on going _to_ or _from_
    // the NS_FORM_INPUT_FILE type, not both, since we'll never confuse values
    // and filenames. But this is safer.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/5a16cced577a
part 1.  Move the changing of HTMLInputElement's mType from ParseAttribute to AfterSetAttr.  r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/126828c05e08
part 2.  Move a few more things from AfterSetAttr to HandleTypeChange.  r=smaug

Comment 9

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/5a16cced577a
https://hg.mozilla.org/mozilla-central/rev/126828c05e08