1307100 - Floating toolbar does not hide when the selection highlight is scrolled out of viewport

Status: VERIFIED FIXED

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

Description

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

Steps to reproduce:
1. Open an arbitrary web page like https://en.m.wikipedia.org/wiki/Main_Page#
2. Long tap to make a selection.
3. Scroll the entire selection up until it's out of viewport.

Expected result:
The floating toolbar hides.

Actual result:
The floating toolbar still shows.

Comment 1

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

I can reproduce this on nightly 52 and aurora 51, but not on release 49.

Comment 2

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

mozregression finds bug 1266369.

Comment 3

[Sebastian Kaspari (:sebastian; :pocmo)]

We are adding a constant value (20dp) to the content rect to make it not overlap the handles of the text selection. However if the rect is scrolled out of the screen then it has a height of 0 and that's what we detect in order to hide the toolbar. After the patch from bug 1266369 the rect is always at least 20dp and we never hide the toolbar. I guess a simple fix would be to just add the 20dp if the height is > 0.

Comment 4

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

Attachment: Bug 1307100 - Add handlesOffset only if height > 0.

Per bug 1307100 comment 3, we use "height equals to 0" to hide the floating
toolbar so we only need to adjust the handlesOffset when height > 0.

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

Comment 5

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

The solution in comment 3 is easy to fix :)

Comment 6

[Sebastian Kaspari (:sebastian; :pocmo)]

Comment on attachment 8797902 [details]
Bug 1307100 - Add handlesOffset only if height > 0.

https://reviewboard.mozilla.org/r/83494/#review82152

LGTM - assuming you tested this? :)

Comment 7

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

Comment on attachment 8797902 [details]
Bug 1307100 - Add handlesOffset only if height > 0.

https://reviewboard.mozilla.org/r/83494/#review82152

Yes. I've tested this, and it works.

Comment 8

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/dbaae495b5bf
Add handlesOffset only if height > 0. r=sebastian

Comment 9

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/dbaae495b5bf

Comment 10

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

Comment on attachment 8797902 [details]
Bug 1307100 - Add handlesOffset only if height > 0.

Approval Request Comment
[Feature/regressing bug #]: Bug 1266369. 
[User impact if declined]: Floating toolbar won't be hide when the selection highlight is scrolled out of viewport.
[Describe test coverage new/current, TreeHerder]: Merge on master, and test manually on my aurora build.
[Risks and why]: Low. Simple one line change.
[String/UUID change made/needed]: None.

Comment 11

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

The original patch cannot be applied cleanly on beta.  I'll post the adjusted patch after manually testing on beta.

Comment 12

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

Attachment: Add handlesOffset only if height > 0. (for beta uplift)

Comment 13

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

Comment on attachment 8799636 [details] [diff] [review]
Add handlesOffset only if height > 0. (for beta uplift)

Approval Request Comment
[Feature/regressing bug #]: Bug 1266369. 
[User impact if declined]: Floating toolbar won't be hide when the selection highlight is scrolled out of viewport.
[Describe test coverage new/current, TreeHerder]: Merge on master, and test manually on my beta build.
[Risks and why]: Low. Simple one line change.
[String/UUID change made/needed]: None.

Comment 14

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Hi Ting-Yu, could you please verify this issue is fixed as expected on a latest Nightly build? Thanks!

Comment 15

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

Comment on attachment 8799636 [details] [diff] [review]
Add handlesOffset only if height > 0. (for beta uplift)

Fixes a new regression in 50, Beta50+

Comment 16

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

Verified as fixed in Nightly 52.0a1 (2016-10-11) on Sony Z3 Compact (Android 6.0.1).

Comment 17

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/2583041da3fc

Comment 18

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-beta/rev/63d6e12ae533

Comment 19

[u549602]

Verified as fixed on both Aurora(51.0a2 / 2016-10-18) and Beta (50.0b8) on a Samsung Galaxy S6 Edge(Android 6.0.