Support selecting text in web content

VERIFIED FIXED in Firefox 7

Status

defect
VERIFIED FIXED
8 years ago
7 years ago

People

(Reporter: mfinkle, Assigned: mfinkle)

Tracking

Firefox Tracking Flags

(firefox7 fixed, fennec7+)

Details

Attachments

(2 attachments, 7 obsolete attachments)

Posted patch WIP v1 (obsolete) — Splinter Review
Mobile browser typically need to support a special mode to allow text in web content to be selected and copied to the clipboard.

We are going to implement the Android model for this behavior. It will be available in all Fennec builds, not just Android.

Attaching a WIP
Assignee: nobody → mark.finkle
Blocks: 659022
tracking-fennec: --- → ?
OS: Linux → All
Hardware: x86 → All
See Also: → 652168
Duplicate of this bug: 661613
Posted patch WIP v2 (obsolete) — Splinter Review
This patch positions the handles better on initial long tap (except when the web content is zoomed!)

The handles are hard to "hit" on devices. We need to enlarge the hit area of the handles somehow (CSS padding or mouse tap "gravity")
Attachment #536747 - Attachment is obsolete: true
tracking-fennec: ? → 7+
Posted patch WIP v3 (obsolete) — Splinter Review
This patch works OK unless you double tap (or pinch zoom). Then the mouse coords seem wrong, but I haven't figured out why.

Another problem with this patch is moving the start handle or a link will activate the link. Doing "mousedown" / "mouseup" on a link will do that. Should be simple enough to trick gecko into not making a "click".

Last problem is making the handles go away when page changes or resets in some way. Again, easy enough to listen for the right events (like Form Helper).

I really need to figure out the offset/scale/ coord problems before moving on.
Attachment #537389 - Attachment is obsolete: true
Attachment #540528 - Flags: feedback?(mbrubeck)
Note: To test, load a page with <p> text, like planet mozilla.

* Try long tap on text with no scrolling or changes in zoom. (should be fine)
* Scroll down the page and try again. (should be fine)
* Double tap or pinch to zoom in and try again. (handles are mis-positioned)

Do the zoom test with no scroll to actually get a chance to see the handles mis-positioned. Otherwise they are off-screen.
Posted patch patch (obsolete) — Splinter Review
With help from Wes, this patch seems to work pretty well. Well enough to get reviewed.
Attachment #540528 - Attachment is obsolete: true
Attachment #540528 - Flags: feedback?(mbrubeck)
Attachment #541169 - Flags: review?(mbrubeck)
Some notes from testing this patch:

If I drag a handle over a link, I trigger the link when I release my mouse/finger.

Zooming with the scroll wheel (rocker switch on Maemo) leaves the text selected but does not adjust the handle positions.  This doesn't matter on Android currently, but we should probably listen for zoom events and either cancel the selection or move the handles.

