Closed Bug 1373714 Opened 2 years ago Closed 2 years ago

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

Categories

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

54 Branch
All
Windows 8.1
defect

Tracking

()

VERIFIED FIXED
mozilla56
Tracking Status
firefox54 --- wontfix
firefox55 --- verified
firefox56 --- verified

People

(Reporter: kats, Assigned: kats)

References

Details

(Whiteboard: [gfx-noted])

Attachments

(1 file)

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.
Assignee: nobody → bugmail
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 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.
Attachment #8881415 - Flags: review?(dao+bmo) → review+
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!
(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.
Flags: needinfo?(dao+bmo)
(In reply to Kartikaya Gupta (email:kats@mozilla.com) 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.
Flags: needinfo?(dao+bmo)
I added the namespaceURI check just to be safe. Also swapped the single quotes for double quotes as requested.
Pushed by kgupta@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/1d652394d0f4
Support touch-dragging the scrollthumb in XUL tree widgets. r=dao
https://hg.mozilla.org/mozilla-central/rev/1d652394d0f4
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla56
Can you check that touch-dragging scrollbar on about:config and in the library window works for you in today's nightly? Thanks!
Flags: needinfo?(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... :-(
Flags: needinfo?(bekpekker)
No worries, thanks anyway! We can get QA to verify.
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
Attachment #8881415 - Flags: approval-mozilla-beta?
Brindusa, it would be good to verify this in nightly.
Flags: qe-verify+
Flags: needinfo?(brindusa.tot)
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
Status: RESOLVED → VERIFIED
Flags: needinfo?(brindusa.tot)
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+
Attachment #8881415 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Verified as fixed on Firefox 55 beta 8 on Microsoft Surface Pro 2 device running Win 10 Insider Preview.
Flags: qe-verify+
You need to log in before you can comment on or make changes to this bug.