Closed Bug 1282306 Opened 3 years ago Closed 3 years ago

Make about:support report actual touch support for APZ

Categories

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

enhancement

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox53 --- fixed
firefox54 --- fixed
firefox55 --- fixed

People

(Reporter: kats, Assigned: kats, Mentored)

Details

(Whiteboard: [lang=c++][gfx-noted])

Attachments

(1 file)

+++ 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
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?
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.
Assignee: nobody → hhraulerson
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.
Raulie, are you still working on this?
Severity: normal → enhancement
(In reply to Kartikaya Gupta (email:kats@mozilla.com) 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!
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
(In reply to Kartikaya Gupta (email:kats@mozilla.com) 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!
Any progress here, Raulie?
Flags: needinfo?(hhraulerson)
Assignee: hhraulerson → nobody
Flags: needinfo?(hhraulerson)
Assignee: nobody → bugmail
Whoops, forgot to update gfxAndroidPlatform.h. Updated patch incoming.
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!
Attachment #8846088 - Flags: review?(botond) → review+
Pushed by kgupta@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/aa2376407a4d
More correctly describe APZ touch support in about:support. r=botond
https://hg.mozilla.org/mozilla-central/rev/aa2376407a4d
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
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
Attachment #8846088 - Flags: approval-mozilla-beta?
Attachment #8846088 - Flags: approval-mozilla-aurora?
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+.
Attachment #8846088 - Flags: approval-mozilla-beta?
Attachment #8846088 - Flags: approval-mozilla-beta+
Attachment #8846088 - Flags: approval-mozilla-aurora?
Attachment #8846088 - Flags: approval-mozilla-aurora+
Setting qe-verify- based on Kartikaya's assessment on manual testing needs (see Comment 15).
Flags: qe-verify-
You need to log in before you can comment on or make changes to this bug.