1275604 - Make the touch-action pref live

Status: RESOLVED FIXED

(Core :: Panning and Zooming, defect)

Description

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

Right now gfxPrefs::TouchActionEnabled() is a "once" pref which makes it hard to flip it on and off in tests. The pref is actually supposed to be a live pref (the CSS parsing code treats it as such) so we should make it live in gfxPrefs as well.

Comment 1

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

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

Comment 2

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=45bc6edb7eca

Comment 3

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

Attachment: MozReview Request: Bug 1275604 - Make the touch-action pref live. r=botond

As part of making the pref live, we need to ensure that if the pref is flipped
while an input block is in the input queue, the behaviour that results is
reasonable. In particular, if a touch block is created while touch-action is
disabled, but the pref is enabled while that block is in the queue, it may
suddenly go from IsReadyForHandling() returning true to IsReadyForHandling()
returning false. This can result in the block getting stuck in the queue and
preventing future touch blocks from being processed at all. To handle this
case, we explicitly set the mAllowedTouchBehaviorSet flag to true if the block
is created with touch-action disabled. This prevents IsReadyForHandling() from
flipping to false in these cases.

Review commit: https://reviewboard.mozilla.org/r/55520/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/55520/

Comment 4

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

Attachment: MozReview Request: Bug 1275604 - Ensure that we schedule a paint if the touch-action property changes. r=tnikkel

Review commit: https://reviewboard.mozilla.org/r/55522/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/55522/

Comment 5

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

Attachment: MozReview Request: Bug 1275604 - Basic touch-action test. r=botond

Review commit: https://reviewboard.mozilla.org/r/55524/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/55524/

Comment 6

[Timothy Nikkel (:tnikkel)]

Comment on attachment 8757010 [details]
MozReview Request: Bug 1275604 - Ensure that we schedule a paint if the touch-action property changes. r=tnikkel

https://reviewboard.mozilla.org/r/55522/#review52284

Should we only do this if event regions are enabled?

Comment 7

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

We're only going to support touch-action with APZ, and APZ requires event regions, so it's kind of implicit - the CSS properties for touch action will be ignored entirely if APZ and event regions aren't enabled.

Comment 8

[Botond Ballo [:botond]]

Comment on attachment 8757009 [details]
MozReview Request: Bug 1275604 - Make the touch-action pref live. r=botond

https://reviewboard.mozilla.org/r/55520/#review52488

Comment 9

[Botond Ballo [:botond]]

Comment on attachment 8757011 [details]
MozReview Request: Bug 1275604 - Basic touch-action test. r=botond

https://reviewboard.mozilla.org/r/55524/#review52492

::: gfx/layers/apz/test/mochitest/helper_touch_action.html:18
(Diff revision 1)
> +  is(window.scrollX, x, desc + " - x axis");
> +  is(window.scrollY, y, desc + " - y axis");
> +}
> +
> +function* test(testDriver) {
> +  const TOUCH_SLOP = 1;

This feels a bit like a magic number. Perhaps we could move it into apz_test_utils.js, and add a comment explaining why it's 1?

::: gfx/layers/apz/test/mochitest/helper_touch_action.html:24
(Diff revision 1)
> +  var target = document.getElementById('target');
> +
> +  document.body.addEventListener('touchend', testDriver, { passive: true });
> +
> +  // drag the page up to scroll down by 50px
> +  yield ok(synthesizeNativeDrag(target, 10, 100, 0, -(50 + TOUCH_SLOP)),

Can we rename this function synthesizeNativeTouchDrag? I realize I was the one who r+'d the original name, but now that look at it again, it's ambiguous with mouse drag.

::: gfx/layers/apz/test/mochitest/helper_touch_action.html:110
(Diff revision 1)
> +  This div makes the page scrollable on both axes.<br>
> +  This is the second line of text.<br>
> +  This is the third line of text.<br>
> +  This is the fourth line of text.
> + </div>
> + <!-- This fixed-position div remains the same place relative to the browser chrome, so we

remains _in_ the same place

Comment 10

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

(In reply to Botond Ballo [:botond] from comment #9)
> > +  const TOUCH_SLOP = 1;
> 
> This feels a bit like a magic number. Perhaps we could move it into
> apz_test_utils.js, and add a comment explaining why it's 1?
> 
> > +  yield ok(synthesizeNativeDrag(target, 10, 100, 0, -(50 + TOUCH_SLOP)),
> 
> Can we rename this function synthesizeNativeTouchDrag? I realize I was the
> one who r+'d the original name, but now that look at it again, it's
> ambiguous with mouse drag.

I added another patch to the queue that does these two changes, since they seem relatively independent and affect pre-existing tests. Just waiting on a local rebase/build/test run to verify the patches are still good.

> > + <!-- This fixed-position div remains the same place relative to the browser chrome, so we
> 
> remains _in_ the same place

Good catch, fixed.

Comment 11

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

Attachment: MozReview Request: Bug 1275604 - Rename synthesizeNativeDrag to synthesizeNativeTouchDrag, and extract the TOUCH_SLOP constant to the helper file. r?botond

Review commit: https://reviewboard.mozilla.org/r/56562/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/56562/

Comment 12

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

Comment on attachment 8757009 [details]
MozReview Request: Bug 1275604 - Make the touch-action pref live. r=botond

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/55520/diff/1-2/

Comment 13

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

Comment on attachment 8757010 [details]
MozReview Request: Bug 1275604 - Ensure that we schedule a paint if the touch-action property changes. r=tnikkel

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/55522/diff/1-2/

Comment 14

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

Comment on attachment 8757011 [details]
MozReview Request: Bug 1275604 - Basic touch-action test. r=botond

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/55524/diff/1-2/

Comment 15

[Botond Ballo [:botond]]

Comment on attachment 8758275 [details]
MozReview Request: Bug 1275604 - Rename synthesizeNativeDrag to synthesizeNativeTouchDrag, and extract the TOUCH_SLOP constant to the helper file. r?botond

https://reviewboard.mozilla.org/r/56562/#review53632

Thanks!

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/ece66ce5876b
Make the touch-action pref live. r=botond
https://hg.mozilla.org/integration/mozilla-inbound/rev/fb9629189809
Ensure that we schedule a paint if the touch-action property changes. r=tnikkel
https://hg.mozilla.org/integration/mozilla-inbound/rev/606a0907dec2
Basic touch-action test. r=botond
https://hg.mozilla.org/integration/mozilla-inbound/rev/154b0716163b
Rename synthesizeNativeDrag to synthesizeNativeTouchDrag, and extract the TOUCH_SLOP constant to the helper file. r=botond

Comment 17

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/ece66ce5876b
https://hg.mozilla.org/mozilla-central/rev/fb9629189809
https://hg.mozilla.org/mozilla-central/rev/606a0907dec2
https://hg.mozilla.org/mozilla-central/rev/154b0716163b