If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

disabled property of input elements is not stored in history

RESOLVED FIXED

Status

()

Core
DOM: Core & HTML
RESOLVED FIXED
14 years ago
9 years ago

People

(Reporter: Harshal Pradhan, Assigned: Harshal Pradhan)

Tracking

Trunk
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 5 obsolete attachments)

(Assignee)

Description

14 years ago
If some page changes disabled through js, you move on to some other page, and
then hit back, then we don't and restore the input element to the old state. My
copy of IE6 does what I would expect in this case. I think we should fix this up.
(Assignee)

Comment 1

14 years ago
Created attachment 130904 [details]
Silly little testcase

Pick Yes to disable the input element. Hit Sumbit. Then hit the back button.
The input element will not be disabled.
(Assignee)

Comment 2

14 years ago
Created attachment 130905 [details] [diff] [review]
Store disabled in history like other properties

This seems to do what I want.
(Assignee)

Comment 3

14 years ago
Created attachment 130907 [details] [diff] [review]
This appears do what I want
(Assignee)

Comment 4

14 years ago
Created attachment 130908 [details] [diff] [review]
This seems to do what I want
(Assignee)

Updated

14 years ago
Attachment #130908 - Flags: superreview?(jst)
Attachment #130908 - Flags: review?(john)
(Assignee)

Comment 5

14 years ago
Sorry for the multiple attachments. I dont know what exactly happened there.
Summary: disabled property of input elements is not stored in history → disabled property of input elements is not stored in history
(Assignee)

Comment 6

14 years ago
Comment on attachment 130904 [details]
Silly little testcase

whoops.
Attachment #130904 - Attachment is patch: false
Attachment #130904 - Attachment mime type: text/plain → text/html
Comment on attachment 130908 [details] [diff] [review]
This seems to do what I want

- In nsHTMLInputElement::SetDisabled(PRBool aDisabled):

+  if (aDisabled) {
+    nsHTMLValue empty(eHTMLUnit_Empty);
+    return SetHTMLAttribute(nsHTMLAtoms::disabled, empty, PR_TRUE);
+  }
+  else {
+    UnsetAttr(kNameSpaceID_None, nsHTMLAtoms::disabled, PR_TRUE);
+    return NS_OK;
+  }
+}

Unnecessary else-after-return, remove it.

- In nsHTMLInputElement::SaveState():

+  if (GET_BOOLBIT(mBitField, BF_DISABLED_CHANGED)) {
+    rv = GetPrimaryPresState(this, getter_AddRefs(state));
...
   return rv;
 }

You're stomping over rv there (and in a few more places inside that if block),
use another local (or use |= when assigning into rv).

sr=jst with the above fixed.
Attachment #130908 - Flags: superreview?(jst) → superreview+
(Assignee)

Comment 8

14 years ago
Created attachment 131213 [details] [diff] [review]
fixed jst's issues
Attachment #130905 - Attachment is obsolete: true
(Assignee)

Updated

14 years ago
Attachment #130907 - Attachment is obsolete: true
Attachment #130908 - Attachment is obsolete: true
(Assignee)

Comment 9

14 years ago
Comment on attachment 130908 [details] [diff] [review]
This seems to do what I want

cancelling obsolete request
Attachment #130908 - Flags: review?(john)
(Assignee)

Updated

14 years ago
Attachment #131213 - Flags: superreview+
Attachment #131213 - Flags: review?(john)
Created attachment 131683 [details]
Sillier little testcase

I may just be totally off my rocker, but it seems to me that the code here
unconditionally restores the disabled state, regardless of whether it was
saved.	The existing code does that for value and checked, but since there was
only one state property a state object would not have existed if we didn't want
to restore the value.

So here's a scenario: what if disabled *starts* as true but does not change,
and then you go back?  It seems like this will end up setting disabled to false
since there was no value saved for disabled.

I suppose a history state object would have to exist, so I have made this
testcase modify the value of the input.

NOTE: I am dubious about whether to really save disabled, but I'll allow it
since we seem to want to move to a "save the DOM" model instead of this
space-saving but much more complex and error-prone "save only the things we
think are important" model.
(Assignee)

Comment 11

14 years ago
Created attachment 131752 [details] [diff] [review]
v2

This should fix the problems that jkeiser pointed out. I had overlooked the
fact that this code relied upon only one property being set per input element,
and having state only when it is set.
(Assignee)

Updated

