Closed
Bug 1082486
Opened 10 years ago
Closed 10 years ago
[Touch Caret] Make touch caret hide when its position is not in the area of the scrollport
Categories
(Core :: Layout, defect, P2)
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 |
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.
Assignee | ||
Comment 1•10 years ago
|
||
Assignee | ||
Comment 2•10 years ago
|
||
Also, check mVisible before calling UpdatePosition() in SyncVisibilityWithCaret().
Attachment #8504630 -
Flags: review?(roc)
Assignee | ||
Comment 3•10 years ago
|
||
PresShell::GetCanvasFrame() already returns nsCanvasFrame*, and it's needed to access nsCanvasFrame's method in a later patch.
Attachment #8504631 -
Flags: review?(roc)
Assignee | ||
Comment 4•10 years ago
|
||
TouchCaretPosition() is needed in a later patch.
Attachment #8504632 -
Flags: review?(roc)
Assignee | ||
Comment 5•10 years ago
|
||
Attachment #8504633 -
Flags: review?(roc)
Assignee | ||
Comment 6•10 years ago
|
||
I think after resolving this bug, we can solve bug 1060193 then.
Attachment #8504630 -
Flags: review?(roc) → review+
Attachment #8504631 -
Flags: review?(roc) → review+
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-
Assignee | ||
Comment 9•10 years ago
|
||
Attach a test html with a nested scrollframe.
Assignee | ||
Comment 10•10 years ago
|
||
Update test_touchcaret_scrollframe.html
Attachment #8505271 -
Attachment is obsolete: true
Assignee | ||
Comment 11•10 years ago
|
||
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
Assignee | ||
Comment 12•10 years ago
|
||
Clamping to only the nearest ancestor scrollframe might not work in nested scrollframe. See scrollframe2.png.
Assignee | ||
Comment 13•10 years ago
|
||
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)
Assignee | ||
Comment 14•10 years ago
|
||
s/TouchCaretPosition/GetTouchCaretPosition() per comment 7.
Attachment #8504632 -
Attachment is obsolete: true
Attachment #8505388 -
Flags: review+
Assignee | ||
Comment 15•10 years ago
|
||
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)
Assignee | ||
Comment 16•10 years ago
|
||
Try result: Got an orange R11 on B2G ICS Emulator Opt. https://tbpl.mozilla.org/?tree=Try&rev=d11be8084309
Updated•10 years ago
|
Priority: -- → P2
Assignee | ||
Comment 17•10 years ago
|
||
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-
Attachment #8506086 -
Flags: review?(roc) → review+
Assignee | ||
Comment 19•10 years ago
|
||
(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().
Assignee | ||
Comment 20•10 years ago
|
||
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-
Assignee | ||
Comment 22•10 years ago
|
||
(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
Assignee | ||
Comment 23•10 years ago
|
||
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)
Assignee | ||
Updated•10 years ago
|
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
Assignee | ||
Comment 24•10 years ago
|
||
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)
Assignee | ||
Updated•10 years ago
|
Flags: needinfo?(roc)
Attachment #8506644 -
Flags: review?(roc) → review+
Attachment #8507497 -
Flags: review?(roc) → review+
Assignee | ||
Updated•10 years ago
|
Keywords: checkin-needed
Comment 25•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/c39e875e01f3 https://hg.mozilla.org/integration/mozilla-inbound/rev/a1c2b6323e6e https://hg.mozilla.org/integration/mozilla-inbound/rev/b66bcf105809 https://hg.mozilla.org/integration/mozilla-inbound/rev/2ec24bc9a5dd https://hg.mozilla.org/integration/mozilla-inbound/rev/c76f0ce30414 https://hg.mozilla.org/integration/mozilla-inbound/rev/0ecbecdbc21f
Keywords: checkin-needed
Comment 26•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/c39e875e01f3 https://hg.mozilla.org/mozilla-central/rev/a1c2b6323e6e https://hg.mozilla.org/mozilla-central/rev/b66bcf105809 https://hg.mozilla.org/mozilla-central/rev/2ec24bc9a5dd https://hg.mozilla.org/mozilla-central/rev/c76f0ce30414 https://hg.mozilla.org/mozilla-central/rev/0ecbecdbc21f
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
You need to log in
before you can comment on or make changes to this bug.
Description
•