1389551 - PlaceholderTransactions do not drop the selection state properly for IME composition

Status: NEW

(Core :: DOM: Editor, enhancement, P3)

Description

[(no longer active)]

While debugging the last test failure for bug 1386480, I have discovered a bug which I think we have had since a very long time ago.  I can't say for sure what the implications are unfortunately.

The issue is that this code <https://searchfox.org/mozilla-central/rev/4b79f3b23aebb4080ea85e94351fd4046116a957/editor/libeditor/EditorBase.cpp#1015> will never get executed if we end up handing over the ownership of our selection state to the PlaceholderTransaction object here <https://searchfox.org/mozilla-central/rev/4b79f3b23aebb4080ea85e94351fd4046116a957/editor/libeditor/EditorBase.cpp#679>, since mSelState will always be null/nothing by the time EndPlaceHolderTransaction() is executed.

Comment 1

[(no longer active)]

Attachment: Drop the selection state for IME composition placeholder transactions properly

Comment 2

[Masayuki Nakano [:masayuki] (he/him)(JST, +0900)]

Comment on attachment 8896356 [details] [diff] [review]
Drop the selection state for IME composition placeholder transactions properly

>+void
>+EditorBase::MaybeDropSelectionStateForIMEComposition(nsIAtom* aPlaceholderName)

I don't like this name because this explains what this method does too closely. For example, other developer may become to need to do same thing with other reasons. Then, they need to rename this method or create other method.

I think that MaybeDropSelectionState() is enough.

diff --git a/editor/libeditor/EditorBase.h b/editor/libeditor/EditorBase.h
>index a927e9d5514f..73c8454c5786 100644
>--- a/editor/libeditor/EditorBase.h
>+++ b/editor/libeditor/EditorBase.h
>@@ -340,16 +340,24 @@ public:
>    */
>   virtual nsresult BeginIMEComposition(WidgetCompositionEvent* aEvent);
>   virtual nsresult UpdateIMEComposition(
>                      WidgetCompositionEvent* aCompositionChangeEvet) = 0;
>   void EndIMEComposition();
> 
>   void SwitchTextDirectionTo(uint32_t aDirection);
> 
>+  /**
>+   * This method can be called when we're about to reset our selection
>+   * state for processing an IME composition, to ensure that we properly
>+   * drop our selection state which may be registered when beginning a
>+   * placeholder transaction.
>+   */

On the other hand, explaining with this comment is fine since other developers can understand what this method does without checking implementation.

>@@ -23,16 +23,17 @@ PlaceholderTransaction::PlaceholderTransaction(
>   : mAbsorb(true)
>   , mForwarding(nullptr)
>   , mCompositionTransaction(nullptr)
>   , mCommitted(false)
>   , mEditorBase(&aEditorBase)
> {
>   // Make sure to move aSelState into a local variable to null out the original
>   // Maybe<SelectionState> variable.
>+  mEditorBase->MaybeDropSelectionStateForIMEComposition(aName);
>   Maybe<SelectionState> selState(Move(aSelState));
>   mStartSel = *selState;

The above comment explains not MaybeDropSelectionStateForIMEComposition() call. So, please move new line to above the comment.

Comment 3

[(no longer active)]

This actually doesn't pass tests unfortunately https://treeherder.mozilla.org/#/jobs?repo=try&revision=5cd6cda2c4bb0932d79a6c6622eb5aa4784ee86c

I won't have time to figure out why in the near future.  I suspect there is a bug related to our IME code at play here but I'm not familiar with that part.

Comment 4

[BugBot [:suhaib / :marco/ :calixte]]

There's a r+ patch which didn't land and no activity in this bug for 2 weeks.
:Ehsan, could you have a look please?

Comment 5

[(no longer active)]

No time unfortunately. :-(