1072769 - [Touch Caret] Touch caret does not show when focusing an input already contains value

Status: RESOLVED DUPLICATE of bug 1082486

(Core :: Layout, defect)

Description

[Ting-Yu Lin [:TYLin] (PST, UTC-8)]

Attachment: test_touchcaret_focus.html

Run the attached test_touchcaret_focus.html to reproduce.

When the <input> already contains value, the touch caret does not show. However 
if the <input> is empty, touch caret shows.

Because the touch caret fails to get caret's focus rect when TouchCaret::SyncVisibilityWithCaret() is called in [1] when focusing the input, touch caret is hidden. Actually, the caret's rect will be available when TouchCaret::UpdatePositionIfNeeded() is called at [2] later.

[1] http://hg.mozilla.org/mozilla-central/file/5e704397529b/layout/base/nsPresShell.cpp#l2240
[2] http://hg.mozilla.org/mozilla-central/file/5e704397529b/layout/base/nsPresShell.cpp#l8724

Comment 1

[Ting-Yu Lin [:TYLin] (PST, UTC-8)]

Attachment: Part 1 - Add TouchCaret::GetCaretFocusFrame().

Comment 2

[Ting-Yu Lin [:TYLin] (PST, UTC-8)]

Attachment: Part 2 - Fix touch caret not show when focusing in js.

IsDisplayable() tests two ideas: whether the caret shows, and whether
the caret's position data is available. We should really separate the
two ideas by adding IsPositionAvailable().

When SyncVisibilityWithCaret() is called in
PresShell::SetCaretEnabled(), caret is visible, but its focus frame and
rect might be unavailable. We use mPendingVisible to track this status,
and get a chance to set touch caret visible later when
UpdatePositionIfNeeded() is called in PresShell::DidDoReflow().

Before fixing this bug, no touch caret will show in bug1072769.html. The
only difference between bug1072769.html and bug1072769-ref.html is the
editable <div> after <input>. The touch caret is expected to show in
both pages.

Comment 3

[Ting-Yu Lin [:TYLin] (PST, UTC-8)]

Attachment: Part 2 - Fix touch caret not show when focusing in js. (v2)

Comment 4

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

Comment on attachment 8495850 [details] [diff] [review]
Part 2 - Fix touch caret not show when focusing in js. (v2)

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

::: layout/base/TouchCaret.cpp
@@ +385,5 @@
> +    SetVisibility(false);
> +    return;
> +  }
> +
> +  if (mPendingVisible) {

Why do we need mPendingVisible? It seems to me we should be able to do the rest of this code unconditionally.

Comment 5

[Ting-Yu Lin [:TYLin] (PST, UTC-8)]

Comment on attachment 8495850 [details] [diff] [review]
Part 2 - Fix touch caret not show when focusing in js. (v2)

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

::: layout/base/TouchCaret.cpp
@@ +385,5 @@
> +    SetVisibility(false);
> +    return;
> +  }
> +
> +  if (mPendingVisible) {

We need mPendingVisible to track a situation that touch caret should be set to visible in SyncVisibilityWithCaret() when calling by PresShell::SetCaretEnabled, but touch caret fails to obtain caret's frame or rect. UpdatePositionIfNeeded() will be called in PresShell::DidDoReflow(), and the caret's frame and rect should be correct at this time. 

If we do the rest of this code unconditionally, it will be just SyncVisibilityWithCaret(). It'll align touch caret's visibility with caret after each reflow, and touch caret will be shown if caret is visible. It is not what we want.

BTW, I forgot to set mPendingVisible to false in the constructor. I'll upload a new patch for that.

Comment 6

[Ting-Yu Lin [:TYLin] (PST, UTC-8)]

Attachment: Part 2 - Fix touch caret not show when focusing in js.

Initialize mPendingVisible to false in TouchCaret's constructor.

Comment 7

[Ting-Yu Lin [:TYLin] (PST, UTC-8)]

Comment on attachment 8496709 [details] [diff] [review]
Part 2 - Fix touch caret not show when focusing in js.

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

Hi Ehsan, roc is on vacation now. Is it OK for you to help review this change?

Comment 8

[(no longer active)]

Comment on attachment 8496709 [details] [diff] [review]
Part 2 - Fix touch caret not show when focusing in js.

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

::: layout/base/TouchCaret.cpp
@@ +337,5 @@
>    // Update touch caret position and visibility.
>    // Hide touch caret while key event causes selection change.
> +  if (aReason == nsISelectionListener::NO_REASON) {
> +    TOUCHCARET_LOG("NO_REASON");
> +    // Do nothing with NO_REASON

Why do you ignore this?  That will break selection changes made through DOM APIs.

In fact, you should probably add a test which fails with your current patch and passes with this hunk reverted.  :)  You can use Seletion.modify to construct such a test.

@@ +364,5 @@
> +    mPendingVisible = true;
> +    return;
> +  }
> +
> +  SetVisibility(true);

