1191277 - Detect ancestor and descendant in the list of cluster elements.

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[Dominique Vincent [:domivinc]]

This bug is a follow up of comment 30 of bug 1135369.

On some pages (ie mobile.twitter.com), the whole content of the page has touch listener. In this case, the method HasTouchListener in the cluster detection process detects that each element has a listener (any text in the page). In fact the listener is not on the text element but on a parent element.
The element returned by the method GetClickableAncestor is not the ancestor with the listener but the element without any listener.

Without the correct clickable ancestor, the cluster detection process cannot work correctly. 


[1] https://bugzilla.mozilla.org/show_bug.cgi?id=1135369#c30

Comment 1

[Dominique Vincent [:domivinc]]

Kats, it looks like that:

 EventListenerManager* elm = aContent->GetExistingListenerManager();

returns all the listeners, the element listeners but also the listeners of the ancestors.

Is it possible to get only the listeners attached to the element aContent?

Comment 2

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

(In reply to Dominique Vincent [:domivinc] from comment #0)
> This bug is a follow up of comment 30 of bug 1135369.
> 
> [1] https://bugzilla.mozilla.org/show_bug.cgi?id=1135369#c30

Pro tip: If you write "bug 1135369 comment 30" bugzilla will auto-link it to the right thing.

(In reply to Dominique Vincent [:domivinc] from comment #1)
>  EventListenerManager* elm = aContent->GetExistingListenerManager();
> 
> returns all the listeners, the element listeners but also the listeners of
> the ancestors.

To the best of my knowledge GetExistingListenerManager should return only the listeners for aContent, and not for the ancestors. That's why GetClickableAncestor does the check on every iteration of the loop as it walks up the ancestor chain. I've used GetExistingListenerManager() in other places in the code base as well and there too I'm pretty sure it only gives the listeners for the content element it's attached to.

Comment 3

[Dominique Vincent [:domivinc]]

Attachment: patch-08082015 3-Bug_1191277___Detect_ancestor_and_descendant_in_the_list_of_cluster_elements__r_kats.patch

Thanks Kats for the tips!
Regarding GetExistingListenerManager it works the way you described. The issue is not here (I updated the title of the bug).

The elements kept in the GetClosest loop contain some duplicates in the case of the html structure used by the twitter page. There is a touch listener on each row, and inside each row you have 2 html links. The listener on each row is an ancestor of the 2 other links. It’s this case that is not correctly detected by the cluster detection process.
I added a new test to avoid the addition of the duplicated element.

Comment 4

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

Comment on attachment 8645267 [details] [diff] [review]
patch-08082015 3-Bug_1191277___Detect_ancestor_and_descendant_in_the_list_of_cluster_elements__r_kats.patch

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

::: layout/base/PositionedEventTargeting.cpp
@@ +441,5 @@
> +          break;
> +        }
> +      }
> +      if (!isAncestorOrDescendant &&
> +          std::find(mContentsInCluster.begin(), mContentsInCluster.end(), clickableContent) == mContentsInCluster.end()) {

This std::find call seems to be redundant. If clickableContent is in mContentsInCluster then the IsDescendant check above will return true and isAncestorOrDescendant will be true.

However I also feel like this code is getting convoluted. I want to think about this some more before finalizing the review.

Comment 5

[Dominique Vincent [:domivinc]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #4)
> Comment on attachment 8645267 [details] [diff] [review]

> This std::find call seems to be redundant. If clickableContent is in
> mContentsInCluster then the IsDescendant check above will return true and
> isAncestorOrDescendant will be true.

Correct, I didn't see this point. The method "IsDescendant(nsIContent* aContent, nsIContent* aAncestor)" should return true if "aAncestor" is equal to "aContent". In this case "std::find" is redundant and we should probably rename "IsDescendant" in "IsSelfOrDescendant" ?

Comment 6

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

(In reply to Dominique Vincent [:domivinc] from comment #3)
> The elements kept in the GetClosest loop contain some duplicates in the case
> of the html structure used by the twitter page. There is a touch listener on
> each row, and inside each row you have 2 html links. The listener on each
> row is an ancestor of the 2 other links. It’s this case that is not
> correctly detected by the cluster detection process.
> I added a new test to avoid the addition of the duplicated element.

I don't think I'm following this. From your description it sounds to me like there's this:

<tr ontouchstart="..."><a href="...">linkA</a> <a href="...">linkB</a></tr>

Now in the GetClosest loop, I assume you see linkA, linkB, and the <tr> all get processed. For all of these elements, the clickableContent will be the element itself, since all of them are clickable. At least linkA and linkB will get added to mContentsInCluster (the <tr> might get rejected because of [1], depending on the ordering). Either way, mContentsInCluster.size() is going to be > 1 and so we'll trigger the zoomed view.

Is that correct? If so, why is this undesirable? If there are two links that are inside the touch radius then we should be triggering the zoomed view, so this sounds like expected behaviour to me. Can you please provide more context on the structure of the page and what you expect to see?

[1] http://mxr.mozilla.org/mozilla-central/source/layout/base/PositionedEventTargeting.cpp?rev=5ff82587c660#401

Comment 7

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

Comment on attachment 8645267 [details] [diff] [review]
patch-08082015 3-Bug_1191277___Detect_ancestor_and_descendant_in_the_list_of_cluster_elements__r_kats.patch

Clearing flag for now.

Comment 8

[Dominique Vincent [:domivinc]]

Attachment: patch-11082015 BIS 3-Bug_1191277___Detect_ancestor_and_descendant_in_the_list_of_cluster_elements__r_kats.patch

First, I found something strange with the process using aClickableAncestor. All the tests using aClickableAncestor are useless because the element aClickableAncestor is kept in the getClosest loop. This element is always the winner because the distance is equal to 0: the click occurs on this element!
I think that aClickableAncestor should be eliminated from the loop using the following code:

if (aClickableAncestor && aClickableAncestor == clickableContent) {
      PET_LOG("  candidate %p was the required ancestor\n", f);
      continue;
 }

With the previous change, I can remove the elimination process in case of Ancestor or Descendant in the cluster elements list. It works on the twitter page.
But I’m still wondering if we should take care of Ancestor or Descendant in the cluster elements list:
One clickable container with several clickable children should be counted for one main element in the cluster list. 
Two clickable containers with several clickable children in each container should be counted for 2 elements in the cluster list.

Comment 9

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

Comment on attachment 8646138 [details] [diff] [review]
patch-11082015 BIS 3-Bug_1191277___Detect_ancestor_and_descendant_in_the_list_of_cluster_elements__r_kats.patch

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

Please answer the questions from my previous comment.

Comment 10

[Dominique Vincent [:domivinc]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #6)
> I don't think I'm following this. From your description it sounds to me like
> there's this:
> 
> <tr ontouchstart="..."><a href="...">linkA</a> <a href="...">linkB</a></tr>
> 
> Now in the GetClosest loop, I assume you see linkA, linkB, and the <tr> all
> get processed. For all of these elements, the clickableContent will be the
> element itself, since all of them are clickable. At least linkA and linkB
> will get added to mContentsInCluster (the <tr> might get rejected because of
> [1], depending on the ordering). Either way, mContentsInCluster.size() is
> going to be > 1 and so we'll trigger the zoomed view.
> 
> Is that correct? If so, why is this undesirable? If there are two links that
> are inside the touch radius then we should be triggering the zoomed view, so
> this sounds like expected behaviour to me. Can you please provide more
> context on the structure of the page and what you expect to see?
> 
> [1]
> http://mxr.mozilla.org/mozilla-central/source/layout/base/
> PositionedEventTargeting.cpp?rev=5ff82587c660#401

As mentioned in the first comment, the structure with the issue is the mobile page of twitter site: mobile.twitter.com
The description you made is correct <tr><linkA><linkB></tr>. In fact the item in the twitter page contains also some text areas: TEXTOUTSIDE<tr ontouchstart="...">TEXT1<linkA>TEXT2<linkB>TEXT3</tr>TEXTOUTSIDE.

Three different points to analyse the twitter cases (A and B) and explore an hypothetical case (C):

A) The behavior you described here: "the <tr> might get rejected because of [1], depending on the ordering" means that in your case the user clicked outside the TR element (TEXTOUTSIDE), the frames captured by the touch area contain a small part of each of the 3 elements: TR, linkA and linkB. 
In this case the patches I provided shouldn't change the current behavior: a cluster is detected with 2 links. This is the "normal" case.

B) Now if the user clicks on TEXT1 , let's say that the frames captured by the touch area contain a small part of each of the 2 elements: TR and linkA. The <tr> should not be processed in the GetClosest loop because it is the clickable ancestor "aClickableAncestor". In the case of the twitter page, the <tr> is in the loop, and it's considered as the closest link (best distance equal to 0). The last patch I provided is a fix for that. The number of cluster elements should be 1 and not 2. Without my fix, the tr element is counted, so the number of elements in the cluster list is not correct (the original issue reported in comment 1). Without my fix the linkA is not correctly highlighted. 

C) Now in my last comment, I mentioned this point " ... if we should take care of Ancestor or Descendant in the cluster elements list ...".
My remark is linked to the following hypothetical case:
TEXTOUTSIDE<tr ontouchstart="...">TEXT1<linkA>TEXT2</tr>TEXTOUTSIDE.
The potential issue is linked to the way the GetClosest loop is done and the order of the frames in the loop. The user clicked in TEXTOUTSIDE. Let's say that the frames captured by the touch area contain a small part of each of the 2 elements: TR and linkA.

If the order of the frames in the loop is TR first then linkA.
TR is kept, added in the cluster elements list. linkA is kept, added in the cluster elements list. The number of elements in the cluster list is 2.

Now same structure, but the order of the frames is different in the loop: linkA first and then TR.
linkA is kept, added in the cluster elements list. TR is rejected due to [1]. The number of elements in the cluster list is 1.

With the same structure but a different order of the frames, the number of elements is different. I think that we should also take care of Ancestor or Descendant in the cluster elements list. It could be done using the first patch. And the number of elements in the cluster list should be 1 applying always the same rule [1] (nested frame).
But as mentioned earlier, it's an hypothetical case ... We could wait to see if it's really an issue in some complex pages.

[1] http://mxr.mozilla.org/mozilla-central/source/layout/base/PositionedEventTargeting.cpp?rev=5ff82587c660#401

Comment 11

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

Ok, thanks for the explanation. I think I understand the different scenarios, and I have some suggestions on how to address this efficiently:

Patch 1: Change mContentsInCluster from a std::vector to a std::map to improve performance. Instead of having to do std::find followed by a conditional push_back, you can just mContentsInCluster[clickableContent] = clickableContent;
This will use a hash lookup and be roughly O(1) rather than linear with no loss in functionality for what we're using it for.

Patch 2: Fix the hypothetical ordering issue. This is to address the scenario where we get a different "bestTarget" result if the TR is processed first vs if the linkA is processed first. To do this, we need to add a condition at the bottom of loop where:
if (distance == bestDistance && nsLayoutUtils::IsProperAncestorFrameCrossDoc(bestTarget, f, aRoot)) {
  // update bestTarget to f
}
This handles the case where the TR is processed first; when we encounter the linkA we should switch the bestTarget to linkA (assuming it has the same distance). It should also handle the problem you're describing in comment 8 because now we can still end up picking descendants of aClickableContent.

Patch 3: Fix the mContentsInCluster size. With the above two patches, we still have the problem where if the TR is processed first, both the TR and linkA will be in mContentsInCluster and the size of that will 2. But if the linkA is processed first, the TR is rejected and mContentsInCluster will have size 1. This is basically the bug you originally filed. To fix this, we change the code from patch 1 so that when we insert clickableContent to mContentsInCluster, we also remove all ancestors of clickableContent from mContentsInCluster. By keeping only the deepest "leaf" nodes that can be targeted we ensure we have an accurate count of targets.

Does that all make sense?

Comment 12

[Dominique Vincent [:domivinc]]

Attachment: patch-12082015 3-Bug_1191277___Detect_ancestor_and_descendant_in_the_list_of_cluster_elements__r_kats.patch

Kats, my main concern is with your “Patch 2”: it doesn’t work. When the TR element is wrongly kept (in the current version of the code), it’s because the TR distance is equal to 0, and the linkA distance is higher than 0. 
You should perhaps trace the loop to get a better view of this case:

>>>>>
B) Now if the user clicks on TEXT1 , let's say that the frames captured by the touch area contain a small part of each of the 2 elements: TR and linkA. The <tr> should not be processed in the GetClosest loop because it is the clickable ancestor "aClickableAncestor".
<<<<<

