Closed Bug 1746104 Opened 3 years ago Closed 3 years ago

ContentCacheInChild shouldn't stop caching text content etc when there is no selection

Categories

(Core :: DOM: UI Events & Focus Handling, defect, P3)

Product:
Core
Component:
DOM: UI Events & Focus Handling
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
99 Branch
Tracking Flags:
Tracking Status
firefox99 --- fixed

People

(Reporter: masayuki, Assigned: masayuki)

References

(Blocks 2 open bugs)

Details

(Keywords: inputmethod)

Attachments

(29 files)

Bug 1746104 - part 1-1: Make `TSFTextStore::Selection` work with `IMENotification::SelectionChangeDataBase` directly r=m_kato!
(deleted), text/x-phabricator-request
Details
Bug 1746104 - part 1-2: Make `TSFTextStore::Selection` have "no selection" state r=m_kato!
(deleted), text/x-phabricator-request
Details
Bug 1746104 - part 1-3: Wrap `TSFTextStore::mPendingSelectionChangeData` with `Maybe` r=m_kato!
(deleted), text/x-phabricator-request
Details
Bug 1746104 - part 1-4: Make `TSFTextStore` handle the no selection case r=m_kato!
(deleted), text/x-phabricator-request
Details
Bug 1746104 - part 1-5: Make `TSFTextStore::Selection` return const reference of `WritingMode` rather than a copy r=m_kato!
(deleted), text/x-phabricator-request
Details
Bug 1746104 - part 2-1: Make `IMMHandler::Selection` capsule its members r=m_kato!
(deleted), text/x-phabricator-request
Details
Bug 1746104 - part 2-2: Get rid of `GetWritingModeName` in IMMHandler.cpp r=m_kato!
(deleted), text/x-phabricator-request
Details
Bug 1746104 - part 2-3: Clean up some code in IMEData.h r=m_kato!
(deleted), text/x-phabricator-request
Details
Bug 1746104 - part 2-4: Make `IMMHandler::Selection` manage offset and data with `Maybe<OffsetAndData<uint32_t>>` r=m_kato!
(deleted), text/x-phabricator-request
Details
Bug 1746104 - part 2-5: Make `IMMHandler` handle the no selection case r=m_kato!
(deleted), text/x-phabricator-request
Details
Bug 1746104 - part 3-1: Get rid of `WritingMode` argument of `IMContextWrapper::CollapseTo` r=m_kato!
(deleted), text/x-phabricator-request
Details
Bug 1746104 - part 3-2: Get rid of `GetWritingModeName` in IMContextWrapper.cpp r=m_kato!
(deleted), text/x-phabricator-request
Details
Bug 1746104 - part 3-3: Make `IMContextWrapper::Selection` capsule its members r=m_kato!
(deleted), text/x-phabricator-request
Details
Bug 1746104 - part 3-4: Make `IMEContentWrapper::Selection` override `<<` operator r=m_kato!
(deleted), text/x-phabricator-request
Details
Bug 1746104 - part 3-5: Wrap `IMEContentWrapper::mSelection` with `Maybe` to represent error state and no cache state r=m_kato!
(deleted), text/x-phabricator-request
Details
Bug 1746104 - part 3-6: Make `IMContextWrapper::Selection` use `OffsetAndData<uint32_t>` r=m_kato!
(deleted), text/x-phabricator-request
Details
Bug 1746104 - part 3-7: make `IMContextWrapper` handle the cases there is no selection range r=m_kato!
(deleted), text/x-phabricator-request
Details
Bug 1746104 - part 4: Make `IMContextWrapper` and `IMMHandler` use same class to store content selection r=m_kato!
(deleted), text/x-phabricator-request
Details
Bug 1746104 - part 5-1: Get rid of `WidgetQueryContentEvent::Reply::mHasSelection` r=m_kato!
(deleted), text/x-phabricator-request
Details
Bug 1746104 - part 5-2: Add some utility methods to `IMENotification::SelectionChangeDataBase` r=m_kato!
(deleted), text/x-phabricator-request
Details
Bug 1746104 - part 5-3: Make `SelectionChangeDataBase` treat no range case r=m_kato!
(deleted), text/x-phabricator-request
Details
Bug 1746104 - part 5-4: Make widget code handle no selection case at getting selection change notifications r=m_kato!
(deleted), text/x-phabricator-request
Details
Bug 1746104 - part 6-0: Clean up logging code in `ContentCache.cpp` r=m_kato!
(deleted), text/x-phabricator-request
Details
Bug 1746104 - part 6-1: Make `IMENotification::SelectionChangeDataBase` and `WidgetQueryContentEvent::Reply` have `AnchorOffset` and `FocusOffset` r=m_kato!
(deleted), text/x-phabricator-request
Details
Bug 1746104 - part 6-2: Make constructors of `ContentCache::Selection` take `IMENotification::SelectionChangeData` or `WidgetQueryContentEvent` r=m_kato!
(deleted), text/x-phabricator-request
Details
Bug 1746104 - part 6-3: Wrap `ContentCache::mText` with `Maybe` r=m_kato!
(deleted), text/x-phabricator-request
Details
Bug 1746104 - part 6-4: Make `ContentCacheInParent` and `ContentCacheInChild` handle the case there is no selection range r=m_kato!
(deleted), text/x-phabricator-request
Details
Bug 1746104 - part 6-5: Make `ContentCache` not stop caching other data when it fails caching something r=m_kato!
(deleted), text/x-phabricator-request
Details
Bug 1746104 - part 7: Make `eQuerySelectedText` event and `ContentEventHandler` work with no selection ranges r=m_kato!
(deleted), text/x-phabricator-request
Details