Shouldn't you set mPendingVisible to false here too?  Or MOZ_ASSERT if it's always false, if that can be guaranteed from UpdatePositionIfNeeded?

But I don't think that's guaranteed...

::: layout/base/tests/bug1072769-ref.html
@@ +14,5 @@
> +    document.getElementById('i').focus();
> +    setTimeout(function() {
> +      /* Wait for a while until touch caret's position is stabilized.*/
> +      document.documentElement.removeAttribute('class');
> +    }, 100);

Here too.

@@ +21,5 @@
> +</head>
> +<body onload="onloadHandler();">
> +  <a target="_blank" href="https://bugzilla.mozilla.org/show_bug.cgi?id=1072769">Mozilla Bug 1072769</a>
> +  <input id='i' value="abc">
> +  <div contenteditable="true"></div>

This div seems to be the only difference between the test and its reference, so I'm not sure if I understand what this test is trying to do.  Shouldn't your reference focus an empty input element, and then set its content to "abc" or something (at least if I'm reading comment 0 correctly...)

::: layout/base/tests/bug1072769.html
@@ +14,5 @@
> +    document.getElementById('i').focus();
> +    setTimeout(function() {
> +      /* Wait for a while until touch caret's position is stabilized.*/
> +      document.documentElement.removeAttribute('class');
> +    }, 100);

Hmm, can't we wait for a mozafterpaint event here?

Comment 9

[Ting-Yu Lin [:TYLin] (PST, UTC-8)]

Comment on attachment 8496709 [details] [diff] [review]
Part 2 - Fix touch caret not show when focusing in js.

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

When focusing on an <input> already contains value in javascript, touch caret often failed to get caret's focus frame and rect because nsTextFrame is dirty [1]. Therefore, even if we enable touch caret's visibility in PresShell::SetCaretEnabled(), touch caret will be hidden later by other selection reasons. I've tried to skip the early return in [1] as the patch in bug 310227 [2] did, but this generates other asserts. I haven't found a proper solution yet.

[1] http://hg.mozilla.org/mozilla-central/annotate/0ed32d9a42d6/layout/generic/nsTextFrame.cpp#l6496
[2] https://bugzilla.mozilla.org/page.cgi?id=splinter.html&bug=310227&attachment=197968

::: layout/base/TouchCaret.cpp
@@ +337,5 @@
>    // Update touch caret position and visibility.
>    // Hide touch caret while key event causes selection change.
> +  if (aReason == nsISelectionListener::NO_REASON) {
> +    TOUCHCARET_LOG("NO_REASON");
> +    // Do nothing with NO_REASON

After adding mPendingVisible, touch caret will be appear then disappear when typing characters if NO_REASON is not ignored. It made me think that adding mPendingVisible might introduce more side effects.

Comment 10

[(no longer active)]

If the text frame is dirty, shouldn't we be flushing layout somewhere up the stack before we try to show the touch caret?

Comment 11

[Ting-Yu Lin [:TYLin] (PST, UTC-8)]

Comment on attachment 8495815 [details] [diff] [review]
Part 1 - Add TouchCaret::GetCaretFocusFrame().

Part 1 as an independent code improvement has been landed in bug 1078991. I'll mark it obsolete.

Comment 12

[Ting-Yu Lin [:TYLin] (PST, UTC-8)]

The solution in bug 1082486 resolved this issue, too.