Closed Bug 1266369 Opened 4 years ago Closed 3 years ago

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

Categories

(Firefox for Android :: Text Selection, defect)

All
Android
defect
Not set

Tracking

()

VERIFIED FIXED
Firefox 50
Tracking Status
firefox45 --- unaffected
firefox46 --- unaffected
firefox47 --- unaffected
firefox48 --- affected
firefox49 --- affected
fennec 48+ ---
firefox50 --- verified

People

(Reporter: sebastian, Assigned: sebastian, Mentored)

References

(Blocks 1 open bug)

Details

(Whiteboard: [lang=java][good next bug])

Attachments

(4 files)

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.
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
Assignee: nobody → s.kaspari
Status: NEW → ASSIGNED
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
Assignee: s.kaspari → nobody
Mentor: s.kaspari
Status: ASSIGNED → NEW
Flags: needinfo?(tlin)
Flags: needinfo?(markcapella)
Whiteboard: [lang=java][good next bug]
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.
Flags: needinfo?(markcapella)
(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. :)
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
Flags: needinfo?(tlin)
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?
Flags: needinfo?(s.kaspari)
(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+.
Flags: needinfo?(s.kaspari)
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.
Flags: needinfo?(s.kaspari)
(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.
Flags: needinfo?(tlin)
Flags: needinfo?(s.kaspari)
Flags: needinfo?(markcapella)
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
Flags: needinfo?(tlin) → needinfo?(s.kaspari)
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.
Flags: needinfo?(s.kaspari)
Flags: needinfo?(markcapella)
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?
tracking-fennec: --- → ?
Flags: needinfo?(amol1994mane)
@sebastian: can't find the time. Better if someone else takes it up.
Flags: needinfo?(amol1994mane)
I agree we should fix this for 48, but we need an assignee. :TYLin do you have time? :sebastian? Margaret, any other suggestions?
tracking-fennec: ? → 48+
Flags: needinfo?(tlin)
Flags: needinfo?(s.kaspari)
Flags: needinfo?(margaret.leibovic)
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.
Flags: needinfo?(tlin)
Okay, I'll write a patch.
Assignee: nobody → s.kaspari
Status: NEW → ASSIGNED
Flags: needinfo?(s.kaspari)
Comment on attachment 8765426 [details]
Bug 1266369 - (Pre) FloatingToolbarTextSelection: Add final keyword where feasible.

https://reviewboard.mozilla.org/r/60808/#review57708
Attachment #8765426 - Flags: review?(ahunt) → review+
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
Attachment #8765427 - Flags: review?(ahunt) → review+
Pushed by s.kaspari@gmail.com:
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
https://hg.mozilla.org/mozilla-central/rev/026d422063f8
https://hg.mozilla.org/mozilla-central/rev/170e18a9341f
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 50
Flags: needinfo?(margaret.leibovic)
Pushed by mstange@themasta.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/fd6e8d0e44be
Don't define kVK_RightCommand when building against the macOS 10.12 SDK. r=masayuki
Depends on: 1307100
Verified as fixed in build Firefox 50.0b6;
Device: Nexus 9 (Android 6.0.1).
Verified as fixed in 50.1.0;
Devices:
 - LG G4 (Android 5.1)
 - LG G4 (Android 6.0)
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.