[e10s] Library window issues on touch devices

VERIFIED FIXED in Firefox 52

Status

()

Toolkit
XUL Widgets
P3
normal
VERIFIED FIXED
a year ago
8 months ago

People

(Reporter: petruta, Assigned: kats)

Tracking

(Blocks: 1 bug, {regression})

51 Branch
mozilla52
All
Windows 10
regression
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(e10s+, firefox49 unaffected, firefox50 unaffected, firefox51 unaffected, firefox52 verified)

Details

(Whiteboard: [gfx-noted][nightly only])

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(2 attachments)

(Reporter)

Description

a year 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
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.
Blocks: 1244402
Priority: -- → P3
Whiteboard: [gfx-noted][nightly only]
Version: Trunk → 51 Branch

Updated

a year ago
tracking-e10s: ? → +
Keywords: regression
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.
status-firefox49: --- → unaffected
status-firefox51: affected → unaffected
status-firefox52: --- → affected
Blocks: 1304703
Looking into fixing this (the scrolling of XUL trees part, anyway).
Assignee: nobody → bugmail
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.
https://treeherder.mozilla.org/#/jobs?repo=try&revision=3067b29ef288&group_state=expanded is looking good.
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
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

a year ago
Neil Deakin is the best reviewer for XUL widget stuff; he's on PTO until next week.
Attachment #8795349 - Flags: review?(jaws) → review?(dao+bmo)

Comment 10

a year 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.
Attachment #8795350 - Flags: review?(botond) → review+
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.
Attachment #8795349 - Flags: review?(dao+bmo) → review+
Component: Panning and Zooming → XUL Widgets
Product: Core → Toolkit
(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.
(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.
If there are good reasons to abort in that case, maybe expand on that in the code comment?
Will do, thanks.
(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

a year 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
No longer blocks: 1304703
Duplicate of this bug: 1304703

Comment 19

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/7de1a998b12e
https://hg.mozilla.org/mozilla-central/rev/40b602b3a870
Status: NEW → RESOLVED
Last Resolved: a year ago
status-firefox52: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
(Reporter)

Comment 20

a year ago
Verified as fixed with 50.0a1 Nightly 2016-10-18.
Status: RESOLVED → VERIFIED
status-firefox52: fixed → verified
(Assignee)

Updated

11 months ago
Blocks: 1334161

Updated

8 months ago
See Also: → bug 1359211
You need to log in before you can comment on or make changes to this bug.