Closed Bug 1286957 Opened 4 years ago Closed 4 years ago

Make pointerevent_touch-action-inherit_child-pan-x-child-pan-y_touch-manual.html pass with APZ

Categories

(Core :: Panning and Zooming, defect, P3)

defect

Tracking

()

RESOLVED FIXED
mozilla50
Tracking Status
firefox50 --- fixed

People

(Reporter: kats, Assigned: kats)

References

Details

(Keywords: correctness, Whiteboard: [gfx-noted])

Attachments

(2 files, 2 obsolete files)

On a windows touch device, the pointerevent_touch-action-inherit_child-pan-x-child-pan-y_touch-manual.html test fails. Note that in order to run this you have to do some fiddling with manifest files and such, it's not run in automation.

The reason it fails is because the page is set up like so:

<div style="overflow:scroll">
 <div style="touch-action:pan-y">
  <div style="touch-action:pan-x">
   stuff
  </div>
 </div>
</div>

Because of the way we collection touch action regions in the code starting at [1], this gives us incorrect behaviour. Specifically, it marks the area as both pan-x and pan-y when in fact it should be neither.

[1] http://searchfox.org/mozilla-central/rev/e269c68bc594b4a858624d73f0d9eb002f3c0844/layout/base/nsDisplayList.cpp#3678
Summary: Make pointerevent_touch-action-inherit_child-pan-x-child-pan-y_touch-manual.html pass → Make pointerevent_touch-action-inherit_child-pan-x-child-pan-y_touch-manual.html pass with APZ
See Also: → 1287142
Attached patch Patch (obsolete) — Splinter Review
Attached patch Patch (obsolete) — Splinter Review
Attachment #8771466 - Attachment is obsolete: true
Attachment #8772140 - Flags: review?(tnikkel)
Yeah that's a good point. Also it looks like the combination of this patch and the patch on bug 1287142 causes a test failure (helper_touch_action_regions.html). The test passes with either patch individually, but when both are combined I guess the pan-y from the scrollframe overlaps with the pan-y from the scrolled frame and it turns into a d-t-c region, which causes the test to fail. I'll need to investigate a bit more though since that should only happen if the scrollframe hasn't been layerized and in that case it should already be in the d-t-c region.
Ah, looks like even after layerization, the scrolled frame gets added twice at [1] and [2]. The first time would add the regions properly and the second time would convert it to d-t-c.

[1] http://searchfox.org/mozilla-central/rev/65bed54efcce67cf04a890f7fe253ccdfa6befdc/layout/generic/nsFrame.cpp#2866
[2] http://searchfox.org/mozilla-central/rev/65bed54efcce67cf04a890f7fe253ccdfa6befdc/layout/generic/nsFrame.cpp#2873
Attached patch Patch v2Splinter Review
Now handling all of those other places you pointed out. Also unfortunately we have to fall back to d-t-c regions in even more cases than I originally thought.
Attachment #8772140 - Attachment is obsolete: true
Attachment #8772993 - Flags: review?(tnikkel)
Attached patch TestsSplinter Review
Attachment #8772994 - Flags: review?(tnikkel)
Comment on attachment 8772993 [details] [diff] [review]
Patch v2

Gross :)
Attachment #8772993 - Flags: review?(tnikkel) → review+
Attachment #8772994 - Flags: review?(tnikkel) → review+
Pushed by kgupta@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/e15ba82ad331
Because combining touch-action areas into the event regions is fraught with peril, dump them into the dispatch-to-content region. r=tnikkel
https://hg.mozilla.org/integration/mozilla-inbound/rev/3d6cdeb53e16
Add some more complex mochitests for touch-action testing. r=tnikkel
https://hg.mozilla.org/mozilla-central/rev/e15ba82ad331
https://hg.mozilla.org/mozilla-central/rev/3d6cdeb53e16
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
You need to log in before you can comment on or make changes to this bug.