Closed Bug 961703 Opened 7 years ago Closed 7 years ago

[TSF] Avoid making TIP confused by odd behavior

Categories

(Core :: Widget: Win32, defect)

x86_64
Windows 8.1
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla29

People

(Reporter: masayuki, Assigned: masayuki)

References

(Blocks 1 open bug)

Details

(Keywords: inputmethod)

Attachments

(1 file, 2 obsolete files)

If our TSF implementation behaves strange, TIP is confused. And after that, TIP may not work well. For avoiding making TIP confused, we should create some walls at:

1. sending text change notification if it occurs during flushing pending actions or composition.
2. sending selection change notification if it occurs during flushing pending actions or composition.
3. destroying text store but it has composition.
Attached patch Patch (obsolete) — Splinter Review
Test case for text change:
> data:text/html,<body contenteditable>Here is editable.</body><script>setInterval(function () { document.body.innerHTML += ", updated!"; }, 1000);</script>

Test case for selection change:
> data:text/html,<body contenteditable>Here is editable.</body><script>setInterval(function () { window.getSelection().selectAllChildren(document.body); window.getSelection().collapseToStart(); }, 1000);</script>
Assignee: nobody → masayuki
Status: NEW → ASSIGNED
Attachment #8362833 - Flags: review?(m_kato)
Comment on attachment 8362833 [details] [diff] [review]
Patch

Sorry for the spam, this is older patch.
Attachment #8362833 - Flags: review?(m_kato)
Attached patch Patch (obsolete) — Splinter Review
Attachment #8362833 - Attachment is obsolete: true
Attachment #8362834 - Flags: review?(m_kato)
Comment on attachment 8362834 [details] [diff] [review]
Patch

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

good, but I have a question for OnTextChange.

::: widget/windows/nsTextStore.cpp
@@ +3129,5 @@
>             ("TSF: 0x%p   nsTextStore::OnTextChangeMsg(), calling"
>              "mSink->OnTextChange(0, { acpStart=%ld, acpOldEnd=%ld, "
>              "acpNewEnd=%ld })...", this, mTextChange.acpStart,
>              mTextChange.acpOldEnd, mTextChange.acpNewEnd));
>      mSink->OnTextChange(0, &mTextChange);

Before calling ITextStoreACPSink::OnTextChange, don't we check IsReadLocked()?

OnTextChangeMsg is from message loop even if OnTextChangeInternal() is called without locked state, so OnTextChangeMsg() will be called with locked state.  If this is always called with unlocked state.  please ignore this comment.
s/OnTextChangeMsg() will be called with locked state./OnTextChangeMsg() may be called with locked state.../
True. However, I'm not sure what we should do it at that case. Discarding the notification? Repost the message?

But it's a hack of initial implementation. At that time, nsTextStateManager notifies widget of text change synchronously during mutation. If it dispatches events during mutation, it causes bugs.

However, now, nsTextStateManager uses runnable event for notifying widget of text change.
http://mxr.mozilla.org/mozilla-central/source/dom/events/nsIMEStateManager.cpp#864

So, I guess that we don't need the hack anymore. I'll check it today and update the patch if we can remove it.
(In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #6)
> True. However, I'm not sure what we should do it at that case. Discarding
> the notification? Repost the message?

Actually, current is discarding this now since IsReadLocked() is true. If reposting the message, we need more test for all TIPs.
Attached patch PatchSplinter Review
This patch depends on the patch of bug 544779.
Attachment #8362834 - Attachment is obsolete: true
Attachment #8362834 - Flags: review?(m_kato)
Attachment #8364145 - Flags: review?(m_kato)
(In reply to Makoto Kato (:m_kato) from comment #7)
> (In reply to Masayuki Nakano (:masayuki) (Mozilla Japan) from comment #6)
> > True. However, I'm not sure what we should do it at that case. Discarding
> > the notification? Repost the message?
> 
> Actually, current is discarding this now since IsReadLocked() is true. If
> reposting the message, we need more test for all TIPs.

If it causes some bugs, let's fix them in other new bugs.
Attachment #8364145 - Flags: review?(m_kato) → review+
https://hg.mozilla.org/mozilla-central/rev/c2e11417e175
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla29
You need to log in before you can comment on or make changes to this bug.