416551 - Cannot use IME on rich formatting editor of gmail (design mode)

Status: RESOLVED FIXED

(Core :: Internationalization, defect, P1)

Description

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

This is a regression of bug 415026.

In such editor, the focused content is always null, so, the new code always disables IME on the document. I think that I need to check whether the doc is editable.

Comment 1

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

Attachment: Patch v1.0

fix.

Comment 2

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

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

Oops, this patch is buggy.

If the clicked not is focusable (e.g., <a>), the result depends on the content.

Comment 3

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

Attachment: Patch v2.0

If the aContent is null (there are no focused contents), nsIMEStateManager checks whether the document is editable. Otherwise it calls nsIContent::GetDesiredIMEState. If the content is not editable, but the document is editable, it returns ENABLE. However, if the content is text editor (input or textarea), the method is overridden by nsGenericHTMLElement, so, the editors in editable document can keep to support ime-mode.

Comment 4

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

Why do we need to return ENABLE for non-editable content in editable documents?

Comment 5

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

(In reply to comment #4)
> Why do we need to return ENABLE for non-editable content in editable documents?

I'm not sure. But in design mode, the editable content doesn't have editable flag :-(

Comment 6

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

It seems that the document is only editable in the design mode.
See nsHTMLDocument::SetDesignMode
http://mxr.mozilla.org/mozilla/source/content/html/document/src/nsHTMLDocument.cpp#4094

Comment 7

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

Peterv, see comment #5 ... is this true? If it is I'd like to know why :-)

Comment 8

[Peter Van der Beken [:peterv]]

Yes, it is. I've been pondering changing it, the downside is that you have to walk the whole tree to reset the flags when designMode changes. But not sure we care about that performance hit. I'll try to make a patch, I don't think a lot of code relies on the way things are now.

Comment 9

[Peter Van der Beken [:peterv]]

BTW, using nsINode::IsEditable would allow you to ignore that issue, it already does the checking for you.

Comment 10

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

Attachment: Patch v3.0

Thank you, Peter. This should be better.

Comment 11

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

roc:

would you check the new patch? If Peter needs much time for his checking for the performance of the ideal way, we should land the current patch, temporary.

Comment 12

[Damon Sicore (:damons)]

+'ing w/ P3.  Please adjust the priority if needed.

Comment 13

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

to P1, this is very serious.

Comment 14

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

checked-in. Sorry for this regression.

Comment 15

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

-> FIXED

# sorry for the spam.

Comment 16

[Peter Van der Beken [:peterv]]

Comment on attachment 302571 [details] [diff] [review]
Patch v3.0

>Index: content/base/public/nsIContent.h
>===================================================================

>+    if (!IsEditableInternal())

Please change this to IsEditable (which is what I told you to use).

Comment 17

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

(In reply to comment #16)
> (From update of attachment 302571 [details] [diff] [review])
> >Index: content/base/public/nsIContent.h
> >===================================================================
> 
> >+    if (!IsEditableInternal())
> 
> Please change this to IsEditable (which is what I told you to use).

Why? The method is protected, not private.
# And did you check me email?

Comment 18

[Peter Van der Beken [:peterv]]

(In reply to comment #17)
> Why? The method is protected, not private.

Internal/External is just an implementation detail to make internal callers of IsEditable get a fast non-virtual function and have it still be callable by external callers. The API is IsEditable and your code won't be any slower by using it.

> # And did you check me email?

Yes.

Comment 19

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

(In reply to comment #18)
> (In reply to comment #17)
> > Why? The method is protected, not private.
> 
> Internal/External is just an implementation detail to make internal callers of
> IsEditable get a fast non-virtual function and have it still be callable by
> external callers. The API is IsEditable and your code won't be any slower by
> using it.

How about they are moved to private method?

> > # And did you check me email?
> 
> Yes.

Thanks, and please do it.