Closed
Bug 673037
Opened 14 years ago
Closed 14 years ago
No selection handles when trying to select text inside iframe
Categories
(Firefox for Android Graveyard :: General, defect, P2)
Tracking
(firefox7 fixed, firefox8 fixed, fennec7+)
VERIFIED
FIXED
Firefox 7
People
(Reporter: martijn.martijn, Assigned: wesj)
References
Details
(Keywords: testcase, verified-aurora, Whiteboard: [inbound])
Attachments
(2 files, 1 obsolete file)
424 bytes,
text/html
|
Details | |
4.42 KB,
patch
|
mfinkle
:
review+
christian
:
approval-mozilla-aurora+
|
Details | Diff | Splinter Review |
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•14 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:
--- → ?
Comment 2•14 years ago
|
||
If this is really two bugs, we should file two bugs
Reporter | ||
Comment 3•14 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
![]() |
||
Updated•14 years ago
|
Priority: -- → P2
Comment 4•14 years ago
|
||
Same issue?
http://www.browserscope.org/jskb/test -> Run tests - unable to select any of the text content
Reporter | ||
Comment 5•14 years ago
|
||
Indeed, those results are inside an iframe, so that is the same issue.
Updated•14 years ago
|
Assignee: nobody → mark.finkle
Assignee | ||
Comment 6•14 years ago
|
||
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•14 years ago
|
||
Attachment #548985 -
Attachment is obsolete: true
Attachment #548996 -
Flags: review?(mark.finkle)
Comment 8•14 years ago
|
||
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•14 years ago
|
||
Whiteboard: [inbound]
Comment 10•14 years ago
|
||
Status: NEW → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 8
Reporter | ||
Comment 11•14 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•14 years ago
|
Status: RESOLVED → VERIFIED
Reporter | ||
Comment 12•14 years ago
|
||
It turns out the issue mentioned in comment 4 now suffers from bug 674531.
Comment 13•14 years ago
|
||
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•14 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+
Updated•14 years ago
|
tracking-fennec: --- → 7+
tracking-firefox7:
? → ---
Comment 15•14 years ago
|
||
Comment 16•14 years ago
|
||
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.
Description
•