Closed Bug 677118 Opened 11 years ago Closed 11 years ago

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

Categories

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

ARM
Android
defect

Tracking

(fennec+)

VERIFIED FIXED
Firefox 10
Tracking Status
fennec + ---

People

(Reporter: martijn.martijn, Assigned: sriram)

References

()

Details

(Keywords: regression, testcase)

Attachments

(2 files, 2 obsolete files)

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.
Duplicate of this bug: 685872
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 PatchSplinter 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: 11 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.