837242 - defect - With ui.mouse.radius.enabled, mouse events may be targeted to an element that is covered by another element

Status: RESOLVED FIXED

(Core :: DOM: UI Events & Focus Handling, defect)

Description

[Matt Brubeck (:mbrubeck)]

FindFrameTargetedByInputEvent (see bug 780847) attempts to retarget touch/mouse events to nearby elements within a certain radius.  In some cases it returns a frame that is completely obscured/covered by other elements, which leads to unexpected results (for example, bug 835175 in the Metro chrome).

Ideally, FindFrameTargetedByInputEvent should only return a frame if it is "visible" to pointer events somewhere within the specified region.

Comment 1

[Matt Brubeck (:mbrubeck)]

Attachment: test case

Steps to reproduce:

1) In about:config set "ui.mouse.radius.enabled" to true, and "ui.mouse.radius.inputSource.touchOnly" to false.

2) Load the attached testcase in a Firefox tab.

3) Open the web console.

4) Click in the box that says "click me."

Expected results: No messages in the log.
Actual results: "FAIL: clicked hidden div" in the log.

NOTE: You should reset the prefs after performing this test, or your mouse will behave somewhat weirdly in the browser.

Comment 2

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Shouldn't be too hard to fix, but will increase the cost of event handling since we'll need to run ComputeVisibility on the display list.

Should there be some minimum size for the visible area as well? If the visible area is very small but nonzero, seems like we wouldn't want that to "count".

Comment 3

[Matt Brubeck (:mbrubeck)]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #2)
> Shouldn't be too hard to fix, but will increase the cost of event handling
> since we'll need to run ComputeVisibility on the display list.

If the cost grows too much, perhaps it could be minimized by calling FindFrameTargetedByInputEvent only for touchstart events.  We shouldn't need it for touchmove or touchend, because the touchstart target automatically "captures" those events.

For mouse events, by default we run the heuristic only on simulated clicks from touch gestures (bug 832101), so we already avoid the cost for normal mouse movements.

> Should there be some minimum size for the visible area as well? If the
> visible area is very small but nonzero, seems like we wouldn't want that to
> "count".

This sounds like an edge case to me: possibly nice to have, but the cases I've seen so far would be handled fine by an all-or-nothing rule.

Comment 4

[Matt Brubeck (:mbrubeck)]

(In reply to Matt Brubeck (:mbrubeck) from comment #3)
> If the cost grows too much, perhaps it could be minimized by calling
> FindFrameTargetedByInputEvent only for touchstart events.

Nevermind, I now see that we already do this.  So the impact is just on touchstart events (and simulated clicks).

Comment 5

[Asa Dotzler [:asa]]

How likely is this to bite users in the wild?

Comment 6

[Matt Brubeck (:mbrubeck)]

I think it's pretty likely.  In particular, this is likely to affect some sites that use fixed-position bars across the top or bottom of the page, or that overlay the page with a full-screen element (e.g. for a "lightbox" effect when displaying a large image or slideshow).

Aside from web content, this has been the source of many subtle bugs in our browser chrome, which we currently need to find and work around individually; I can't guarantee that we will find all such chrome bugs on our own.

Comment 7

[Asa Dotzler [:asa]]

OK. It sounds like we need this.

Comment 8

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #2)
> Shouldn't be too hard to fix, but will increase the cost of event handling
> since we'll need to run ComputeVisibility on the display list.
>
I tried to fix it calling ComputeVisibility for nsDisplayItem in the list returned by http://mxr.mozilla.org/mozilla-central/source/layout/base/nsLayoutUtils.cpp#1833 in nsLayoutUtils::GetFramesForArea but it seems like this list is only made of top level elements (xul:window, html).

It seems like a bit too hard for my layout skills. Roc do you have someone to suggest for helping here?

Fixing this will also let us install it for metro and I hope for Firefox for Android as well. I wonder if it has been landed/backed out there because of this bug? Wes do you know?

Comment 9

[Wesley Johnston (:wesj)]

We have a JS implementation of this on Android, but I'm starting to think I've seen the same bug happen when I was plyaing with making file dir listings mobile friendly this week (bug 875366).

When I attempted to flip on the platform support for us (bug 788073) some tests that rely on firing clicks near (but not on) links stopped broke. I assume those tests were fixed(?) or aren't running for b2g or metro yet...

Comment 10

[Matt Brubeck (:mbrubeck)]

(In reply to Wesley Johnston (:wesj) from comment #9)
> When I attempted to flip on the platform support for us (bug 788073) some
> tests that rely on firing clicks near (but not on) links stopped broke. I
> assume those tests were fixed(?) or aren't running for b2g or metro yet...

The platform pref is disabled in B2G (bug 789358) and we don't have any tests in automation for Metro yet (bug 847442).

Comment 11

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I'm hoping to work on this but I haven't had the time recently...

Comment 12

[Doug Turner (:dougt)]

smaug, is this something you can pick up?

Comment 13

[Olli Pettay [:smaug][[e-mail]]]

hit testing is more layout stuff than event handling. I could look at this, but don't expect
to have time before... end of next week.

Comment 14

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Hmm. ComputeVisibility doesn't do exactly what we want here, because it only takes account of covering elements that are opaque. We need to think about "opaque to mouse clicks" instead.

Comment 15

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I think we need to add to nsDisplayItem::HitTest so that we pass a region instead of a rect and subtract from that region in a similar way to what ComputeVisibility does.

Comment 16

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: Part 1: convert bool parameters to flags

Comment 17

[Robert O'Callahan (:roc) (email my personal email if necessary)]

It seems to me that the hard case to handle is when a clickable element is covered by two or more other elements that don't individually cover the clickable element, but do together.

The natural thing to do will be to pass around an nsRegion* to the hittest methods and make them behave similarly to ComputeVisibility. This is a bit of a pain, though, and we only want to subtract from that region when the EXCLUDE_COVERED_FRAMES flag is set, since some callers to GetFramesForArea need to get all the frames intersecting the area.

Comment 18

[Timothy Nikkel (:tnikkel)]

Attachment: check point

Check point, not usable yet.

Comment 19

[Timothy Nikkel (:tnikkel)]

Attachment: check point

Seems usable now.

Comment 20

[Timothy Nikkel (:tnikkel)]

Attachment: just use the output frames array and frames bounds to do basic covered analysis after the fact

This just modifies PositionedEventTargetting.cpp to use the output frames list from the current hit testing code and then subtract the frame bounds from the rect as we go from top to bottom in the output list, this will in turn change the distance metrics that we compute.

This won't be as precise as the approach outlined by roc and implemented in the 60kb+ patch but it's much simpler.

Comment 21

[Timothy Nikkel (:tnikkel)]

Attachment: check point

More fixes.

Comment 22

[Timothy Nikkel (:tnikkel)]

Attachment: check point

Comment 23

[Timothy Nikkel (:tnikkel)]

roc, do you have any thoughts on the two different approaches here? (comment 20 v comment 17)

Comment 24

[Robert O'Callahan (:roc) (email my personal email if necessary)]

If the approach in comment #20 works OK then that sounds fine to me.

Comment 25

[Timothy Nikkel (:tnikkel)]

Attachment: Use area as the metric instead of distance

This is on top of the patch of comment 20. This makes us use area (of the intersection of the target rect and the bounding box of the frame) as the metric for determining which frame is the target instead of distance (from the touch point to the frame rect). Not sure if this is better or worse.

Comment 26

[Timothy Nikkel (:tnikkel)]

Attachment: check point

This is in pretty good shape now. Todo: actual testing.

Comment 27

[Matt Brubeck (:mbrubeck)]

(In reply to Timothy Nikkel (:tn) from comment #25)
> Created attachment 779936 [details] [diff] [review]
> Use area as the metric instead of distance

I tested with the patch from comment 20 applied, with and without the additional patch in comment 25.  The test case from comment 1 still fails for me in both Windows Metro and Windows desktop.  I haven't tested the larger patch yet.

Comment 28

[Matt Brubeck (:mbrubeck)]

Comment on attachment 771598 [details] [diff] [review]
Part 1: convert bool parameters to flags

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

::: layout/base/PositionedEventTargeting.cpp
@@ +270,5 @@
>  
>    nsRect targetRect = GetTargetRect(aRootFrame, aPointRelativeToRootFrame, prefs);
>    nsAutoTArray<nsIFrame*,8> candidates;
>    nsresult rv = nsLayoutUtils::GetFramesForArea(aRootFrame, targetRect, candidates,
> +      flags | nsLayoutUtils::nsLayoutUtils::EXCLUDE_COVERED_FRAMES);

Drive-by comment: There's a duplicated "nsLayoutUtils::" on this line.

Comment 29

[Matt Brubeck (:mbrubeck)]

Comment on attachment 778774 [details] [diff] [review]
just use the output frames array and frames bounds to do basic covered analysis after the fact

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

::: layout/base/PositionedEventTargeting.cpp
@@ +278,5 @@
>      }
> +    if (preservesAxisAlignedRectangles) {
> +      // Subtract from the exposed region if we have a transform that won't make
> +      // the bounds include a bunch of area that we don't actually cover.
> +      SubtractFromExposedRegion(&exposedRegion, region);

It seems like this would work if we could do it before the IsElementClickable check at the top of the loop, instead of after.  At it is, this ignores non-clickable elements, which are exactly the ones that we are having trouble with.

Comment 30

[Mats Palmgren (inactive)]

Comment on attachment 771598 [details] [diff] [review]
Part 1: convert bool parameters to flags

>layout/base/PositionedEventTargeting.cpp
>-  bool ignoreRootScrollFrame = (aFlags & INPUT_IGNORE_ROOT_SCROLL_FRAME) != 0;
>+  uint32_t flags =
>+    (aFlags & INPUT_IGNORE_ROOT_SCROLL_FRAME) ? nsLayoutUtils::IGNORE_ROOT_SCROLL_FRAME : 0;

exceeds 80 columns

>+      flags | nsLayoutUtils::nsLayoutUtils::EXCLUDE_COVERED_FRAMES);

Double nsLayoutUtils:: as Matt noted above.


>layout/base/nsLayoutUtils.cpp
> nsLayoutUtils::GetFrameForPoint(nsIFrame* aFrame, nsPoint aPt,
>-                                bool aShouldIgnoreSuppression,
>-                                bool aIgnoreRootScrollFrame)
>+                                uint32_t aFlags)

No need to put "aFlags" on a separate line.

>   PROFILER_LABEL("nsLayoutUtils","GetFramesForArea");
>   nsDisplayListBuilder builder(aFrame, nsDisplayListBuilder::EVENT_DELIVERY,
>-		                       false);
>+		                           false);

I would line up "false);" directly under "aFrame".

>layout/base/nsLayoutUtils.h
>+  enum {
>+    IGNORE_PAINT_SUPPRESSION = 0x01,
>+    IGNORE_ROOT_SCROLL_FRAME = 0x02,
>+    EXCLUDE_COVERED_FRAMES = 0x04,
>+  };

Each value needs to be documented.

>   /**
>    * Given aFrame, the root frame of a stacking context, find its descendant
>    * frame under the point aPt that receives a mouse event at that location,
>    * or nullptr if there is no such frame.
>    * @param aPt the point, relative to the frame origin
>    * @param aShouldIgnoreSuppression a boolean to control if the display
>    * list builder should ignore paint suppression or not
>    * @param aIgnoreRootScrollFrame whether or not the display list builder
>    * should ignore the root scroll frame.
>    */
>   static nsIFrame* GetFrameForPoint(nsIFrame* aFrame, nsPoint aPt,
>-                                    bool aShouldIgnoreSuppression = false,
>-                                    bool aIgnoreRootScrollFrame = false);
>+                                    uint32_t aFlags = 0);

Remove the documentation for the removed params.
Add "@param aFlags one or more bits defined by the enum above" or some such.
Ditto for GetFramesForArea.

Comment 31

[Mats Palmgren (inactive)]

"zero or more bits" even :)

