When checking for nearby clickable elements PositionedEventTargeting doesn't stop at the body

RESOLVED FIXED in Firefox 35, Firefox OS v2.2

Status

()

Core
Layout
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: kats, Assigned: kats)

Tracking

Trunk
mozilla36
All
Android
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox34 wontfix, firefox35 fixed, firefox36 fixed, b2g-v2.1 wontfix, b2g-v2.2 fixed)

Details

Attachments

(3 attachments)

(Assignee)

Description

4 years ago
actuallybug1078029
Spinoff from bug 1089029, specifically comment 39 on that bug. STR:

(In reply to Dominique Vincent [:domivinc] from comment #39)
> In this page:
> http://office.microsoft.com/fr-fr/training/cours-de-formation-sur-office-
> 2013-HA104096598.aspx
> 
> Try to touch just under 2013 of "Power Point 2013"
> Using Firefox 33, you can highlight the link "Power Point 2013" clicking
> near the link.
> With your version, the closest link is never found, the highlight never
> occurs.
> 
> In this page
> http://office.microsoft.com/fr-fr/
> Try to highlight the bottom right links (for instance touch right of link
> "Office 365 Famille") 

I investigated this and found that the IsElementClickable call at [1] is not capped to the body element. So in any page where the body has mouse/touch listeners, all elements are considered clickable, and the event retargeting effectively does nothing. Note that the code at [2] correctly limits the clickability check to children of the body.

[1] http://mxr.mozilla.org/mozilla-central/source/layout/base/PositionedEventTargeting.cpp?rev=c3c317bd388f#334
[2] http://mxr.mozilla.org/mozilla-central/source/layout/base/PositionedEventTargeting.cpp?rev=c3c317bd388f#374
Created attachment 8516782 [details] [diff] [review]
Limit walking up the content tree to the body
Attachment #8516782 - Flags: review?(roc)
Comment on attachment 8516782 [details] [diff] [review]
Limit walking up the content tree to the body

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

test?
Attachment #8516782 - Flags: review?(roc) → review+
Created attachment 8517581 [details] [diff] [review]
Part 2 - Logging

I've written something equivalent to this patch about three times now, so it's probably time to just check it in.
Attachment #8517581 - Flags: review?(roc)
sorry had to backout part 3 for test failures like https://treeherder.mozilla.org/ui/logviewer.html#?job_id=3655601&repo=mozilla-inbound
Keywords: leave-open
Test failure on B2G desktop was likely just because the screen size is small on those platforms and the click at (258,258) was off-screen. I did a try push moving that click a little closer in and it passed: https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=43ec8131c342

Relanded with that change: https://hg.mozilla.org/integration/mozilla-inbound/rev/c2e3537ca09f
Keywords: leave-open
https://hg.mozilla.org/mozilla-central/rev/c2e3537ca09f
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
Comment on attachment 8516782 [details] [diff] [review]
Limit walking up the content tree to the body

Approval Request Comment
[Feature/regressing bug #]: pre-existing bug that affects B2G, but exposed in fennec by bug 788073
[User impact if declined]: mouse events are supposed to be retargeted to nearby clickable elements, but on pages with mouse/touch listeners on the body element this doesn't work. users may be frustrated by things being harder to click on such pages.
[Describe test coverage new/current, TBPL]: added a test in part 3
[Risks and why]: small patch, low risk.
[String/UUID change made/needed]: none
Attachment #8516782 - Flags: approval-mozilla-aurora?
Attachment #8517581 - Flags: approval-mozilla-aurora?
Attachment #8517583 - Flags: approval-mozilla-aurora?
status-firefox34: --- → wontfix
status-firefox35: --- → affected
status-firefox36: --- → fixed
status-b2g-v2.1: --- → affected
status-b2g-v2.2: --- → fixed
Attachment #8516782 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #8517581 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
Attachment #8517583 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+
https://hg.mozilla.org/releases/mozilla-aurora/rev/8b5ef735d76c
https://hg.mozilla.org/releases/mozilla-aurora/rev/7a3c51b96a2b
https://hg.mozilla.org/releases/mozilla-aurora/rev/bff65964c8a6

Will need separate approval for b2g34 still if you intend it to land there as well.
status-b2g-v2.1: affected → fixed
status-firefox35: affected → fixed
status-b2g-v2.1: fixed → affected
I'll just let it ride the trains for B2G. Partly because nobody had complained about this on B2G and partly because it might expose other bugs. There's a report of a possible regression from this in bug 1095709, for example.
status-b2g-v2.1: affected → wontfix
Depends on: 1095709
You need to log in before you can comment on or make changes to this bug.