1373714 - Touch-dragging scrollbar doesn't work on about:config and some UI elements

Status: VERIFIED FIXED

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

Description

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

Dragging the scrollbar thumb in about:config and the library window doesn't work with touch. Bug 1341992 fixed thumb-dragging in web content but these UI elements seem to have special touch handling.

This was reported via email by Joop.

Comment 1

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

Attachment: Bug 1373714 - Support touch-dragging the scrollthumb in XUL tree widgets.

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

Comment 2

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

The problem here is that even though gecko does handle touch-dragging the scrollbar thumb and touch-scrollbar interaction in general, the XUL widgets have touch-specific code that overrides that behaviour. Allowing touches that land in the scrollbar to pass through normal gecko flow resolves this issue.

Comment 3

[Dão Gottwald [:dao]]

Comment on attachment 8881415 [details]
Bug 1373714 - Support touch-dragging the scrollthumb in XUL tree widgets.

https://reviewboard.mozilla.org/r/152570/#review158918

::: toolkit/content/widgets/tree.xml:690
(Diff revision 1)
>      <handlers>
>        <handler event="touchstart">
>          <![CDATA[
> -          if (event.touches.length > 1) {
> +          function isScrollbarElement(target) {
> +            return target.tagName == 'xul:thumb' ||
> +                   target.tagName == 'xul:slider';

I think checking nodeName would be saner since the xul: prefix is pretty arbitrary.

Comment 4

[Dão Gottwald [:dao]]

Comment on attachment 8881415 [details]
Bug 1373714 - Support touch-dragging the scrollthumb in XUL tree widgets.

https://reviewboard.mozilla.org/r/152570/#review158920

::: toolkit/content/widgets/tree.xml:690
(Diff revision 1)
>      <handlers>
>        <handler event="touchstart">
>          <![CDATA[
> -          if (event.touches.length > 1) {
> +          function isScrollbarElement(target) {
> +            return target.tagName == 'xul:thumb' ||
> +                   target.tagName == 'xul:slider';

Also, please use double quotes. Thanks!

Comment 5

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

(In reply to Dão Gottwald [::dao] from comment #3)
> 
> I think checking nodeName would be saner since the xul: prefix is pretty
> arbitrary.

Doesn't nodeName also include the "xul:" prefix here? I could check the localName and namespaceURI to be more exact, if you prefer that.

Comment 6

[Dão Gottwald [:dao]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #5)
> (In reply to Dão Gottwald [::dao] from comment #3)
> > 
> > I think checking nodeName would be saner since the xul: prefix is pretty
> > arbitrary.
> 
> Doesn't nodeName also include the "xul:" prefix here?

Oops. Yes, it does.

> I could check the
> localName and namespaceURI to be more exact, if you prefer that.

I think just localName would be enough since trees shouldn't have arbitrary child nodes, but feel free to check namespaceURI too if you prefer.

Comment 7

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

Comment on attachment 8881415 [details]
Bug 1373714 - Support touch-dragging the scrollthumb in XUL tree widgets.

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

Comment 8

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

I added the namespaceURI check just to be safe. Also swapped the single quotes for double quotes as requested.

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/1d652394d0f4
Support touch-dragging the scrollthumb in XUL tree widgets. r=dao

Comment 10

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/1d652394d0f4

Comment 11

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

Can you check that touch-dragging scrollbar on about:config and in the library window works for you in today's nightly? Thanks!

Comment 12

[bekpekker]

Unfortunately I broke the screen of my tablet while I'm in Kyrgyzstan. Not sure when/if it can be fixed here at all, so testing is an issue currently... :-(

Comment 13

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

No worries, thanks anyway! We can get QA to verify.

Comment 14

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

Comment on attachment 8881415 [details]
Bug 1373714 - Support touch-dragging the scrollthumb in XUL tree widgets.

Approval Request Comment
[Feature/Bug causing the regression]: APZ touch support
[User impact if declined]: touch-dragging the scrollbar thumb in some XUL widgets (about:config, the bookmarks library, etc.) doesn't work
[Is this code covered by automated tests?]: not really
[Has the fix been verified in Nightly?]: verified locally
[Needs manual test from QE? If yes, steps to reproduce]: yes please. STR in comment 0
[List of other uplifts needed for the feature/fix]: none
[Is the change risky?]: pretty low risk
[Why is the change risky/not risky?]: small change, well understood code
[String changes made/needed]: none

Comment 15

[Julien Cristau [:jcristau] (back August 18)]

Brindusa, it would be good to verify this in nightly.

Comment 16

[Brindusa Tot, DTE]

Reproduced the issue on Nightly 56.0a1, on a build from 06.16.2017 with a Microsoft Surface Pro 2 device running Win 10 Insider Preview.

Verified as fixed on latest Nightly 56.0a1, build ID 20170706060058 on Windows 8.1 x64 touch and a Microsoft Surface Pro 2 device running Win 10 Insider Preview

Comment 17

[Julien Cristau [:jcristau] (back August 18)]

Comment on attachment 8881415 [details]
Bug 1373714 - Support touch-dragging the scrollthumb in XUL tree widgets.

fix for touch-drag on some widgets, verified in nightly, beta55+

Comment 18

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/releases/mozilla-beta/rev/b8f32d3d5d85

Comment 19

[Petruta Horea [:phorea]]

Verified as fixed on Firefox 55 beta 8 on Microsoft Surface Pro 2 device running Win 10 Insider Preview.