1359211 - Handle touch-scrolling of XUL <listbox> with APZ enabled

Status: RESOLVED FIXED

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

Description

[Botond Ballo [:botond]]

XUL <listbox> is a special snowflake in that it builds an nsIScrollableFrame (which makes it use APZ scrolling by default), but doesn't play nicely with APZ scrolling (doesn't respect the displayport, and is designed to scroll by whole-line increments only).

Wheel-scrolling it works fine because it listens for preventDefault()s wheel events, so APZ doesn't scroll in response to them, and scrolls itself instead.

Scrollbar dragging also works fine: it uses a custom scrollbar mediator, and, as of bug 1355376, opts out of the APZ drag codepath.

For touch events, though, it doesn't currently do anything, causing APZ to try to touch-scroll it as if it were a normal scroll frame, resulting in bad behaviour (like unpainted regions being brought into view and then never being painted).

<listbox> needs to handle touch events similar to how it handles wheel events: preventDefault() them and scroll itself. We did this in bug 1302736 for <tree>, so it should just be a matter of adapting that code for <listbox>.

Comment 1

[Botond Ballo [:botond]]

(Every platform is an APZ platform now :)).

Comment 2

[Botond Ballo [:botond]]

Attachment: Bug 1359211 - Handle touch-scrolling of XUL <listbox> in JS.

The APZ scrolling codepath doesn't do the right thing for <listbox>
without special handling, so have it scroll in JS instead, like we
did in bug 1302736 for <tree>.

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

Comment 3

[Botond Ballo [:botond]]

Comment on attachment 8861180 [details]
Bug 1359211 - Handle touch-scrolling of XUL <listbox> in JS.

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

Comment 4

[Botond Ballo [:botond]]

Here's what I'm asking for reviews on:

  dao: the change to listbox.xml
  kats: the C++ changes
  bz: WebIDL approval

Comment 5

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

Comment on attachment 8861180 [details]
Bug 1359211 - Handle touch-scrolling of XUL <listbox> in JS.

https://reviewboard.mozilla.org/r/133144/#review135946

Looks good to me, but please get :dao or one of the other toolkit peers to sign off on this, and you'll need a DOM peer to sign off on the .webidl change, I think.

::: toolkit/content/widgets/listbox.xml:84
(Diff revision 1)
>              extends="chrome://global/content/bindings/general.xml#basecontrol">
>  
>      <implementation implements="nsIDOMXULMultiSelectControlElement">
>        <field name="_lastKeyTime">0</field>
>        <field name="_incrementalString">""</field>
> -
> +      

looks like whitespace was added, please remove

::: toolkit/content/widgets/listbox.xml:875
(Diff revision 1)
>                this.ensureIndexIsVisible(targetIndex);
>                break;
>            }
>          ]]>
>        </handler>
> +      

nit: trailing ws

::: toolkit/content/widgets/listbox.xml:910
(Diff revision 1)
> +      <handler event="touchend">
> +        <![CDATA[
> +          this._touchY = -1;
> +        ]]>
> +      </handler>
> +      

more trailing ws

Comment 6

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8861180 [details]
Bug 1359211 - Handle touch-scrolling of XUL <listbox> in JS.

https://reviewboard.mozilla.org/r/133144/#review136430

r=me on the webidl bits

Comment 7

[Dão Gottwald [:dao]]

Comment on attachment 8861180 [details]
Bug 1359211 - Handle touch-scrolling of XUL <listbox> in JS.

https://reviewboard.mozilla.org/r/133144/#review137240

::: toolkit/content/widgets/listbox.xml:84
(Diff revision 2)
>              extends="chrome://global/content/bindings/general.xml#basecontrol">
>  
>      <implementation implements="nsIDOMXULMultiSelectControlElement">
>        <field name="_lastKeyTime">0</field>
>        <field name="_incrementalString">""</field>
> -
> +      

please drop this change

::: toolkit/content/widgets/listbox.xml:875
(Diff revision 2)
>                this.ensureIndexIsVisible(targetIndex);
>                break;
>            }
>          ]]>
>        </handler>
> +      

trailing spaces

::: toolkit/content/widgets/listbox.xml:895
(Diff revision 2)
> +      <handler event="touchmove">
> +        <![CDATA[
> +          if (event.touches.length == 1 &&
> +              this._touchY >= 0) {
> +            var deltaY = this._touchY - event.touches[0].screenY;
> +            var lines = Math.trunc(deltaY / this.listBoxObject.getRowHeight());

please use let instead of var

::: toolkit/content/widgets/listbox.xml:910
(Diff revision 2)
> +      <handler event="touchend">
> +        <![CDATA[
> +          this._touchY = -1;
> +        ]]>
> +      </handler>
> +      

trailing spaces again

Comment 8

[Botond Ballo [:botond]]

Comment on attachment 8861180 [details]
Bug 1359211 - Handle touch-scrolling of XUL <listbox> in JS.

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

Comment 9

[Botond Ballo [:botond]]

Addressed review comments and autolanded.

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/76bcb2239fb0
Handle touch-scrolling of XUL <listbox> in JS. r=bz,dao,kats

Comment 11

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/76bcb2239fb0