555642 - [IMM32][TSF] Shouldn't paint caret during composition when the caret is in selected clause

Status: RESOLVED FIXED

(Core :: DOM: Editor, defect)

Description

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

This is a regression of bug 553975. Now, the caret is painted over IME selection. However, this looks bad on Windows build.

If the caret is in selected clause of composition, we shouldn't paint caret even if IME has caret position on Windows. However, caret should be always painted on Linux, it's the native behavior, so, we cannot fix this bug simply.

I think that if a widget doesn't need caret, it shouldn't set NS_TEXTRANGE_CARETPOSITION range of nsTextEvent. But even if I do so, the caret is displayed at the end of composition string. So, we need to change the caret or editor's behavior too.

Comment 1

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=d267b19b8f67

Comment 2

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=6c710b3fc209

Comment 3

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=ef780ad99a33

Comment 4

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=3c2493c4272d

Comment 5

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=47ff0af41582

Comment 6

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

Attachment: part.1 nsCaret should have a way to override the caret visible state for hiding caret temporarily and nsEditor should hide caret if composition string doesn't have caret information

Smaug, if you are not a good person to review this patch, let me know that. I'll ask roc or somebody.

This patch allows nsEditor to hide caret temporarily. That feature is necessary for rendering composition string on Windows. On Windows, IME may not insert caret into the composition string. But our editor always inserts normal selection which causes showing caret always.

Unfortunately, nsCaret::SetVisible() is called very stateful. It's called when somebody gets focus, a window is activated or inactivated, etc. So, editor cannot hide caret with that (restoring proper visible state is difficult because it may be changed after editor hides the caret).

For solving it, this patch adds methods to force to hide the caret or cancel that with call count. That allows multiple modules can hide the caret. However, it's difficult IMETextTxn to control the count properly. Therefore, nsEditor does it with a bool member, mHidingCaret. nsEditor should guarantee to cancel it when composition is committed or being destroyed. Otherwise, caret never shows up.

Comment 7

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

Attachment: part.2 IME handlers on Windows shouldn't append caret range if the caret is in the target clause which doesn't have specific style

I think that caret in composition string shouldn't be shown when:

1: IME doesn't specify caret position (IMM)
2: Caret is in the selected range (IMM/TSF)
3: But caret should be in the selected range if the range has custom style (TSF)

I believe that #1 is IMM's expected behavior. #2 is the old behavior. On Windows, caret shouldn't be shown with normal selection (for native LnF). And our selected clause is rendered as normal selection in IMM mode or TSF mode if TIP doesn't specify the clause rendering style (I don't know such TIP). With WordPad, caret is always visible. So, #3 must be new native LnF on Windows.

If we don't append caret range, nsEditor hides the caret with the previous patch.

Comment 8

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

Comment on attachment 8647442 [details] [diff] [review]
part.1 nsCaret should have a way to override the caret visible state for hiding caret temporarily and nsEditor should hide caret if composition string doesn't have caret information

Oops, smaug is on vacation. Roc, could you review this? The detail is written in the comment 6.

FYI: Ehsan likes NS_ENSURE_TRUE(), so, I used it only in nsEditor.

Comment 9

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 8647442 [details] [diff] [review]
part.1 nsCaret should have a way to override the caret visible state for hiding caret temporarily and nsEditor should hide caret if composition string doesn't have caret information

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

Split the nsCaret changes into their own patch.

::: layout/base/nsCaret.cpp
@@ +641,5 @@
>  void nsCaret::ResetBlinking()
>  {
>    mIsBlinkOn = true;
>  
> +  if (mReadOnly || !IsVisibleInternal()) {

I don't think you need this here. It's simpler if we get rid of IsVisibleInternal and just check mHideCount from nsCaret::IsVisible.

::: layout/base/nsCaret.h
@@ +85,5 @@
> +     * Especially, in the latter case, it's too difficult to decide if the
> +     * caret should be actually visible or not because caret visible state
> +     * is set from a lot of event handlers.  So, it's very stateful.
> +     */
> +    void ForceToHide();

Call this "AddForceHide()".

@@ +91,5 @@
> +     * CancelForceToHide() decreases mForceHideCount if it's over 0.
> +     * If the value becomes 0, this may show the caret if SetVisible(true)
> +     * has been called.
> +     */
> +    void CancelForceToHide();

Call this "RemoveForceHide()".

@@ +230,5 @@
>      /**
> +     * mForceHideCount is not 0, it means that somebody doesn't want the caret
> +     * to be visible.  See ForceToHide() and CancelForceToHide().
> +     */
> +    uint32_t              mForceHideCount;

Call this "mHideCount".

Comment 10

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=114b9337521e

Comment 11

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

Attachment: part.1 nsCaret should have a way to override the caret visible state for hiding caret temporarily and nsEditor should hide caret if composition string doesn't have caret information

Thank you, roc.

Comment 12

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

url:        https://hg.mozilla.org/integration/mozilla-inbound/rev/f84b3c91aeceeeefd94cdf13cd5955d89342d13d
changeset:  f84b3c91aeceeeefd94cdf13cd5955d89342d13d
user:       Masayuki Nakano <[e-mail]>
date:       Mon Aug 17 20:58:38 2015 +0900
description:
Bug 555642 part.1 nsCaret should have a way to override the caret visible state for hiding caret temporarily and nsEditor should hide caret if composition string doesn't have caret information r=roc

url:        https://hg.mozilla.org/integration/mozilla-inbound/rev/77f71a695592baff88cc3084dfd41ca3f2830bf7
changeset:  77f71a695592baff88cc3084dfd41ca3f2830bf7
user:       Masayuki Nakano <[e-mail]>
date:       Mon Aug 17 20:58:38 2015 +0900
description:
Bug 555642 part.2 IME handlers on Windows shouldn't append caret range if the caret is in the target clause which doesn't have specific style r=m_kato

Comment 13

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/f84b3c91aece
https://hg.mozilla.org/mozilla-central/rev/77f71a695592