The default bug view has changed. See this FAQ.

Remove the hack for bug 23558

RESOLVED FIXED in mozilla13

Status

()

Core
Editor
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: masayuki, Assigned: masayuki)

Tracking

({inputmethod})

Trunk
mozilla13
inputmethod
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [inbound])

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

6 years ago
Bug 23558 fixed the bug by hacky code. That makes a problem for implementing DOM3 composition event.

If first composition string in a transaction is empty string, editor does nothing. See nsPlaintextEditor::UpdateIMEComposition(). Against this, nsTextStore dispatches dummy text event. This will cause wrong composition event behavior. Therefore, I need to remove the hack first.
(Assignee)

Comment 1

5 years ago
Created attachment 595332 [details] [diff] [review]
Patch
(Assignee)

Updated

5 years ago
Status: NEW → ASSIGNED
(Assignee)

Comment 2

5 years ago
Created attachment 595342 [details] [diff] [review]
Patch
Attachment #595332 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Blocks: 271815
(Assignee)

Updated

5 years ago
Blocks: 725233
(Assignee)

Comment 3

5 years ago
Comment on attachment 595342 [details] [diff] [review]
Patch

Now, aTextRangeList must not be NULL even if OOM happened.

mIMETextNode is already NULL after InsertText. If the composition string is empty, nsEditor::InsertTextIntoTextNodeImpl() sets NULL.
http://mxr.mozilla.org/mozilla-central/source/editor/libeditor/base/nsEditor.cpp#2490

By this change, empty text event causes deleting selected text even if it's fired immediately after compositionstart event. This is important for implementing TSF of Windows. And also, such behavior is better for consistency with other actions.

The bugs which caused current hacky implementation are tested by the new automated tests.
Attachment #595342 - Flags: review?(ehsan)
Comment on attachment 595342 [details] [diff] [review]
Patch

Review of attachment 595342 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with the blow.

::: editor/libeditor/text/nsPlaintextEditor.cpp
@@ +978,2 @@
>  
> +  if (true) {

Please remove this.
(Assignee)

Comment 5

5 years ago
(In reply to Ehsan Akhgari [:ehsan] from comment #4)
> ::: editor/libeditor/text/nsPlaintextEditor.cpp
> @@ +978,2 @@
> >  
> > +  if (true) {
> 
> Please remove this.

Did you mean, should I use just |{}| instead of |if () {}|? The block is needed for nsAutoPlaceHolderBatch. After it's destroyed, we need to call NotifyEditorObservers().

Updated

5 years ago
Attachment #595342 - Flags: review?(ehsan) → review+
(Assignee)

Comment 6

5 years ago
I removed |if (true)|, it's not just |{}|.
https://hg.mozilla.org/integration/mozilla-inbound/rev/2eaea6a86276
Whiteboard: [inbound]
Target Milestone: --- → mozilla13
https://hg.mozilla.org/mozilla-central/rev/2eaea6a86276
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.