Comment 32

[Carsten Book [:Tomcat]]

backed this out https://hg.mozilla.org/integration/mozilla-inbound/rev/44ebfcf61a6c for causing https://tbpl.mozilla.org/php/getParsedLog.php?id=25760331&tree=Mozilla-Inbound on all platforms. 

Will see if the builds come back green and will open the tree then

Comment 33

[Timothy Nikkel (:tnikkel)]

(In reply to Matt Brubeck (:mbrubeck) from comment #29)
> It seems like this would work if we could do it before the
> IsElementClickable check at the top of the loop, instead of after.  At it
> is, this ignores non-clickable elements, which are exactly the ones that we
> are having trouble with.

The way it is written the loop will only return clickable elements, but in the testcase in this bug the correct behaviour would be to return the non-clickable div that is on top. When it seems like it works it is because we just return null and nothing gets the event. Perhaps we should just not care at all if the element is clickable and apply our metric to return the best target element whether it is clickable or not? That would be even simpler, and we should probably try the simplest solution and only move on to move complicated ones if the simpler ones don't work.

Comment 34

[Matt Brubeck (:mbrubeck)]

(In reply to Timothy Nikkel (:tn) from comment #33)
> The way it is written the loop will only return clickable elements, but in
> the testcase in this bug the correct behaviour would be to return the
> non-clickable div that is on top. When it seems like it works it is because
> we just return null and nothing gets the event. Perhaps we should just not
> care at all if the element is clickable and apply our metric to return the
> best target element whether it is clickable or not?

No, there are definitely cases where we should prefer a farther clickable element over a nearer non-clickable one.  The purpose of this code is to determine which link you are trying to click when using imprecise touch input.  So if you have "<span>Not</span> <a href='foo'>clickable</a>" and you click on the "Not" then we should return the "clickable."

Conceptually, we want to check if your pointer hit a clickable element; if not then try a little farther away until you do hit one or until we reach the distance limit.  But the current code does not take into account that some elements with click listeners cannot actually be hit because they are covered.  It doesn't just click "farther" -- it can also click "under."

I think the right logic might be something like this (pseudo-code):

exposedRegion = aTargetRect;
for (f in aCandidates) {
  borderBox = ...;
  region = borderBox AND exposedRegion;
  SubtractFromExposedRegion(&exposedRegion, region);

  if (!IsElementClickable(f))
    return;

  distance = ComputeDistanceFromRegion(aPointRelativeToRootFrame, region);
  if (distance < bestDistance) {
    bestDistance = distance;
    bestTarget = f;
  }
}

This is the same as the patch, except that it subtracts from exposedRegion every time through the loop.  I'm assuming that aCandidates is sorted by z order, which I'm not certain is actually true.

Comment 35

[Timothy Nikkel (:tnikkel)]

(In reply to Matt Brubeck (:mbrubeck) from comment #34)
> Conceptually, we want to check if your pointer hit a clickable element; if
> not then try a little farther away until you do hit one or until we reach
> the distance limit.  But the current code does not take into account that
> some elements with click listeners cannot actually be hit because they are
> covered.  It doesn't just click "farther" -- it can also click "under."

I think this approach tries a little too hard to try to find a clickable element. For example if we modify the testcase in this bug to expose 1 row of pixels in the covered but clickable div then it will be targeted almost every time. In general if a clickable element is completely covered by a non-clickable element except for one single pixel (inside the target rect) then that almost completely covered but clickable element will be targeted every time. I think we need to limit this somehow.

Comment 36

[Timothy Nikkel (:tnikkel)]

(In reply to Matt Brubeck (:mbrubeck) from comment #34)
>I'm assuming that aCandidates is sorted by z
> order, which I'm not certain is actually true.

If it isn't that would be a bug. It should be the same order as we use for painting.

Comment 37

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Part 1 landed: https://hg.mozilla.org/integration/mozilla-inbound/rev/d6fd6156acc3

Comment 38

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/d6fd6156acc3

Comment 39

[Timothy Nikkel (:tnikkel)]

(In reply to Timothy Nikkel (:tn) from comment #35)
> (In reply to Matt Brubeck (:mbrubeck) from comment #34)
> > Conceptually, we want to check if your pointer hit a clickable element; if
> > not then try a little farther away until you do hit one or until we reach
> > the distance limit.  But the current code does not take into account that
> > some elements with click listeners cannot actually be hit because they are
> > covered.  It doesn't just click "farther" -- it can also click "under."
> 
> I think this approach tries a little too hard to try to find a clickable
> element.

But it's still an improvement that shouldn't make this problem any worse, so I'll go forward with it.

Comment 40

[Timothy Nikkel (:tnikkel)]

Attachment: Track a rough exposed area for list of frames returned from GetFramesForArea

Comment 41

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 782661 [details] [diff] [review]
Track a rough exposed area for list of frames returned from GetFramesForArea

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

Move the nsRegion::Area change to its own patch.

::: layout/base/PositionedEventTargeting.cpp
@@ +252,5 @@
> +    bool preservesAxisAlignedRectangles = false;
> +    nsRect borderBox = nsLayoutUtils::TransformFrameRectToAncestor(f,
> +        nsRect(nsPoint(0, 0), f->GetSize()), aRoot, &preservesAxisAlignedRectangles);
> +    nsRegion region;
> +    region.And(exposedRegion, borderBox);

The entire declaration of nsRegion and the And operation can be moved into the "if" block.

Comment 42

[Timothy Nikkel (:tnikkel)]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #41)
> Comment on attachment 782661 [details] [diff] [review]
> Track a rough exposed area for list of frames returned from GetFramesForArea
> ::: layout/base/PositionedEventTargeting.cpp
> @@ +252,5 @@
> > +    bool preservesAxisAlignedRectangles = false;
> > +    nsRect borderBox = nsLayoutUtils::TransformFrameRectToAncestor(f,
> > +        nsRect(nsPoint(0, 0), f->GetSize()), aRoot, &preservesAxisAlignedRectangles);
> > +    nsRegion region;
> > +    region.And(exposedRegion, borderBox);
> 
> The entire declaration of nsRegion and the And operation can be moved into
> the "if" block.

If I'm understanding you correctly we can't do this because we use the region lower down in ComputeDistanceFromRegion.

Comment 43

[Timothy Nikkel (:tnikkel)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/84d183bfe6fc
https://hg.mozilla.org/integration/mozilla-inbound/rev/1d2c1f01e757

Comment 44

[Timothy Nikkel (:tnikkel)]

roc please see comment 42 to make sure we are on the same page on what I landed,

Comment 45

[Robert O'Callahan (:roc) (email my personal email if necessary)]

You're right.

Comment 46

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/84d183bfe6fc
https://hg.mozilla.org/mozilla-central/rev/1d2c1f01e757

Comment 47

[Matt Brubeck (:mbrubeck)]

I don't think this patch had the desired effect.  The test case in comment 1 still fails with this patch applied.  (We should probably add it to the automated tests for this feature, once it is actually fixed.)

Also, I suspect these patches caused a new regression: bug 901036.

Comment 48

[Timothy Nikkel (:tnikkel)]

I'm going to leave this closed because a patch landed to better handle possible regressions. Bug 901175 has a patch the two problems mentioned.