Closed Bug 1302736 Opened 3 years ago Closed 3 years ago

[e10s] Library window issues on touch devices

Categories

(Toolkit :: XUL Widgets, defect, P3)

51 Branch
All
Windows 10
defect

Tracking

()

VERIFIED FIXED
mozilla52
Tracking Status
e10s + ---
firefox49 --- unaffected
firefox50 --- unaffected
firefox51 --- unaffected
firefox52 --- verified

People

(Reporter: petruta.rasa, Assigned: kats)

References

(Blocks 1 open bug)

Details

(Keywords: regression, Whiteboard: [gfx-noted][nightly only])

Attachments

(2 files)

[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
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.
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.
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.
Neil Deakin is the best reviewer for XUL widget stuff; he's on PTO until next week.
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?
(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.
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
https://hg.mozilla.org/mozilla-central/rev/7de1a998b12e
https://hg.mozilla.org/mozilla-central/rev/40b602b3a870
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla52
Verified as fixed with 50.0a1 Nightly 2016-10-18.
Status: RESOLVED → VERIFIED
See Also: → 1359211
You need to log in before you can comment on or make changes to this bug.