No selection handles when trying to select text inside iframe

VERIFIED FIXED in Firefox 7

Status

defect
P2
normal
VERIFIED FIXED
8 years ago
8 years ago

People

(Reporter: martijn.martijn, Assigned: wesj)

Tracking

({testcase, verified-aurora})

Dependency tree / graph

Firefox Tracking Flags

(firefox7 fixed, firefox8 fixed, fennec7+)

Details

(Whiteboard: [inbound])

Attachments

(2 attachments, 1 obsolete attachment)

Posted file 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.
The second problem (not being able to use the Android back button) is very annoying, so I'm asking tracking-firefox 7? for that.
If this is really two bugs, we should file two bugs
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
Same issue? 

http://www.browserscope.org/jskb/test -> Run tests - unable to select any of the text content
Indeed, those results are inside an iframe, so that is the same issue.
Assignee: nobody → mark.finkle
Posted patch WIP (obsolete) — Splinter 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
Posted patch Patch v1Splinter Review
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 += 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
Attachment #548996 - Flags: review?(mark.finkle) → review+
http://hg.mozilla.org/mozilla-central/rev/5a0b3bee1f8a
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 8
Depends on: 675454
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.
Status: RESOLVED → VERIFIED
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 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+
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.