Closed Bug 1224994 Opened 9 years ago Closed 8 years ago

[TSF] TSFTextStore should cache whole contents during composition

Categories

(Core :: Widget: Win32, defect)

Product:
Core
Component:
Widget: Win32
Platform:
x86
Windows
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla50
Tracking Flags:
Tracking Status
firefox45 --- affected
firefox50 --- fixed

People

(Reporter: masayuki, Assigned: masayuki)

References

(Blocks 1 open bug,
URL
)

Details

(Keywords: inputmethod)

Attachments

(13 files)

Bug 1224994 part.1 Rename TSFTextStore::mLockedContent to TSFTextStore::mContentForTSF
58 bytes, text/x-review-board-request
m_kato
: review+
Details
Bug 1224994 part.2 Rename TSFTextStore::LockedContent() to TSFTextStore::ContentForTSFRef()
58 bytes, text/x-review-board-request
m_kato
: review+
Details
Bug 1224994 part.3 Rename TSFTextStore::mDeferClearingLockedContent to TSFTextStore::mDeferClearingContentForTSF
58 bytes, text/x-review-board-request
m_kato
: review+
Details
Bug 1224994 part.4 Rename the variable name which is for storing the result of TSFTextStore::ContentForTSFRef() to contentForTSF
58 bytes, text/x-review-board-request
m_kato
: review+
Details
Bug 1224994 part.5 Implement TSFTextStore::IsComposingInContent() to check if the focused editor has composition
58 bytes, text/x-review-board-request
m_kato
: review+
Details
Bug 1224994 part.6 Don't clear TSFTextStore::mContentForTSF until active composition is committed
58 bytes, text/x-review-board-request
m_kato
: review+
Details
Bug 1224994 part.7 Don't notify TSF of selection changes while there is a cached content
58 bytes, text/x-review-board-request
m_kato
: review+
Details
Bug 1224994 part.8 Don't notify TSF of text changes while there is cached content
58 bytes, text/x-review-board-request
m_kato
: review+
Details
Bug 1224994 part.9 TSFTextStore shouldn't set selection when there is unknown pending text changes
58 bytes, text/x-review-board-request
m_kato
: review+
Details
Bug 1224994 part.10 Rename TSFTextStore::mSelection to TSFTextStore::mSelectionForTSF for making its meaning clearer
58 bytes, text/x-review-board-request
m_kato
: review+
Details
Bug 1224994 part.11 Rename TSFTextStore::CurrentSelection() to TSFTextStore::SelectionForTSF()
58 bytes, text/x-review-board-request
m_kato
: review+
Details
Bug 1224994 part.12 Rename the variable names for the result of TSFTextStore::SelectionForTSF() to selectionForTSF
58 bytes, text/x-review-board-request
m_kato
: review+
Details
Bug 1224994 part.13 Fix some comments which say "locked content" in TSFTextStore.cpp
58 bytes, text/x-review-board-request
m_kato
: review+
Details 
TSFTextStore currently caches the content from starting to lock the document until remote content is handled all sending events.

However, unfortunately, at compositionstart, selected range can be changed. This is allowed by web standard specs but TSF doesn't allow this.

If we commit forcibly at this selection change, some websites may not be available for IME users.

If we send selection change honestly, some TIP may be confused since such selection change isn't allowed in TSF spec, or some TIP may commit composition forcibly. The latter isn't useful for the users due to same above case.

Therefore, I think that whole editor content should be cached by TSFTextStore at starting composition. And it should be kept until the composition is finished. Then, from the TIP, the selection looks like it's never changed. For emulating this, we need to adjust offset at every eQuery* event.

I wonder, Apple's IME for OS X may have similar problem. See bug 1205945. In such case, we need to implement this feature in XP level like ContentCache.
Summary: [TSF] TSFTextStore should cache during composition → [TSF] TSFTextStore should cache whole contents during composition
For fixing a lot of bugs in Google Docs and Facebook, we should fix this bug as soon as possible.

