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...

(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.

This patch only includes the cleanups.

(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 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?

(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 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)?

(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.

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.

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.

I don't see what's wrong in this test case, but please file another bug if you think there's something broken. Thanks!

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...