This case is not an issue in the cluster counter, it’s an issue in the highlighted link process. When this issue will be corrected, the cluster counter will be correct (at least on the twitter page).
The touch occurs in the TR element, so we are in the case where a clickable ancestor exists. The clickable ancestor is the TR element. With your proposal, the linkA will never be highlighted. 
In the last patch, I still eliminate the “TR” element using the following test:
if (aClickableAncestor && aClickableAncestor == clickableContent) {


In your “Patch 1” I understood that you want to change:
std::vector<nsIContent*> mContentsInCluster; 
by:
std::map<nsIContent*, nsIContent*> mContentsInCluster;
in order to be able to make:
mContentsInCluster[clickableContent] = clickableContent;

I’m not a C++ expert so I’m really not sure of the following arguments in favor of the std::vector!
With the map, we will gain in processing time when we have to retrieve an element. But we will lose in term of memory usage for all the elements.
With the vector, we will lose in processing time if we have to retrieve one specific element. But we will gain in memory usage.

In the last patch I tried to avoid the processing time to retrieve one specific element using std:vector: I already have the element I want to remove, because I remove it in the loop.


Your “Patch 3”: It was my first explanation of this strange behavior in the twitter page. But I was wrong. This case doesn’t occur in the twitter page. But I implemented the fix of the deepest "leaf" nodes to ensure we have an accurate count in the cluster list.



Sorry, I made only one patch but I tried to add a maximum of comments. You should easily retrieve the 3 different cases we have to fix: 

1- the current element is the "aClickableAncestor" element
2- the current element is ignored because it is an ancestor of an existing element in the cluster list
3- the current element replaces an existing element of the cluster list because it's a descendant

Case 1 is an issue in the highlighted link process.
Cases 2 and 3 are two issues in the cluster list.

Comment 13

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

(In reply to Dominique Vincent [:domivinc] from comment #12)
> 
> Kats, my main concern is with your “Patch 2”: it doesn’t work. When the TR
> element is wrongly kept (in the current version of the code), it’s because
> the TR distance is equal to 0, and the linkA distance is higher than 0. 
> You should perhaps trace the loop to get a better view of this case:

To me this doesn't sound like a bug. If the TR distance is 0 and the linkA distance is > 0, that means the user touched inside the TR but outside linkA. Since the TR is clickable, that's what should get clicked, and that's what happens. I don't know why you are expecting the linkA to be highlighted/clicked when it's not inside the touch radius and the TR is.

> >>>>>
> B) Now if the user clicks on TEXT1 , let's say that the frames captured by
> the touch area contain a small part of each of the 2 elements: TR and linkA.
> The <tr> should not be processed in the GetClosest loop because it is the
> clickable ancestor "aClickableAncestor".
> <<<<<
> 
> This case is not an issue in the cluster counter, it’s an issue in the
> highlighted link process. When this issue will be corrected, the cluster
> counter will be correct (at least on the twitter page).
> The touch occurs in the TR element, so we are in the case where a clickable
> ancestor exists. The clickable ancestor is the TR element. With your
> proposal, the linkA will never be highlighted. 

Yes. If the clickable ancestor is the TR element then the linkA should *not* be highlighted unless the user directly clicks on linkA. If the user *does* directly click on linkA, then linkA will also have a distance of 0. Since the TR is an ancestor of linkA then what I described as "Patch 2" would cause linkA to be selected.

> In your “Patch 1” I understood that you want to change:
> std::vector<nsIContent*> mContentsInCluster; 
> by:
> std::map<nsIContent*, nsIContent*> mContentsInCluster;
> in order to be able to make:
> mContentsInCluster[clickableContent] = clickableContent;
> 
> I’m not a C++ expert so I’m really not sure of the following arguments in
> favor of the std::vector!
> With the map, we will gain in processing time when we have to retrieve an
> element. But we will lose in term of memory usage for all the elements.
> With the vector, we will lose in processing time if we have to retrieve one
> specific element. But we will gain in memory usage.

Agreed, yes, there is a memory/speed tradeoff here. However the memory allocation is temporary (stack space inside the function) so I'm not too concerned about that here, I would rather have the faster implementation.

Comment 14

[Dominique Vincent [:domivinc]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #13)
> (In reply to Dominique Vincent [:domivinc] from comment #12)
> > 
> > Kats, my main concern is with your “Patch 2”: it doesn’t work. When the TR
> > element is wrongly kept (in the current version of the code), it’s because
> > the TR distance is equal to 0, and the linkA distance is higher than 0. 
> > You should perhaps trace the loop to get a better view of this case:
> 
> To me this doesn't sound like a bug. If the TR distance is 0 and the linkA
> distance is > 0, that means the user touched inside the TR but outside
> linkA. Since the TR is clickable, that's what should get clicked, and that's
> what happens. I don't know why you are expecting the linkA to be
> highlighted/clicked when it's not inside the touch radius and the TR is.
> 

In this case I don't understand all the code changes implemented in bug 1181763.
The GetClosest method is called for nothing because we already know that the best element will be aClickableAncestor.
The touch point is inside this clickable element, a frame of this element will be in the loop, the distance with this frame linked to this element will be 0! The closest element returned is always aClickableAncestor.

My understanding of your code changes in bug 1181763 was the following: when the user clicks in a clickable container (aClickableAncestor) we are trying to find some clickable children in the touch radius. Your idea is to give the priority to the children in the touch radius. If there are no clickable children in the touch radius, the target will be the clickable container. I think that it's the best way to handle this kind of structure : clickable container with clickable children.

With the current code it doesn't work because the method IsDescendant(nsIFrame* aFrame, nsiContent* aAncestor....) returns true for the frames of aAncestor.
This method should return false: an element cannot be a descendant of itself.

My patch fix this issue in another way but if you don't want to fix it, we should remove all the changes done in bug 1181763. The issues in the twitter page are coming from this change.

Comment 15

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

Ok, sorry, I got a little confused with what the "distance" was measuring. For some reason I thought it was the distance to the fluff zone rather than the actual touch point so a distance > 0 meant it was outside the fluff zone. That was obviously wrong, since all elements processed inside the GetClosest loop are already inside the fluff zone.

So yes, I agree with you that if both TR and linkA are processed inside the GetClosest loop, we should always be picking linkA. However now I'm confused because there is a test that tests this exact scenario [1] and it's currently passing on m-c.

Looking at the code again, I think that GetFramesForArea always returns the frames in a specific order (in fact it sorts them explicitly in [2]). In the case of the test, it is probably returning the inner element (equivalent to linkA) earlier in the array, with the clickable ancestor (equivalent to the TR) later in the array. That means the TR gets rejected in the GetClosest loop as an ancestor of the "bestTarget". Maybe in the twitter case you have that is not the case, and the TR is coming earlier than the linkA? If so then we just need to address that. Does that make sense, or am I talking crazy again?

[1] http://mxr.mozilla.org/mozilla-central/source/layout/base/tests/test_event_target_radius.html?rev=285d33e0631d&force=1#196
[2] http://mxr.mozilla.org/mozilla-central/source/layout/base/nsDisplayList.cpp?rev=751d8abf0b76#1805

Comment 16

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

Assuming comment 15 makes sense, I'd like to see a reduced test case for this new scenario where the loop processes the TR first. And the "Patch 2" that I described above might still work if we just removed the "distance == bestDistance" part of the check - that is, if we encounter a descendant of the bestTarget we unconditionally take it.

Comment 17

[Dominique Vincent [:domivinc]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #15)

> So yes, I agree with you that if both TR and linkA are processed inside the
> GetClosest loop, we should always be picking linkA. However now I'm confused
> because there is a test that tests this exact scenario [1] and it's
> currently passing on m-c.
> 
There is a missing part in [1] here:

<div class="target" id="t6" onmousedown="x=1" >
    <div id="t6_inner" style="position:absolute; left:-40px; top:20px; width:60px; height:60px; background:yellow;"></div>
  </div>

Try to add onmousedown="x=1" on the "t6_inner" element like that:

<div class="target" id="t6" onmousedown="x=1" >
    <div id="t6_inner" style="position:absolute; left:-40px; top:20px; width:60px; height:60px; background:yellow;"  onmousedown="x=1"></div>
  </div>

Comment 18

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

The test passes for me with that change also. t6_inner is considered clickable regardless because its parent (t6) is clickable.

Comment 19

[Dominique Vincent [:domivinc]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #16)
> Assuming comment 15 makes sense, I'd like to see a reduced test case for
> this new scenario where the loop processes the TR first. And the "Patch 2"
> that I described above might still work if we just removed the "distance ==
> bestDistance" part of the check - that is, if we encounter a descendant of
> the bestTarget we unconditionally take it.

The twitter case is visible using the following html structure:

  <div class="target" id="t6" onmousedown="x=1" >
	<div>BLA BLA BLA BLA BLA</div>
    <div id="t6_inner"  onmousedown="x=1" style="position:absolute; left:-40px; top:20px; width:60px; height:60px; background:yellow;"></div>
  </div>
  <div id="t6_outer" style="position:absolute; left:160px; top:120px; width:60px; height:60px; background:green;" onmousedown="x=1" ></div>

I'm not sure that your description of the issue is the good one in this case (the order of the TR element ...).
If you click on the "BLA BLA BLA" element, this frame is the best frame but this frame is not an ancestor of the frame for "t6_inner". The clickable container element is the element "t6".
The element "t6_inner" is also kept. 
At the end, the highlighted link is "t6" but it should be "t6_inner". And there is 2 elements in the cluster list. 

Using my patch, the issue is "twitter" case is fixed.
If you click on the "BLA BLA BLA" element, this frame is not the best frame because the clickable element of this frame is the container ("t6"). This frame is rejected.
The element "t6_inner" is kept. 
At the end, the highlighted link is "t6_inner". And there is only one element in the cluster list.

Comment 20

[Dominique Vincent [:domivinc]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #18)
> The test passes for me with that change also. t6_inner is considered
> clickable regardless because its parent (t6) is clickable.

Yes because your test is not exactly like the twitter case.

Comment 21

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

Comment on attachment 8647251 [details] [diff] [review]
patch-12082015 3-Bug_1191277___Detect_ancestor_and_descendant_in_the_list_of_cluster_elements__r_kats.patch

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

(In reply to Dominique Vincent [:domivinc] from comment #19)
> The twitter case is visible using the following html structure:
> 
>   <div class="target" id="t6" onmousedown="x=1" >
> 	<div>BLA BLA BLA BLA BLA</div>
>     <div id="t6_inner"  onmousedown="x=1" style="position:absolute;
> left:-40px; top:20px; width:60px; height:60px; background:yellow;"></div>
>   </div>
>   <div id="t6_outer" style="position:absolute; left:160px; top:120px;
> width:60px; height:60px; background:green;" onmousedown="x=1" ></div>

Ah, thank you! This reduced test case makes sense, and I can see where the problem comes from - both BLA and t6_inner are considered clickable, and even though the user is clicking directly on BLA you want to pick t6_inner as the target. I guess I got confused from comment 10 where we ignored the fact that TEXTOUTSIDE/TEXT1/TEXT2 also get processed in the GetClosest loop. In other words, it's not the TR element that has a distance of 0, it's the TEXT1 element. The TR is (correctly) rejected in the loop, but since it is the clickable ancestor of TEXT1 it gets highlighted anyway. Is that accurate?

Please add this as a new test to test_event_target_radius.html in your patch.

(In reply to Dominique Vincent [:domivinc] from comment #8)
> But I’m still wondering if we should take care of Ancestor or Descendant in
> the cluster elements list:
> One clickable container with several clickable children should be counted
> for one main element in the cluster list. 
> Two clickable containers with several clickable children in each container
> should be counted for 2 elements in the cluster list.

This I'm not sure I agree with. If you have one clickable container with several clickable children I think it makes sense to show the zoomed view to disambiguate between the clickable children.

::: layout/base/PositionedEventTargeting.cpp
@@ +415,5 @@
>        continue;
>      }
> +    if (aClickableAncestor && aClickableAncestor == clickableContent) {
> +      PET_LOG("  candidate %p was the required ancestor\n", f);
> +      continue;

This part makes sense, and I'm ok with it

@@ +435,5 @@
>      // the cluster elements counter
>      if (labelTargetId.IsEmpty() || !IsElementPresent(aCandidates, labelTargetId)) {
> +      bool isAncestor = false;
> +      for(std::vector<nsIContent*>::iterator it = mContentsInCluster.begin(); it != mContentsInCluster.end(); ++it) {
> +        // If one of the cluster element is a descendant of "clickableContent", do not add the

I would rather leave this part out of this patch. As you said it's not necessary right now, it's still a hypothetical fix. I would rather wait until we find a scenario that requires this, and then write a test to go along with it.

Comment 22

[Dominique Vincent [:domivinc]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=5e45d0aa1429

Comment 23

[Dominique Vincent [:domivinc]]

Attachment: patch-14082015 BIS 1-Bug_1191277___Detect_ancestor_and_descendant_in_the_list_of_cluster_elements__r_kats.patch

Comment 24

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

So.. if this patch regresses the behaviour from bug 1181763 there's not much point to landing it since we could just back out 1181763 instead and get the same result. This is just a roundabout way of doing that. I spent some more time thinking about this bug and I've changed my mind (again!). I think that the code as it stands now finds the correct target, but detects clusters when it probably shouldn't. I have an idea on how to fix that.

Comment 25

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

Attachment: Patch

Does this stop the zoomed view from popping up when you click on the twitter thing?

Comment 26

[Dominique Vincent [:domivinc]]

Comment on attachment 8648161 [details] [diff] [review]
Patch

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #25)
> Created attachment 8648161 [details] [diff] [review]
> Patch
> 
> Does this stop the zoomed view from popping up when you click on the twitter
> thing?

From the "zoomed view" point of view, it looks good on the twitter page. I didn't find any difference between the 2 solutions.

Comment 27

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

Attachment: Patch

Updated commit message. Please let me know if you have any other objections to this patch; if not we can get it landed. For the record:

- Prior to bug 1181763, if the user clicked on a frame X that was clickable, that frame X would always be the final target no matter what (we would not go into the GetClosest loop).

- After bug 1181763, if the user clicked on a frame X that was clickable, we would walk up to the ancestor Y with the actual click/touch/whatever listener, and the final target would always be a descendant of Y. In order to find the "best" descendant we would go into the GetClosest loop. In practice this would always find X unless X == Y, in which case it could find a descendant of Y that was inside the fluff zone. Therefore, the final target would be either X or a descendant of X. The problem for the zoomed view is that in the case where X != Y there might be descendants other than X in the fluff zone, and so the cluster count would go up.

- With this patch, if the user clicks on a frame X that is clickable, we explicitly look for a descendant of X as the final target, even if it's not X that has the actual click/touch/whatever listener. During the GetClosest loop we restrict the search to descendants of X in the fluff zone. If there are multiple such targets we will still find a cluster, which I think is correct.

The reason I think the final target should be a descendant of X is because of the same invariant from bug 1181763 - if the user clicks on something ("X") and that thing is clickable, then we should make sure that listeners on X are triggered either directly or via bubbling. Moving the target to some sibling of X can result in X becoming unclickable.

Comment 28

[Dominique Vincent [:domivinc]]

Comment on attachment 8648282 [details] [diff] [review]
Patch

The code looks good to commit (fyi, the reviewer is empty in the commit message).

Comment 29

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/51eebda696d4

Comment 30

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/51eebda696d4