No selection handles when trying to select text inside iframe



7 years ago
7 years ago


(Reporter: martijn.martijn, Assigned: wesj)


({testcase, verified-aurora})

Firefox 7
Windows 7
testcase, verified-aurora
Dependency tree / graph


(Whiteboard: [inbound])


(2 attachments, 1 obsolete attachment)



7 years ago
Created attachment 547314 [details]

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

7 years ago
The second problem (not being able to use the Android back button) is very annoying, so I'm asking tracking-firefox 7? for that.
tracking-firefox7: --- → ?
If this is really two bugs, we should file two bugs

Comment 3

7 years ago
You're right, I have now filed bug 673166 for the second problem. I suspect they might be connected problems, however.
Summary: No selection handles and Android back button doesn't work when awesome bar is open and focus is in iframe in web content → No selection handles when trying to select text inside iframe
Priority: -- → P2
Same issue? -> Run tests - unable to select any of the text content

Comment 5

7 years ago
Indeed, those results are inside an iframe, so that is the same issue.
Assignee: nobody → mark.finkle

Comment 6

7 years ago
Created attachment 548985 [details] [diff] [review]

This starts to fix things. Needs more work to position handles correctly and fix the range sent to the parent process.
Assignee: mark.finkle → wjohnston

Comment 7

7 years ago
Created attachment 548996 [details] [diff] [review]
Patch v1
Attachment #548985 - Attachment is obsolete: true
Attachment #548996 - Flags: review?(mark.finkle)
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 += + scrollOffset.y;
>+          aY -= + 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
Attachment #548996 - Flags: review?(mark.finkle) → review+
Last Resolved: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 8


7 years ago
Depends on: 675454

Comment 11

7 years ago
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.


7 years ago

Comment 12

7 years ago
It turns out the issue mentioned in comment 4 now suffers from bug 674531.
Comment on attachment 548996 [details] [diff] [review]
Patch v1

mobile only and it helps stabilize the text selection feature
Attachment #548996 - Flags: approval-mozilla-aurora?

Comment 14

7 years ago
Comment on attachment 548996 [details] [diff] [review]
Patch v1

Approved for releases/mozilla-aurora. Please land ASAP.
Attachment #548996 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
tracking-fennec: --- → 7+
tracking-firefox7: ? → ---
status-firefox7: --- → fixed
status-firefox8: --- → fixed
Target Milestone: Firefox 8 → Firefox 7
Verified Fixed on Aurora
Mozilla/5.0 (Android; Linux armv7l; rv:7.0a2) Gecko/20110805 Firefox/7.0a2 Fennec/7.0a2
Keywords: verified-aurora
You need to log in before you can comment on or make changes to this bug.