https://searchfox.org/mozilla-central/rev/f465d324513f09dbe33ed79fabe6a9ef98aa51ca/widget/ContentCache.cpp#132,150-151

There can be no selection (e.g., when Selection.removeAllRanges() is called). Therefore, ContentCacheInChild shouldn't stop handling to cache the content in the case.

Selection.removeAllRanges can make there is no selection ranges. Therefore,
TSFTextStore::Selection needs to represent the state. This patch changes its
mACP to Maybe<TS_SELECTION_ACP> for making Nothing of it mean no
selection ranges. Then, GetSelection will return TS_E_NOSELECTION and
InsertTextAtSelection will return "no change" if it's called for emulation.

According to Chromium:
https://source.chromium.org/chromium/chromium/src/+/main:ui/base/ime/win/tsf_text_store_unittest.cc
They don't check this case's behavior, and it seems that they ignore selection
change at removeAllRanges:
https://source.chromium.org/chromium/chromium/src/+/main:ui/base/ime/win/tsf_text_store.cc;l=1153;drc=d50b5b8a78d3d4878c98b07af006a00ed65fddd0
or fill cached selection with collapsed at invalid position (UINT32_MAX).
https://source.chromium.org/chromium/chromium/src/+/main:ui/base/ime/win/tsf_text_store.cc;l=1238;drc=d50b5b8a78d3d4878c98b07af006a00ed65fddd0

So anyway, we don't have better handling in the case.

Depends on D137404

It needs to have a state whether it's set or unset, and whether there is at
least one selection range or no selection range. Currently, its valid state
represents both of them, but it'll has no selection state in the following
patches. Therefore, the former state should be manged with Maybe.

Depends on D137405

Most change has already been done in part 1-2. This patch does the remaining
work.

Depends on D137406

I'd like to use OffsetAndData in IMMHandler to store selection range.
Before doing this, this patch adds some useful methods into it.

Depends on D137410

For making it simpler and have "no selection" state, this patch rewrites it
with OffsetAndData<uint32_t> wrapped with Maybe. Although IMMHandler
does nothing if there is no selection.

Depends on D137411

With wrapping IMMHandler::mSelection with Maybe, we can represent there is
no unexpected cases, but there is no selection.

Depends on D137412

It's always overwritten with same value so that it is not necessary at least for
now and on Windows, we just do not update it in same situation. Therefore,
we should drop it until we become need to use it.

Depends on D137413

For consistency with IMMHandler::Selection, let's make it use
OffsetAndData<uint32_t> for preparing them to use a new common class.

Depends on D137418

Perhaps, we should disable IME when DOM Selection does not have range. However,
it's out of scope of this bug. This patch makes IMContextWrapper try to
keep working with no selection range except at "compositionstart".

Depends on D137419

Now, IMContextWrapper::Selection and IMMHandler::Selection have same
structure. Therefore, we can merge them into one place. This will help to
fix bug 1259690 in the future.

Depends on D137420

It's intended to indicate whether the selection is collapsed or not, but it can
be referred by other members, there is no reasonable user and the name makes
developers confused.

Depends on D137421

Unfortunately, it cannot has Maybe because of used by a union. Therefore,
it needs to manage with bool members of whether it's initialized with or
without error and whether it has a range.

Note that this changes nsITextInputProcessorCallback.idl which is currently
only for automated tests too. The new behavior will be tested by the last
patch in this bug.

