It's not needed for nsHTMLInputElement (seems to work without the call). nsHTMLTextAreaElement shouldn't need it too but I looks like there is a bug and the value disappear from nsTextEditorState. I didn't investigate far enough to understand why. I will attach a patch. Ehsan, if you can have a look, it would be great :)

You should see foobar in <input> and <textarea> (default values shown by the frame).

So, not calling GetDefaultValueFromContent seems to work for <input>. However, for <textarea>, it doesn't. However, it's clearly not needed. The textarea value should be the default value if the dirty flag is false. I put a break point on GetValue and the first time the value is correct. However, it comes more than once and the value disappear. I didn't took the time to look why the value is set to the empty string but that seems to be wrong.

This patch is clearly wrong. I don't understand why you expect it to work.

(my comment to Mounir on IRC): volkmar: at least one case where that will (probably) fails is when you call reset() on a textarea, and then modify the text content underneath it manually, I think

IIRC, we discussed about that later and you told me it wasn't working for textarea because the first condition in nsTextEditorState::GetValue is always true. Regarding comment 4, I don't understand why that would not work given that textarea is listening to it's content/descendants.

BTW, I'm not sure it's clear: the reason why I want to remove this call is because nsHTMLInputElement::GetDefaultValueFromContent is seriously suboptimized because the default value should not be sanitize, we have to sanitize it before returning it. Getting directly the real value make sure it's the correct _and sanitized_ value. There are no major reasons to remove the call for textarea but it's not required too AFAICT.