For now, I don't try to fix this bug in XP level.
Status: NEW → ASSIGNED
Review commit: https://reviewboard.mozilla.org/r/61742/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/61742/
Attachment #8767041 - Flags: review?(m_kato)
Attachment #8767042 - Flags: review?(m_kato)
Attachment #8767043 - Flags: review?(m_kato)
Attachment #8767044 - Flags: review?(m_kato)
Attachment #8767045 - Flags: review?(m_kato)
Attachment #8767046 - Flags: review?(m_kato)
Attachment #8767047 - Flags: review?(m_kato)
Attachment #8767048 - Flags: review?(m_kato)
Attachment #8767049 - Flags: review?(m_kato)
Attachment #8767050 - Flags: review?(m_kato)
Attachment #8767051 - Flags: review?(m_kato)
Attachment #8767052 - Flags: review?(m_kato)
This patch stop clearing mContentForTSF at unlocking the document because we should keep it until active composition is committed.  If so, TSF/TIP won't be confused by content changes by JS.  So, this is important for a11y of TIP users in some complicated websites like GoogleDocs, Facebook, etc.

Note that this patch doesn't work well without following patches.  We need to stop notifying TSF of selection changes and text changed while mContentForTSF is valid.

Review commit: https://reviewboard.mozilla.org/r/61752/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/61752/
TSFTextStore shouldn't notify TSF of selection change until MaybeFlushPendingNotifications() is called and there is no cached content because while there is cached content, neither TSF nor TIP may allow to change selection by web applications.  Therefore, ITextStoreACP::GetSelection() and similar methods need to use mSelection instead of actual selection in the focused editor.  Therefore, TSFTextStore should store selection change data during keeping storing content cache and notify it when the cache is cleared. So, when TSFTextStore notifies TSF of selection change, TSFTextStore needs to update mSelection to the actual selection which is stored in mPendingSelectionChangeData.

Review commit: https://reviewboard.mozilla.org/r/61754/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/61754/
Same as selection change notification, text change notification shouldn't be notified to TSF while there is cachec content because neither TSF nor TIP may allow to change text by web applications during keeping storing cached content.

This patch makes TSFTextStore stores and merges text changes until MaybeFlushPendingNotifications() is called and there is no cached content.

Review commit: https://reviewboard.mozilla.org/r/61756/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/61756/
I'm still not sure what we should do in this case, though.

If mContentForTSF is initialized and there are some unknown changes in actual contents, i.e., not caused by composition of the active TIP itself, we cannot set selection range properly in some cases.

For example, if TSF tires to set non-empty selection range but the range has been removed by web apps.

For now, let's try to return E_FAIL in such case because that should occur at reconversion or something immediately after previous content change not caused by previous composition.  If TIP does nothing in this case, user can retry with same operation after all pending text changes are notified to TSF.

Review commit: https://reviewboard.mozilla.org/r/61758/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/61758/
Comment on attachment 8767041 [details]
Bug 1224994 part.1 Rename TSFTextStore::mLockedContent to TSFTextStore::mContentForTSF

https://reviewboard.mozilla.org/r/61742/#review58720
Attachment #8767041 - Flags: review?(m_kato) → review+
Attachment #8767042 - Flags: review?(m_kato) → review+
Comment on attachment 8767042 [details]
Bug 1224994 part.2 Rename TSFTextStore::LockedContent() to TSFTextStore::ContentForTSFRef()

https://reviewboard.mozilla.org/r/61744/#review58722
Comment on attachment 8767043 [details]
Bug 1224994 part.3 Rename TSFTextStore::mDeferClearingLockedContent to TSFTextStore::mDeferClearingContentForTSF

https://reviewboard.mozilla.org/r/61746/#review58724
Attachment #8767043 - Flags: review?(m_kato) → review+
Attachment #8767044 - Flags: review?(m_kato) → review+
Comment on attachment 8767044 [details]
Bug 1224994 part.4 Rename the variable name which is for storing the result of TSFTextStore::ContentForTSFRef() to contentForTSF

https://reviewboard.mozilla.org/r/61748/#review58726
Comment on attachment 8767045 [details]
Bug 1224994 part.5 Implement TSFTextStore::IsComposingInContent() to check if the focused editor has composition

https://reviewboard.mozilla.org/r/61750/#review58728
Attachment #8767045 - Flags: review?(m_kato) → review+
Attachment #8767046 - Flags: review?(m_kato) → review+
Comment on attachment 8767046 [details]
Bug 1224994 part.6 Don't clear TSFTextStore::mContentForTSF until active composition is committed

https://reviewboard.mozilla.org/r/61752/#review58736

