230307 - user input not preserved on cloned HTML textarea elements

Status: RESOLVED FIXED

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

Description

[Martin Honnen]

Following the discussion in the DOM newsgroup thread
http://groups.google.com/groups?hl=en&lr=&ie=UTF-8&oe=UTF-8&threadm=btfdbi%241dj1%40ripley.netscape.com&rnum=1&prev=/groups%3Fq%3Dgroup:*mozilla*dom*%26hl%3Den%26lr%3D%26ie%3DUTF-8%26oe%3DUTF-8%26scoring%3Dd
I file this bug. 
Mozilla when using the W3C DOM Core cloneNode method on HTML form controls
preserves the value property of text and password input controls but not of
textarea controls.
Similarly the checked property of radio and checkbox input controls is preserved
but not the selectedIndex property nor the value property of select controls,
nor the selected property of any contain option elements.
So the suggestion is to preserve the user input/selection on HTML form controls
consistently, meaning for textarea controls the value property is cloned and for
select elements the selectedIndex property and the value property is cloned as
well as the select property on any contained option element.
I file that on the DOM: HTML component as it relates to cloning HTML form
controls, if it belongs in DOM: Core change as needed.

Comment 1

[Martin Honnen]

Attachment: JavaScript file used in test case

Comment 2

[Martin Honnen]

Attachment: test case (enter text in the text controls, make selections for checkbox/radio/form controls and clone form with button provided, inspect cloned form and look at debug output)

Tested with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.7a)
Gecko/20040106.

Comment 3

[Boris Zbarsky [:bzbarsky]]

Interesting.  Select and textarea break because they just do no formstate
storing on clone.  But file inputs break for a fun reason -- the "frame always
owns value" assumption.  We call SetValueInternal on the file input, and that
sets mValue, since it has no frame.  But then we ask it for the .value _after_
it has a frame, and it always just asks the frame for the value.  And since we
didn't set the value via the frame, the frame has no idea of the new value...

The reason this doesn't bite us all the time is that there is no way to set the
value of a file control via the DOM in general, so values _are_ in fact always
set by the frame... except in this case.

Comment 4

[HARUNAGA Hirotoshi]

*** Bug 271081 has been marked as a duplicate of this bug. ***

Comment 5

[bruno abdon]

Attachment: Simpler testcase

A way simpler testcase (the Javascript code is 2 lines long).

Double clicking the textarea or input-text will clone it and alert() the
clone's 'value'.

The 'value' attribute is cloned for the input but not for the textarea. 

(this testcase was originally posted to bug 300817 (a dup)).

Comment 6

[bruno abdon]

*** Bug 300817 has been marked as a duplicate of this bug. ***

Comment 7

[Admin Korkman]

Confirming this bug is still open in 3.0a2.

Comment 8

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

Assigning to me to not loose track of it since these things tend to annoy me. Not working on it yet though so if anyone else is interested please do take it.

Comment 9

[Mike Samuel]

Below is a JS only testcase that demonstrates the problem.  It fails on both FF2 and FF3.0.4.
I ran this in square free shell.  The comment lines are the responses.  The expected output is B,B


var sel = document.createElement('SELECT')
var opt1 = document.createElement('OPTION')
var opt2 = document.createElement('OPTION')
opt1.value = 'A'
// A
opt2.value = 'B'
// B
opt1.selected = opt1.defaultSelected = true
// true
sel.appendChild(opt1)
// [object HTMLOptionElement]
sel.appendChild(opt2)
// [object HTMLOptionElement]
opt2.selected = true
// true
var clone = sel.cloneNode(true)
[clone.value, sel.value]
A,B

Comment 10

[baszero]

Why does it take to fix this so long? Reported in 2004 (yes, not 2014).... So please Firefox, fix this!!!!

Comment 11

[Michał Gołębiowski-Owczarek [:mgol]]

