1404854 - Cannot scroll page by dragging text selection handles to the edge of the screen

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: Text Selection, defect, P2)

Description

[Jan Henning [:JanH]]

Scrolling up is barely possible, although not very pleasant, while scrolling down by dragging the selection handle doesn't seem to work at all.

Comment 1

[James Willcox (:snorp) ([e-mail]) (he/him)]

Kats, botond, what do you think about this? Feels like APZ needs to be aware that you're dragging a selection handle around then scroll for you.

Comment 2

[Kartikaya Gupta (email:[e-mail])]

I think this is more in the selection code than APZ. Even on desktop if you do a mouse-drag-selection, the page will scroll if you select stuff beyond the edge of the visible page, and APZ has nothing to do with that. It's handled by the selection code.

Comment 3

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

AccessibleCaretManager::DragCaretInternal() has some code [1] to scroll the page, but apparently, it didn't work properly. I'll look into it.

[1] https://dxr.mozilla.org/mozilla-central/rev/19b32a138d08f73961df878a29de6f0aad441683/layout/base/AccessibleCaretManager.cpp#1283-1288

Comment 4

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

Attachment: Bug 1404854 Part 1 - Move null pointers checking from DragCaretInternal() to DragCaret().

We don't need to check GetFrameSelection() because valid GetSelection()
implies valid GetFrameSelection().

Review commit: https://reviewboard.mozilla.org/r/187170/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/187170/

Comment 5

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

Attachment: Bug 1404854 Part 2 - Move adding mOffsetYToCaretLogicalPosition into DragCaretInternal().

Review commit: https://reviewboard.mozilla.org/r/187172/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/187172/

Comment 6

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

Attachment: Bug 1404854 Part 3 - Fix page cannot be scrolled automatically when dragging caret.

The issue is: when we hit some failure conditions in DragCaretInternal()
such as the frame is not selectable, or no frame is under the point, etc.,
we'll early return so that the auto scroll code is not being executed.

The logic in StartSelectionAutoScrollTimer() is similar to how
nsFrame::HandleDrag() handles the auto scrolling.

Review commit: https://reviewboard.mozilla.org/r/187174/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/187174/

Comment 7

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

Comment on attachment 8915917 [details]
Bug 1404854 Part 1 - Move null pointers checking from DragCaretInternal() to DragCaret().

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/187170/diff/1-2/

Comment 8

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

Comment on attachment 8915918 [details]
Bug 1404854 Part 2 - Move adding mOffsetYToCaretLogicalPosition into DragCaretInternal().

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/187172/diff/1-2/

Comment 9

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

Comment on attachment 8915919 [details]
Bug 1404854 Part 3 - Fix page cannot be scrolled automatically when dragging caret.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/187174/diff/1-2/

Comment 10

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

Comment on attachment 8915917 [details]
Bug 1404854 Part 1 - Move null pointers checking from DragCaretInternal() to DragCaret().

https://reviewboard.mozilla.org/r/187170/#review193410

Comment 11

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

Comment on attachment 8915918 [details]
Bug 1404854 Part 2 - Move adding mOffsetYToCaretLogicalPosition into DragCaretInternal().

https://reviewboard.mozilla.org/r/187172/#review193414

Comment 12

[Morris Tseng [:mtseng] [:Morris] (Inactive)]

Comment on attachment 8915919 [details]
Bug 1404854 Part 3 - Fix page cannot be scrolled automatically when dragging caret.

https://reviewboard.mozilla.org/r/187174/#review193416

Comment 13

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

Morris, thanks for the review!

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/1e5e53f0cfa8
Part 1 - Move null pointers checking from DragCaretInternal() to DragCaret(). r=mtseng
https://hg.mozilla.org/integration/autoland/rev/d5fc6f59adce
Part 2 - Move adding mOffsetYToCaretLogicalPosition into DragCaretInternal(). r=mtseng
https://hg.mozilla.org/integration/autoland/rev/cc0c99d812fd
Part 3 - Fix page cannot be scrolled automatically when dragging caret. r=mtseng

Comment 15

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

https://hg.mozilla.org/mozilla-central/rev/1e5e53f0cfa8
https://hg.mozilla.org/mozilla-central/rev/d5fc6f59adce
https://hg.mozilla.org/mozilla-central/rev/cc0c99d812fd

Comment 16

[Ryan VanderMeulen [:RyanVM]]

We've shipped this bug in 2 releases already AFAICT. Given that we're trying to limit uplifts for 57 to bugs that would be potentially driving a dot release, I'm going to mark this as wontfix for 57. Feel free to set it back to affected and request Beta approval if you feel strongly about it.

Comment 17

[Ting-Yu Lin [:TYLin] (PDT, UTC-7)]

This is a long-standing issue, so I'm OK to let this ride the train.