Closed Bug 1275914 Opened 8 years ago Closed 8 years ago

TextComposition should update mCompositionStart and mCompositionTargetOffset even after compositionstart is dispatched

Categories

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

Product:
Core
Component:
DOM: UI Events & Focus Handling
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

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

People

(Reporter: masayuki, Assigned: masayuki)

References

(
URL
)

Details

(Keywords: inputmethod, Whiteboard: btpp-active)

Attachments

(8 files)

Bug 1275914 part.1 TextComposition should store target clause offset from start of the composition instead of start of the editor
58 bytes, text/x-review-board-request
smaug
: review+
Details
Bug 1275914 part.2 Modify TextComposition::mCompositionStartOffset after every composition event dispatch
58 bytes, text/x-review-board-request
smaug
: review+
Details
Bug 1275914 part.3 TextComposition should use IMEContentObserver or ContentEventHandler directly when it queries the selection
58 bytes, text/x-review-board-request
smaug
: review+
Details
Bug 1275914 part.4 Add a method to retrieve focused content's selection controller to nsIPresShell
58 bytes, text/x-review-board-request
smaug
: review+
Details
Bug 1275914 part.5 Support special selections at handling eQuerySelectedText
58 bytes, text/x-review-board-request
smaug
: review+
Details
Bug 1275914 part.6 ContentEventHandler::OnQuerySelectedText() shouldn't refer anchor and focus of selection if there are 2 or more selection ranges
58 bytes, text/x-review-board-request
smaug
: review+
Details
Bug 1275914 part.7 Add automated tests to query IME selections
58 bytes, text/x-review-board-request
smaug
: review+
Details
Bug 1275914 part.8 TextComposition should always update its composition start offset during composing
58 bytes, text/x-review-board-request
smaug
: review+
Details 
As far as I've tested, changing selection from compositionupdate event handler affects to the replacing range or the start offset of starting composition.

So, until start offset of composition string is fixed, we should update mCompositionStart and mCompositionTargetOffset of TextComposition.
Status: NEW → ASSIGNED
Whiteboard: btpp-active
It's enough to store target clause offset from start of the composition and better to modify mCompositionStartOffset because when even if mCompositionStartOffset is changed, we don't need to modify the target clause offset.

This patch renames mCompositionTargetOffset to mTargetClauseOffsetInComposition.

Review commit: https://reviewboard.mozilla.org/r/59142/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/59142/
Attachment #8762464 - Flags: review?(bugs)
Attachment #8762465 - Flags: review?(bugs)
Attachment #8762466 - Flags: review?(bugs)
Attachment #8762467 - Flags: review?(bugs)
Attachment #8762468 - Flags: review?(bugs)
Attachment #8762469 - Flags: review?(bugs)
Attachment #8762470 - Flags: review?(bugs)
Attachment #8762471 - Flags: review?(bugs)
When composition string hasn't been non-empty, insertion point of the composition string can be changed by a DOM event handler.  E.g., compositionstart, first compositionupdate and first text.  Therefore, TextComposition should update the composition start offset cache after every event dispatch.

Review commit: https://reviewboard.mozilla.org/r/59144/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/59144/
TextComposition queries selection start offset a lot of times.  Therefore, for reducing the runtime cost, it should use IMEContentObserver if it's available or ContentEventHandler, otherwise.

Review commit: https://reviewboard.mozilla.org/r/59146/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/59146/
ContentEventHandler will need to support query any types of selection.  Therefore, it cannot use nsCopySupport::GetSelectionForCopy().  Instead, a part of it should be implemented by nsIPresShell::GetSelectionControllerForFocusedContent() and it should be shared by nsCopySupport::GetSelectionForCopy() and ContentEventHandler.

Review commit: https://reviewboard.mozilla.org/r/59148/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/59148/
TextComposition needs to query first IME selection.  Therefore, we need to add support to query special selection range with eQuerySelectedText.

