Last Comment Bug 673037 - No selection handles when trying to select text inside iframe
: No selection handles when trying to select text inside iframe
Status: VERIFIED FIXED
[inbound]
: testcase, verified-aurora
Product: Fennec Graveyard
Classification: Graveyard
Component: General (show other bugs)
: Trunk
: x86 Windows 7
: P2 normal (vote)
: Firefox 7
Assigned To: Wesley Johnston (:wesj)
:
:
Mentors:
Depends on: 675454
Blocks: 661388
  Show dependency treegraph
 
Reported: 2011-07-20 19:47 PDT by Martijn Wargers [:mwargers] (not working for Mozilla)
Modified: 2011-08-05 06:49 PDT (History)
4 users (show)
See Also:
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
testcase (424 bytes, text/html)
2011-07-20 19:47 PDT, Martijn Wargers [:mwargers] (not working for Mozilla)
no flags Details
WIP (2.83 KB, patch)
2011-07-27 17:24 PDT, Wesley Johnston (:wesj)
no flags Details | Diff | Splinter Review
Patch v1 (4.42 KB, patch)
2011-07-27 17:43 PDT, Wesley Johnston (:wesj)
mark.finkle: review+
christian: approval‑mozilla‑aurora+
Details | Diff | Splinter Review

Description Martijn Wargers [:mwargers] (not working for Mozilla) 2011-07-20 19:47:47 PDT
Created attachment 547314 [details]
testcase

See testcase.

Steps to reproduce:
- Long tap on the text inside the iframe.
Notice how you're not getting to see any selection handles (that's bug nr. 1)
- Tap on the url bar, the awesome bar opens.
- Press the Android 'back' button
Notice how you're not able to get back to the web page in question, you're staying in the awesome bar.
Comment 1 Martijn Wargers [:mwargers] (not working for Mozilla) 2011-07-21 09:23:10 PDT
The second problem (not being able to use the Android back button) is very annoying, so I'm asking tracking-firefox 7? for that.
Comment 2 Mark Finkle (:mfinkle) (use needinfo?) 2011-07-21 11:00:48 PDT
If this is really two bugs, we should file two bugs
Comment 3 Martijn Wargers [:mwargers] (not working for Mozilla) 2011-07-21 11:06:59 PDT
You're right, I have now filed bug 673166 for the second problem. I suspect they might be connected problems, however.
Comment 4 Aaron Train [:aaronmt] 2011-07-26 11:21:20 PDT
Same issue? 

http://www.browserscope.org/jskb/test -> Run tests - unable to select any of the text content
Comment 5 Martijn Wargers [:mwargers] (not working for Mozilla) 2011-07-26 12:09:24 PDT
Indeed, those results are inside an iframe, so that is the same issue.
Comment 6 Wesley Johnston (:wesj) 2011-07-27 17:24:22 PDT
Created attachment 548985 [details] [diff] [review]
WIP

This starts to fix things. Needs more work to position handles correctly and fix the range sent to the parent process.
Comment 7 Wesley Johnston (:wesj) 2011-07-27 17:43:28 PDT
Created attachment 548996 [details] [diff] [review]
Patch v1
Comment 8 Mark Finkle (:mfinkle) (use needinfo?) 2011-07-27 18:25:24 PDT
Comment on attachment 548996 [details] [diff] [review]
Patch v1


>+        // if this is an iframe, dig down to find the document that was clicked
>+        var aX = json.x;
>+        var aY = json.y;

var -> let

>+        let elem = utils.elementFromPoint(aX, aY, true, false);
>+        while (elem && (elem instanceof HTMLIFrameElement || elem instanceof HTMLFrameElement)) {
>+          // adjust client coordinates' origin to be top left of iframe viewport
>+          let rect = elem.getBoundingClientRect();
>+          scrollOffset = ContentScroll.getScrollOffset(elem.ownerDocument.defaultView);
>+          offsetX += rect.left;
>+          aX -= rect.left;
>+
>+          offsetY += rect.top + scrollOffset.y;
>+          aY -= rect.top + scrollOffset.y;
>+          utils = elem.contentDocument.defaultView.QueryInterface(Ci.nsIInterfaceRequestor).getInterface(Ci.nsIDOMWindowUtils);
>+          elem = utils.elementFromPoint(aX, aY, true, false);
>+        }

No easy way to reuse elementFromPoint function at the top of the script I assume. It is using "getClosest" stuff, which probably isn't good for this

>+        let contentWindow = elem.ownerDocument.defaultView;
>+        let currentDocShell = contentWindow.QueryInterface(Ci.nsIInterfaceRequestor).
>+                                                         getInterface(Ci.nsIWebNavigation).
>+                                                         QueryInterface(Ci.nsIDocShell);

No wrap needed here

>+        this.contentWindow = contentWindow;
Can we assign this right away and drop the extra local variable?

>       case "Browser:SelectionEnd": {
>         try {
>           // The selection might already be gone
>-          content.getSelection().collapseToStart();
>+          this.contentWindow.getSelection().collapseToStart();
>+          this.contentWindow = null;

We could use an "if (this.contentWindow)" but I guess we are in a try/catch so it should be fine

>         // Cache the selected text since the selection might be gone by the time we get the "end" message
>-        this.selectedText = content.getSelection().toString().trim();
>+        this.selectedText = this.contentWindow.getSelection().toString().trim();

if we don't have a contentWindow things would be bad, but we should probably return early from the function itself, not just here.

Try to make sure we check this.contentWindow a bit more defensively

Nicely done
Comment 9 Wesley Johnston (:wesj) 2011-07-28 11:03:25 PDT
inbound http://hg.mozilla.org/integration/mozilla-inbound/rev/5a0b3bee1f8a
Comment 10 Marco Bonardo [::mak] 2011-07-29 03:08:11 PDT
http://hg.mozilla.org/mozilla-central/rev/5a0b3bee1f8a
Comment 11 Martijn Wargers [:mwargers] (not working for Mozilla) 2011-07-30 12:06:45 PDT
Verified fixed, using today's build on the LG Optimus Black.


The issue mentioned in comment 4 doesn't seem to be fixed, so I guess a new bug should be filed for that.
Comment 12 Martijn Wargers [:mwargers] (not working for Mozilla) 2011-07-30 15:32:50 PDT
It turns out the issue mentioned in comment 4 now suffers from bug 674531.
Comment 13 Mark Finkle (:mfinkle) (use needinfo?) 2011-08-04 12:33:59 PDT
Comment on attachment 548996 [details] [diff] [review]
Patch v1

mobile only and it helps stabilize the text selection feature
Comment 14 christian 2011-08-04 14:47:44 PDT
Comment on attachment 548996 [details] [diff] [review]
Patch v1

Approved for releases/mozilla-aurora. Please land ASAP.
Comment 15 Mark Finkle (:mfinkle) (use needinfo?) 2011-08-04 20:15:43 PDT
http://hg.mozilla.org/releases/mozilla-aurora/rev/9c7d60e7020b
Comment 16 Aaron Train [:aaronmt] 2011-08-05 06:49:36 PDT
Verified Fixed on Aurora
Mozilla/5.0 (Android; Linux armv7l; rv:7.0a2) Gecko/20110805 Firefox/7.0a2 Fennec/7.0a2

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