Closed Bug 1082486 Opened 5 years ago Closed 5 years ago

[Touch Caret] Make touch caret hide when its position is not in the area of the scrollport

Categories

(Core :: Layout, defect, P2)

x86
macOS
defect

Tracking

()

RESOLVED FIXED
mozilla36

People

(Reporter: TYLin, Assigned: TYLin)

References

Details

Attachments

(11 files, 5 obsolete files)

175 bytes, text/html
Details
31.44 KB, image/png
Details
1.24 KB, patch
roc
: review+
Details | Diff | Splinter Review
1.91 KB, patch
roc
: review+
Details | Diff | Splinter Review
376 bytes, text/html
Details
266.02 KB, image/png
Details
266.68 KB, image/png
Details
3.33 KB, patch
TYLin
: review+
Details | Diff | Splinter Review
2.06 KB, patch
roc
: review+
Details | Diff | Splinter Review
3.96 KB, patch
roc
: review+
Details | Diff | Splinter Review
4.53 KB, patch
roc
: review+
Details | Diff | Splinter Review
Attached file touchcaret_seen.html
Steps to reproduce:
1) Set touchcaret.enabled to true if testing on browser.
2) Open the attached touchcaret_seen.html, which has an <input> with "position: absolute; top: -10em".

Result:
The touch caret is seen. (It's been clamped to scroll frame.)

Expected:
The touch caret should be hidden.
Also, check mVisible before calling UpdatePosition() in
SyncVisibilityWithCaret().
Attachment #8504630 - Flags: review?(roc)
PresShell::GetCanvasFrame() already returns nsCanvasFrame*, and it's
needed to access nsCanvasFrame's method in a later patch.
Attachment #8504631 - Flags: review?(roc)
TouchCaretPosition() is needed in a later patch.
Attachment #8504632 - Flags: review?(roc)
I think after resolving this bug, we can solve bug 1060193 then.
Comment on attachment 8504632 [details] [diff] [review]
Part 3 - Refactor UpdatePosition().

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

::: layout/base/TouchCaret.h
@@ +114,5 @@
>    /**
> +   * Retrieve the position of the touch caret.
> +   * The returned point is relative to the canvas frame.
> +   */
> +  nsPoint TouchCaretPosition();

Call this GetTouchCaretPosition, because TouchCaretPosition sounds like "Touch" could be a verb.
Attachment #8504632 - Flags: review?(roc) → review+
Comment on attachment 8504633 [details] [diff] [review]
Part 4 - Do not show touch caret if its position not within canvas area.

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

I don't understand why we want to clamp to the scrollport in some cases but not this case. Can you explain that in more detail? Should we only be clamping to the nearest ancestor scrollframe?
Attachment #8504633 - Flags: review?(roc) → review-
Attached file test_touchcaret_scrollframe.html (obsolete) —
Attach a test html with a nested scrollframe.
Update test_touchcaret_scrollframe.html
Attachment #8505271 - Attachment is obsolete: true
Attached image scrollframe1.png
I think clamping the touch caret position to nearest ancestor scrollframe is to solve this displaying issue: when the text is beneath the boundary of a scrollframe, we want touch caret to attach to the bottom boundary of the frame rather than floating beneath the frame.

For example (test_touchcaret_scrollframe.html, scrollframe1.png): the caret is between 'y' and 'z', and the touch caret attaches to the bottom boundary.

However clamping the touch caret postion to the scrollframe causes another issue that touch caret always shows within the top level scrollframe even if the element which caret is on cannot be seen by the user such as 

<input style="position: absolute; top: -100px; left: -100px;">

Example 1: bug 1060193, which has an <input> with CSS [1].
Example 2: bug 1080465.

Part 4 should fix example 1.

[1] https://github.com/mozilla-b2g/gaia/blob/master/apps/settings/style/screen_lock.css#L109
Attached image scrollframe2.png
Clamping to only the nearest ancestor scrollframe might not work in nested scrollframe. See scrollframe2.png.
I have an new idea. Rather than doing part 4, we can extended the height of visualRect by focusRect.height in [1], and check if this extended visualRect contains the touch caret's position. If yes, we clamp touch caret's to the scrollframe as usual. Otherwise, we hide touch caret since caret doesn't display in this visualRect either.

[1] http://hg.mozilla.org/mozilla-central/file/eb1b8ecbefde/layout/base/TouchCaret.cpp#l446
Flags: needinfo?(roc)
s/TouchCaretPosition/GetTouchCaretPosition() per comment 7.
Attachment #8504632 - Attachment is obsolete: true
Attachment #8505388 - Flags: review+
This patch check whether touch caret is in or is near scroll frame. If
yes, its position will be clamp to scroll frame as before. Otherwise, we
hide it since the nsCaret will not be seen in this case.
Attachment #8504633 - Attachment is obsolete: true
Attachment #8505389 - Flags: review?(roc)
Try result: Got an orange R11 on B2G ICS Emulator Opt.
https://tbpl.mozilla.org/?tree=Try&rev=d11be8084309
Priority: -- → P2
When loading an html, painting is suppressed in PresShell. Therefore the
rect of nsCaret or the rect of nearest ancestor scroll frames will not
be correct. Touch caret will hide incorrectly because it cannot get the
necessary rect to calculate its position.

I added a condition in IsDisplayable() to skip when painting is
suppressed. Touch caret should sync its visibility with nsCaret again
when painting is unsuppressed, and those rects needed by touch caret
should be ready then.

This patch fixed the test failure in comment 16.

Try result:
https://tbpl.mozilla.org/?tree=Try&rev=21a9be6e4c76
Attachment #8506086 - Flags: review?(roc)
Comment on attachment 8505389 [details] [diff] [review]
Part 4 - Check touch caret position is in or near scroll frame.

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

::: layout/base/TouchCaret.cpp
@@ +477,5 @@
> +
> +    // Consider the position close to scroll frame if the y-distance between the
> +    // position and the bottom edge of scrollPortRect is less than or equal to
> +    // caretRect's height.
> +    scrollPortRect.SetBottomEdge(scrollPortRect.YMost() + caretRect.height);

How about we just transform the regular caret rect to closestScrollFrame and check whether it intersects the scrollport?
Attachment #8505389 - Flags: review?(roc) → review-
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #18)
> How about we just transform the regular caret rect to closestScrollFrame and
> check whether it intersects the scrollport?

That should work as well. Then we should change IsPositionInScrollFrame(const nsPoint& aPosition) to something like IsCaretIntersectsScrollFrame().
Check whether nsCaret's rect intersects scrollport as suggested in comment 18.
Attachment #8505389 - Attachment is obsolete: true
Attachment #8506625 - Flags: review?(roc)
Comment on attachment 8506625 [details] [diff] [review]
Part 4 - Check whether nsCaret shows in the scroll frame. r=roc (v2)

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

::: layout/base/TouchCaret.cpp
@@ +481,5 @@
> +
> +    // Get next ancestor scroll frame.
> +    closestScrollFrame =
> +      nsLayoutUtils::GetClosestFrameOfType(closestScrollFrame->GetParent(),
> +                                           nsGkAtoms::scrollFrame);

What I meant was, we just check the nearest scrollframe only.

And we shouldn't need to transform to the canvasFrame. We can transform the caret rect to its nearest scrollframe, do the check there, then we're done.
Attachment #8506625 - Flags: review?(roc) → review-
(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #21)
> Comment on attachment 8506625 [details] [diff] [review]
> Part 4 - Check whether nsCaret shows in the scroll frame. r=roc (v2)
> 
> Review of attachment 8506625 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: layout/base/TouchCaret.cpp
> @@ +481,5 @@
> > +
> > +    // Get next ancestor scroll frame.
> > +    closestScrollFrame =
> > +      nsLayoutUtils::GetClosestFrameOfType(closestScrollFrame->GetParent(),
> > +                                           nsGkAtoms::scrollFrame);
> 
> What I meant was, we just check the nearest scrollframe only.
> 
> And we shouldn't need to transform to the canvasFrame. We can transform the
> caret rect to its nearest scrollframe, do the check there, then we're done.

I agree that no need to transform caret's rect to canvasFrame. Transform it to scrollframe should work.

However, we still need to check the ancestor scrollframe repeatedly. Consider the nested scroll frame case in [1], and the caret's rect is in the inner scrollframe. It's possible that the inner scrollframe is been scrolling out of the outer scrollframe. Thus the caret's rect does not intersect the outer scroll frame's rect. We should hide touch caret in this case.

[1] https://bugzilla.mozilla.org/attachment.cgi?id=8505274
Transform the caret rect to its nearest scrollframe rather than canvas frame.

Still checking the ancestor scrollframe repeatedly as explained in comment 22.
Attachment #8506625 - Attachment is obsolete: true
Attachment #8506644 - Flags: review?(roc)
Summary: [Touch Caret] Make touch caret hide when its position is not in canvas area → [Touch Caret] Make touch caret hide when its position is not in the area of the scrollport
Touch caret should not be show in the two test cases since caret does
not show in the scroll frame.

Try result:
https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=b093f7703209
Attachment #8507497 - Flags: review?(roc)
Flags: needinfo?(roc)
Duplicate of this bug: 1060193
Duplicate of this bug: 1072769
Duplicate of this bug: 1080465
You need to log in before you can comment on or make changes to this bug.