Closed Bug 1310865 Opened 8 years ago Closed 8 years ago

cloneNode() removes line breaks from <input type=hidden>

Categories

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

Product:
Core
Component:
DOM: Core & HTML
Version:
49 Branch
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla52
Tracking Flags:
Tracking Status
firefox49 --- wontfix
firefox50 - wontfix
firefox51 + fixed
firefox52 - fixed

People

(Reporter: drkninja, Assigned: ayg)

References

(
URL
)

Details

(Keywords: regression)

Attachments

(2 files)

Bug 1310865 - Newlines removed from hidden input fie.html
297 bytes, text/html
Details
Bug 1310865 - Don't process cloned <input> until all attributes are copied;
58 bytes, text/x-review-board-request
hsivonen
: review+
gchang
: approval-mozilla-beta+
Details 
User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_9_5) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/53.0.2785.143 Safari/537.36

Steps to reproduce:

Clone a hidden input using jQuery .clone() and POST it e.g. https://jsfiddle.net/zdq9aqkp/


Actual results:

Newline characters were removed from the clone


Expected results:

Newline characters should have been preserved as per previous versions < 49
It should be "cloned-hidden": "Hidden\r\nNewline".

Regression range:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=922be574731fabc93c41ab50ca7861958df57a29&tochange=45cde9858aa0dd06f18de23e517b11e334d9d410

Aryeh Gregor — Bug 1264270 - Parser should output attributes in source order, not reversed; r=hsivonen,bgrins