Actually, When handing PendingAction::COMPOSITION_END, although mDeferClearingContentForTSF is clear, we might have to check that the queue has another PendingAction::COMPOSITION_START on Korean TIP.  But is is OK since we should consider it by bug 1238326 if needed.
Comment on attachment 8767047 [details]
Bug 1224994 part.7 Don't notify TSF of selection changes while there is a cached content

https://reviewboard.mozilla.org/r/61754/#review58744
Attachment #8767047 - Flags: review?(m_kato) → review+
No, it's not cleared in normal path.

>       case PendingAction::COMPOSITION_END: {
>         MOZ_LOG(sTextStoreLog, LogLevel::Debug,
>                ("TSF: 0x%p   TSFTextStore::FlushPendingActions() "
>                 "flushing COMPOSITION_END={ mData=\"%s\" }",
>                 this, NS_ConvertUTF16toUTF8(action.mData).get()));
> 
>         // Dispatching eCompositionCommit causes a DOM text event, then,
>         // the IME will be notified of NOTIFY_IME_OF_COMPOSITION_EVENT_HANDLED.
>         // In this case, we should not clear the locked content until we notify
>         // the IME of the composition update.
>         mDeferClearingContentForTSF = true;
> 
>         MOZ_LOG(sTextStoreLog, LogLevel::Debug,
>                ("TSF: 0x%p   TSFTextStore::FlushPendingActions(), "
>                 "dispatching compositioncommit event...", this));
>         WidgetEventTime eventTime = mWidget->CurrentMessageWidgetEventTime();
>         nsEventStatus status;
>         rv = mDispatcher->CommitComposition(status, &action.mData, &eventTime);
>         if (NS_WARN_IF(NS_FAILED(rv))) {
>           MOZ_LOG(sTextStoreLog, LogLevel::Error,
>             ("TSF: 0x%p   TSFTextStore::FlushPendingActions() "
>              "FAILED to dispatch compositioncommit event, "
>              "IsComposingInContent()=%s",
>              this, GetBoolName(IsComposingInContent())));
>           mDeferClearingContentForTSF = !IsComposingInContent();
>         }
>         break;
>       }

# Oh, and I should search "locked content" in the comments in TSFTextStore.cpp...
Comment on attachment 8767041 [details]
Bug 1224994 part.1 Rename TSFTextStore::mLockedContent to TSFTextStore::mContentForTSF

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/61742/diff/1-2/
Comment on attachment 8767042 [details]
Bug 1224994 part.2 Rename TSFTextStore::LockedContent() to TSFTextStore::ContentForTSFRef()

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/61744/diff/1-2/
Comment on attachment 8767043 [details]
Bug 1224994 part.3 Rename TSFTextStore::mDeferClearingLockedContent to TSFTextStore::mDeferClearingContentForTSF

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/61746/diff/1-2/
Comment on attachment 8767044 [details]
Bug 1224994 part.4 Rename the variable name which is for storing the result of TSFTextStore::ContentForTSFRef() to contentForTSF

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/61748/diff/1-2/
Comment on attachment 8767045 [details]
Bug 1224994 part.5 Implement TSFTextStore::IsComposingInContent() to check if the focused editor has composition

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/61750/diff/1-2/
Comment on attachment 8767046 [details]
Bug 1224994 part.6 Don't clear TSFTextStore::mContentForTSF until active composition is committed

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/61752/diff/1-2/
Comment on attachment 8767047 [details]
Bug 1224994 part.7 Don't notify TSF of selection changes while there is a cached content

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/61754/diff/1-2/
Comment on attachment 8767048 [details]
Bug 1224994 part.8 Don't notify TSF of text changes while there is cached content

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/61756/diff/1-2/
Comment on attachment 8767049 [details]
Bug 1224994 part.9 TSFTextStore shouldn't set selection when there is unknown pending text changes

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/61758/diff/1-2/
Comment on attachment 8767050 [details]
Bug 1224994 part.10 Rename TSFTextStore::mSelection to TSFTextStore::mSelectionForTSF for making its meaning clearer

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/61760/diff/1-2/
Comment on attachment 8767051 [details]
Bug 1224994 part.11 Rename TSFTextStore::CurrentSelection() to TSFTextStore::SelectionForTSF()

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/61762/diff/1-2/
Comment on attachment 8767052 [details]
Bug 1224994 part.12 Rename the variable names for the result of TSFTextStore::SelectionForTSF() to selectionForTSF

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/61764/diff/1-2/
Attachment #8767048 - Flags: review?(m_kato) → review+
Comment on attachment 8767048 [details]
Bug 1224994 part.8 Don't notify TSF of text changes while there is cached content

