1302736 - [e10s] Library window issues on touch devices

Status: VERIFIED FIXED

(Toolkit :: UI Widgets, defect, P3)

Description

[Petruta Horea [:phorea], Desktop Test Engineering]

[Note]:
- Create 'browser.tabs.remote.force-enable' preference and set it to true. Restart the browser.

[Affected versions]:
- Nightly 51.0a1 2016-09-14

[Affected platforms]:
- Microsoft Surface 2 Pro with Win 10 64-bit

[Steps to reproduce]:
1. Open Firefox with a profile that has several history items and bookmarks
2. Open the History/Bookmarks Library
3. Touch scroll the history and the bookmarks lists
4. Double tap on an expandable folder (eg "> Bookmarks Toolbar") to view it's content
5. Tap another entry from the Library

[Expected result]:
3. The Library items can be scrolled using the finger.
5. Double tap expands the folders

[Actual result]:
3. Cannot touch scroll inside the Library window
5. The Library window becomes unresponsive

Comment 1

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

Interesting. I think items 3 and 5 are separate issues - the unresponsiveness is weird, since everything repaints again properly after the mouse is moved. I'll probably split this into two bugs later, when I look into fixing it.

Comment 2

[Markus Stange [:mstange]]

The library window uses a XUL tree, which scrolls by processing DOMMouseScroll events. The history sidebar, and about:config, also use XUL trees. XUL trees also put <scrollbars> on the side and react to scrollbar position changes.

Comment 3

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

Looking into fixing this (the scrolling of XUL trees part, anyway).

Comment 4

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

Making the XUL trees scroll on touch was relatively straightforward. I didn't bother adding any flinging animations or physics of that sort, it just scrolls while your finger is down. XUL is on the chopping block anyway so I don't think it's worth spending the time and adding the type of complexity that results from adding fling animations and trying to make them match the other fling animations.

For the double-tapping (item 4 in the STR, as well as mentioned in bug 1304703) I made the GestureEventListener code use it's existing double-tap detection code even in the case where double-tap is disabled or disallowed. That then results in a regular click event being sent to content, but with clickCount=2. This is the same thing that Chrome does, and it seems to make sense. (In fact Chrome detects triple-taps as well on my Windows surface device).

I'll push the patches to try once the tree is open again and put them up for review if everything is good.

Comment 5

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

https://treeherder.mozilla.org/#/jobs?repo=try&revision=3067b29ef288&group_state=expanded is looking good.

Comment 6

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

Attachment: Bug 1302736 - Add rudimentary touch-scrolling support to the XUL tree widget.

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

Comment 7

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

Attachment: Bug 1302736 - Fire click events with a clickCount of 2 when the user does a double-tap gesture with double taps not allowed.

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

Comment 8

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

Wasn't sure who would be a good reviewer for the first patch - jaws, feel free to bounce it to somebody else if you think they would more appropriate.

Comment 9

[Benjamin Smedberg]

Neil Deakin is the best reviewer for XUL widget stuff; he's on PTO until next week.

Comment 10

[Botond Ballo [:botond]]

Comment on attachment 8795350 [details]
Bug 1302736 - Fire click events with a clickCount of 2 when the user does a double-tap gesture with double taps not allowed.

https://reviewboard.mozilla.org/r/81430/#review80406

::: gfx/layers/apz/src/GestureEventListener.cpp:202
(Diff revision 1)
>      break;
>    case GESTURE_FIRST_SINGLE_TOUCH_UP:
> +  case GESTURE_SECOND_SINGLE_TOUCH_DOWN:
>      // Cancel wait for double tap
>      CancelMaxTapTimeoutTask();
> -    SetState(GESTURE_MULTI_TOUCH_DOWN);
> +    MOZ_ASSERT(mSingleTapSent);

This reads as "assert that a single tap was sent", which is misleading. I'd write it as:

  MOZ_ASSERT(mSingleTapSent.IsSome());

::: gfx/layers/apz/src/GestureEventListener.cpp:206
(Diff revision 1)
>      CancelMaxTapTimeoutTask();
> -    SetState(GESTURE_MULTI_TOUCH_DOWN);
> +    MOZ_ASSERT(mSingleTapSent);
> +    if (!mSingleTapSent.value()) {
> -    TriggerSingleTapConfirmedEvent();
> +      TriggerSingleTapConfirmedEvent();
> -    // Prevent APZC::OnTouchStart() from handling MULTITOUCH_START event
> -    rv = nsEventStatus_eConsumeNoDefault;
> +    }
> +    mSingleTapSent = Nothing();

This can also be written as |mSingleTapSent.reset()|, although feel free to keep this form if you think it reads beetter.

::: widget/InputData.h:490
(Diff revision 1)
>      TAPGESTURE_LONG,
>      TAPGESTURE_LONG_UP,
>      TAPGESTURE_UP,
>      TAPGESTURE_CONFIRMED,
>      TAPGESTURE_DOUBLE,
> +    TAPGESTURE_SECOND,

It's not obvious to me from the name what this signifies. Add a comment?

Update: a pointer to GeckoContentController::TapType is fine.

Comment 11

[Dão Gottwald [:dao]]

Comment on attachment 8795349 [details]
Bug 1302736 - Add rudimentary touch-scrolling support to the XUL tree widget.

>      <handler event="touchstart">
>        <![CDATA[
>          if (event.touches.length > 1) {
>            // Multiple touch points detected, abort.
>            this._touchY = -1;
>          } else {
>            this._touchY = event.touches[0].screenY;
>          }
>        ]]>
>      </handler>

How about:

      <handler event="touchstart">
        <![CDATA[
          // Don't handle if we have multiple touch points.
          if (event.touches.length == 1) {
            this._touchY = event.touches[0].screenY;
          }
        ]]>
      </handler>


>      <handler event="touchmove">
>        <![CDATA[
>          if (event.touches.length == 1 &&
>              this._touchY != -1) {

I'd use this._touchY > -1 or this._touchY >= 0 here.

Comment 12

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

(In reply to Dão Gottwald [:dao] from comment #11)
> How about:
> 
>       <handler event="touchstart">
>         <![CDATA[
>           // Don't handle if we have multiple touch points.
>           if (event.touches.length == 1) {
>             this._touchY = event.touches[0].screenY;
>           }
>         ]]>
>       </handler>

That's not really the same, though. If the user starts with one finger down, and then puts a second finger down, I would like that second finger to abort the panning gesture until all fingers are cleared. With your suggestion it would resume panning after the user lifted one of the fingers.

> 
> I'd use this._touchY > -1 or this._touchY >= 0 here.

Sure.

Comment 13

[Dão Gottwald [:dao]]

(In reply to Kartikaya Gupta (email:[e-mail]) from comment #12)
> That's not really the same, though. If the user starts with one finger down,
> and then puts a second finger down, I would like that second finger to abort
> the panning gesture until all fingers are cleared. With your suggestion it
> would resume panning after the user lifted one of the fingers.

Yes, I figured. I thought it made sense that way but I guess you've thought about it longer than I have.

Comment 14

[Dão Gottwald [:dao]]

If there are good reasons to abort in that case, maybe expand on that in the code comment?

Comment 15

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

Will do, thanks.

Comment 16

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

(In reply to Botond Ballo [:botond] from comment #10)
> 
> This reads as "assert that a single tap was sent", which is misleading. I'd
> write it as:
> 
>   MOZ_ASSERT(mSingleTapSent.IsSome());

Good point, I updated this throughout the patch.

> ::: gfx/layers/apz/src/GestureEventListener.cpp:206
> > +    mSingleTapSent = Nothing();
> 
> This can also be written as |mSingleTapSent.reset()|, although feel free to
> keep this form if you think it reads beetter.

I left this as is - using Nothing() makes it more obvious that it's a Maybe<> which I think is good, particularly given the previous comment.

> ::: widget/InputData.h:490
> >      TAPGESTURE_CONFIRMED,
> >      TAPGESTURE_DOUBLE,
> > +    TAPGESTURE_SECOND,
> 
> It's not obvious to me from the name what this signifies. Add a comment?
> 
> Update: a pointer to GeckoContentController::TapType is fine.

Added.

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/7de1a998b12e
Add rudimentary touch-scrolling support to the XUL tree widget. r=dao
https://hg.mozilla.org/integration/mozilla-inbound/rev/40b602b3a870
Fire click events with a clickCount of 2 when the user does a double-tap gesture with double taps not allowed. r=botond

Comment 19

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/7de1a998b12e
https://hg.mozilla.org/mozilla-central/rev/40b602b3a870

Comment 20

[Petruta Horea [:phorea], Desktop Test Engineering]

Verified as fixed with 50.0a1 Nightly 2016-10-18.