First, WidgetQueryContentEvent::mInput should have mSelectionType which should be initialized with InitForQuerySelectedText() but unfortunately, there is no method for eQuerySelectedText.  Therefore, this patch initializes WidgetQueryContentEvent::mInit::mSelectionType with SelectionType::eNormal in its constructor.

Next, ContentEventHandler needs to support to handle eQuerySelectedText with special selection types.  However, we need to create 2 paths in some cases, one is for normal selection and the other is for special selections because there are no selection ranges may be usual case for special selections but not so for normal selection.  Therefore, ContentEventHandler::InitCommon() becomes a little bit more complicated.  ContentEventHandler::mSelection and ContentEventHandler::mFirstSelectedRange is initialized with the specified selection type but normal selection type is also necessary to compute the selection root since the selection root is computed from the first selected range which may not be anywhere if its selection type is not normal.

Finally, ContentEventHandler::OnQuerySelectedText() returns "there are no selections" as succeeded case in special selection type cases.

Review commit: https://reviewboard.mozilla.org/r/59150/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/59150/
Selection's focus and anchor node and offset are stored only for the last range.  However, ContentEventHandler needs its first range.  Therefore, ContentEventHandler shouldn't refer them if there are two or more selection ranges.

Review commit: https://reviewboard.mozilla.org/r/59152/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/59152/
TextComposition should update its composition start offset after every DOM event dispatch with first clause's selection type if there is composition string.

Review commit: https://reviewboard.mozilla.org/r/59156/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/59156/
Comment on attachment 8762464 [details]
Bug 1275914 part.1 TextComposition should store target clause offset from start of the composition instead of start of the editor

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/59142/diff/1-2/
Comment on attachment 8762465 [details]
Bug 1275914 part.2 Modify TextComposition::mCompositionStartOffset after every composition event dispatch

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/59144/diff/1-2/
Comment on attachment 8762466 [details]
Bug 1275914 part.3 TextComposition should use IMEContentObserver or ContentEventHandler directly when it queries the selection

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/59146/diff/1-2/
Comment on attachment 8762467 [details]
Bug 1275914 part.4 Add a method to retrieve focused content's selection controller to nsIPresShell

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/59148/diff/1-2/
Comment on attachment 8762468 [details]
Bug 1275914 part.5 Support special selections at handling eQuerySelectedText

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/59150/diff/1-2/
Comment on attachment 8762469 [details]
Bug 1275914 part.6 ContentEventHandler::OnQuerySelectedText() shouldn't refer anchor and focus of selection if there are 2 or more selection ranges

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/59152/diff/1-2/
Comment on attachment 8762470 [details]
Bug 1275914 part.7 Add automated tests to query IME selections

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/59154/diff/1-2/
Comment on attachment 8762471 [details]
Bug 1275914 part.8 TextComposition should always update its composition start offset during composing

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/59156/diff/1-2/
Sorry for the spam, there was debug code in part.7.
Comment on attachment 8762464 [details]
Bug 1275914 part.1 TextComposition should store target clause offset from start of the composition instead of start of the editor

https://reviewboard.mozilla.org/r/59142/#review56428
Attachment #8762464 - Flags: review?(bugs) → review+
Attachment #8762465 - Flags: review?(bugs) → review+
Comment on attachment 8762465 [details]
Bug 1275914 part.2 Modify TextComposition::mCompositionStartOffset after every composition event dispatch

https://reviewboard.mozilla.org/r/59144/#review56430

(just wondering what happens if event listener changes the text of some input field randomly even when the composition string is empty.)
Comment on attachment 8762466 [details]
Bug 1275914 part.3 TextComposition should use IMEContentObserver or ContentEventHandler directly when it queries the selection

https://reviewboard.mozilla.org/r/59146/#review56432
Attachment #8762466 - Flags: review?(bugs) → review+
Attachment #8762467 - Flags: review?(bugs) → review+
Comment on attachment 8762467 [details]
Bug 1275914 part.4 Add a method to retrieve focused content's selection controller to nsIPresShell

