421827 - Non editable <input> should not have an editable flag

Status: RESOLVED FIXED

(Core :: DOM: Editor, defect)

Description

[Chris Pearce [:cpearce (Not reading bugmail)]]

Attachment: 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).

Comment 1

[Boris Zbarsky [:bzbarsky]]

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.

Comment 2

[Chris Pearce [:cpearce (Not reading bugmail)]]

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

Comment 3

[Boris Zbarsky [:bzbarsky]]

I think we should just check the input type in the last if() condition in that function....

Comment 4

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Why should text and password inputs have the editable flag? Their anonymous contents, sure, but the inputs themselves aren't really "editable".

Comment 5

[Boris Zbarsky [:bzbarsky]]

> Why should text and password inputs have the editable flag?

That's what css3-ui seems to imply, basically.

Comment 6

[Chris Pearce [:cpearce (Not reading bugmail)]]

Attachment: Patch v1

Adds check to nsGenericHTMLFormElement::UpdateEditableFormControlState() so that we only enable editable flag for non-readonly password and text inputs.

Comment 7

[Boris Zbarsky [: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..

Comment 8

[Chris Pearce [:cpearce (Not reading bugmail)]]

Attachment: Patch v2

V1 with BZ's suggested change.

Comment 9

[Chris Pearce [:cpearce (Not reading bugmail)]]

Comment on attachment 308472 [details] [diff] [review]
Patch v2

This patch makes the caret not appear in <textarea>. :(

Comment 10

[Boris Zbarsky [:bzbarsky]]

Oh.  Well, textarea should also follow the readonly attribute...

Comment 11

[Chris Pearce [:cpearce (Not reading bugmail)]]

Attachment: Patch v3

As v2 with NS_FORM_TEXTAREA check.

Comment 12

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 308551 [details] [diff] [review]
Patch v3

See bug 427245 comment 3. This patch is important for us. Please land this.

Comment 13

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 308551 [details] [diff] [review]
Patch v3

Oops, this patch doesn't change |nsHTMLInputElement::AfterSetAttr|. This patch is baggy.

Comment 14

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Attachment: test case

click the "Toggle the input type" button, the input elements will be type="text". But you cannot input any text to them.

Comment 15

[Chris Pearce [:cpearce (Not reading bugmail)]]

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!

Comment 16

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

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

Comment 17

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

landed, thanks!

Comment 18

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

backed-out.

Comment 19

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

re-landed.

Comment 20

[Boris Zbarsky [:bzbarsky]]

Please check in a test for this.  It should be trivial to write a reftest, given the HTML already in the bug.