1266369 - Consider adding "handles" into contentRect for floating text selection toolbar

Status: VERIFIED FIXED

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

Description

[Sebastian Kaspari (:sebastian; :pocmo)]

Attachment: device-2016-04-21-144110.png

See attached screenshot.

The "handles" of the selection are not part of the contentRect. Therefore the floating toolbar might cover them.

To avoid this we can either include the handles into the contentRect or always add some static bottom padding to the rect manually.

Comment 1

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

The b2g floating text selection dialog addressed this issue by adding a constant pixels [1] so that it won't cover the carects . A good reference is the spec version 1.7 page 17 in bug 921965.

The function calculateDialogPostion() to calculate the text selection dialog position is at [1].

cc Jeremy that he knows the details about the positioning.

[1] https://github.com/mozilla-b2g/gaia/blob/dcbf86d523064fa6018a99d084cf631fd402c66d/apps/system/js/app_text_selection_dialog.js#L86-L92
[2] https://github.com/mozilla-b2g/gaia/blob/dcbf86d523064fa6018a99d084cf631fd402c66d/apps/system/js/app_text_selection_dialog.js#L422

Comment 2

[Sebastian Kaspari (:sebastian; :pocmo)]

Capella, TYLin:  Has someone of you time to look into this (Additionally I'll mark it as mentor bug)? It's always at the bottom of my queue and I never get to it.

This is where we get the rect from gecko and calculate the rect for Android:
https://dxr.mozilla.org/mozilla-central/source/mobile/android/base/java/org/mozilla/gecko/text/FloatingToolbarTextSelection.java#173-190

Comment 3

[Mark Capella [:capella]]

I haven't looked, but this sounds reasonable on the face of it. I'd imagine the patch would be a prefable option on the AccessibleCaretManager side.

Comment 4

[Sebastian Kaspari (:sebastian; :pocmo)]

(In reply to Mark Capella [:capella] from comment #3)
> I haven't looked, but this sounds reasonable on the face of it. I'd imagine
> the patch would be a prefable option on the AccessibleCaretManager side.

Good point. The AccessibleCaretManager probably knows the size of the handles. Doing this on Java side might be a bit hacky. :)

Comment 5

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

It'll be easier to get the caret rects from AccessibleCaretManager.  However, I'd rather keep the current contentRect, i.e. CaretStateChangedEvent::mBoundingClientRect [1], containing only the selection highlight rect, and add another attribute like mBoundingCaretsRect for the carets rect.  Since B2G resolved this issue by adding a constant pixel offsets to its floating toolbar, adding carets rect to the existing rect will break B2G.

[1] https://dxr.mozilla.org/mozilla-central/rev/d0be57e84807ce0853b2406de7ff6abb195ac898/dom/webidl/CaretStateChangedEvent.webidl#19

Comment 6

[amol1994mane]

Attachment: What I see in my emulator

I'm trying to fix this bug, but when I try to recreate it, this is what I see. Is there a specific way to cause it?

Comment 7

[Sebastian Kaspari (:sebastian; :pocmo)]

(In reply to amol1994mane from comment #6)
> Created attachment 8760127 [details]
> What I see in my emulator
> 
> I'm trying to fix this bug, but when I try to recreate it, this is what I
> see. Is there a specific way to cause it?

It looks like you are using an older version of Android. The floating toolbar is only available on Android 6+.

Comment 8

[amol1994mane]

I've been trying to fix this bug for a while now but haven't made much progress. Could I get some pointers on how to solve this, or where to look? I took a look at CaretStateChangedEvent.webidl and it's autogenerated .h file, but haven't found the part where the distance of the floating toolbar from the carets is set.

Comment 9

[Sebastian Kaspari (:sebastian; :pocmo)]

(In reply to amol1994mane from comment #8)
> I've been trying to fix this bug for a while now but haven't made much
> progress. Could I get some pointers on how to solve this, or where to look?
> I took a look at CaretStateChangedEvent.webidl and it's autogenerated .h
> file, but haven't found the part where the distance of the floating toolbar
> from the carets is set.

That's something capella or TYLin might know.

Comment 10

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

Sebastian, what do you think about the comment 5?  I guess it would be simpler if we just add some static bottom space manually when the floating toolbar is below the selection rect.

The selection highlight bounding rect |mBoundingClientRect| is set from the C++ side in [1].  I had no idea how floating toolbar uses the |boundingClientRect| in CaretStateChangedEvent.webidl.  Sebastian, would you point where the code is?

[1] https://dxr.mozilla.org/mozilla-central/rev/5f95858f8ddf21ea2271a12810332efd09eff138/layout/base/AccessibleCaretManager.cpp#1383

Comment 11

[Sebastian Kaspari (:sebastian; :pocmo)]

Yeah! (I just talked to TYLin in person)

I agree. It's much easier to add the offset (size of the handles) to the bottom of the content rect.

I guess we want to do it somewhere over here:
https://dxr.mozilla.org/mozilla-central/source/mobile/android/base/java/org/mozilla/gecko/text/FloatingToolbarTextSelection.java#176

Or here:
https://dxr.mozilla.org/mozilla-central/source/mobile/android/base/java/org/mozilla/gecko/text/FloatingActionModeCallback.java#63

@amol: Do you want to look into this? I guess we basically only need the size of the handles in dp, calculate the actual size in pixels, depending on the device, and add it to the bottom of the rect.

Comment 12

[Sebastian Kaspari (:sebastian; :pocmo)]

Setting tracking flag: We probably should fix this in 48. We have roughly 5 weeks until merge day. @amol: Do you have time to write a patch for this or should someone else look into this?

Comment 13

[amol1994mane]

@sebastian: can't find the time. Better if someone else takes it up.

Comment 14

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

I agree we should fix this for 48, but we need an assignee. :TYLin do you have time? :sebastian? Margaret, any other suggestions?

Comment 15

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

Sorry that I don't have time to take this.  I'm out of office next week.

I've talked to :sebastian about this bug in London.  The approach would be in Java side as described in comment 11.

Comment 16

[Sebastian Kaspari (:sebastian; :pocmo)]

Okay, I'll write a patch.

Comment 17

[Sebastian Kaspari (:sebastian; :pocmo)]

Attachment: Bug 1266369 - (Pre) FloatingToolbarTextSelection: Add final keyword where feasible.

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

Comment 18

[Sebastian Kaspari (:sebastian; :pocmo)]

Attachment: Bug 1266369 - FloatingToolbarTextSelection: Add an offset to the content rect to avoid overlap.

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

Comment 19

[Andrzej Hunt :ahunt]

Comment on attachment 8765426 [details]
Bug 1266369 - (Pre) FloatingToolbarTextSelection: Add final keyword where feasible.

https://reviewboard.mozilla.org/r/60808/#review57708

Comment 20

[Andrzej Hunt :ahunt]

Comment on attachment 8765427 [details]
Bug 1266369 - FloatingToolbarTextSelection: Add an offset to the content rect to avoid overlap.

https://reviewboard.mozilla.org/r/60810/#review57712

Comment 21

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/026d422063f8
(Pre) FloatingToolbarTextSelection: Add final keyword where feasible. r=ahunt
https://hg.mozilla.org/integration/mozilla-inbound/rev/170e18a9341f
FloatingToolbarTextSelection: Add an offset to the content rect to avoid overlap. r=ahunt

Comment 22

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/026d422063f8
https://hg.mozilla.org/mozilla-central/rev/170e18a9341f

Comment 23

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/fd6e8d0e44be
Don't define kVK_RightCommand when building against the macOS 10.12 SDK. r=masayuki

Comment 24

[Carsten Book [:Tomcat]]

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

Comment 25

[Sorina Florean [:sflorean]]

Verified as fixed in build Firefox 50.0b6;
Device: Nexus 9 (Android 6.0.1).

Comment 26

[Bogdan Surd, QA [:bsurd, NI]]

Verified as fixed in 50.1.0;
Devices:
 - LG G4 (Android 5.1)
 - LG G4 (Android 6.0)