The default bug view has changed. See this FAQ.

No selection handles when trying to select text inside iframe

VERIFIED FIXED in Firefox 7

Status

Fennec Graveyard
General
P2
normal
VERIFIED FIXED
6 years ago
6 years ago

People

(Reporter: Martijn Wargers (dead), Assigned: wesj)

Tracking

({testcase, verified-aurora})

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

Details

(Whiteboard: [inbound])

Attachments

(2 attachments, 1 obsolete attachment)

(Reporter)

Description

6 years ago
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.
(Reporter)

Comment 1

6 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
(Reporter)

Comment 3

6 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? 

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

Comment 5

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

Comment 6

6 years ago
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.
Assignee: mark.finkle → wjohnston
(Assignee)

Comment 7

6 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 += 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+
(Assignee)

Comment 9

6 years ago
inbound http://hg.mozilla.org/integration/mozilla-inbound/rev/5a0b3bee1f8a
Whiteboard: [inbound]
http://hg.mozilla.org/mozilla-central/rev/5a0b3bee1f8a
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 8
(Reporter)

Updated

6 years ago
Depends on: 675454
(Reporter)

Comment 11

6 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.
(Reporter)

Updated

6 years ago
Status: RESOLVED → VERIFIED
(Reporter)

Comment 12

6 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

6 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: ? → ---
http://hg.mozilla.org/releases/mozilla-aurora/rev/9c7d60e7020b
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.