Last Comment Bug 661388 - Support selecting text in web content
: Support selecting text in web content
Status: VERIFIED FIXED
:
Product: Fennec Graveyard
Classification: Graveyard
Component: General (show other bugs)
: Trunk
: All All
: -- normal with 2 votes (vote)
: Firefox 7
Assigned To: Mark Finkle (:mfinkle) (use needinfo?)
:
Mentors:
: 661613 (view as bug list)
Depends on: 666915 666917 667046 667066 667067 667069 667986 668228 672718 673297 674405 675724 676097 676268 678789 678790 679715 685058 688262 692525 695697 699698 666977 667062 667071 667097 667243 667571 667982 667983 668017 669816 670222 671052 672316 673037 674515 674531 674659 674791 675459 676008 676223 676527 676912 677118 677509 677574 677676 680926 681718 684923
Blocks: 659022 668201 672543
  Show dependency treegraph
 
Reported: 2011-06-01 14:47 PDT by Mark Finkle (:mfinkle) (use needinfo?)
Modified: 2012-07-19 15:48 PDT (History)
8 users (show)
aaron.train: in‑litmus+
See Also:
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
WIP v1 (30.23 KB, patch)
2011-06-01 14:47 PDT, Mark Finkle (:mfinkle) (use needinfo?)
no flags Details | Diff | Splinter Review
WIP v2 (30.56 KB, patch)
2011-06-04 16:04 PDT, Mark Finkle (:mfinkle) (use needinfo?)
no flags Details | Diff | Splinter Review
WIP v3 (30.13 KB, patch)
2011-06-20 10:47 PDT, Mark Finkle (:mfinkle) (use needinfo?)
no flags Details | Diff | Splinter Review
patch (32.00 KB, patch)
2011-06-22 13:53 PDT, Mark Finkle (:mfinkle) (use needinfo?)
no flags Details | Diff | Splinter Review
patch final? (32.38 KB, patch)
2011-06-23 12:50 PDT, Mark Finkle (:mfinkle) (use needinfo?)
mbrubeck: review+
Details | Diff | Splinter Review
patch for tests (10.38 KB, patch)
2011-06-23 19:09 PDT, Mark Finkle (:mfinkle) (use needinfo?)
mbrubeck: review+
Details | Diff | Splinter Review
simple patch to fix browser_tapping.js failing test without crashing the browser (1.18 KB, patch)
2011-06-24 11:13 PDT, Joel Maher ( :jmaher)
no flags Details | Diff | Splinter Review
patch with bustage fixed (35.09 KB, patch)
2011-06-24 12:20 PDT, Mark Finkle (:mfinkle) (use needinfo?)
mbrubeck: review+
Details | Diff | Splinter Review
interdiff (5.29 KB, patch)
2011-06-24 12:20 PDT, Mark Finkle (:mfinkle) (use needinfo?)
no flags Details | Diff | Splinter Review

Description Mark Finkle (:mfinkle) (use needinfo?) 2011-06-01 14:47:03 PDT
Created attachment 536747 [details] [diff] [review]
WIP v1

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
Comment 1 Mark Finkle (:mfinkle) (use needinfo?) 2011-06-02 12:44:53 PDT
*** Bug 661613 has been marked as a duplicate of this bug. ***
Comment 2 Mark Finkle (:mfinkle) (use needinfo?) 2011-06-04 16:04:19 PDT
Created attachment 537389 [details] [diff] [review]
WIP v2

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")
Comment 3 Mark Finkle (:mfinkle) (use needinfo?) 2011-06-20 10:47:17 PDT
Created attachment 540528 [details] [diff] [review]
WIP v3

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.
Comment 4 Mark Finkle (:mfinkle) (use needinfo?) 2011-06-20 10:50:59 PDT
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.
Comment 5 Mark Finkle (:mfinkle) (use needinfo?) 2011-06-22 13:53:02 PDT
Created attachment 541169 [details] [diff] [review]
patch

With help from Wes, this patch seems to work pretty well. Well enough to get reviewed.
Comment 6 Matt Brubeck (:mbrubeck) 2011-06-23 11:18:34 PDT
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.
Comment 7 Mark Finkle (:mfinkle) (use needinfo?) 2011-06-23 12:49:35 PDT
(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.
Comment 8 Mark Finkle (:mfinkle) (use needinfo?) 2011-06-23 12:50:42 PDT
Created attachment 541468 [details] [diff] [review]
patch final?

This patch also adds the TapMove listener to the window, making it easier to drag the handles.
Comment 9 Matt Brubeck (:mbrubeck) 2011-06-23 14:09:39 PDT
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.
Comment 10 Mark Finkle (:mfinkle) (use needinfo?) 2011-06-23 18:35:27 PDT
http://hg.mozilla.org/mozilla-central/rev/dbd02a315176

Filed bug 666819 for platform API help
Comment 11 Mark Finkle (:mfinkle) (use needinfo?) 2011-06-23 19:09:51 PDT
Created attachment 541575 [details] [diff] [review]
patch for tests

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"
Comment 12 Marco Bonardo [::mak] 2011-06-24 05:45:39 PDT
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
Comment 13 Joel Maher ( :jmaher) 2011-06-24 11:13:13 PDT
Created attachment 541724 [details] [diff] [review]
simple patch to fix browser_tapping.js failing test without crashing the browser

feel free to use this patch.  Tested on a tegra and it passes the browser_tapping.js testcase
Comment 14 Mark Finkle (:mfinkle) (use needinfo?) 2011-06-24 12:20:03 PDT
Created attachment 541757 [details] [diff] [review]
patch with bustage fixed

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
Comment 15 Mark Finkle (:mfinkle) (use needinfo?) 2011-06-24 12:20:39 PDT
Created attachment 541758 [details] [diff] [review]
interdiff

diffs from the original patch
Comment 16 Mark Finkle (:mfinkle) (use needinfo?) 2011-06-24 12:48:44 PDT
pushed again:
http://hg.mozilla.org/mozilla-central/rev/99708970cdf5
Comment 17 Aaron Train [:aaronmt] 2011-06-30 09:58:57 PDT
Verified Fixed based on current implementation
Mozilla/5.0 (Android; Linux armv7l; rv:7.0a1) Gecko/20110630 Firefox/7.0a1 Fennec/7.0a1

Note You need to log in before you can comment on or make changes to this bug.