Closed Bug 1275604 Opened 4 years ago Closed 4 years ago

Make the touch-action pref live

Categories

(Core :: Panning and Zooming, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla49
Tracking Status
firefox49 --- fixed

People

(Reporter: kats, Assigned: kats)

References

Details

Attachments

(4 files)

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.
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/
Attachment #8757009 - Flags: review?(botond)
Attachment #8757010 - Flags: review?(tnikkel)
Attachment #8757011 - Flags: review?(botond)
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?
Attachment #8757010 - Flags: review?(tnikkel) → review+
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 on attachment 8757009 [details]
MozReview Request: Bug 1275604 - Make the touch-action pref live. r=botond

https://reviewboard.mozilla.org/r/55520/#review52488
Attachment #8757009 - Flags: review?(botond) → review+
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
Attachment #8757011 - Flags: review?(botond) → review+
(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.
Review commit: https://reviewboard.mozilla.org/r/56562/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/56562/
Attachment #8757009 - Attachment description: MozReview Request: Bug 1275604 - Make the touch-action pref live. r?botond → MozReview Request: Bug 1275604 - Make the touch-action pref live. r=botond
Attachment #8757010 - Attachment description: MozReview Request: Bug 1275604 - Ensure that we schedule a paint if the touch-action property changes. r?tnikkel → MozReview Request: Bug 1275604 - Ensure that we schedule a paint if the touch-action property changes. r=tnikkel
Attachment #8757011 - Attachment description: MozReview Request: Bug 1275604 - Basic touch-action test. r?botond → MozReview Request: Bug 1275604 - Basic touch-action test. r=botond
Attachment #8758275 - Flags: review?(botond)
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 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 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/
Attachment #8758275 - Flags: review?(botond) → review+
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!
Pushed by kgupta@mozilla.com:
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
You need to log in before you can comment on or make changes to this bug.