Depends on D137423

StartOffset and EndOffset of IMENotification::SelectionChangeDataBase
depend on whether the range is reversed or not. Therefore, their actual
meaning is "anchor offset" and "focus offset" in the DOM selection terms.

Similarly, SelectionStartOffset and SelectionEndOffset of
WidgetQueryContentEvent::Reply are exactly same ones.

Therefore, they should be renamed to AnchorOffset and FocusOffset for
making what they return clearer.

Depends on D137426

For managing the state of text content cache independent from selection, it
should be wrapped with Maybe.

Depends on D137428

Even after applying this patch, it still returns error when queried with a
relative offset from selection and only when there is no composition string.
However, this shouldn't cause any problem because in this case, widget should
not try to query content with relative offset.

Depends on D137429

Currently, mSelection is cached only when mText is some, text rects are
cached only when mSelection is cached. However, as far as widget can work
with IME, it should cache content as far as possible. Therefore, this patch
makes it keep querying content data even after something fails.

Depends on D137430

Finally, making WidgetQueryContentEvent::Succeeded return true when there
is no selection when its message is eQuerySelectedText, and making
ContentEventHandler::OnQuerySelectedText set the no selection range data even
when the queried selection type is "normal. Then, all things which are
changed by the previous patches start to work.

And this patch adds a simple test for ContentCache and update tests of
nsITextInputProcessor.

Depends on D137431

Pushed by masayuki@d-toybox.com: https://hg.mozilla.org/integration/autoland/rev/14e68c94b303 part 1-1: Make `TSFTextStore::Selection` work with `IMENotification::SelectionChangeDataBase` directly r=m_kato https://hg.mozilla.org/integration/autoland/rev/ff44b008f146 part 1-2: Make `TSFTextStore::Selection` have "no selection" state r=m_kato https://hg.mozilla.org/integration/autoland/rev/1971c9f47541 part 1-3: Wrap `TSFTextStore::mPendingSelectionChangeData` with `Maybe` r=m_kato https://hg.mozilla.org/integration/autoland/rev/1dc1117af560 part 1-4: Make `TSFTextStore` handle the no selection case r=m_kato https://hg.mozilla.org/integration/autoland/rev/60724af309c9 part 1-5: Make `TSFTextStore::Selection` return const reference of `WritingMode` rather than a copy r=m_kato https://hg.mozilla.org/integration/autoland/rev/8435cab898aa part 2-1: Make `IMMHandler::Selection` capsule its members r=m_kato https://hg.mozilla.org/integration/autoland/rev/13d7ba297d37 part 2-2: Get rid of `GetWritingModeName` in IMMHandler.cpp r=m_kato https://hg.mozilla.org/integration/autoland/rev/46dfc3a53efb part 2-3: Clean up some code in IMEData.h r=m_kato https://hg.mozilla.org/integration/autoland/rev/2a959b3c8938 part 2-4: Make `IMMHandler::Selection` manage offset and data with `Maybe<OffsetAndData<uint32_t>>` r=m_kato https://hg.mozilla.org/integration/autoland/rev/670aeb65b5d7 part 2-5: Make `IMMHandler` handle the no selection case r=m_kato https://hg.mozilla.org/integration/autoland/rev/8ce50cd6760e part 3-1: Get rid of `WritingMode` argument of `IMContextWrapper::CollapseTo` r=m_kato https://hg.mozilla.org/integration/autoland/rev/76e3c19c0d4f part 3-2: Get rid of `GetWritingModeName` in IMContextWrapper.cpp r=m_kato https://hg.mozilla.org/integration/autoland/rev/23a249b011f8 part 3-3: Make `IMContextWrapper::Selection` capsule its members r=m_kato https://hg.mozilla.org/integration/autoland/rev/63e8e1bc2799 part 3-4: Make `IMEContentWrapper::Selection` override `<<` operator r=m_kato https://hg.mozilla.org/integration/autoland/rev/7642d525be8e part 3-5: Wrap `IMEContentWrapper::mSelection` with `Maybe` to represent error state and no cache state r=m_kato https://hg.mozilla.org/integration/autoland/rev/d3fdb943d696 part 3-6: Make `IMContextWrapper::Selection` use `OffsetAndData<uint32_t>` r=m_kato https://hg.mozilla.org/integration/autoland/rev/432291215383 part 3-7: make `IMContextWrapper` handle the cases there is no selection range r=m_kato https://hg.mozilla.org/integration/autoland/rev/dc81d96d0adb part 4: Make `IMContextWrapper` and `IMMHandler` use same class to store content selection r=m_kato https://hg.mozilla.org/integration/autoland/rev/5f90ad3d8781 part 5-1: Get rid of `WidgetQueryContentEvent::Reply::mHasSelection` r=m_kato https://hg.mozilla.org/integration/autoland/rev/5cd908f74145 part 5-2: Add some utility methods to `IMENotification::SelectionChangeDataBase` r=m_kato https://hg.mozilla.org/integration/autoland/rev/fa7bd59d3ed6 part 5-3: Make `SelectionChangeDataBase` treat no range case r=m_kato https://hg.mozilla.org/integration/autoland/rev/0d81c2841c4d part 5-4: Make widget code handle no selection case at getting selection change notifications r=m_kato,geckoview-reviewers https://hg.mozilla.org/integration/autoland/rev/4eb5dc745ee8 part 6-0: Clean up logging code in `ContentCache.cpp` r=m_kato https://hg.mozilla.org/integration/autoland/rev/2529c39ed6bf part 6-1: Make `IMENotification::SelectionChangeDataBase` and `WidgetQueryContentEvent::Reply` have `AnchorOffset` and `FocusOffset` r=m_kato,geckoview-reviewers https://hg.mozilla.org/integration/autoland/rev/88cd68e70502 part 6-2: Make constructors of `ContentCache::Selection` take `IMENotification::SelectionChangeData` or `WidgetQueryContentEvent` r=m_kato https://hg.mozilla.org/integration/autoland/rev/4fe6068164c1 part 6-3: Wrap `ContentCache::mText` with `Maybe` r=m_kato https://hg.mozilla.org/integration/autoland/rev/7d1d1864b99b part 6-4: Make `ContentCacheInParent` and `ContentCacheInChild` handle the case there is no selection range r=m_kato https://hg.mozilla.org/integration/autoland/rev/989ad9507a0c part 6-5: Make `ContentCache` not stop caching other data when it fails caching something r=m_kato https://hg.mozilla.org/integration/autoland/rev/3f6deedb2ffa part 7: Make `eQuerySelectedText` event and `ContentEventHandler` work with no selection ranges r=m_kato