(In reply to comment #5) > IIRC, we discussed about that later and you told me it wasn't working for > textarea because the first condition in nsTextEditorState::GetValue is always > true. I don't recall our exact discussion on this (I wish we had saved the irc log somewhere), but that first condition is only true when there's a frame. Maybe what I meant was that if there is a frame, there is also an editor. But I don't see why it has any significance here. The reason that this patch is clearly wrong is that you're removing one of the three possible cases of values for text control frames, and you're not replacing it by anything else. (The three possible cases being the default value living in the DOM, the value stored in the text editor state object's mValue member, and the value stored under the anonymous div managed by the editor when it's present and the control is framed. > Regarding comment 4, I don't understand why that would not work given that > textarea is listening to it's content/descendants. It's not! (In reply to comment #6) > BTW, I'm not sure it's clear: the reason why I want to remove this call is > because nsHTMLInputElement::GetDefaultValueFromContent is seriously > suboptimized because the default value should not be sanitize, we have to > sanitize it before returning it. Getting directly the real value make sure it's > the correct _and sanitized_ value. Well, in that case, why don't you just change nsHTMLInputElement::GetDefaultValueFromContent to return the same thing as GetValue (if that's what we want)? > There are no major reasons to remove the call for textarea but it's not > required too AFAICT. There _are_ major reasons to not remove this call for textareas. See comment 4 for one such case. I'm sure I can think of other cases where your patch would break textareas.

(In reply to comment #7) > > Regarding comment 4, I don't understand why that would not work given that > > textarea is listening to it's content/descendants. > > It's not! Looks like it is: http://mxr.mozilla.org/mozilla-central/source/content/html/content/src/nsHTMLTextAreaElement.cpp#1063 > > BTW, I'm not sure it's clear: the reason why I want to remove this call is > > because nsHTMLInputElement::GetDefaultValueFromContent is seriously > > suboptimized because the default value should not be sanitize, we have to > > sanitize it before returning it. Getting directly the real value make sure it's > > the correct _and sanitized_ value. > > Well, in that case, why don't you just change > nsHTMLInputElement::GetDefaultValueFromContent to return the same thing as > GetValue (if that's what we want)? It seems wrong to return the value when the default value is requested. And nsHTMLInputElement::GetValue() calls nsTextEditorState::GetValue() which calls nsHTMLInputElement::GetDefaultValueFromContent() which would call nsHTMLInputElement::GetValue(). > > There are no major reasons to remove the call for textarea but it's not > > required too AFAICT. > > There _are_ major reasons to not remove this call for textareas. See comment 4 > for one such case. I'm sure I can think of other cases where your patch would > break textareas. I guess I should know more about value handling between the frame and the content but at least what's done for <input> seems wrong to me.

(In reply to comment #8) > (In reply to comment #7) > > > Regarding comment 4, I don't understand why that would not work given that > > > textarea is listening to it's content/descendants. > > > > It's not! > > Looks like it is: > http://mxr.mozilla.org/mozilla-central/source/content/html/content/src/nsHTMLTextAreaElement.cpp#1063 So, how does your patch treat the test case in comment 4? > > > BTW, I'm not sure it's clear: the reason why I want to remove this call is > > > because nsHTMLInputElement::GetDefaultValueFromContent is seriously > > > suboptimized because the default value should not be sanitize, we have to > > > sanitize it before returning it. Getting directly the real value make sure it's > > > the correct _and sanitized_ value. > > > > Well, in that case, why don't you just change > > nsHTMLInputElement::GetDefaultValueFromContent to return the same thing as > > GetValue (if that's what we want)? > > It seems wrong to return the value when the default value is requested. > And nsHTMLInputElement::GetValue() calls nsTextEditorState::GetValue() which > calls nsHTMLInputElement::GetDefaultValueFromContent() which would call > nsHTMLInputElement::GetValue(). Forgetting about the implementation for a minute, what is the behavior that is incorrect right now, and what are you trying to fix? > > > There are no major reasons to remove the call for textarea but it's not > > > required too AFAICT. > > > > There _are_ major reasons to not remove this call for textareas. See comment 4 > > for one such case. I'm sure I can think of other cases where your patch would > > break textareas. > > I guess I should know more about value handling between the frame and the > content but at least what's done for <input> seems wrong to me. I still don't see what's wrong at all. Can you create a testcase which shows the problem?

(In reply to comment #9) > (In reply to comment #8) > > (In reply to comment #7) > > > > Regarding comment 4, I don't understand why that would not work given that > > > > textarea is listening to it's content/descendants. > > > > > > It's not! > > > > Looks like it is: > > http://mxr.mozilla.org/mozilla-central/source/content/html/content/src/nsHTMLTextAreaElement.cpp#1063 > > So, how does your patch treat the test case in comment 4? Calling Reset() will make the value being equals to the default value. Then, if you change the content, it will also call Reset() which is going to do the same thing. > Forgetting about the implementation for a minute, what is the behavior that is > incorrect right now, and what are you trying to fix? The behavior is correct but the implementation is not correct. The fact that we should sanitize the default value of the input element is the hint.

So, this should fix it. mValue should always be the current value given that SetValue() is always called when the element's value is changed (even when the value is changed because the default value has changed). In that case, when we call GetValue() we should assume that mValue always has the correct value. The issue I had with textareas were very simple actually: there is some code checking that textarea's value should be the empty string when the dirty value flag isn't true. Setting the value to the empty string set mValue to the empty string so GetValue() wasn't returning the correct value. It's not an issue with the correct implementation because we don't use mValue when the dirty value flag isn't true. Ehsan, let me know if that sounds correct.

Unfortunately, we can't remove GetDefaultValueFromContent() given that it will break API froze. However, we can remove the call and remove the method after 2.0 branching.

(In reply to comment #10) > (In reply to comment #9) > > (In reply to comment #8) > > > (In reply to comment #7) > > > > > Regarding comment 4, I don't understand why that would not work given that > > > > > textarea is listening to it's content/descendants. > > > > > > > > It's not! > > > > > > Looks like it is: > > > http://mxr.mozilla.org/mozilla-central/source/content/html/content/src/nsHTMLTextAreaElement.cpp#1063 > > > > So, how does your patch treat the test case in comment 4? > > Calling Reset() will make the value being equals to the default value. Then, if > you change the content, it will also call Reset() which is going to do the same > thing. Well, I mean, given a real HTML test file doing what I described in comment 4, what's the result before and after your patch? > > Forgetting about the implementation for a minute, what is the behavior that is > > incorrect right now, and what are you trying to fix? > > The behavior is correct but the implementation is not correct. The fact that we > should sanitize the default value of the input element is the hint. OK.

Comment on attachment 494593 [details] [diff] [review] Patch v1 Yeah, I think this should be fine.

If that's the use case you mentioned (i'm not sure I got it correcty), it's working the same way with my change: data:text/html,<form><textarea></textarea></form><button onclick="document.forms[0].reset(); document.getElementsByTagName('textarea')[0].innerHTML = 'foo';">click me</button>

Cool!

This change break XUL textbox with multiline="true" because it sets the value as a text node inside the html:textarea but it looks like DoneAddingChildren is never called in that context. Or maybe mutation listener doesn't work in this context. A way or another, it's really annoying and I will leave this bug for post gecko 2.0 then :(

Comment on attachment 494593 [details] [diff] [review] Patch v1 Oups, I didn't meant to remove ehsan's r+...

Comment on attachment 494593 [details] [diff] [review] Patch v1 No problem. Next time that this happens, please re-request the review flag so that the reviewer can restore it. I spotted this in my bugmail almost by chance!

This seems to break accessibility tests: <http://tinderbox.mozilla.org/showlog.cgi?log=Cedar/1300828177.1300829379.19447.gz> so I backed it out: http://hg.mozilla.org/projects/cedar/rev/9c0816e8ad56

(In reply to comment #21) > This seems to break accessibility tests: > > <http://tinderbox.mozilla.org/showlog.cgi?log=Cedar/1300828177.1300829379.19447.gz> > > so I backed it out: http://hg.mozilla.org/projects/cedar/rev/9c0816e8ad56 Hmm, it was mentioned in the whiteboard actually. It's now updated to make it clearer.

(In reply to Mounir Lamouri (:mounir) from comment #23) > https://hg.mozilla.org/integration/mozilla-inbound/rev/c4d628fe6be2 :mounir , we are < 2 weeks away from the merge date, can you please help backout of this on aurora asap per https://bugzilla.mozilla.org/show_bug.cgi?id=841882#c16 ?

Backed out from mozilla-central: https://hg.mozilla.org/integration/mozilla-inbound/rev/b180ce838e43 and mozilla-aurora: https://hg.mozilla.org/releases/mozilla-aurora/rev/7f7dd0633410 Sorry for the delay :(

mxr indicates this method is now gone.