Henry, could you find someone available to work on this regression as :ayg and :bgrins are not available at the moment?
Blocks: 1264270
Status: UNCONFIRMED → NEW
Has Regression Range: --- → yes
Has STR: --- → yes
status-firefox49: --- → affected
status-firefox50: --- → affected
status-firefox51: --- → affected
status-firefox52: --- → affected
tracking-firefox50: --- → ?
tracking-firefox51: --- → ?
tracking-firefox52: --- → ?
Component: Untriaged → HTML: Parser
Ever confirmed: true
Flags: needinfo?(hsivonen)
Keywords: regression
OS: Unspecified → All
Product: Firefox → Core
Hardware: Unspecified → All
The bug is not in JQuery. This happens with plain cloneNode(). (I'm very surprised that the patch for bug 1264270 could break this.)

Keeping myself needinfoed so that I remember to get back to this.
URL: https://jsfiddle.net/uokopxs4/1/
Component: HTML: Parser → DOM: Core & HTML
Summary: Newlines removed from hidden input field → cloneNode() removes line breaks from <input type=hidden>
If cloneNode() sets the "type" attribute after the "value" attribute, newlines will be lost because the "type" value is "text" until it is changed to "hidden".
(In reply to Henri Sivonen (:hsivonen) from comment #2)
> (I'm very surprised that the patch for bug 1264270 could break this.)

I tested again, same regression range.
(In reply to Masatoshi Kimura [:emk] from comment #3)
> If cloneNode() sets the "type" attribute after the "value" attribute,
> newlines will be lost because the "type" value is "text" until it is changed
> to "hidden".

Good point. Maybe we on cloning we should have a similar "element not done yet" state as when parsing.

It seems that Aryeh will be back next week. Aryeh, can you take this bug?
I'm having trouble producing a minimal test-case.  Can someone provide one, not using jQuery and using as few lines as possible?  From the comments in the bug, it looks like this outputs "0":

  <!DOCTYPE html>
  <input value="&#10;" type=hidden>
  <script>document.body.textContent = document.body.firstChild.cloneNode(false).value.length</script>

But I get "1" in Nightly 52.0a (2016-10-25), as expected.
(In reply to Aryeh Gregor (:ayg) (working until November 1) from comment #6)
> I'm having trouble producing a minimal test-case.  Can someone provide one,
> not using jQuery and using as few lines as possible?  From the comments in
> the bug, it looks like this outputs "0":
> 
>   <!DOCTYPE html>
>   <input value="&#10;" type=hidden>
>   <script>document.body.textContent =
> document.body.firstChild.cloneNode(false).value.length</script>
> 
> But I get "1" in Nightly 52.0a (2016-10-25), as expected.

So long as the value isn't just newlines i.e. changing the value to something like "A&#10;B"; should see it return 2 instead of the expected 3.

If the value just contains newline chars then they're not removed.
Thanks, Alice and M!  The order in the markup actually doesn't matter, presumably because type is mapped and value is not, so previously we would (by complete accident) always set the type first and now we always set the value first.

The question is, how to implement this?  I could add a flag of some kind that's checked by HTMLInputElement::ParseAttribute, but it would have to be done in some way that doesn't affect performance elsewhere at all.  Henri, I could probably write a patch for this if I get some guidance on how to do it (quickly, because I'm vanishing again very soon).
(In reply to Aryeh Gregor (:ayg) (working until November 1) from comment #9)
> Henri, I
> could probably write a patch for this if I get some guidance on how to do it
> (quickly, because I'm vanishing again very soon).

Per comment 5, I suggest reusing the HTMLInputElement::DoneCreatingElement() machinery. I.e. have the clone start out with mParserCreating set to true (and the field renamed not to suggest it's a parser-only thing anymore), copying over the attributes and then calling HTMLInputElement::DoneCreatingElement().
Flags: needinfo?(hsivonen)
Priority: -- → P2
Assignee: nobody → ayg
Status: NEW → ASSIGNED
Comment on attachment 8805106 [details]
Bug 1310865 - Don't process cloned <input> until all attributes are copied;

https://reviewboard.mozilla.org/r/88958/#review88326
Attachment #8805106 - Flags: review?(hsivonen) → review+
Pushed by ayg@aryeh.name:
https://hg.mozilla.org/integration/autoland/rev/aadb57a9ddf8
Don't process cloned <input> until all attributes are copied; r=hsivonen
https://hg.mozilla.org/mozilla-central/rev/aadb57a9ddf8
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox52: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
Tracking 52- as this is now resolved fixed.
tracking-firefox52: ? → -
Track 51+ as regression.

Hi :ayg,
Since this is a regression and also affects 51, do you think the patch is worth uplifting to 51?
tracking-firefox51: ? → +
Flags: needinfo?(ayg)
It sounds reasonable to me, but I'm not sure, so I'll punt the question to Henri.
Flags: needinfo?(ayg) → needinfo?(hsivonen)
I think functionality-wise this is worth uplifting, but rist-wise this is big enough a change that I'd like to let this bake for a week or two before doing the uplift.
Flags: needinfo?(hsivonen)
Noting that in a week or so it will be an uplift to 51 in early beta.   Likely too late to land in 50.
status-firefox49: affected → wontfix
50 is in RC mode, will go-live in a few days. It is too late, this is now a wontfix.
status-firefox50: affected → wontfix
tracking-firefox50: ? → -
Hi Henri,
What do think about the patch, is it OK to uplift to 51 in early beta? If yes, can you help create uplift request?
Flags: needinfo?(hsivonen)
Comment on attachment 8805106 [details]
Bug 1310865 - Don't process cloned <input> until all attributes are copied;

Approval Request Comment
> [Feature/regressing bug #]:

Bug 1264270.

> [User impact if declined]:

Forms that try to round-trip multi-line text in a hidden form field lose the line breaks.

> [Describe test coverage new/current, TreeHerder]:

There is a mochitest for what was being fixed: test_bug1310865.html. As for the fix breaking something else, the fix has baked on Nightly for two weeks.

> [Risks and why]: 

The patch involves a significant change to how HTML input elements are initialized via DOM operations. While the change should be very straight-forward (reusing an existing code path that the parser already used), there could be room for a mistake subtly breaking something else related to input element initialization.

> [String/UUID change made/needed]:

None.
Flags: needinfo?(hsivonen)
Attachment #8805106 - Flags: approval-mozilla-beta?
(In reply to Henri Sivonen (:hsivonen) from comment #22)
> Forms that try to round-trip multi-line text in a hidden form field lose the
> line breaks.

...but only if the hidden field on cloned on the way.
Comment on attachment 8805106 [details]
Bug 1310865 - Don't process cloned <input> until all attributes are copied;

Fix a regression that multi-line text will lose the line breaks in a hidden form field. Beta51+. Should be in 51 beta 2.
Attachment #8805106 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: