1188185 - Zoomed View appears when the two links are the same link

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

Attachment: 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.

Comment 1

[Dominique Vincent [:domivinc]]

Attachment: 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...

Comment 2

[Mark Capella [:capella]]

Ah nice! I have a working patch I was polishing but this is more succinct :-) Feel free to steal

Comment 3

[Mark Capella [:capella]]

Attachment: bug1188185.diff

For yucks, mine was a little more involved but functional :-)

Comment 4

[Dominique Vincent [:domivinc]]

Attachment: 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.

Comment 5

[Kartikaya Gupta (email:[e-mail])]

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.

Comment 6

[Mark Capella [:capella]]

Ah, nice :)

Comment 7

[Kartikaya Gupta (email:[e-mail])]

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.

Comment 8

[Dominique Vincent [:domivinc]]

Attachment: 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".

Comment 9

[Kartikaya Gupta (email:[e-mail])]

(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 10

[Kartikaya Gupta (email:[e-mail])]

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

Comment 11

[Dominique Vincent [:domivinc]]

Attachment: 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.

Comment 12

[Dominique Vincent [:domivinc]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=8ee7da72d1d4

Comment 13

[Dominique Vincent [:domivinc]]

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?

Comment 14

[Kartikaya Gupta (email:[e-mail])]

(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.

Comment 15

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/5ff82587c660

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/5ff82587c660