Remove GetDefaultValueFromContent method

RESOLVED WORKSFORME

Status

()

defect
RESOLVED WORKSFORME
9 years ago
6 years ago

People

(Reporter: mounir, Assigned: mounir)

Tracking

Trunk
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [leave open], )

Attachments

(1 attachment, 1 obsolete attachment)

Assignee

Description

9 years ago
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 :)
Assignee

Comment 1

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

Comment 2

9 years ago
Posted patch Patch (obsolete) — Splinter Review
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.

Comment 3

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

Comment 4

9 years ago
(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
Assignee

Comment 5

9 years ago
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.
Assignee

Comment 6

9 years ago
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.

Comment 7

9 years ago
(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.

Updated

9 years ago
Whiteboard: WONTFIX?
Assignee

Comment 8

9 years ago
(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.

Comment 9

9 years ago
(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?
Assignee

Comment 10

9 years ago
(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.
Whiteboard: WONTFIX?
Assignee

Comment 11

9 years ago
Posted patch Patch v1Splinter Review
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.
Assignee: nobody → mounir.lamouri
Attachment #476372 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #494593 - Flags: feedback?(ehsan)
Assignee

Comment 12

9 years ago
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.
Assignee

Updated

9 years ago
Whiteboard: [needs review]

Comment 13

9 years ago
(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 14

9 years ago
Comment on attachment 494593 [details] [diff] [review]
Patch v1

Yeah, I think this should be fine.
Attachment #494593 - Flags: feedback?(ehsan) → feedback+
Assignee

Updated

9 years ago
Attachment #494593 - Flags: review?(jonas)

Updated

9 years ago
Attachment #494593 - Flags: review?(jonas)
Attachment #494593 - Flags: review+
Attachment #494593 - Flags: feedback+
Assignee

Updated

9 years ago
Attachment #494593 - Flags: approval2.0?
Assignee

Comment 15

9 years ago
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>
Whiteboard: [needs review] → [needs approval]

Comment 16

9 years ago
Cool!
Assignee

Comment 17

9 years ago
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 :(
Whiteboard: [needs approval]
Assignee

Updated

9 years ago
Attachment #494593 - Flags: review+
Assignee

Comment 18

9 years ago
Comment on attachment 494593 [details] [diff] [review]
Patch v1

Oups, I didn't meant to remove ehsan's r+...
Attachment #494593 - Flags: approval2.0?

Comment 19

9 years ago
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!
Attachment #494593 - Flags: review+

Comment 21

8 years ago
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
Whiteboard: fixed-in-cedar → not-ready
Assignee

Comment 22

8 years ago
(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.
Whiteboard: not-ready → [do not land][see comment 17]
Assignee

Updated

8 years ago
Whiteboard: [do not land][see comment 17] → [not-ready][do not land][see comment 17]
Assignee

Updated

6 years ago
Depends on: 841001
Assignee

Comment 23

6 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/c4d628fe6be2
Flags: in-testsuite-
Whiteboard: [not-ready][do not land][see comment 17]
Target Milestone: --- → mozilla21
https://hg.mozilla.org/mozilla-central/rev/c4d628fe6be2
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Depends on: 841882
(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 ?
Flags: needinfo?(mounir)

Updated

6 years ago
Depends on: 853803
Assignee

Comment 26

6 years ago
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 :(
Status: RESOLVED → REOPENED
Flags: needinfo?(mounir)
Resolution: FIXED → ---
Whiteboard: [leave open]
Target Milestone: mozilla21 → ---
mxr indicates this method is now gone.
Status: REOPENED → RESOLVED
Closed: 6 years ago6 years ago
Resolution: --- → WORKSFORME
You need to log in before you can comment on or make changes to this bug.