Last Comment Bug 665612 - More cleanup around mInputData and IsSingleLineTextControl()
: More cleanup around mInputData and IsSingleLineTextControl()
Status: VERIFIED FIXED
:
Product: Core
Classification: Components
Component: DOM: Core & HTML (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla7
Assigned To: Mounir Lamouri (:mounir)
:
: Andrew Overholt [:overholt]
Mentors:
Depends on:
Blocks: 665655
  Show dependency treegraph
 
Reported: 2011-06-20 10:26 PDT by Mounir Lamouri (:mounir)
Modified: 2011-09-07 23:47 PDT (History)
5 users (show)
mounir: in‑testsuite-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch v1 (8.12 KB, patch)
2011-06-20 10:26 PDT, Mounir Lamouri (:mounir)
no flags Details | Diff | Splinter Review
Patch v2 (6.75 KB, patch)
2011-06-20 12:04 PDT, Mounir Lamouri (:mounir)
bzbarsky: review+
ehsan: feedback+
Details | Diff | Splinter Review
test case (651 bytes, text/html)
2011-09-06 08:00 PDT, Ioana (away)
no flags Details

Description Mounir Lamouri (:mounir) 2011-06-20 10:26:39 PDT
Created attachment 540523 [details] [diff] [review]
Patch v1

With this patch, we will be able to have input types with value mode being 'value' (ie. having a default value that can be different from .value) without using nsTextControlFrame.
It's much more a cleanup because allowing this is four lines of code but I had to clean a lot of stuff around. I can split the patch in two to make that clearer if needed.

There is no need to explicitly test this because as soon as <input type=number> will use that path, it will be implicitly testing it, testing would be as simple as:
input.setAttribute('value', 'bar');
is(input.value, 'bar');
input.value = 'foo';
is(input.value, 'foo');

Though, I'm not 100% sure there is a test testing the first case for all input types. I will probably open a follow-up for that...
Comment 1 :Ehsan Akhgari 2011-06-20 11:48:21 PDT
(In reply to comment #0)
> Created attachment 540523 [details] [diff] [review] [review]
> Patch v1
> 
> With this patch, we will be able to have input types with value mode being
> 'value' (ie. having a default value that can be different from .value)
> without using nsTextControlFrame.
> It's much more a cleanup because allowing this is four lines of code but I
> had to clean a lot of stuff around. I can split the patch in two to make
> that clearer if needed.

Can you do that, please?

> There is no need to explicitly test this because as soon as <input
> type=number> will use that path, it will be implicitly testing it, testing
> would be as simple as:
> input.setAttribute('value', 'bar');
> is(input.value, 'bar');
> input.value = 'foo';
> is(input.value, 'foo');
> 
> Though, I'm not 100% sure there is a test testing the first case for all
> input types. I will probably open a follow-up for that...

That is a good idea.
Comment 2 Mounir Lamouri (:mounir) 2011-06-20 12:04:14 PDT
Created attachment 540553 [details] [diff] [review]
Patch v2

This patch only includes the cleanups.
Comment 3 Mounir Lamouri (:mounir) 2011-06-20 12:10:52 PDT
(In reply to comment #1)
> (In reply to comment #0)
> > Created attachment 540523 [details] [diff] [review] [review] [review]
> > Patch v1
> > 
> > With this patch, we will be able to have input types with value mode being
> > 'value' (ie. having a default value that can be different from .value)
> > without using nsTextControlFrame.
> > It's much more a cleanup because allowing this is four lines of code but I
> > had to clean a lot of stuff around. I can split the patch in two to make
> > that clearer if needed.
> 
> Can you do that, please?

bug 665655

> > There is no need to explicitly test this because as soon as <input
> > type=number> will use that path, it will be implicitly testing it, testing
> > would be as simple as:
> > input.setAttribute('value', 'bar');
> > is(input.value, 'bar');
> > input.value = 'foo';
> > is(input.value, 'foo');
> > 
> > Though, I'm not 100% sure there is a test testing the first case for all
> > input types. I will probably open a follow-up for that...
> 
> That is a good idea.

Will check that nothing is already testing that and open a bug if needed.
Comment 4 :Ehsan Akhgari 2011-06-20 12:55:56 PDT
Comment on attachment 540553 [details] [diff] [review]
Patch v2

I'm not really that familiar with the value mode stuff, but one thing caught my eyes which I couldn't explain.  Why is the code removal in SetValueChanged safe?
Comment 5 Mounir Lamouri (:mounir) 2011-06-20 13:27:29 PDT
(In reply to comment #4)
> Comment on attachment 540553 [details] [diff] [review] [review]
> Patch v2
> 
> I'm not really that familiar with the value mode stuff, but one thing caught
> my eyes which I couldn't explain.  Why is the code removal in
> SetValueChanged safe?

The thing is, currently, FreeData() only has a real effect on text control input element, so, this code is currently useless. With bug 665655, FreeData() will have a real impact on all input elements with value mode being 'value' so we should be more careful.
Though, the only situation were this code could be called is when we call SetValueChanged(PR_FALSE) and it happens only when calling Reset(). IMO, this code *was* here to clear mValue when ::Reset() was called but we are already doing the value resetting dance in ::Reset() and that's were it should be done.
Comment 6 Boris Zbarsky [:bz] (still a bit busy) 2011-06-21 14:24:57 PDT
Comment on attachment 540553 [details] [diff] [review]
Patch v2

r=me, but does this mean that on reset we end up sticking the defaultvalue into the value for text inputs right now (and making a copy of it)?
Comment 7 Mounir Lamouri (:mounir) 2011-06-22 03:28:44 PDT
(In reply to comment #6)
> Comment on attachment 540553 [details] [diff] [review] [review]
> Patch v2
> 
> r=me, but does this mean that on reset we end up sticking the defaultvalue
> into the value for text inputs right now (and making a copy of it)?

Exactly. I will open a follow-up to have GetValueInternal checking for mValue being not null and use GetAttr() in that case. Then, we will be able to not copy the value. When we use mState, nsTextEditorState is owning the value so we can't do otherwise with the current design.
Comment 8 Mounir Lamouri (:mounir) 2011-06-22 03:32:47 PDT
Oh oups, what I said apply for bug 665655. This bug doesn't change how we handle the value on reset and, indeed, we always use mState and never really look inside the value attribute (but just make sure that they are the same). It goes without saying that it applies only for text fields.
Comment 9 Matt Brubeck (:mbrubeck) 2011-06-22 10:24:45 PDT
http://hg.mozilla.org/mozilla-central/rev/ff8bd5d40a57
Comment 10 Ioana (away) 2011-09-06 08:00:03 PDT
Created attachment 558476 [details]
test case
Comment 11 Ioana (away) 2011-09-06 08:09:20 PDT
Verified on:
Mozilla/5.0 (Windows NT 6.1; rv:7.0) Gecko/20100101 Firefox/7.0
Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:7.0) Gecko/20100101 Firefox/7.0
Mozilla/5.0 (X11; Linux i686; rv:7.0) Gecko/20100101 Firefox/7.0
Mozilla/5.0 (Windows NT 5.1; rv:7.0) Gecko/20100101 Firefox/7.0

Steps:
1. Open the test case from the above comment in Fx7.
2. Click on the text box.
3. Click on the text box again.

The value is set fine for the first time with both
document.getElementById("txtbx").value = ... and document.getElementById("txtbx").setAttribute(...).

Yet I have noticed something peculiar: if you set a value with .setAttribute(...) you can change it with .value = ... but it doesn't work the other way around. This is very easily visible in when reproducing the above steps and repeating step 3.
Please let me know if you want me to reopen this bug because this or to log another one.
Comment 12 :Ehsan Akhgari 2011-09-06 08:27:07 PDT
I don't see what's wrong in this test case, but please file another bug if you think there's something broken.  Thanks!
Comment 13 Ioana (away) 2011-09-07 23:47:50 PDT
Apparently if you set a value with .value = ...  you can't change it with .setAttribute(...) in any browser so there is no need to file a new bug. Closing this bug per the above comments...

Note You need to log in before you can comment on or make changes to this bug.