14 years ago
Attachment #131213 - Attachment is obsolete: true
(Assignee)

Updated

14 years ago
Attachment #131213 - Flags: review?(john)
(Assignee)

Updated

14 years ago
Attachment #131752 - Flags: review?(john)
Comment on attachment 131752 [details] [diff] [review]
v2

Final remaining problem: if you have <input name="blah" value="something"> and
the user deleted all the text in the 

There are two possible ways I can see to deal with this problem: (a) create
nsIPresState::HasStateProperty(), or (b) give GetStateProperty() multiple
return values like getAttr(): in other words, have an equivalent of
NS_CONTENT_ATTR_HAS_VALUE and NS_CONTENT_ATTR_NOT_THERE (perhaps even use those
exact values).

I prefer the second one personally, but I won't knock it if you do the first.

The current definition of NS_CONTENT_ATTR_*:
http://lxr.mozilla.org/mozilla/source/content/base/public/nsContentErrors.h#44

An example of GetAttr returning said values:
http://lxr.mozilla.org/mozilla/source/content/base/src/nsGenericElement.cpp#355
4

An example usage:
http://lxr.mozilla.org/mozilla/source/content/html/content/src/nsHTMLInputEleme
nt.cpp#1046
Attachment #131752 - Flags: review?(john) → review-
(Assignee)

Comment 13

14 years ago
Created attachment 131801 [details] [diff] [review]
v3

Yep, that case wouldn't have worked. 

I've done b) as you suggested. I introduced new errors in nsLayoutErrors.h.
(Feel free to suggest better names.) Re-using the content attribute error
values seemed like the wrong thing to do. Also, in this case, it looks like we
don't really need the equivalent of NS_CONTENT_ATTR_NO_VALUE.

I had a brief look at the other callers of GetStateProperty() and it didn't
appear like any of them would be affected by this change. (but I need to make
sure.)
(Assignee)

Updated

14 years ago
Attachment #131752 - Attachment is obsolete: true
(Assignee)

Updated

14 years ago
Attachment #131801 - Flags: review?(john)
Comment on attachment 131801 [details] [diff] [review]
v3

Looks good.  Thanks!
Attachment #131801 - Flags: review?(john) → review+
(Assignee)

Updated

14 years ago
Attachment #131801 - Flags: superreview?(jst)
Comment on attachment 131801 [details] [diff] [review]
v3

sr=jst
Attachment #131801 - Flags: superreview?(jst) → superreview+
This has r/sr...isn't it ready to land now that the tree is open for checkins?
(Assignee)

Comment 17

14 years ago
Yeah, I need to land this. Haven't had a build environment or time to watch the
tree for the last few weeks. I'll land it this weekend. 
(Assignee)

Comment 18

14 years ago
Checked in.
Status: NEW → RESOLVED
Last Resolved: 14 years ago
Resolution: --- → FIXED
I'm really surprised at this.
From comment 0:
"If some page changes disabled through js, you move on to some other page, and
then hit back, then we don't and restore the input element to the old state."
This is exactly what my copy of IE6 is doing, but apparently not what reporter's
IE6 version is doing?
Has IE6's behavior changed since this was fixed? Or do I have a strange IE6 version?
This fix might have caused bug 300364. 
Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; rv:1.8b3) Gecko/20050710
Firefox/1.0+ ID:2005071017
Win2k + sp4 + rollup package 1
IE 6.0.2800.1106

In IE: Going to the 'Silly little testcase' and changing 'Disabled?' to YES
doesn't grey out the textbox like firefox. However, the textbox is disabled
(can't get a caret in it). On hitting 'submit', then 'back' the 'Disabled?' says
YES but the textbox accepts a caret now.

In firefox, on hitting back the 'Disabled' is displaying YES, and the textbox IS
greyed out (disabled).

So from what I can see, firefox is doing the right thing, and IE isn't.
Thanks, Steve. So IE6 isn't doing what is stated in comment 0 of this bug, and
the fix from this bug makes Mozilla act different regarding to IE6.

I prefer IE6's behavior, it makes more sense to me.
The fix here is in fact incomplete, since it only touches input's and not
textareas and selects.
Note that with bfcache enabled, this is a non-issue, since you always get the
saved dom state on pressing back.

Updated

9 years ago
Component: DOM: HTML → DOM: Core & HTML
QA Contact: ian → general
You need to log in before you can comment on or make changes to this bug.