This issue gets reported to jQuery (see https://github.com/jquery/jquery/issues/2776, https://github.com/jquery/jquery/issues/3115) so I'm adding it to the meta-bug of issues that affect jQuery.

Comment 12

[baszero]

Great, and the guys of jQuery say that this has to be fixed in Firefox... Come on! Really? I hope you both sort this out soon, it's overdue since years!

(In reply to Michał Gołębiowski [:m_gol] from comment #11)
> This issue gets reported to jQuery (see
> https://github.com/jquery/jquery/issues/2776,
> https://github.com/jquery/jquery/issues/3115) so I'm adding it to the
> meta-bug of issues that affect jQuery.

Comment 13

[baszero]

See https://github.com/jquery/jquery/issues/3115

(In reply to baszero from comment #12)
> Great, and the guys of jQuery say that this has to be fixed in Firefox...
> Come on! Really? I hope you both sort this out soon, it's overdue since
> years!
> 
> (In reply to Michał Gołębiowski [:m_gol] from comment #11)
> > This issue gets reported to jQuery (see
> > https://github.com/jquery/jquery/issues/2776,
> > https://github.com/jquery/jquery/issues/3115) so I'm adding it to the
> > meta-bug of issues that affect jQuery.

Comment 14

[Boris Zbarsky [:bzbarsky]]

Per HTML spec, cloning an <input> does this:

  The cloning steps for input elements must propagate the value, dirty value flag, checkedness,
  and dirty checkedness flag from the node being cloned to the copy.

On the other hand, there are no cloning steps defined for textarea or select, so nothing magic happens there.  All that's cloned is the DOM rooted at the element.  This is exactly what we implement.  I did check and for textarea, Blink, Edge, and WebKit all copy the value, and presumably the "dirty value flag".  I filed https://github.com/whatwg/html/issues/1233 to get the spec changed for textarea, I guess, since no one except us is following it.

Going to focus this bug on textarea; I spun off <select> into bug 1272029.

Comment 15

[Boris Zbarsky [:bzbarsky]]

But note that I think the <select> issue is likely to end up wontfix: We agree with all the other browsers and the spec on the <select> behavior.

Comment 16

[Boris Zbarsky [:bzbarsky]]

> I did check and for textarea, Blink, Edge, and WebKit all copy the value

Er, I was wrong.  WebKit (Safari) does not; I thought I was testing Safari but I must have gotten the wrong browser window.  Given that, I'm not entirely sure we should change the behavior here, since the web compat case is a lot weaker if it's not just us...

Comment 17

[Boris Zbarsky [:bzbarsky]]

Attachment: This changes the behavior, if we decide to do it

Still needs tests, obviously.  I'm not going to spend any more time on this until the spec issue is sorted out one way or the other.

Comment 18

[Domenic Denicola]

The spec has been settled to add cloning steps, as both Gecko and WebKit were willing to do so and Chrome already does so. (Edge also has some weird "deep cloning steps" thing.)

- New spec: https://html.spec.whatwg.org/#the-textarea-element:concept-node-clone-ext
- Discussion: https://github.com/whatwg/html/issues/1233
- Tests: https://github.com/w3c/web-platform-tests/pull/3734
- Spec commit: https://github.com/whatwg/html/commit/957fd0c91c6ee3abd77538be3a2b4c5531754f62

Comment 19

[Boris Zbarsky [:bzbarsky]]

Attachment: When cloning a <textarea>, preserve the current value just like we do for <input>

The web platform test is just lifted wholesale from
<https://github.com/w3c/web-platform-tests/pull/3734>.

Comment 20

[(no longer active)]

Comment on attachment 8791857 [details] [diff] [review]
When cloning a <textarea>, preserve the current value just like we do for <input>

Review of attachment 8791857 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/html/HTMLTextAreaElement.cpp
@@ +105,5 @@
> +{
> +  *aResult = nullptr;
> +  already_AddRefed<mozilla::dom::NodeInfo> ni =
> +    RefPtr<mozilla::dom::NodeInfo>(aNodeInfo).forget();
> +  RefPtr<HTMLTextAreaElement> it = new HTMLTextAreaElement(ni);

This is kind of weird.  How about rewriting this as:

  RefPtr<HTMLTextAreaElement> it = new HTMLTextAreaElement(do_AddRef(aNodeInfo));

Bonus points if you also want to fix HTMLInputElement::Clone() accordingly, but I won't hold it against you if you don't.  :-)

Comment 21

[Boris Zbarsky [:bzbarsky]]

> How about rewriting this as:

Node constructors take a non-const ref to an already_AddRefed.  So I can't pass in a temporary there.

They should really take an rvalue ref, but I'd rather do that in a separate bug if we do it.


Done, and same for HTMLInputElement::Clone (where I cribbed it from) and NS_IMPL_ELEMENT_CLONE/CLONE_WITH_INIT (where I assume HTMLInputElement cribbed it from).  I also changed those last two to not bother null-checking after new.

Comment 22

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/9169054bf504
When cloning a <textarea>, preserve the current value just like we do for <input>.  r=ehsan

Comment 23

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/9169054bf504