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

Non editable <input> should not have an editable flag

RESOLVED FIXED

Status

()

Core
Editor
RESOLVED FIXED
10 years ago
10 years ago

People

(Reporter: cpearce, Assigned: masayuki)

Tracking

unspecified
x86
Windows Vista
Points:
---
Dependency tree / graph
Bug Flags:
blocking1.9 -
wanted1.9 +
in-testsuite ?

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 2 obsolete attachments)

Created attachment 308335 [details]
Testcase - show which <input> are editable (-moz-read-write)

This was picked up in bug 421169. <input> elements have their editable flag set, even for non-editable types such as button, checkbox, file, hidden, image, radio, reset, submit.

The attached testcase is taken from bug 421169, and lists all the <input> types, with a red box after them if they're editable (if they match a -moz-read-write selector).
Flags: blocking1.9?
I'd guess the issue is that nsGenericHTMLFormElement::UpdateEditableFormControlState sets any non-readonly form control as editable...  That seems wrong: that attribute should only affect text and password inputs.
(In reply to comment #1)
> I'd guess the issue is that
> nsGenericHTMLFormElement::UpdateEditableFormControlState sets any non-readonly
> form control as editable...  That seems wrong: that attribute should only
> affect text and password inputs.
> 

The readonly attribute should only affect text and password inputs, but the others shouldn't be non-readonly anyway right? Where can we turn change the default for input to be non-readonly (except in the text & password case)?
I think we should just check the input type in the last if() condition in that function....
Why should text and password inputs have the editable flag? Their anonymous contents, sure, but the inputs themselves aren't really "editable".
> Why should text and password inputs have the editable flag?

That's what css3-ui seems to imply, basically.
Created attachment 308372 [details] [diff] [review]
Patch v1

Adds check to nsGenericHTMLFormElement::UpdateEditableFormControlState() so that we only enable editable flag for non-readonly password and text inputs.
Assignee: nobody → chris
Status: NEW → ASSIGNED
Attachment #308372 - Flags: review?(bzbarsky)
Comment on attachment 308372 [details] [diff] [review]
Patch v1

>Index: content/html/content/src/nsGenericHTMLElement.cpp
>+    nsIFormControl *fctl = static_cast<nsIFormControl*>(this);
>+    const PRInt32 type = fctl->GetType();

You can just call GetType() directly (as a member method of |this|) without the casting, no?

The rest looks ok, but given that I suggested the change it would make more sense to have peterv review it..
Created attachment 308472 [details] [diff] [review]
Patch v2

V1 with BZ's suggested change.
Attachment #308372 - Attachment is obsolete: true
Attachment #308472 - Flags: review?(peterv)
Attachment #308372 - Flags: review?(bzbarsky)
Comment on attachment 308472 [details] [diff] [review]
Patch v2

This patch makes the caret not appear in <textarea>. :(
Attachment #308472 - Attachment is obsolete: true
Attachment #308472 - Flags: review?(peterv)
Oh.  Well, textarea should also follow the readonly attribute...
Created attachment 308551 [details] [diff] [review]
Patch v3

As v2 with NS_FORM_TEXTAREA check.
Attachment #308551 - Flags: review?(peterv)
Flags: wanted1.9+
Flags: blocking1.9?
Flags: blocking1.9-
Attachment #308551 - Flags: superreview+
Attachment #308551 - Flags: review?(peterv)
Attachment #308551 - Flags: review+
Comment on attachment 308551 [details] [diff] [review]
Patch v3

See bug 427245 comment 3. This patch is important for us. Please land this.
Attachment #308551 - Flags: approval1.9?
Comment on attachment 308551 [details] [diff] [review]
Patch v3

Oops, this patch doesn't change |nsHTMLInputElement::AfterSetAttr|. This patch is baggy.
Attachment #308551 - Flags: approval1.9?
Created attachment 314320 [details]
test case

click the "Toggle the input type" button, the input elements will be type="text". But you cannot input any text to them.
Masayuki, could you please take this bug and finish it? You have a much better understanding of how this affects IME input, I don't even have IME or know how to use it!
(In reply to comment #15)
> Masayuki, could you please take this bug and finish it? You have a much better
> understanding of how this affects IME input, I don't even have IME or know how
> to use it!

O.K. I'll continue to work on bug 427245. That already has the patch.

Assignee: chris → masayuki
Status: ASSIGNED → NEW
(Assignee)

Updated

10 years ago
Status: NEW → ASSIGNED
Depends on: 427245
landed, thanks!
Status: ASSIGNED → RESOLVED
Last Resolved: 10 years ago
Resolution: --- → FIXED
backed-out.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
re-landed.
Status: REOPENED → RESOLVED
Last Resolved: 10 years ago10 years ago
Resolution: --- → FIXED
Flags: in-testsuite?
Please check in a test for this.  It should be trivial to write a reftest, given the HTML already in the bug.
You need to log in before you can comment on or make changes to this bug.