Once in a while I get in a state where one the end handle is correctly positioned, but the start handle is offset vertically.  It's drawn too far down by a fixed number of pixels.  The offset remains the same size regardless of what text I select or where I zoom/scroll.
(In reply to comment #6)
> Some notes from testing this patch:
> 
> If I drag a handle over a link, I trigger the link when I release my
> mouse/finger.

Fixed using a hack: Don't fire a mouseup, only a mousedown. That is enough to position the caret. A platform fix would be better. I'll file a bug.

> Zooming with the scroll wheel (rocker switch on Maemo) leaves the text
> selected but does not adjust the handle positions.  This doesn't matter on
> Android currently, but we should probably listen for zoom events and either
> cancel the selection or move the handles.

Fixed. Added a few events to hide the handles

> Once in a while I get in a state where one the end handle is correctly
> positioned, but the start handle is offset vertically.  It's drawn too far
> down by a fixed number of pixels.  The offset remains the same size
> regardless of what text I select or where I zoom/scroll.

Could be related to the link issue, which I "fixed" It seemed to work better for me on device with the fix. The platform fix is the best solution.

I have found the selection to be slow to update at times too, especially during a page load. Again, passing mouse events/messages and using the mouse event hack are probably the cause. We'll see what a proper platform API does to improve the perf.
Posted patch patch final? (obsolete) — Splinter Review
This patch also adds the TapMove listener to the window, making it easier to drag the handles.
Attachment #541169 - Attachment is obsolete: true
Attachment #541169 - Flags: review?(mbrubeck)
Attachment #541468 - Flags: review?(mbrubeck)
Comment on attachment 541468 [details] [diff] [review]
patch final?

Looks good to me.  Might still cause some issues on sites with onmousedown handlers in content, but that can be fixed in the platform later.

SelectionHelper could be moved to its own lazy-loaded JS file, if we want to prevent common-ui.js from growing too large.
Attachment #541468 - Flags: review?(mbrubeck) → review+
http://hg.mozilla.org/mozilla-central/rev/dbd02a315176

Filed bug 666819 for platform API help
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Posted patch patch for tests (obsolete) — Splinter Review
I broke a test, so this patch fixes it and adds a simple test for tapping in a block of text.

The broken test was an <img> wrapped in a <div> and <div>s are fair game for text selection, so we got a "content-text" type being returned too. The fix was to add that to the expected types check. I also added a plain text tap check too. It should only return "content-text"
Attachment #541575 - Flags: review?(mbrubeck)
Attachment #541575 - Flags: review?(mbrubeck) → review+
backed out to investigate why after this changeset we didn't see anymore a green b-c on Android.
http://hg.mozilla.org/mozilla-central/rev/a87ee7550f6a
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Flags: in-litmus?(aaron.train)
Depends on: 666917
Depends on: 666915
Depends on: 666977
feel free to use this patch.  Tested on a tegra and it passes the browser_tapping.js testcase
This patch is the same basic patch as before. It has a little more error handling and Joel's simple fix for the failing test.

I'll post an interdiff in a moment
Attachment #541468 - Attachment is obsolete: true
Attachment #541575 - Attachment is obsolete: true
Attachment #541724 - Attachment is obsolete: true
Attachment #541757 - Flags: review?(mbrubeck)
Posted patch interdiffSplinter Review
diffs from the original patch
Attachment #541757 - Flags: review?(mbrubeck) → review+
pushed again:
http://hg.mozilla.org/mozilla-central/rev/99708970cdf5
Status: REOPENED → RESOLVED
Closed: 8 years ago8 years ago
Resolution: --- → FIXED
Depends on: 667046
Depends on: 667062
Depends on: 667066
Depends on: 667069
Depends on: 667071
Depends on: 667067
Depends on: 667571
Target Milestone: --- → Firefox 7
Depends on: 667982
Depends on: 667986
Depends on: 667983
Depends on: 667993
Depends on: 668017

Updated

8 years ago
Blocks: 668201
Depends on: 668228
Verified Fixed based on current implementation
Mozilla/5.0 (Android; Linux armv7l; rv:7.0a1) Gecko/20110630 Firefox/7.0a1 Fennec/7.0a1
Status: RESOLVED → VERIFIED
Depends on: 668790
Depends on: 669816
Depends on: 667097
No longer depends on: 668790
Depends on: 671052
Depends on: 672316
Blocks: 672543
Depends on: 670222
Depends on: 672718
Depends on: 673037
Depends on: 673297
Depends on: 674405
Depends on: 674531
Depends on: 674515
Depends on: 674659
Depends on: 674791
Depends on: 675459
Depends on: 675724
Depends on: 676008
Depends on: 676097
Depends on: 676223
Depends on: 676268
Depends on: 676527
Depends on: 676912
Depends on: 677118
Depends on: 677509
Depends on: 677574
Depends on: 677676
Depends on: 677896
Depends on: 679715
Depends on: 680926
Depends on: 681718
Depends on: 684923
Depends on: 685058
Depends on: 688262
Depends on: 692525
Depends on: 695697
Depends on: 699698
No longer depends on: 677896
No longer depends on: 667993
You need to log in before you can comment on or make changes to this bug.