Closed Bug 773718 Opened 12 years ago Closed 12 years ago

_pointInSelection fails if you pan the page after adjusting selection

Categories

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

Product:
Firefox for Android Graveyard
Component:
Text Selection
Platform:
ARM
Android
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(firefox15 verified)

Status:
VERIFIED FIXED
Milestone:
Firefox 16
Tracking Flags:
Tracking Status
firefox15 --- verified

People

(Reporter: Margaret, Assigned: Margaret)

References

Details

Attachments

(1 file, 2 obsolete files)

patch v3
3.95 KB, patch
mbrubeck
: review+
lsblakk
: approval-mozilla-beta+
Details | Diff | Splinter Review 
STR:
1) Make a selection
2) Scroll the page
3) Long tap below the selection (but where it used to be positioned)
4) Context menu will appear

It looks like this is because we're not updating cache.rect when we pan the page, and that's a problem because getBoundingClientRect returns values relative to the viewport.

Since we only use cache.rect in _pointInSelection, there's not really any reason to cache it. Patch forthcoming!
Attached patch patch (obsolete) — — Splinter Review 
This also saves us a getBoundingClientRect() call on every touchmove.
Attachment #641953 - Flags: review?(mbrubeck)
Attachment #641953 - Flags: review?(mbrubeck) → review+
Attached patch patch (obsolete) — — Splinter Review 
That first patch was actually wrong because we were clearing the selection before ever calling _pointInSelection. This fixes that.
Attachment #641953 - Attachment is obsolete: true
Attachment #642064 - Flags: review?(mbrubeck)
Attached patch patch v3 — — Splinter Review 
Sigh, I was trying to be smart, but there's another caller to _pointInSelection, and it doesn't have the selection to pass in.
Attachment #642064 - Attachment is obsolete: true
Attachment #642064 - Flags: review?(mbrubeck)
Attachment #642070 - Flags: review?(mbrubeck)
Attachment #642070 - Flags: review?(mbrubeck) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/22611700863c
Target Milestone: --- → Firefox 16
https://hg.mozilla.org/mozilla-central/rev/22611700863c
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Depends on: 776390
Comment on attachment 642070 [details] [diff] [review]
patch v3

[Approval Request Comment]
Bug caused by (feature/regressing bug #): performance problem with original text selection implementation, but still affects implementation with native handles
User impact if declined: slightly worse text selection performance
Testing completed (on m-c, etc.): landed on m-c 7/14 to make Firefox 16
Risk to taking this patch (and alternatives if risky): low-risk, one follow-up patch needed to fix a regression (bug 776390)
String or UUID changes made by this patch: n/a
Attachment #642070 - Flags: approval-mozilla-beta?
Comment on attachment 642070 [details] [diff] [review]
patch v3

approved for beta as part of the dep bugs for native handles for text selection.
Attachment #642070 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Context menu is not triggered anymore.

Firefox Mobile 15.0b3/Nightly 17.0a1 2012-08-02/Aurora 16.0a2 2012-08-02
HTC Desire(Android 2.2.2)/ Samsung Galaxy Tab (Android 3.1)
Status: RESOLVED → VERIFIED
status-firefox15: fixed → verified
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.