https://reviewboard.mozilla.org/r/61756/#review58776
Comment on attachment 8767049 [details]
Bug 1224994 part.9 TSFTextStore shouldn't set selection when there is unknown pending text changes

https://reviewboard.mozilla.org/r/61758/#review58784
Attachment #8767049 - Flags: review?(m_kato) → review+
Attachment #8767050 - Flags: review?(m_kato) → review+
Comment on attachment 8767050 [details]
Bug 1224994 part.10 Rename TSFTextStore::mSelection to TSFTextStore::mSelectionForTSF for making its meaning clearer

https://reviewboard.mozilla.org/r/61760/#review58846
Comment on attachment 8767051 [details]
Bug 1224994 part.11 Rename TSFTextStore::CurrentSelection() to TSFTextStore::SelectionForTSF()

https://reviewboard.mozilla.org/r/61762/#review58944
Attachment #8767051 - Flags: review?(m_kato) → review+
Comment on attachment 8767052 [details]
Bug 1224994 part.12 Rename the variable names for the result of TSFTextStore::SelectionForTSF() to selectionForTSF

https://reviewboard.mozilla.org/r/61764/#review58946
Attachment #8767052 - Flags: review?(m_kato) → review+
Attachment #8767575 - Flags: review?(m_kato) → review+
Comment on attachment 8767575 [details]
Bug 1224994 part.13 Fix some comments which say "locked content" in TSFTextStore.cpp

https://reviewboard.mozilla.org/r/62026/#review58948
Pushed by masayuki@d-toybox.com:
https://hg.mozilla.org/integration/autoland/rev/154d0ebf99e0
part.1 Rename TSFTextStore::mLockedContent to TSFTextStore::mContentForTSF r=m_kato
https://hg.mozilla.org/integration/autoland/rev/f83dca35bb32
part.2 Rename TSFTextStore::LockedContent() to TSFTextStore::ContentForTSFRef() r=m_kato
https://hg.mozilla.org/integration/autoland/rev/1927603da3c0
part.3 Rename TSFTextStore::mDeferClearingLockedContent to TSFTextStore::mDeferClearingContentForTSF r=m_kato
https://hg.mozilla.org/integration/autoland/rev/08af14bb5e38
part.4 Rename the variable name which is for storing the result of TSFTextStore::ContentForTSFRef() to contentForTSF r=m_kato
https://hg.mozilla.org/integration/autoland/rev/e29577344d51
part.5 Implement TSFTextStore::IsComposingInContent() to check if the focused editor has composition r=m_kato
https://hg.mozilla.org/integration/autoland/rev/ce51850dddc6
part.6 Don't clear TSFTextStore::mContentForTSF until active composition is committed r=m_kato
https://hg.mozilla.org/integration/autoland/rev/d0b6fa38f05d
part.7 Don't notify TSF of selection changes while there is a cached content r=m_kato
https://hg.mozilla.org/integration/autoland/rev/8866c7c9a21d
part.8 Don't notify TSF of text changes while there is cached content r=m_kato
https://hg.mozilla.org/integration/autoland/rev/ff08df084602
part.9 TSFTextStore shouldn't set selection when there is unknown pending text changes r=m_kato
https://hg.mozilla.org/integration/autoland/rev/fadd96dff61f
part.10 Rename TSFTextStore::mSelection to TSFTextStore::mSelectionForTSF for making its meaning clearer r=m_kato
https://hg.mozilla.org/integration/autoland/rev/d7eefd29bfef
part.11 Rename TSFTextStore::CurrentSelection() to TSFTextStore::SelectionForTSF() r=m_kato
https://hg.mozilla.org/integration/autoland/rev/99a286cee695
part.12 Rename the variable names for the result of TSFTextStore::SelectionForTSF() to selectionForTSF r=m_kato
https://hg.mozilla.org/integration/autoland/rev/ded9d3adbde1
part.13 Fix some comments which say "locked content" in TSFTextStore.cpp r=m_kato
Blocks: 1206387
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: