Closed Bug 677118 Opened 13 years ago Closed 13 years ago

Long tapping on link which has div inside does not show link context menu

Categories

(Firefox for Android Graveyard :: General, defect, P2)

Product:
Firefox for Android Graveyard
Component:
General
Platform:
ARM
Android
Type:
defect

Tracking

(fennec+)

Status:
VERIFIED FIXED
Milestone:
Firefox 10
Tracking Flags:
Tracking Status
fennec + ---

People

(Reporter: martijn.martijn, Assigned: sriram)

References

(
URL
)

Details

(Keywords: regression, testcase)

Attachments

(2 files, 2 obsolete files)

testcase
359 bytes, text/html
Details
Patch
1.85 KB, patch
mbrubeck
: review+
mfinkle
: review+
Details | Diff | Splinter Review
Attached file testcase — 
See testcase, steps to reproduce:
- Long tap on the link in the testcase

Expected result:
- Link context menu comes up

Actual result:
- Link is followed to google.

This regressed with the fix for bug 661388.
OS: Windows 7 → Android
Priority: -- → P2
Hardware: x86 → ARM
And now, the link isn't followed at all, now, selection comes up with selection handles. I guess this could be a regression from bug 666977.

I'm seeing this problem on http://mobiel.nujij.nl/ , btw.
URL: http://mobiel.nujij.nl/
tracking-fennec: --- → ?
Assignee: nobody → sriram
tracking-fennec: ? → +
Attached patch WIP (obsolete) — — Splinter Review 
My approach is to go up the DOM tree until one hits the BODY tag.
If there is a <a> tag on the way up the DOM tree, it breaks there, and that is recorded.
This way, context menu popsup for <a> tag.
If there is no <a> tag, then the state is recorded as text (as it was earlier).

Worst case scenario: Long press on a piece of text, which does the traversal all the way up to <BODY> tag.
Best case scenario: The test case scenario attached with this bug.
Attachment #562596 - Flags: review?(mbrubeck)
Attachment #562596 - Flags: review?(mark.finkle)
Comment on attachment 562596 [details] [diff] [review]
WIP

>+    while (elem && !(elem instanceof Ci.nsIDOMHTMLBodyElement)) {
>       // ...
>       elem = elem.parentNode;
>     }
> 
>+    if(elem instanceof Ci.nsIDOMHTMLBodyElement) {
>+      state.types.push("content-text");
>+    }

It seems like this will effectively enable text selection on any element on any HTML page, as long as none of the other types are detected.  That actually would be fine with me, but it would be simpler to do this by removing the whole "content-text" type from the while loop here (and removing the check for it in SelectionHelper.js).
Attachment #562596 - Flags: review?(mbrubeck) → review-
Attached patch Patch (obsolete) — — Splinter Review 
This addresses the issue as you had asked.
Attachment #562596 - Attachment is obsolete: true
Attachment #562596 - Flags: review?(mark.finkle)
Attachment #562885 - Flags: review?(mbrubeck)
Comment on attachment 562885 [details] [diff] [review]
Patch

>+    if(isText)

Add a space after "if".

r=mbrubeck with that nit fixed.  Let's have Mark take another look at this too.
Attachment #562885 - Flags: review?(mbrubeck)
Attachment #562885 - Flags: review?(mark.finkle)
Attachment #562885 - Flags: review+
Attached patch Patch — — Splinter Review 
Made the change to "if" clause.
Attachment #562885 - Attachment is obsolete: true
Attachment #562885 - Flags: review?(mark.finkle)
Attachment #562891 - Flags: review?(mbrubeck)
Attachment #562891 - Flags: review?(mark.finkle)
Comment on attachment 562891 [details] [diff] [review]
Patch

You can keep the r+ if I have previously r+'d a patch and you are just fixing review nits.
Attachment #562891 - Flags: review?(mbrubeck) → review+
Attachment #562891 - Flags: review?(mark.finkle) → review+
What's the status of this bug?  Seems someone forgot to add checkin-needed to the keywords
Thanks for the reminder.  Pushed to inbound:
http://hg.mozilla.org/integration/mozilla-inbound/rev/8ecc8e76bf7b
Status: NEW → ASSIGNED
Target Milestone: --- → Firefox 10
https://hg.mozilla.org/mozilla-central/rev/8ecc8e76bf7b
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Flags: in-litmus?
Resolution: --- → FIXED
Flags: in-litmus? → in-litmus?(martijn.martijn)
Verified fixed using 20111010 nightly on EVO4G running CyanogenMod 7.1
Status: RESOLVED → VERIFIED
Flags: in-litmus?(martijn.martijn)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: