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

Status: RESOLVED FIXED

(Core :: Layout, defect)

Description

[Kartikaya Gupta (email:[e-mail])]

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 1

[Kartikaya Gupta (email:[e-mail])]

Attachment: Limit walking up the content tree to the body

Comment 2

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

Comment on attachment 8516782 [details] [diff] [review]
Limit walking up the content tree to the body

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

test?

Comment 3

[Kartikaya Gupta (email:[e-mail])]

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

Comment 4

[Kartikaya Gupta (email:[e-mail])]

Attachment: Part 3 - Test

https://treeherder.mozilla.org/ui/#/jobs?repo=try&revision=ccb1a367b84b

Comment 5

[Kartikaya Gupta (email:[e-mail])]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/b1e482f69d96
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/0c95949fef14
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/f1989cb41965

Comment 6

[Carsten Book [:Tomcat]]

sorry had to backout part 3 for test failures like https://treeherder.mozilla.org/ui/logviewer.html#?job_id=3655601&repo=mozilla-inbound

Comment 7

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/b1e482f69d96
https://hg.mozilla.org/mozilla-central/rev/0c95949fef14

Comment 8

[Kartikaya Gupta (email:[e-mail])]

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

Comment 9

[Ryan VanderMeulen [:RyanVM]]

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

Comment 10

[Kartikaya Gupta (email:[e-mail])]

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

Comment 11

[Ryan VanderMeulen [:RyanVM]]

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.

Comment 12

[Kartikaya Gupta (email:[e-mail])]

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.