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.

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.

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

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

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

How likely is this to bite users in the wild?

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.

OK. It sounds like we need this.

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

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

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

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

smaug, is this something you can pick up?

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.

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.

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.

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.

Check point, not usable yet.

Seems usable now.

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.

More fixes.

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

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

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.

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

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

"zero or more bits" even :)

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

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

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

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

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

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

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

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

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

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

You're right.

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

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.

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.