1282306 - Make about:support report actual touch support for APZ

Status: RESOLVED FIXED

(Core :: Panning and Zooming, enhancement, P5)

Description

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

+++ This bug was initially created as a clone of Bug #1276994 +++

The SupportsApzTouchInput function is used by about:support to display whether or not touch input is supported, in the "Asynchronous Pan/Zoom" status line. As of bug 1276994, both the windows and linux implementations just check the pref to see if touch is enabled in APZ. However, it would be more useful, when the pref value is "2" (i.e. autodetect), to check if the hardware actually supports touch. The TouchEvent::PrefEnabled code [1] actually does this, so maybe we can just call that.

[1] http://searchfox.org/mozilla-central/rev/ef24c234ed53b3ba50a1734f6b946942e4434b5b/dom/events/TouchEvent.cpp#166

Comment 1

[Raulie Raulerson]

Would this be another good first bug to work on?  If not, do you have any other bugs that you could recommend I work on?

Comment 2

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

This should be a good one to work on although it would be easier to test if you had a windows or Linux touch-enabled device. But no matter, I can test the patch for you. The hardest part of this bug is figuring out what we want to have happen. I think we basically want to report the value of TouchEvent::PrefEnabled since that will return true if the pref is "1" (unconditionally enabled) and will check for hardware support if the pref is "2" (autodetect).

So I think we can even remove the platform-specific overrides of SupportsApzTouchInput and make the base class version return TouchEvent::PrefEnabled; that will return the right thing on all platforms.

Comment 3

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

Bug 970346 might complicate this. With that in mind I think we should have about:support report more detailed information about touch events - if it's enabled, autodetected, forced on or off, etc. This is probably a harder bug to work on as a result.

Comment 4

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

Raulie, are you still working on this?

Comment 5

[Raulie Raulerson]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #4)
> Raulie, are you still working on this?

Kartikaya, I need some more guidance on how to correct this bug.  I want to work on the bug, I just need a shove in the right direction.  Thank you sir!

Comment 6

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

Sure! The simplest thing to do is to modify the function at [1] to return TouchEvent::PrefEnabled(nullptr), and remove the overrides in the various Platform subclasses ([2], [3], [4]). That change in itself will be an improvement, because it will take into account the runtime presence of a touch device, and so will not report "touch input enabled" if the pref is set to 2 and there is no physical touch device.

I was thinking about how to additionally support the changes made in bug 970346 but that seems more involved and will probably involve touch the JS code for about:support so we can leave that as a follow-up.

[1] http://searchfox.org/mozilla-central/rev/09ae31fddfbc229ef1b8f1a3ccd87ad05680e695/gfx/thebes/gfxPlatform.h#633
[2] http://searchfox.org/mozilla-central/rev/09ae31fddfbc229ef1b8f1a3ccd87ad05680e695/gfx/thebes/gfxPlatformGtk.h#124
[3] http://searchfox.org/mozilla-central/rev/09ae31fddfbc229ef1b8f1a3ccd87ad05680e695/gfx/thebes/gfxAndroidPlatform.h#71
[4] http://searchfox.org/mozilla-central/rev/09ae31fddfbc229ef1b8f1a3ccd87ad05680e695/gfx/thebes/gfxWindowsPlatform.h#218

Comment 7

[Raulie Raulerson]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #6)
> Sure! The simplest thing to do is to modify the function at [1] to return
> TouchEvent::PrefEnabled(nullptr), and remove the overrides in the various
> Platform subclasses ([2], [3], [4]). That change in itself will be an
> improvement, because it will take into account the runtime presence of a
> touch device, and so will not report "touch input enabled" if the pref is
> set to 2 and there is no physical touch device.
> 
> I was thinking about how to additionally support the changes made in bug
> 970346 but that seems more involved and will probably involve touch the JS
> code for about:support so we can leave that as a follow-up.
> 
> [1]
> http://searchfox.org/mozilla-central/rev/
> 09ae31fddfbc229ef1b8f1a3ccd87ad05680e695/gfx/thebes/gfxPlatform.h#633
> [2]
> http://searchfox.org/mozilla-central/rev/
> 09ae31fddfbc229ef1b8f1a3ccd87ad05680e695/gfx/thebes/gfxPlatformGtk.h#124
> [3]
> http://searchfox.org/mozilla-central/rev/
> 09ae31fddfbc229ef1b8f1a3ccd87ad05680e695/gfx/thebes/gfxAndroidPlatform.h#71
> [4]
> http://searchfox.org/mozilla-central/rev/
> 09ae31fddfbc229ef1b8f1a3ccd87ad05680e695/gfx/thebes/gfxWindowsPlatform.h#218

Thanks for the help!  I will try to have a patch posted in the next few days!

Comment 8

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

Any progress here, Raulie?

Comment 9

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

Attachment: Bug 1282306 - More correctly describe APZ touch support in about:support.

Instead of just reporting the value of the pref, also make sure that about:support
incorporates the runtime check of whether or not a touch device was discovered
in the widget code. The code to do this already exists in TouchEvent::PrefEnabled,
so we can just reuse that.

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

Comment 10

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

Whoops, forgot to update gfxAndroidPlatform.h. Updated patch incoming.

Comment 11

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

Comment on attachment 8846088 [details]
Bug 1282306 - More correctly describe APZ touch support in about:support.

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

Comment 12

[Botond Ballo [:botond]]

Comment on attachment 8846088 [details]
Bug 1282306 - More correctly describe APZ touch support in about:support.

https://reviewboard.mozilla.org/r/119162/#review121222

Much simpler, thanks!

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/aa2376407a4d
More correctly describe APZ touch support in about:support. r=botond

Comment 14

[Carsten Book [:Tomcat]]

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

Comment 15

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

Comment on attachment 8846088 [details]
Bug 1282306 - More correctly describe APZ touch support in about:support.

Approval Request Comment
[Feature/Bug causing the regression]: APZ touch support
[User impact if declined]: no user impact really. it just makes about:support reporting of APZ touch slightly more useful
[Is this code covered by automated tests?]: no
[Has the fix been verified in Nightly?]: yes
[Needs manual test from QE? If yes, steps to reproduce]: no
[List of other uplifts needed for the feature/fix]: none
[Is the change risky?]: no
[Why is the change risky/not risky?]: very small change, code is not used in any user-facing features, just diagnostic information in about:support
[String changes made/needed]: none

Comment 16

[Gerry Chang [:gchang]]

Comment on attachment 8846088 [details]
Bug 1282306 - More correctly describe APZ touch support in about:support.

This patch makes about:support report APZ touch support. Beta53+ & Aurora54+.

Comment 17

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/f7d0c4c627ab

Comment 18

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-beta/rev/9c9fb1e26d9e

Comment 19

[Andrei Vaida [:avaida]]

Setting qe-verify- based on Kartikaya's assessment on manual testing needs (see Comment 15).