Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Assignee
	Description • 17 years ago

IME is enabled on non text editable input element (i.e., non <input type="text"> and <input type="password">).

nsIContent::GetDesiredIMEState() is checking IsEditableInternal(), however, such elements returns TRUE for the element. The input element's editable state is wrong.

	Robert O'Callahan (:roc) (email my personal email if necessary)
	Comment 1 • 17 years ago

Chris, didn't we reject a patch like this just recently?

	Chris Pearce [:cpearce (Not reading bugmail)]
	Comment 2 • 17 years ago

Yes, bug 421827, its patch looks roughly equivalent to this one. Bug 421827 is wanted-1.9. Masayuki, do you know of anything critical that requires this change?

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Assignee
	Comment 3 • 17 years ago

The IME state controling is one of the marketing points of Mozilla Japan. So, I want to fix this before Fx3. And by this bug, when the non editable input elements has focus and IME is opened, any shortcut keys and accesskeys are not used. And I think that the patch is not resky. So, we should/can fix this in Fx3.

	Damon Sicore (:damons)
	Comment 4 • 17 years ago

While it may not be risky, I don't think this is something we'd hold the release for.  Masayuki, we could probably take the patch if it has a test as well (and it passes a review (I'll leave that judgment to chris and roc due to something like this was rejected in bug 421827).

wanted1.9.0.x+ 

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Assignee
	Comment 5 • 17 years ago

roc and peter:

Please hurry up to review this. I want to land before 1.9.0.

The changing of content/html/content/src/nsGenericHTMLElement.cpp is same as the patch of bug 421827.

And also the patch has content/html/content/src/nsHTMLInputElement.cpp part. If the type attr is changed, we need to update the editable flag, but the patch of bug 421827 didn't so.

	Peter Van der Beken [:peterv]
	Comment 6 • 17 years ago

Comment on attachment 313817 [details] [diff] [review]
Patch v1.0

>Index: content/html/content/src/nsHTMLInputElement.cpp
>===================================================================

>     if (aName == nsGkAtoms::type) {
>+      UpdateEditableState();

You need to add NS_EVENT_STATE_MOZ_READONLY and NS_EVENT_STATE_MOZ_READWRITE to the ContentStatesChanged call below and some tests.

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Assignee
	Comment 7 • 17 years ago

Thank you, roc and Peter.

	Mike Beltzner [:beltzner, not reading bugmail]
	Comment 9 • 17 years ago

Comment on attachment 314920 [details] [diff] [review]
Patch v1.1

a1.9=beltzner

	Mike Beltzner [:beltzner, not reading bugmail]
	Comment 10 • 17 years ago

can we get that testcase in litmus?

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Assignee
	Comment 11 • 17 years ago

landed, and the tests are passed on Mac.

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Assignee
	Comment 12 • 17 years ago

the same test (test_bug415498.xul) cannot be passed on qm-centos5-01. backed-out.

	Peter Van der Beken [:peterv]
	Comment 13 • 17 years ago

Comment on attachment 314920 [details] [diff] [review]
Patch v1.1

>Index: content/html/content/src/nsHTMLInputElement.cpp
>===================================================================

>     if (aName == nsGkAtoms::type) {
>       // Changing type means notifying on state changes.  Just start a batch
>       // now.
>       nsIDocument* document = GetCurrentDoc();
>       MOZ_AUTO_DOC_UPDATE(document, UPDATE_CONTENT_STATE, aNotify);
>-      
>+
>+      UpdateEditableState();
>+      if (document) {
>+        MOZ_AUTO_DOC_UPDATE(document, UPDATE_CONTENT_STATE, PR_TRUE);
>+        document->ContentStatesChanged(this, nsnull,
>+                                       NS_EVENT_STATE_MOZ_READONLY |
>+                                       NS_EVENT_STATE_MOZ_READWRITE);
>+      }

This is not what I asked and it's wrong. You need to add NS_EVENT_STATE_MOZ_READONLY and NS_EVENT_STATE_MOZ_READWRITE to the ContentStatesChanged call below (see http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/content/html/content/src/nsHTMLInputElement.cpp&rev=1.477&mark=602-617#600).

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Assignee
	Comment 14 • 17 years ago

(In reply to comment #12)
> the same test (test_bug415498.xul) cannot be passed on qm-centos5-01.
> backed-out.

mmm, the test is passed before my backed-out... it's not my fault.

(In reply to comment #13)
> This is not what I asked and it's wrong. You need to add
> NS_EVENT_STATE_MOZ_READONLY and NS_EVENT_STATE_MOZ_READWRITE to the
> ContentStatesChanged call below (see
> http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/content/html/content/src/nsHTMLInputElement.cpp&rev=1.477&mark=602-617#600).

Oh, thanks. I'll post the new patch.

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Assignee
	Comment 15 • 17 years ago

please re-review this.

	Masayuki Nakano [:masayuki] (he/him)(JST, +0900) Assignee
	Comment 16 • 17 years ago

Thank you, Peter.

re-landed it. (the actual logic doesn't change, a1.9 is carried over.)