Closed Bug 1083629 Opened 5 years ago Closed 5 years ago

Composition in a designMode editor is canceled when you click the editor on Linux

Categories

(Core :: Editor, defect, major)

x86_64
Linux
defect
Not set
major

Tracking

()

RESOLVED FIXED
mozilla36
Tracking Status
firefox34 --- unaffected
firefox35 + fixed
firefox36 --- fixed

People

(Reporter: masayuki, Assigned: masayuki)

References

Details

(Keywords: inputmethod, regression)

Attachments

(1 file)

Looks like this is asynchronous commit issue. Therefore, I guess that this is a regression of bug 1065835 and you can reproduce this with iBus but I've not confirmed it yet.

1. Load attachment 613596 [details]
2. Type something in an editor with IME (don't commit it)
3. Click in the editor

Actual result:
Composition is canceled.

Expected result:
Composition is committed.
This is a bug of nsHTMLEditorEventListener::MouseDown().
Component: Event Handling → Editor
Good:
https://hg.mozilla.org/integration/mozilla-inbound/rev/11fc11a90d6b
Mozilla/5.0 (X11; Linux i686; rv:35.0) Gecko/20100101 Firefox/35.0 ID:20140925165440
Bad:
https://hg.mozilla.org/integration/mozilla-inbound/rev/73b8079308bd
Mozilla/5.0 (X11; Linux i686; rv:35.0) Gecko/20100101 Firefox/35.0 ID:20140925170540

Pushlog:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=11fc11a90d6b&tochange=73b8079308bd
Thank you, Alice-san. I wonder how do you test it. When I try to test with mozregression on my Ubuntu 14.04 environment, IME doesn't work on Nightly :-(

[Tracking Requested - why for this release]:

This is a regression of 1065835 and this could break some web apps which use element.blur() and element.focus() hack for committing composition forcibly. The fix will be simple.
(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #3)
> Thank you, Alice-san. I wonder how do you test it. When I try to test with
> mozregression on my Ubuntu 14.04 environment, IME doesn't work on Nightly :-(

FYI, I am using tar.bz2 build on Ububtu12.04 32bit iBus+anthy, not mozregression tool.
Thank you for the information. It sounds you're hard worker! I'll file a bug of mozregression.
Attached patch PatchSplinter Review
In nsEditorEventListener::MouseDown(), IMEStateManager::NotifyIME(REQUEST_TO_COMMIT) is called. However, if the editor is nsHTMLEditor, nsHTMLEditorEventListener::MouseDown() handles mousedown events first. At the first of the method, it checks if the mousedown event is fired on an element which is a descendant of editable root. However, in designMode, the editable root is <body>. Therefore, if mousedown event is fired on <html> element, the handler believes that the event is fired outside of it. Then, NotifyIME() isn't called. This is the cause of this bug. (Although, I guess there is another cause around nsGtkIMModule and TextComposition)

In designMode editor, actual editable node is the document node. So, the result of the check is not expected result. Instead of IsDescendantOfEditorRoot(), nsHTMLEditorEventListener should use IsAcceptableInputEvent(). It checks any cases as expected for mouse events.

At least for now, this is enough and I think this is the safest approach.
# We need to uplift this patch to Aurora.
Attachment #8506129 - Flags: review?(ehsan.akhgari)
Attachment #8506129 - Flags: review?(ehsan.akhgari) → review+
https://hg.mozilla.org/mozilla-central/rev/030d8d468498
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
Looks like this needs to be uplifted to Aurora to ensure we don't ship this regression, can you please nominate for uplift?
Flags: needinfo?(masayuki)
Yeah, I'll do that today. Thank you for the ping.
Flags: needinfo?(masayuki)
Comment on attachment 8506129 [details] [diff] [review]
Patch

Approval Request Comment
[Feature/regressing bug #]: bug 1083067
[User impact if declined]: Typically, composing string with IME should be committed if user presses mouse button. However, this causes canceling the composition. I.e., if user expects to commit composition by mouse operation, this bug is a data loss bug. This may occur on rich text editor like web mail composer.
[Describe test coverage new/current, TBPL]: I tested manually. For creating automated tests for this, we need to hack TextComposition with some risk which shouldn't be taken to uplift.
[Risks and why]: The risk must be low. The old code checks if the clicked element is a descendant of editable root element. However, in designMode, comparing with the editable root element (<body> in this case) doesn't make sense because <html> element is also a part of editor. And IsAcceptableInputEvent() is used for filtering mouse button events in other mouse button event handlers of HTML editor. So, we can trust this alternative method.
[String/UUID change made/needed]: Nothing.
Attachment #8506129 - Flags: approval-mozilla-aurora?
Comment on attachment 8506129 [details] [diff] [review]
Patch

Thanks!
Attachment #8506129 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
You need to log in before you can comment on or make changes to this bug.