Closed Bug 416551 Opened 12 years ago Closed 12 years ago

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

Categories

(Core :: Internationalization, defect, P1, major)

defect

Tracking

()

RESOLVED FIXED
mozilla1.9beta4

People

(Reporter: masayuki, Assigned: masayuki)

References

Details

(Keywords: inputmethod, regression)

Attachments

(1 file, 2 obsolete files)

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.
Flags: blocking1.9?
Attached patch Patch v1.0 (obsolete) — Splinter Review
fix.
Attachment #302301 - Flags: superreview?(roc)
Attachment #302301 - Flags: review?(roc)
Summary: Cannot use IME on rich formatting editor of gmail → Cannot use IME on rich formatting editor of gmail (design mode)
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.
Attachment #302301 - Flags: superreview?(roc)
Attachment #302301 - Flags: review?(roc)
Attachment #302301 - Flags: review-
Attached patch Patch v2.0 (obsolete) — Splinter Review
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.
Attachment #302301 - Attachment is obsolete: true
Attachment #302304 - Flags: superreview?(roc)
Attachment #302304 - Flags: review?(roc)
Why do we need to return ENABLE for non-editable content in editable documents?
(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 :-(

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
Peterv, see comment #5 ... is this true? If it is I'd like to know why :-)
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.
BTW, using nsINode::IsEditable would allow you to ignore that issue, it already does the checking for you.
Attached patch Patch v3.0Splinter Review
Thank you, Peter. This should be better.
Attachment #302304 - Attachment is obsolete: true
Attachment #302571 - Flags: superreview?(roc)
Attachment #302571 - Flags: review?(roc)
Attachment #302304 - Flags: superreview?(roc)
Attachment #302304 - Flags: review?(roc)
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.
Blocks: 417063
+'ing w/ P3.  Please adjust the priority if needed.
Flags: blocking1.9? → blocking1.9+
Priority: -- → P3
to P1, this is very serious.
Priority: P3 → P1
Attachment #302571 - Flags: superreview?(roc)
Attachment #302571 - Flags: superreview+
Attachment #302571 - Flags: review?(roc)
Attachment #302571 - Flags: review+
checked-in. Sorry for this regression.
-> FIXED

# sorry for the spam.
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
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).
(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?
(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.
(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.
You need to log in before you can comment on or make changes to this bug.