673037 - No selection handles when trying to select text inside iframe

Status: VERIFIED FIXED

Description

[Martijn Wargers (zombie)]

Attachment: 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 (zombie)]

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?)]

If this is really two bugs, we should file two bugs

Comment 3

[Martijn Wargers (zombie)]

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]]

Same issue? 

http://www.browserscope.org/jskb/test -> Run tests - unable to select any of the text content

Comment 5

[Martijn Wargers (zombie)]

Indeed, those results are inside an iframe, so that is the same issue.

Comment 6

[Wesley Johnston (:wesj)]

Attachment: 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)]

Attachment: Patch v1

Comment 8

[Mark Finkle (:mfinkle) (use needinfo?)]

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)]

inbound http://hg.mozilla.org/integration/mozilla-inbound/rev/5a0b3bee1f8a

Comment 10

[Marco Bonardo [::mak]]

http://hg.mozilla.org/mozilla-central/rev/5a0b3bee1f8a

Comment 11

[Martijn Wargers (zombie)]

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 (zombie)]

It turns out the issue mentioned in comment 4 now suffers from bug 674531.

Comment 13

[Mark Finkle (:mfinkle) (use needinfo?)]

Comment on attachment 548996 [details] [diff] [review]
Patch v1

mobile only and it helps stabilize the text selection feature

Comment 14

[LegNeato]

Comment on attachment 548996 [details] [diff] [review]
Patch v1

Approved for releases/mozilla-aurora. Please land ASAP.

Comment 15

[Mark Finkle (:mfinkle) (use needinfo?)]

http://hg.mozilla.org/releases/mozilla-aurora/rev/9c7d60e7020b

Comment 16

[Aaron Train [:aaronmt]]

Verified Fixed on Aurora
Mozilla/5.0 (Android; Linux armv7l; rv:7.0a2) Gecko/20110805 Firefox/7.0a2 Fennec/7.0a2