Closed Bug 1093686 Opened 5 years ago Closed 5 years ago

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

Categories

(Core :: Layout, defect)

All
Android
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla36
Tracking Status
firefox34 --- wontfix
firefox35 --- fixed
firefox36 --- fixed
b2g-v2.1 --- wontfix
b2g-v2.2 --- fixed

People

(Reporter: kats, Assigned: kats)

References

Details

Attachments

(3 files)

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
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+
Attached patch Part 2 - LoggingSplinter Review
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)
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
https://hg.mozilla.org/mozilla-central/rev/c2e3537ca09f
Status: NEW → RESOLVED
Closed: 5 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?
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+
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.
You need to log in before you can comment on or make changes to this bug.