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

RESOLVED FIXED in mozilla25

Status

()

defect
RESOLVED FIXED
7 years ago
3 months ago

People

(Reporter: mbrubeck, Assigned: tnikkel)

Tracking

({testcase})

Trunk
mozilla25
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: feature=defect c=content_features u=metro_firefox_user)

Attachments

(5 attachments, 5 obsolete attachments)

Reporter

Description

7 years ago
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.
Reporter

Updated

7 years ago
Blocks: 835175
Reporter

Comment 1

7 years ago
Posted file 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.
Reporter

Updated

7 years ago
Attachment #709215 - Attachment is patch: false
Attachment #709215 - Attachment mime type: text/plain → text/html
Reporter

Updated

7 years ago
Keywords: testcase
Component: DOM: Events → Event Handling
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".
Reporter

Comment 3

7 years ago
(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.
Reporter

Comment 4

7 years ago
(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).
Reporter

Updated

6 years ago
Blocks: 831976
Whiteboard: [metro-mvp?]
How likely is this to bite users in the wild?
Reporter

Comment 6

6 years ago
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.
Summary: With ui.mouse.radius.enabled, mouse events may be targeted to an element that is covered by another element → defect - With ui.mouse.radius.enabled, mouse events may be targeted to an element that is covered by another element
Whiteboard: [metro-mvp?] → feature=defect c=content_features u=metro_firefox_user
Reporter

Updated

6 years ago
Blocks: 856119
(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?
Flags: needinfo?(wjohnston)
Flags: needinfo?(roc)
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...
Flags: needinfo?(wjohnston)
Reporter

Comment 10

6 years ago
(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...
Flags: needinfo?(roc)
smaug, is this something you can pick up?
Flags: needinfo?(bugs)
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.
Flags: needinfo?(bugs)
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.
Assignee: roc → tnikkel
Assignee

Comment 18

6 years ago
Posted patch check point (obsolete) — Splinter Review
Check point, not usable yet.
Assignee

Comment 19

6 years ago
Posted patch check point (obsolete) — Splinter Review
Seems usable now.
Attachment #776201 - Attachment is obsolete: true
Assignee

Comment 20

6 years ago
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.
Assignee

Comment 21

6 years ago
Posted patch check point (obsolete) — Splinter Review
More fixes.
Attachment #778281 - Attachment is obsolete: true
Assignee

Comment 22

6 years ago
Posted patch check point (obsolete) — Splinter Review
Assignee

Comment 23

6 years ago
roc, do you have any thoughts on the two different approaches here? (comment 20 v comment 17)
Flags: needinfo?(roc)
If the approach in comment #20 works OK then that sounds fine to me.
Flags: needinfo?(roc)
Assignee

Updated

6 years ago
Attachment #778877 - Attachment is obsolete: true
Assignee

Comment 25

6 years ago
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.
Assignee

Comment 26

6 years ago
Posted patch check pointSplinter Review
This is in pretty good shape now. Todo: actual testing.
Attachment #778881 - Attachment is obsolete: true
Reporter

Comment 27

6 years ago
(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.
Reporter

Comment 28

6 years ago
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.
Reporter

Comment 29

6 years ago
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

6 years ago
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.
Attachment #771598 - Flags: review?(matspal) → review+

Comment 31

6 years ago
"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
Assignee

Comment 33

6 years ago
(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.
Reporter

Comment 34

6 years ago
(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.
Assignee

Comment 35

6 years ago
(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.
Assignee

Comment 36

6 years ago
(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
Whiteboard: feature=defect c=content_features u=metro_firefox_user → feature=defect c=content_features u=metro_firefox_user [leave open]
Assignee

Comment 39

6 years ago
(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.
Attachment #782661 - Flags: review?(roc) → review+
Assignee

Comment 42

6 years ago
(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.
Assignee

Comment 43

6 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/84d183bfe6fc
https://hg.mozilla.org/integration/mozilla-inbound/rev/1d2c1f01e757
Whiteboard: feature=defect c=content_features u=metro_firefox_user [leave open] → feature=defect c=content_features u=metro_firefox_user
Assignee

Comment 44

6 years ago
roc please see comment 42 to make sure we are on the same page on what I landed,
Flags: needinfo?(roc)
You're right.
Flags: needinfo?(roc)
https://hg.mozilla.org/mozilla-central/rev/84d183bfe6fc
https://hg.mozilla.org/mozilla-central/rev/1d2c1f01e757
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla25

Updated

6 years ago
Blocks: 899987
Reporter

Comment 47

6 years ago
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.
Status: RESOLVED → REOPENED
Depends on: 901036
Resolution: FIXED → ---
Assignee

Updated

6 years ago
Depends on: 901175
Assignee

Comment 48

6 years ago
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.
Status: REOPENED → RESOLVED
Closed: 6 years ago6 years ago
Resolution: --- → FIXED
Component: Event Handling → User events and focus handling
You need to log in before you can comment on or make changes to this bug.