Closed
Bug 1266369
Opened 9 years ago
Closed 8 years ago
Consider adding "handles" into contentRect for floating text selection toolbar
Categories
(Firefox for Android Graveyard :: Text Selection, defect)
Tracking
(firefox45 unaffected, firefox46 unaffected, firefox47 unaffected, firefox48 affected, firefox49 affected, fennec48+, firefox50 verified)
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.
Assignee | ||
Updated•9 years ago
|
status-firefox45:
--- → unaffected
status-firefox46:
--- → unaffected
status-firefox47:
--- → unaffected
status-firefox48:
--- → affected
Comment 1•9 years ago
|
||
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 | ||
Updated•9 years ago
|
Assignee: nobody → s.kaspari
Status: NEW → ASSIGNED
Assignee | ||
Comment 2•9 years ago
|
||
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
status-firefox49:
--- → affected
Flags: needinfo?(tlin)
Flags: needinfo?(markcapella)
Whiteboard: [lang=java][good next bug]
Comment 3•9 years ago
|
||
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)
Assignee | ||
Comment 4•9 years ago
|
||
(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•9 years ago
|
||
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
Blocks: AccessibleCaret
Flags: needinfo?(tlin)
Comment 6•9 years ago
|
||
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)
Assignee | ||
Comment 7•9 years ago
|
||
(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)
Comment 8•8 years ago
|
||
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)
Assignee | ||
Comment 9•8 years ago
|
||
(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)
Comment 10•8 years ago
|
||
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)
Assignee | ||
Comment 11•8 years ago
|
||
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)
Assignee | ||
Comment 12•8 years ago
|
||
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•8 years ago
|
||
@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)
Comment 15•8 years ago
|
||
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)
Assignee | ||
Comment 16•8 years ago
|
||
Okay, I'll write a patch.
Assignee: nobody → s.kaspari
Status: NEW → ASSIGNED
Flags: needinfo?(s.kaspari)
Assignee | ||
Comment 17•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/60808/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/60808/
Attachment #8765426 -
Flags: review?(ahunt)
Attachment #8765427 -
Flags: review?(ahunt)
Assignee | ||
Comment 18•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/60810/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/60810/
Comment 19•8 years ago
|
||
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 20•8 years ago
|
||
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+
Comment 21•8 years ago
|
||
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
Comment 22•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/026d422063f8
https://hg.mozilla.org/mozilla-central/rev/170e18a9341f
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 50
Updated•8 years ago
|
Flags: needinfo?(margaret.leibovic)
Comment 23•8 years ago
|
||
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
Comment 24•8 years ago
|
||
bugherder |
Comment 25•8 years ago
|
||
Verified as fixed in build Firefox 50.0b6;
Device: Nexus 9 (Android 6.0.1).
Comment 26•8 years ago
|
||
Verified as fixed in 50.1.0;
Devices:
- LG G4 (Android 5.1)
- LG G4 (Android 6.0)
Status: RESOLVED → VERIFIED
Updated•4 years ago
|
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•