https://hg.mozilla.org/mozilla-central/rev/14e68c94b303
https://hg.mozilla.org/mozilla-central/rev/ff44b008f146
https://hg.mozilla.org/mozilla-central/rev/1971c9f47541
https://hg.mozilla.org/mozilla-central/rev/1dc1117af560
https://hg.mozilla.org/mozilla-central/rev/60724af309c9
https://hg.mozilla.org/mozilla-central/rev/8435cab898aa
https://hg.mozilla.org/mozilla-central/rev/13d7ba297d37
https://hg.mozilla.org/mozilla-central/rev/46dfc3a53efb
https://hg.mozilla.org/mozilla-central/rev/2a959b3c8938
https://hg.mozilla.org/mozilla-central/rev/670aeb65b5d7
https://hg.mozilla.org/mozilla-central/rev/8ce50cd6760e
https://hg.mozilla.org/mozilla-central/rev/76e3c19c0d4f
https://hg.mozilla.org/mozilla-central/rev/23a249b011f8
https://hg.mozilla.org/mozilla-central/rev/63e8e1bc2799
https://hg.mozilla.org/mozilla-central/rev/7642d525be8e
https://hg.mozilla.org/mozilla-central/rev/d3fdb943d696
https://hg.mozilla.org/mozilla-central/rev/432291215383
https://hg.mozilla.org/mozilla-central/rev/dc81d96d0adb
https://hg.mozilla.org/mozilla-central/rev/5f90ad3d8781
https://hg.mozilla.org/mozilla-central/rev/5cd908f74145
https://hg.mozilla.org/mozilla-central/rev/fa7bd59d3ed6
https://hg.mozilla.org/mozilla-central/rev/0d81c2841c4d
https://hg.mozilla.org/mozilla-central/rev/4eb5dc745ee8
https://hg.mozilla.org/mozilla-central/rev/2529c39ed6bf
https://hg.mozilla.org/mozilla-central/rev/88cd68e70502
https://hg.mozilla.org/mozilla-central/rev/4fe6068164c1
https://hg.mozilla.org/mozilla-central/rev/7d1d1864b99b
https://hg.mozilla.org/mozilla-central/rev/989ad9507a0c
https://hg.mozilla.org/mozilla-central/rev/3f6deedb2ffa

Status: ASSIGNED → RESOLVED
Closed: 3 years ago
status-firefox99: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → 99 Branch
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: