Petruta Horea [:phorea], Desktop QA Reporter
	Description • 8 years ago

[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

	Kartikaya Gupta (email:kats@mozilla.staktrace.com) Assignee
	Comment 1 • 8 years ago

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.

	Markus Stange [:mstange]
	Comment 2 • 8 years ago

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.

	Kartikaya Gupta (email:kats@mozilla.staktrace.com) Assignee
	Comment 3 • 8 years ago

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

	Kartikaya Gupta (email:kats@mozilla.staktrace.com) Assignee
	Comment 4 • 8 years ago

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.

	Kartikaya Gupta (email:kats@mozilla.staktrace.com) Assignee
	Comment 5 • 8 years ago

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

	Kartikaya Gupta (email:kats@mozilla.staktrace.com) Assignee
	Comment 8 • 8 years ago

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.

	Benjamin Smedberg
	Comment 9 • 8 years ago

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

	Botond Ballo [:botond]
	Comment 10 • 8 years ago
mozreview-review

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.

	Dão Gottwald [:dao]
	Comment 11 • 8 years ago

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.

	Kartikaya Gupta (email:kats@mozilla.staktrace.com) Assignee
	Comment 12 • 8 years ago

(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.

	Dão Gottwald [:dao]
	Comment 13 • 8 years ago

(In reply to Kartikaya Gupta (email:kats@mozilla.com) 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.

	Dão Gottwald [:dao]
	Comment 14 • 8 years ago

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

	Kartikaya Gupta (email:kats@mozilla.staktrace.com) Assignee
	Comment 15 • 8 years ago

Will do, thanks.

	Kartikaya Gupta (email:kats@mozilla.staktrace.com) Assignee
	Comment 16 • 8 years ago

(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.

	Pulsebot
	Comment 17 • 8 years ago

Pushed by kgupta@mozilla.com:
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

	Carsten Book [:Tomcat]
	Comment 19 • 8 years ago
bugherder

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

	Petruta Horea [:phorea], Desktop QA Reporter
	Comment 20 • 8 years ago

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