https://reviewboard.mozilla.org/r/59148/#review56436
Attachment #8762469 - Flags: review?(bugs) → review+
Comment on attachment 8762469 [details]
Bug 1275914 part.6 ContentEventHandler::OnQuerySelectedText() shouldn't refer anchor and focus of selection if there are 2 or more selection ranges

https://reviewboard.mozilla.org/r/59152/#review56440

A bit unclear why this... but I assume other patches explain why.
https://reviewboard.mozilla.org/r/59144/#review56430

If web developers use <input>.value or <textarea>.value, we request to commit composition to IME. I'm not sure what happens if DOM tree is modified in contenteditable editor.
https://reviewboard.mozilla.org/r/59152/#review56440

I found this bug with the automated tests in part.7.

If I add two same type clauses to composition string, even though mFirestSelectedRange is the first range, but the result (mReply.mString) is the last range of same selection type since focus node and anchor information of Selection is for the last range.
Comment on attachment 8762471 [details]
Bug 1275914 part.8 TextComposition should always update its composition start offset during composing

https://reviewboard.mozilla.org/r/59156/#review56668

::: widget/TextRange.h:301
(Diff revision 2)
>    }
> +
> +  const TextRange* GetFirstClause() const
> +  {
> +    for (const TextRange& range : *this) {
> +      if (range.IsClause() && !range.mStartOffset) {

This could use a comment why we check !range.mStartOffset.
Attachment #8762471 - Flags: review?(bugs) → review+
Comment on attachment 8762470 [details]
Bug 1275914 part.7 Add automated tests to query IME selections

https://reviewboard.mozilla.org/r/59154/#review56670

::: dom/interfaces/base/nsIDOMWindowUtils.idl:1091
(Diff revision 2)
>    const unsigned long QUERY_CONTENT_FLAG_USE_XP_LINE_BREAK     = 0x0001;
>  
>    /**
> +   * sendQueryContentEvent()'s aAdditionalFlags may have one of following
> +   * flags when aType is QUERY_SELECTED_TEXT.  If one of them is set,
> +   * the result is first range of the selection type.  See also

the first
Attachment #8762470 - Flags: review?(bugs) → review+
Comment on attachment 8762468 [details]
Bug 1275914 part.5 Support special selections at handling eQuerySelectedText

https://reviewboard.mozilla.org/r/59150/#review56674

Since this stuff is a bit complicated, I'd like to see those possibly minor issues fixed and then look at the new patch again.

::: dom/events/ContentEventHandler.cpp:135
(Diff revision 2)
>  {
> -  if (mSelection) {
> -    return NS_OK;
> +  RefPtr<Selection> selection = mSelection;
> +  if (mSelection->Type() != SelectionType::eNormal) {
> +    nsCOMPtr<nsISelection> iselection;
> +    nsresult rv =
> +      aSelectionController->GetSelection(

Why are we always using nsISelectionController::SELECTION_NORMAL here?
Since InitRootContent is called just once, could this code be moved to that place where the method is called?

::: dom/events/ContentEventHandler.cpp:250
(Diff revision 2)
> +    MOZ_ASSERT(!mFirstSelectedRange);
> +    return NS_OK;
> +  }
> +
> +  // But otherwise, we need to assume that there is a selection range at
> +  // beginning of the root content if aSelectionType is eNormal.

the beginning

::: dom/events/IMEContentObserver.cpp:711
(Diff revision 2)
>    // not sending selection change notification to IME, don't use the cached
>    // value.  Note that don't update selection cache here since if you update
>    // selection cache here, IMENotificationSender won't notify IME of selection
>    // change because it looks like that the selection isn't actually changed.
>    if (aEvent->mMessage == eQuerySelectedText && aEvent->mUseNativeLineBreak &&
> +      aEvent->mInput.mSelectionType == SelectionType::eNormal &&

Please explain this change.
Attachment #8762468 - Flags: review?(bugs) → review-
https://reviewboard.mozilla.org/r/59150/#review56674

> Please explain this change.

Because IMEContentObserver caches normal selection. Therefore, if the query is for retrieving a special selection, the cache isn't useful.
Could you add a comment there in the code that since we cache only normal selection...
Comment on attachment 8762464 [details]
Bug 1275914 part.1 TextComposition should store target clause offset from start of the composition instead of start of the editor

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/59142/diff/2-3/
Attachment #8762468 - Flags: review- → review?(bugs)
Comment on attachment 8762465 [details]
Bug 1275914 part.2 Modify TextComposition::mCompositionStartOffset after every composition event dispatch

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/59144/diff/2-3/
Comment on attachment 8762466 [details]
Bug 1275914 part.3 TextComposition should use IMEContentObserver or ContentEventHandler directly when it queries the selection

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/59146/diff/2-3/
Comment on attachment 8762467 [details]
Bug 1275914 part.4 Add a method to retrieve focused content's selection controller to nsIPresShell

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/59148/diff/2-3/
Comment on attachment 8762468 [details]
Bug 1275914 part.5 Support special selections at handling eQuerySelectedText

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/59150/diff/2-3/
Comment on attachment 8762469 [details]
Bug 1275914 part.6 ContentEventHandler::OnQuerySelectedText() shouldn't refer anchor and focus of selection if there are 2 or more selection ranges

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/59152/diff/2-3/
Comment on attachment 8762470 [details]
Bug 1275914 part.7 Add automated tests to query IME selections

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/59154/diff/2-3/
Comment on attachment 8762471 [details]
Bug 1275914 part.8 TextComposition should always update its composition start offset during composing

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/59156/diff/2-3/
https://reviewboard.mozilla.org/r/59150/#review56674

> Why are we always using nsISelectionController::SELECTION_NORMAL here?
> Since InitRootContent is called just once, could this code be moved to that place where the method is called?

I added the reason to the patch. The reason is, there is at least one range if it's normal selection but not so if it's one of special selections. If there are no selection ranges, we need to guess the editor root content with acestor limiter of the selection.

On the other hand, I'm not sure if your apporach of InitRootContent() is reasonable because my apporach is hidding always to use normal selection into the method but your apporach makes the method depend on the caller. Please check the new implementation with diff.
https://reviewboard.mozilla.org/r/59156/#review56668

> This could use a comment why we check !range.mStartOffset.

Added, if you want, please check it.
Comment on attachment 8762468 [details]
Bug 1275914 part.5 Support special selections at handling eQuerySelectedText

https://reviewboard.mozilla.org/r/59150/#review56794

::: dom/events/ContentEventHandler.cpp:134
(Diff revision 3)
> -  nsresult rv = InitBasic();
> -  NS_ENSURE_SUCCESS(rv, rv);
>  
> -  nsCOMPtr<nsISelection> sel;
> -  nsCopySupport::GetSelectionForCopy(mPresShell->GetDocument(),
> -                                     getter_AddRefs(sel));
> +  // Root content should be computed with normal selection because normal
> +  // selection is typically has at least one range but the other selections
> +  // not so.  If there is a range, we can compute its root easy, but if

s/we can compute its root easy/computing its root is easy/

::: dom/events/ContentEventHandler.cpp:198
(Diff revision 3)
> +  }
> +
> +  nsresult rv = InitBasic();
> +  NS_ENSURE_SUCCESS(rv, rv);
> +
> +  mSelection = nullptr;

Shouldn't we clear mSelection, mFirstSelectedRange and mRootContent already before InitBasic, just to be consistent that if InitCommon fails, mSelection is null. Though, maybe it doesn't matter too much, and would need to fix also other cases when error value is returned. So no need to fix I guess.

::: dom/events/ContentEventHandler.cpp:274
(Diff revision 3)
>  {
>    NS_ASSERTION(aEvent, "aEvent must not be null");
>  
> -  nsresult rv = InitCommon();
> +  // Note that we should ignore WidgetQueryContentEvent::Input::mSelectionType
> +  // if the event isn't eQuerySelectedText.
> +  SelectionType selectionType =

do we or can we assert that if message != eQuerySelectedText, aEvent->mInput.mSelectionType should be eNormal?

::: dom/events/IMEContentObserver.cpp:703
(Diff revision 3)
>  }
>  
>  nsresult
>  IMEContentObserver::HandleQueryContentEvent(WidgetQueryContentEvent* aEvent)
>  {
> -  // If the instance has cache, it should use the cached selection which was
> +  // If the instance has normal selection cache and the query event queries

Btw, hopefully we clear the cache whenever needed, even with all this new stuff.
Attachment #8762468 - Flags: review?(bugs) → review+
https://reviewboard.mozilla.org/r/59150/#review56794

> Shouldn't we clear mSelection, mFirstSelectedRange and mRootContent already before InitBasic, just to be consistent that if InitCommon fails, mSelection is null. Though, maybe it doesn't matter too much, and would need to fix also other cases when error value is returned. So no need to fix I guess.

Oh, right. As you said, this does not cause any problems with current m-c because nobody reuses ContentEventHandler instance. But for making the method's job clearer, I'll move them before InitBasic().

> do we or can we assert that if message != eQuerySelectedText, aEvent->mInput.mSelectionType should be eNormal?

Sure.

> Btw, hopefully we clear the cache whenever needed, even with all this new stuff.

I'm not sure what did you mean here. The cache is necessary to compair with new selection because IMEContentObserver shouldn't notify widget and IME of selection change when it's not actually changed. Therefore, the cache is never cleared after initialized with actual selection range.
Comment on attachment 8762464 [details]
Bug 1275914 part.1 TextComposition should store target clause offset from start of the composition instead of start of the editor

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/59142/diff/3-4/
Comment on attachment 8762465 [details]
Bug 1275914 part.2 Modify TextComposition::mCompositionStartOffset after every composition event dispatch

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/59144/diff/3-4/
Comment on attachment 8762466 [details]
Bug 1275914 part.3 TextComposition should use IMEContentObserver or ContentEventHandler directly when it queries the selection

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/59146/diff/3-4/
Comment on attachment 8762467 [details]
Bug 1275914 part.4 Add a method to retrieve focused content's selection controller to nsIPresShell

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/59148/diff/3-4/
Comment on attachment 8762468 [details]
Bug 1275914 part.5 Support special selections at handling eQuerySelectedText

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/59150/diff/3-4/
Comment on attachment 8762469 [details]
Bug 1275914 part.6 ContentEventHandler::OnQuerySelectedText() shouldn't refer anchor and focus of selection if there are 2 or more selection ranges

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/59152/diff/3-4/
Comment on attachment 8762470 [details]
Bug 1275914 part.7 Add automated tests to query IME selections

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/59154/diff/3-4/
Comment on attachment 8762471 [details]
Bug 1275914 part.8 TextComposition should always update its composition start offset during composing

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/59156/diff/3-4/
Pushed by masayuki@d-toybox.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/b202c72df4f1
part.1 TextComposition should store target clause offset from start of the composition instead of start of the editor r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/a6edf7da37d3
part.2 Modify TextComposition::mCompositionStartOffset after every composition event dispatch r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/dd927e5e00a2
part.3 TextComposition should use IMEContentObserver or ContentEventHandler directly when it queries the selection r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/c3d1a45c8796
part.4 Add a method to retrieve focused content's selection controller to nsIPresShell r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/76c08df53e1a
part.5 Support special selections at handling eQuerySelectedText r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/68e44df1e61e
part.6 ContentEventHandler::OnQuerySelectedText() shouldn't refer anchor and focus of selection if there are 2 or more selection ranges r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/d758ecd88650
part.7 Add automated tests to query IME selections r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/0aeee92e288e
part.8 TextComposition should always update its composition start offset during composing r=smaug
Component: Event Handling → User events and focus handling
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: