Zoomed View appears when the two links are the same link

RESOLVED FIXED in Firefox 42

Status

()

RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: mcomella, Assigned: domivinc)

Tracking

unspecified
Firefox 42
All
Android
Points:
---

Firefox Tracking Flags

(firefox42 fixed)

Details

Attachments

(2 attachments, 4 obsolete attachments)

Created attachment 8639616 [details]
Screenshot of the issue

STR:
1) Go to reddit.com
2) Find a link that wraps two lines
3) Click in between the two lines of the link

Expected: The link opens
Actual: The zoomed view appears

See the attached screenshot.
Assignee: nobody → markcapella
Status: NEW → ASSIGNED
(Assignee)

Comment 1

3 years ago
Created attachment 8640166 [details] [diff] [review]
patch-28072015 1-Bug_1188185___Zoomed_View_appears_when_the_two_links_are_the_same_link__r_kats.patch

Mark, I already tested this patch to fix the issue. It seems to work...
Ah nice! I have a working patch I was polishing but this is more succinct :-) Feel free to steal
Created attachment 8640207 [details] [diff] [review]
bug1188185.diff

For yucks, mine was a little more involved but functional :-)
Assignee: markcapella → nobody
Status: ASSIGNED → NEW
(Assignee)

Updated

3 years ago
Assignee: nobody → domivinc
(Assignee)

Updated

3 years ago
Attachment #8640166 - Flags: review?(bugmail.mozilla)
(Assignee)

Comment 4

3 years ago
Created attachment 8640458 [details] [diff] [review]
patch-29072015 Ter 1-Bug_1188185___Zoomed_View_appears_when_the_two_links_are_the_same_link__r_kats.patch

Kats, debugging an issue for bug 1186738, I found that my previous patch didn’t work for the html structure used by the download page. I didn’t record the correct clickable content. I made the change to get the correct clickable content and record it to avoid duplicate in the list of elements mContentsInCluster.

I also tested my patch using the reddit page, and I found an issue on some areas of the page. Try to click on the “ to “  just after a link for instance “by Andrew to /r/pics”. The “ to “ is near 3 links and the zoomed view is never displayed. To fix this issue, I added the test of aClickableAncestor (bug 1181763) here:
if (!aClickableAncestor && !nsLayoutUtils::IsAncestorFrameCrossDoc(aRestrictToDescendants, f, aRoot)) {

The current patch implements the 2 changes.
Attachment #8640166 - Attachment is obsolete: true
Attachment #8640166 - Flags: review?(bugmail.mozilla)
Attachment #8640458 - Flags: review?(bugmail.mozilla)
I haven't looked at the patch too closely yet but if you have some time it would be nice to add tests for these changes as well. See test_event_target_radius.html for examples.
Ah, nice :)
Comment on attachment 8640458 [details] [diff] [review]
patch-29072015 Ter 1-Bug_1188185___Zoomed_View_appears_when_the_two_links_are_the_same_link__r_kats.patch

Review of attachment 8640458 [details] [diff] [review]:
-----------------------------------------------------------------

Overall the idea looks fine but there are some comments/questions below. I also worry a bit about the performance overhead of all this walking around in the tree multiple times and doing linear searches in the vector. However I'm ok with leaving it like this for now and then optimizing it later if it turns out to be a performance bottleneck.

::: layout/base/PositionedEventTargeting.cpp
@@ +191,1 @@
>  IsElementClickable(nsIFrame* aFrame, nsIAtom* stopAt = nullptr, nsAutoString* aLabelTargetId = nullptr)

Please rename this to GetClickableAncestor

@@ +394,5 @@
>          PET_LOG("  candidate %p is not a descendant of required ancestor\n", f);
>          continue;
>        }
> +      clickableContent = IsElementClickable(f, nsGkAtoms::body, &labelTargetId);
> +    } else if ((clickableContent = IsElementClickable(f, nsGkAtoms::body, &labelTargetId)) == nullptr) {

Can you rearrange the code like so:

nsAutoString labelTargetId;
if (aClickableAncestor && !IsDescendant(...)) {
  PET_LOG(...);
  continue;
}
nsIContent* clickableContent = IsElementClickable(f, ...);
if (!aClickableAncestor && clickableContent == nullptr) {
  PET_LOG(...);
  continue;
}

@@ +404,5 @@
>      if (bestTarget && nsLayoutUtils::IsProperAncestorFrameCrossDoc(f, bestTarget, aRoot)) {
>        PET_LOG("  candidate %p was ancestor for bestTarget %p\n", f, bestTarget);
>        continue;
>      }
> +    if (!aClickableAncestor && !nsLayoutUtils::IsAncestorFrameCrossDoc(aRestrictToDescendants, f, aRoot)) {

I don't understand why this condition needs changing.

@@ +528,5 @@
>      PET_LOG("Retargeting disabled\n");
>      return target;
>    }
>    nsIContent* clickableAncestor = nullptr;
> +  if (target && (clickableAncestor = IsElementClickable(target, nsGkAtoms::body)) != nullptr) {

I don't like having assignments in the middle of conditionals, it makes it hard to read. Please rearrange this.
Attachment #8640458 - Flags: review?(bugmail.mozilla) → review-
(Assignee)

Comment 8

3 years ago
Created attachment 8640989 [details] [diff] [review]
patch-30072015 1-Bug_1188185___Zoomed_View_appears_when_the_two_links_are_the_same_link__r_kats.patch

All the changes done!

About your question here:
>>>>>>
if (!aClickableAncestor && !nsLayoutUtils::IsAncestorFrameCrossDoc(aRestrictToDescendants, f, aRoot)) {
I don't understand why this condition needs changing.
<<<<<<

First if there is aClickableAncestor, the isAncestorFrameCrossDoc test is useless, you already tested that the frame f was a descendant of aClickableAncestor here: 
if (aClickableAncestor && !IsDescendant(f, aClickableAncestor, &labelTargetId)) 

Second, in the reddit page, it looks like this test "isAncestorFrameCrossDoc" eliminates some valid links.
Try to click on the “ to “  just after a link for instance “by Andrew to /r/pics”. The “ to “ is near 3 links and the zoomed view is never displayed. The frame of those links are eliminated by the test "isAncestorFrameCrossDoc".
The reddit page is really complex in term of number of click listeners and frames. 
I suspect that the frame f and the element aClickableAncestor are in the same root element, but this root element is not the frame "aRoot". In this case the frame f is wrongly eliminated by the test "isAncestorFrameCrossDoc".
Attachment #8640458 - Attachment is obsolete: true
Attachment #8640989 - Flags: review?(bugmail.mozilla)
(In reply to Dominique Vincent [:domivinc] from comment #8)
> First if there is aClickableAncestor, the isAncestorFrameCrossDoc test is
> useless, you already tested that the frame f was a descendant of
> aClickableAncestor here: 
> if (aClickableAncestor && !IsDescendant(f, aClickableAncestor,
> &labelTargetId)) 

Ok, this part makes sense.

> Second, in the reddit page, it looks like this test
> "isAncestorFrameCrossDoc" eliminates some valid links.
> Try to click on the “ to “  just after a link for instance “by Andrew to
> /r/pics”. The “ to “ is near 3 links and the zoomed view is never displayed.
> The frame of those links are eliminated by the test
> "isAncestorFrameCrossDoc".
> The reddit page is really complex in term of number of click listeners and
> frames. 
> I suspect that the frame f and the element aClickableAncestor are in the
> same root element, but this root element is not the frame "aRoot". In this
> case the frame f is wrongly eliminated by the test "isAncestorFrameCrossDoc".

This part sounds like a bug. Can you turn on the PET_LOG and frame tree dump to see what the frame tree looks like and what's going on here? I think this needs more investigation.
Comment on attachment 8640989 [details] [diff] [review]
patch-30072015 1-Bug_1188185___Zoomed_View_appears_when_the_two_links_are_the_same_link__r_kats.patch

Review of attachment 8640989 [details] [diff] [review]:
-----------------------------------------------------------------

r+ with nits addressed, but I'd still like to know why the restrictroot check is filtering things when it shouldn't be.

::: layout/base/PositionedEventTargeting.cpp
@@ +392,2 @@
>          PET_LOG("  candidate %p is not a descendant of required ancestor\n", f);
>          continue;

adjust indentation here

@@ +393,5 @@
>          continue;
> +    }
> +
> +    nsIContent* clickableContent = GetClickableAncestor(f, nsGkAtoms::body, &labelTargetId);
> +    if (!aClickableAncestor && clickableContent == nullptr) {

replace the ==nullptr with a ! for consistency

@@ +529,5 @@
>    }
>    nsIContent* clickableAncestor = nullptr;
> +  if (target) {
> +    clickableAncestor = GetClickableAncestor(target, nsGkAtoms::body);
> +    if (clickableAncestor != nullptr) {

drop the !=nullptr
Attachment #8640989 - Flags: review?(bugmail.mozilla) → review+
(Assignee)

Comment 11

3 years ago
Created attachment 8641701 [details] [diff] [review]
patch-31072015 BIS 1-Bug_1188185___Zoomed_View_appears_when_the_two_links_are_the_same_link__r_kats.patch

Last small changes done. 
I'm going to investigate more in details the reddit page issue.
Attachment #8640989 - Attachment is obsolete: true
(Assignee)

Comment 13

3 years ago
I cannot reproduce the following issue I got 2 days ago:

> The frame of those links are eliminated by the test
> "isAncestorFrameCrossDoc".

The issue was probably due to my one my code changes. 
But I'm going to keep and eye on this area of the reddit page...

Regarding the tests, you want to add new tests in "test_event_target_radius.html" or you want to add another test case with a new test html page?
(Assignee)

Updated

3 years ago
Keywords: checkin-needed
(In reply to Dominique Vincent [:domivinc] from comment #13)
> Regarding the tests, you want to add new tests in
> "test_event_target_radius.html" or you want to add another test case with a
> new test html page?

Either is fine.
Attachment #8640207 - Attachment is obsolete: true
https://hg.mozilla.org/mozilla-central/rev/5ff82587c660
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox42: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 42
You need to log in before you can comment on or make changes to this bug.