920956 - DevTools touch emulation: suppress regular mouse events, emulate 300ms delay

Status: RESOLVED FIXED

(DevTools :: Responsive Design Mode, defect)

Description

[Patrick H. Lauke]

Attachment: Screen Shot 2013-09-26 at 11.08.46.png

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_8_5) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/29.0.1547.76 Safari/537.36

Steps to reproduce:

1) open responsive web view (just introduced in Aurora 26.0a2 ?), switch on touch event emulation, go to http://patrickhlauke.github.io/touch/tests/event-listener.html
2) move mouse pointer over the button without clicking
3) click the button


Actual results:

2) regular mouse events mouseover, mousemove, mouseout are being fired
3) correct sequence of touchstart > touchend > mousedown > (a single) mousemove > mouseup > click are being fired, but the 300ms delay between touchend and mousedown is not emulated


Expected results:

2) regular mouse events should be suppressed when touch emulation is turned on (additionally, perhaps scrolling should work similar to touch as well, i.e. user has to click and drag to scroll similar to finger scrolling on touch)
3) there should be a 300ms delay between touchend and the mouse events (unless it's optimised away, as on real mobile devices, when meta viewport has user-scalable=no or minimum-scale and maximum-scale are set to the same value)

Comment 1

[Paul Rouget [:paul]]

Simulation code lives here:

http://mxr.mozilla.org/mozilla-central/source/browser/devtools/shared/touch-events.js

Comment 2

[Paul Rouget [:paul]]

Kevin, would that be a problem for Gaia if we fix this bug?

Comment 3

[Kevin Grandon :kgrandon]

I don't think we want to suppress mouse events at all. For better or worse, iOS has set "the standard" when it comes to touch events and mouse events working together. I know we've actually done some work recently to follow those conventions. In the long run we'll want to use pointer events everywhere, but for now we should stay as-is I believe. 

Also, we should probably ensure that we are compliant with this: http://www.w3.org/TR/touch-events/#mouse-events

Comment 4

[Patrick H. Lauke]

kevin, you seem to misunderstand this bug. the issue here is when you move the mouse pointer over things while touch event emulation is enabled. it should not fire traditional mouse events when the mouse is moving (as this is equivalent to hovering your finger above the touchscreen, as in nothing should happen at all). only once the user presses the mouse button down (equivalent to a touch on a real device) should events fire when touch emulation is on.

Comment 5

[Kevin Grandon :kgrandon]

(In reply to Patrick H. Lauke from comment #4)
> kevin, you seem to misunderstand this bug. the issue here is when you move
> the mouse pointer over things while touch event emulation is enabled. it
> should not fire traditional mouse events when the mouse is moving (as this
> is equivalent to hovering your finger above the touchscreen, as in nothing
> should happen at all). only once the user presses the mouse button down
> (equivalent to a touch on a real device) should events fire when touch
> emulation is on.

Yes, this description is more clear than the original bug description. The spec reads, "If the user agent dispatches both touch events and mouse events in response to a single user action, then the touchstart event type must be dispatched before any mouse event types for that action".

It appears that the proper fix here would be that when touch events are enabled, we suppress mousemove events until after the touchstart event. @paul - this would have no negative impact for gaia developers, and we would prefer if our implementation followed the spec closer.

Comment 6

[Patrick H. Lauke]

i.e. i'm NOT talking about the synthetic/fallback mouse events that are fired as per touch spec (touchstart > touchmove > touchend > [300ms delay] > mouseover > mousemove (only a single one) > mousedown > mouseup > click), but i'm saying that one touch emulation is turned on in responsive mode, simply *moving* your mouse over elements should not fire mouseover/mouseout that is just a result of mouse movement itself, rather than the result of a clicking of the mouse button.

Comment 7

[Patrick H. Lauke]

not just mousemove events, but also mouseover/mouseout.

the second part would be to simulate the 300ms delay after touchend (but only in situations where it's not optimised away, as per the bug report)

Comment 8

[Kevin Grandon :kgrandon]

Yes - it's totally clear to me now Patrick. It makes sense that we should fix these issues!

It also appears that if the default action of a touchstart event is prevented, we should not fire any mouse events. It doesn't seem like we're handling that currently, this is probably another bug though.

Comment 9

[Patrick H. Lauke]

yes, the preventDefault issue would be bug https://bugzilla.mozilla.org/show_bug.cgi?id=861876 (also filed by me ;) )

Comment 10

[Hubert B Manilla]

hi paul,
Can i tackle this?

Comment 11

[Denis Washington]

Attachment: Don't fire non-touch mouse events during touch emulation

A colleague and I have created this patch for an university assignment. We didn't realize the bug was already assigned, but I hope it is still ok that we work on this.

The patch suppresses the mouseover, mouseenter, mouseleave and mouseout events when touch emulation is enabled, as well as mousemove events without event target (i.e., without held-down mouse button), by using event.stopPropagation(). The 300ms delay between touch and mouse events is not yet implemented, though.

We hope our approach is worthwhile and are eagerly awaiting your feedback!

Comment 12

[Paul Rouget [:paul]]

Thanks Denis. When you submit a patch, it's good to flag it "feedback ?" then someone will look at it.

Go to "details", add the feedback flag ("?") and add ":paul" (me).

Comment 13

[Denis Washington]

Attachment: Suppress non-touch mouse events, add 300ms delay before legacy mouse events

This version of the patch improves upon the previous one by including the requested 300ms delay between touch and legacy mouse events. This turned out to be a trivial change, as the mouse event triggering already happened within a setTimeout(0) block, which we just changed into a setTimeout(300) one.

@paul: Should we write an accompanying regression test for this bug? We would be happy to do so, and would welcome any pointers towards the technologies/infrastructure we should use for that, if you wish.

Comment 14

[Patrick H. Lauke]

Note that the 300ms timeout doesn't happen in certain conditions - for instance, if user-scalable=no or the same minimum-scale and maximum-scale are set in the viewport meta (and, once supported, the equivalent in CSS @viewport). see slides 60-61 in http://www.slideshare.net/redux/getting-touchy-an-introduction-to-touch-and-pointer-events-future-of-web-apps-london-24102013

Comment 15

[Denis Washington]

Thanks for pointing this out! How would we check for these things? Are there specific API's for querying such viewport attributes? Also, it would be nice to know when exactly the delay appears and when not. Are there other conditions as the ones you described, and if yes, where can they be looked up?

With regards to regression testing, https://developer.mozilla.org/en-US/docs/Mozilla/QA/Automated_testing#reftest_%28R%29 suggests that browser-chrome would be the approach to use here. However, we could not find any browser-chrome tests within toolkit/devtools or browser/devtools. Does the devtools component use browser-chrome tests, and would it be desirable for you to have such a test for this bug?

Comment 16

[Denis Washington]

Thinking about this, I guess a simple approach for checking the viewport constraint would be to search for viewport <meta> tags with document.querySelector("meta[name=viewport]") and simply look for user-scalable, minimum-scale and maximum-scale values in the "content" attribute. Is this a sensible idea, or is there a higher-level API within the Mozilla codebase?

Comment 17

[Paul Rouget [:paul]]

Comment on attachment 831604 [details] [diff] [review]
Suppress non-touch mouse events, add 300ms delay before legacy mouse events

Patch looks good.

For the test, you want to add some checks here:

http://mxr.mozilla.org/mozilla-central/source/browser/devtools/responsivedesign/test/browser_responsiveui_touch.js

To run this test:

./mach mochitest-browser browser/devtools/responsivedesign/test/browser_responsiveui_touch.js


For the 300ms delay, I'm not sure how this can play with a non-touch browser. Maybe you just want to ignore the viewport (viewports are not supported on desktop) and not support the 0s delay. Maybe playing with these methods might help:

http://mxr.mozilla.org/mozilla-central/source/dom/interfaces/base/nsIDOMWindowUtils.idl#163
http://mxr.mozilla.org/mozilla-central/source/content/base/public/nsIFrameLoader.idl#20

(especially the last one)

Also, we need to sync-up with the Gaia people. I'm afraid we'd break Gaia on Desktop with these changes (again). I'll ask the concerned people.

Comment 18

[Denis Washington]

We are working on adding tests to the file you pointed us to right now. Thanks for pointing us there!

If viewport is not supported by desktop Firefox at all, always simulating the 300ms delay seems sensible, especially as the 0s delay seems to be more or less an optimization. Do you think it'll it'll be a serious problem if the 0s cases are not considered in the touch emulation?

Also, any update on the Gaia situation?

Comment 19

[Denis Washington]

Re regression test: we are currently having a very strange problem. We tried to extend the mochitest touch.html with:

  div.addEventListener("mouseover", function(evt) {
    div.style.backgroundColor = "red";
  });

And testWithTouch() in browser_responsiveui_touch.js with the following new check:

    EventUtils.synthesizeMouse(div, x, y, {type: "mouseover"}, content);
    isnot(div.style.backgroundColor, "red", "mouseover not fired");

As "mouseover" is stopPropagation()'d in touch emulation mode (verified manually), the test <div> shouldn't have a red background, but turns out it does anyway! Further investigation revealed that the problem was in the previously executed testWithoutTouch() (where touch emulation isn't turned on); this line

   EventUtils.synthesizeMouse(div, x, y, {type: "mousemove"}, content);

causes the "moveover" event handler to run (tested by commenting out everything else) although it synthesizes a "mousemove", not a "mouseover" event. Even more strangely, replacing the generated "mousemove" event with an "mouseover" event in testWithoutTouch() causes the div *not* to get a red background, although "mouseover" is exactly what the event handler in touch.html is bound to! We have no idea what is going on there.

If you have any idea what we might be doing wrong, we would be extremely grateful.

Comment 20

[Denis Washington]

Attachment: Updated patch with test case

We finally managed to add tests to the patch. While we weren't able to directly emit a "mouseover" event, we found out that the sequence of events triggered in the existing test (moving the mouse over the test <div>, pressing the mouse button, dragging) quite logically has the side effect of triggering "mouseover", so the suppression can still be tested quite nicely. Also, we extended the test case to include a "mouseout" event and check if that event is suppressed, too.

Due to the test, we revealed a bug in the original patch that would cause "mouseover" etc. not to be suppressed in the 300ms timeframe between a touch event and the delayed mouse event. We could fix this by moving up the suppression code a bit.

We consider the patch ready and would be happy if you could review it. (Does it need a second reviewer or a superreview because of the potential problems for Gaia developers?)

Comment 21

[Paul Rouget [:paul]]

I'll test with Firefox OS on Desktop and see how it feels (and if see if this patch breaks anything).

About the 300ms…

I think we should and can support that. We have some builtin functions that might help us to guess is the delay is here or not:

You can run that in a chrome scratchpad
(see https://developer.mozilla.org/en-US/docs/Tools/Scratchpad#Using_Scratchpad_to_access_Firefox_internals)

let window = gBrowser.selectedBrowser.contentWindow;
let u = window.QueryInterface(Ci.nsIInterfaceRequestor).getInterface(Ci.nsIDOMWindowUtils);

let vp = {
    defaultZoom: {},
    allowZoom: {},
    minZoom: {},
    maxZoom: {},
    width: {},
    height: {},
    autoSize: {}
}

u.getViewportInfo(2000, 2500, vp.defaultZoom, vp.allowZoom, vp.minZoom, vp.maxZoom, vp.width, vp.height, vp.autoSize);


u.getDocumentMetadata("viewport-initial-scale");

Checking allowZoom would allow use to know if we should use a delay or not.


But this code doesn't appear to work well here, I'm trying to figure out what's going on…

Comment 22

[Paul Rouget [:paul]]

This appears to work for me:


let contentWindow = gBrowser.selectedBrowser.contentWindow;
let u = contentWindow.QueryInterface(Ci.nsIInterfaceRequestor).getInterface(Ci.nsIDOMWindowUtils);
let allowZoom = {};

u.getViewportInfo(contentWindow.innerWidth, contentWindow.innerHeight, {}, allowZoom, {}, {}, {}, {}, {});

allowZoom.value; // if this is false, don't use the delay

Comment 23

[Denis Washington]

Cool, we'll integrate this into the patch as soon as possible.

Comment 24

[Denis Washington]

Attachment: Updated patch including the conditional 300ms delay

@Paul: We have incorporated your code and and now only delay the mouse events if allowZoom is true.

Comment 25

[Paul Rouget [:paul]]

Comment on attachment 8337928 [details] [diff] [review]
Updated patch including the conditional 300ms delay

>diff --git a/toolkit/devtools/touch-events.js b/toolkit/devtools/touch-events.js
>   let TouchEventHandler = {
>     enabled: false,
>-    events: ['mousedown', 'mousemove', 'mouseup', 'click'],
>+    events: [
>+      // Suppressed events
>+      'mouseenter', 'mouseover', 'mouseout', 'mouseleave',
>+      // Events causing simulated touch events to be fired
>+      'mousedown', 'mousemove', 'mouseup', 'click'
>+    ],

Maybe you want to create 2 arrays. Up to you.

>         case 'mousemove':
>-          if (!eventTarget)
>+          if (!eventTarget) {
>+            // Suppress "mousemove" events while the simulated finger (mouse
>+            // button) is not held down, as these events cannot possibly occur
>+            // in a touch environment.
>+            evt.stopPropagation();
>             return;
>+          }

So if there no eventTarget, it means the mouse button is not pressed?

>+    delayBeforeMouseEvent: function(evt) {
>+      // On mobile platforms, Firefox inserts a 300ms delay between
>+      // touch events and accompanying mouse events, except if the
>+      // content window is not zoomable.
>+
>+      let content = this.getContent(evt.target);
>+      let utils = content.QueryInterface(Ci.nsIInterfaceRequestor)
>+                         .getInterface(Ci.nsIDOMWindowUtils);
>+
>+      let allowZoom = {};
>+      utils.getViewportInfo(content.innerWidth, content.innerHeight,
>+                            {}, allowZoom, {}, {}, {}, {}, {});
>+
>+      return allowZoom.value ? 300 : 0;
>+    },

Great!


-------


We have some unrelated issues with touch events (see bug 891882), so I don't want to land this patch until it gets fixed (hopefully very soon). Also, I need to make sure we didn't break Gaia (just need to run some Gaia unit tests with this patch applied).

Comment 26

[Paul Rouget [:paul]]

I'll r+ this patch once you explained the mousemove thing.

Comment 27

[Paul Rouget [:paul]]

Comment on attachment 8337928 [details] [diff] [review]
Updated patch including the conditional 300ms delay

Kevin, can you take a quick look at this? Do you think it will break anything on the Gaia side?

Comment 28

[Kevin Grandon :kgrandon]

Comment on attachment 8337928 [details] [diff] [review]
Updated patch including the conditional 300ms delay

Review of attachment 8337928 [details] [diff] [review]:
-----------------------------------------------------------------

Did not test, but looking at the code I don't see anything that should cause issues. Thanks for the patch!

Comment 29

[Denis Washington]

(In reply to Paul Rouget [:paul] from comment #25)
> Comment on attachment 8337928 [details] [diff] [review]
> Updated patch including the conditional 300ms delay
> 
> >diff --git a/toolkit/devtools/touch-events.js b/toolkit/devtools/touch-events.js
> >   let TouchEventHandler = {
> >     enabled: false,
> >-    events: ['mousedown', 'mousemove', 'mouseup', 'click'],
> >+    events: [
> >+      // Suppressed events
> >+      'mouseenter', 'mouseover', 'mouseout', 'mouseleave',
> >+      // Events causing simulated touch events to be fired
> >+      'mousedown', 'mousemove', 'mouseup', 'click'
> >+    ],
> 
> Maybe you want to create 2 arrays. Up to you.

We thought that adding the event names to the existing array made it clearer that they dispatch to the same handleEvent method.
 
> >         case 'mousemove':
> >-          if (!eventTarget)
> >+          if (!eventTarget) {
> >+            // Suppress "mousemove" events while the simulated finger (mouse
> >+            // button) is not held down, as these events cannot possibly occur
> >+            // in a touch environment.
> >+            evt.stopPropagation();
> >             return;
> >+          }
> 
> So if there no eventTarget, it means the mouse button is not pressed?

eventTarget is a slightly misleading name from the exisiting code. The variable does not actually hold event.target, but this.target (with "this" being the TouchEventHandler), which is set on "mousedown" and unset on "mouseup". So eventTarget is really only set while the mouse button is pressed.

Comment 30

[Paul Rouget [:paul]]

Comment on attachment 8337928 [details] [diff] [review]
Updated patch including the conditional 300ms delay

Thank you Denis!

Try: https://tbpl.mozilla.org/?tree=Try&rev=4f6ec56e05bf

Comment 31

[Denis Washington]

The Try build shows a mochitest-browser timeout failure in a somewhat unrelated test (browser/devtools/styleinspector/test/browser_ruleview_livepreview.js). I cannot reproduce it on my Linux machine. As this is a timeout error, could this be a spurious failure that disappears on a rebuild?

Comment 32

[Paul Rouget [:paul]]

My try code was a little bit old, and this failure is related to another part of the code we are working on. Let's not worry about that.

Comment 33

[Denis Washington]

Sorry for my lacke of knowledge for the process, but what exactly is there left to do for the patch to be accepted into trunk? Is there someone who will have to approve the try build before this happens?

Comment 34

[Paul Rouget [:paul]]

(In reply to Denis Washington from comment #33)
> Sorry for my lacke of knowledge for the process, but what exactly is there
> left to do for the patch to be accepted into trunk? Is there someone who
> will have to approve the try build before this happens?

We wait until bug 891882 get fixed. Then we'll need to add "checkin-needed" in the keywords field.

Comment 35

[Paul Rouget [:paul]]

Bug 891882 will land very soon. We should check that this patches still works as expected. We might need to rebase it.

Comment 36

[Denis Washington]

I'll check this today-

Comment 37

[Denis Washington]

Attachment: Rebased patch

This version of the patch is rebased to the current tip of mozilla-central. It should apply cleanly again now.

Comment 38

[Paul Rouget [:paul]]

try: https://tbpl.mozilla.org/?tree=Try&rev=a7e72a2c6f31

Comment 39

[Paul Rouget [:paul]]

Comment on attachment 8365704 [details] [diff] [review]
Rebased patch

Alex, with all the issues we ran into with bug 891882, I want to make sure this won't break the whole world. What do you think?

Comment 40

[Alexandre Poirot [:ochameau]]

Comment on attachment 8365704 [details] [diff] [review]
Rebased patch

Review of attachment 8365704 [details] [diff] [review]:
-----------------------------------------------------------------

I'm really sorry to get back so late on this patch.
I'm not such a expert of touch events and it took me a bit to get in touch with a real one!

Let's land the mouse events cancellation which looks good.

::: toolkit/devtools/touch-events.js
@@ +73,5 @@
>        }
>  
> +      // Suppress events which are impossible to generate by touch.
> +      if (['mouseenter', 'mouseover', 'mouseout', 'mouseleave'].indexOf(evt.type) != -1) {
> +        evt.stopPropagation();

As in other cancellations being done in this file, you need to do:
  evt.stopImmediatePropagation();
  evt.preventDefault();

@@ +108,5 @@
> +          if (!eventTarget) {
> +            // Suppress "mousemove" events while the simulated finger (mouse
> +            // button) is not held down, as these events cannot possibly occur
> +            // in a touch environment.
> +            evt.stopPropagation();

Same thing here.

@@ +160,5 @@
>              } catch(e) {
>                Cu.reportError('Exception in touch event helper: ' + e);
>              }
>              ignoreEvents = false;
> +         }, this.delayBeforeMouseEvent(evt), this);

Can you revert to a 0 timeout and remove delayBeforeMouseEvent from this patch?

That isn't that easy and I'd like you to land the mouse{enter,over,out,leave} cancellation right now and then figure out the delay issue with event experts (that I'm not!)

I discussed with Vivien, who is the very original author of this code and he told me that there isn't such 300ms delay. There is a one but way shorter than this, 10ms.

Comment 41

[Alexandre Poirot [:ochameau]]

Vivien, Can you give some more concrete detail(s) about touch event delay?
May be some platform code, some docs or anything more useful than the "someone told me" sentence I gave!

Comment 42

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

(In reply to Alexandre Poirot (:ochameau) from comment #41)
> Vivien, Can you give some more concrete detail(s) about touch event delay?
> May be some platform code, some docs or anything more useful than the
> "someone told me" sentence I gave!

Sorry I didn't followed all the discussion on this bug when speaking IRL with ochameau.

Basically when the first touch start hit the screen there is a latency introduce by: http://mxr.mozilla.org/mozilla-central/source/gfx/layers/ipc/GestureEventListener.cpp#27

This latency of 300 ms is used to see if a double tap is coming. As mentionned in #c0 this latency should not be here if the page is not zoomable.

I didn't look at the patc but I think the current approach is fine. Sorry any confusion.

Comment 43

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

No updates in many months, unassigning.

Comment 44

[Dan Huang[:danhuang]]

Attachment: 0001-Bug-920956-add-300ms-delay-between-touch-and-mouse-e.patch

Hi Ryan,

This is the patch to emulate 300 ms delay and suppress mouse events base on the above comments.
Could you help to give it a feedback?

Comment 45

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8741321 [details] [diff] [review]
0001-Bug-920956-add-300ms-delay-between-touch-and-mouse-e.patch

Review of attachment 8741321 [details] [diff] [review]:
-----------------------------------------------------------------

It would be great to test the delay somehow as well.

It seems like your commit message is not formatted quite right.  If you are using Git, you may want to try this workflow[1].

A few nits, but I think it's on the right track.

[1]: https://developer.mozilla.org/en-US/docs/Tools/Contributing#Creating_a_patch_to_check_in

::: devtools/shared/touch/simulator-content.js
@@ +146,5 @@
> +      case "mouseenter":
> +      case "mouseover":
> +      case "mouseout":
> +      case "mouseleave":
> +        // stop propagate events which are not related to touch events

Nit: "Don't propagate events which are not related to touch events"

@@ +170,4 @@
>  
>        case "mousemove":
>          if (!eventTarget) {
> +          // stop propagate mousemove event when touchstart event doesn't fired

Nit: "Don't propagate mousemove event when touchstart event isn't fired"

@@ +322,5 @@
>      return win;
> +  },
> +
> +  getDelayBeforeMouseEvent(evt) {
> +    // simulate 300ms delay between touch event and click event as

The comment from the previous patch seems more clear to me:

      // On mobile platforms, Firefox inserts a 300ms delay between
      // touch events and accompanying mouse events, except if the
      // content window is not zoomable.

You don't have to use that exactly, but the reason I prefer it is it says we are just recreating a behavior of mobile Firefox.  The current comment here makes it seem like the value was made up for unknown reasons.

Comment 46

[Patrick H. Lauke]

On the subject of the 300ms delay, note that the scenarios in which the delay is now suppressed have been expanded...not just when viewport isn't zoomable (e.g. user-scalable=no or minimum-scale and maximum-scale set to same), but also when it's a "mobile" viewport (width=device-width) and when touch-action:none or touch-action:manipulation act on the element being tapped - see http://patrickhlauke.github.io/touch/tests/results/#suppressing-300ms-delay

Comment 47

[Dan Huang[:danhuang]]

Thanks for your feedback!

I am tying to add test case for the 300ms delay. But the test result got failed.

I add meta tag with viewport and the following code in touch.html (/devtools/client/responsivedesign/test) to check the delay:

  var time = 0;
  
  div.addEventListener("touchend", function(evt) {
    if (!evt.touches.length) {
      div.style.transform = "none";
    }
    time = new Date().getTime();
    div.dataset.isDelay = "false";
  }, true);

  div.addEventListener("mousedown", function(evt) {
    if (time !== 0) {
      var now = new Date().getTime();
      div.dataset.isDelay = (now - time >= 300) ? "true" : "false";
    }
  }, true);


Add code to add_task() for opening inspector with responsive simulator in browser_responsiveui_touch.js:
  
  let {inspector, view} = yield openComputedView();
  yield selectNode("div", inspector);

And code to testWithTouch() for new test case in browser_responsiveui_touch.js:
  
  div.click();
  is(div.dataset.isDelay, "true", "300ms delay between touch events and mouse events should work");

I have dump some message in mochitest and it seems that the div.click() only fire click event but didn't fired the sequence of events: touchstart > touchend > mousedown > mousemove > mouseup > click.

I have no idea why div.click() wouldn't work correctly. 

Could you give some advice of writing this test case?

touch.html: https://goo.gl/VG3LJc
browser_responsiveui_touch.js: https://goo.gl/nuEaFz

Comment 48

[Dan Huang[:danhuang]]

(In reply to Patrick H. Lauke from comment #46)
> On the subject of the 300ms delay, note that the scenarios in which the
> delay is now suppressed have been expanded...not just when viewport isn't
> zoomable (e.g. user-scalable=no or minimum-scale and maximum-scale set to
> same), but also when it's a "mobile" viewport (width=device-width) and when
> touch-action:none or touch-action:manipulation act on the element being
> tapped - see
> http://patrickhlauke.github.io/touch/tests/results/#suppressing-300ms-delay

Thanks for you advice. I would add the conditions you mentioned to check the 300ms delay.

Comment 49

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

(In reply to Dan Huang[:danhuang] from comment #47)
> I have no idea why div.click() wouldn't work correctly. 
> 
> Could you give some advice of writing this test case?

I believe the issue is that the touch simulator ignores "synthesized" events[1], such as the ones coming from `div.click()` in your example.

This is why the other lines in the test use the privileged API `BrowserTestUtils.synthesizeMouse`: it allows forcing `isSynthesized: false` for testing purposes.

[1]: https://dxr.mozilla.org/mozilla-central/rev/ae7413abfa4d3954a6a4ce7c1613a7100f367f9a/devtools/shared/touch/simulator-content.js#134

Comment 50

[Dan Huang[:danhuang]]

Attachment: Bug920956.patch

Hi Ryan,

Thanks for your advice! 

I find that using BrowserTestUtils.synthesizeMouse() to send mouseup event after mousedown event would cause simulator-content.js receive a click event!
So I use these method to finish 300ms delay test case.

Could you help review this patch? 
Thanks

Comment 51

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8743732 [details] [diff] [review]
Bug920956.patch

Review of attachment 8743732 [details] [diff] [review]:
-----------------------------------------------------------------

::: devtools/client/responsivedesign/test/browser_responsiveui_touch.js
@@ +2,5 @@
>     http://creativecommons.org/publicdomain/zero/1.0/ */
>  
>  "use strict";
>  
> +XPCOMUtils.defineLazyModuleGetter(this, "Promise",

The head.js file[1] for this directory imports devtools shared-head.js[2] which does `let promise = require("promise")` to use the common promise library for DevTools code.

So, you should able to remove this and use lowercase `promise` instead.

[1]: https://dxr.mozilla.org/mozilla-central/source/devtools/client/responsivedesign/test/head.js
[2]: https://dxr.mozilla.org/mozilla-central/source/devtools/client/framework/test/shared-head.js#28

@@ +12,3 @@
>  
>  add_task(function*() {
> +  yield pushPrefs([domViewportEnabled, true]);

By setting this here in the test, doesn't it mean that we are only examining the viewport values during the test, and not during regular use of the touch simulator?

What's the behavior when this is not set?  I am not saying you need to change "dom.meta-viewport.enabled" in the real touch simulator necessarily (it would be more ideal to add a tab-specific API, and that can be done in some follow up bug).

Mainly I want to understand the following: On desktop where "dom.meta-viewport.enabled" is false, will we always get the 300ms delay, or are there cases where it still gets suppressed?

@@ +45,5 @@
> +  yield BrowserTestUtils.synthesizeMouse("div", x, y,
> +        { type: "mousemove", isSynthesized: false }, gBrowser.selectedBrowser);
> +  is(div.style.backgroundColor, "", "mouseout or mouseleave should work");
> +
> +  yield new Promise(r => {setTimeout(r, 500)});

Generally speaking, `setTimeout` is a bad idea in tests, since it can lead to intermittents when the tests are run on very slow machines in AWS, debug runs, etc.

Can we avoid the `setTimeout` by having the test watch for some event / message from the page?

::: devtools/client/responsivedesign/test/touch.html
@@ +16,5 @@
>  
>  <script>
>    var div = document.querySelector("div");
>    var initX, initY;
> +  var previousEvent = '', touchendTime = 0;

Nit: use "" to match quote style in the file

::: devtools/shared/touch/simulator-content.js
@@ +340,5 @@
> +        maxZoom = {},
> +        autoZoom = {};
> +
> +    utils.getViewportInfo(content.innerWidth, content.innerHeight, {},
> +                          allowZoom, minZoom, maxZoom, {}, {}, autoZoom);

Rename autoZoom to autoSize, which is what the `getViewportInfo` API calls it.

@@ +346,5 @@
> +    // FIXME: On Safari and Chrome mobile platform, if the css property
> +    // touch-action set to none or manipulation would also suppress 300ms
> +    // deley. But Firefox didn't support this property now, we can't get
> +    // this value from utils.getVisitedDependentComputedStyle() to check
> +    // if we should suppress 300ms deley.

Nit: delay

Comment 52

[Dan Huang[:danhuang]]

Thanks for your feedback!

> @@ +12,3 @@
> >  
> >  add_task(function*() {
> > +  yield pushPrefs([domViewportEnabled, true]);
> 
> By setting this here in the test, doesn't it mean that we are only examining
> the viewport values during the test, and not during regular use of the touch
> simulator?
> 
> What's the behavior when this is not set?  I am not saying you need to
> change "dom.meta-viewport.enabled" in the real touch simulator necessarily
> (it would be more ideal to add a tab-specific API, and that can be done in
> some follow up bug).
> 
> Mainly I want to understand the following: On desktop where
> "dom.meta-viewport.enabled" is false, will we always get the 300ms delay, or
> are there cases where it still gets suppressed?

It would always suppress the 300ms delay when "dom.meta-viewport.enabled" is false. Because the "allowZoom" value would always be false[1]. 
I would like to add more checks in the getDelayBeforeMouseEvent() to handle these scenario. But I'm not sure what is the correct behavior of 300ms delay when "dom.meta-viewport.enabled" is false. 
In my option, when "dom.meta-viewport.enabled" is false, it should always have 300ms delay except for touch-action:none and touch-action:manipulation.

Do you think this is the correct behavior?

And also I would add more test case for changing the value of dom.meta-viewport.enabled.

[1]https://dxr.mozilla.org/mozilla-central/source/dom/base/nsDocument.cpp#7932

> @@ +45,5 @@
> > +  yield BrowserTestUtils.synthesizeMouse("div", x, y,
> > +        { type: "mousemove", isSynthesized: false }, gBrowser.selectedBrowser);
> > +  is(div.style.backgroundColor, "", "mouseout or mouseleave should work");
> > +
> > +  yield new Promise(r => {setTimeout(r, 500)});
> 
> Generally speaking, `setTimeout` is a bad idea in tests, since it can lead
> to intermittents when the tests are run on very slow machines in AWS, debug
> runs, etc.
> 
> Can we avoid the `setTimeout` by having the test watch for some event /
> message from the page?
> 

I would update the test case without the 'setTimeout' !

Comment 53

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

(In reply to Dan Huang[:danhuang] from comment #52)
> Thanks for your feedback!
> 
> > @@ +12,3 @@
> > >  
> > >  add_task(function*() {
> > > +  yield pushPrefs([domViewportEnabled, true]);
> > 
> > By setting this here in the test, doesn't it mean that we are only examining
> > the viewport values during the test, and not during regular use of the touch
> > simulator?
> > 
> > What's the behavior when this is not set?  I am not saying you need to
> > change "dom.meta-viewport.enabled" in the real touch simulator necessarily
> > (it would be more ideal to add a tab-specific API, and that can be done in
> > some follow up bug).
> > 
> > Mainly I want to understand the following: On desktop where
> > "dom.meta-viewport.enabled" is false, will we always get the 300ms delay, or
> > are there cases where it still gets suppressed?
> 
> It would always suppress the 300ms delay when "dom.meta-viewport.enabled" is
> false. Because the "allowZoom" value would always be false[1]. 
> I would like to add more checks in the getDelayBeforeMouseEvent() to handle
> these scenario. But I'm not sure what is the correct behavior of 300ms delay
> when "dom.meta-viewport.enabled" is false. 
> In my option, when "dom.meta-viewport.enabled" is false, it should always
> have 300ms delay except for touch-action:none and touch-action:manipulation.
> 
> Do you think this is the correct behavior?

Yes, I think you are describing the behavior we want to have.  So to implement it correctly, it seems like getDelayBeforeMouseEvent should check if "dom.meta-viewport.enabled" is false, to detect that case.  If it is false, we should not call `getViewportInfo` since it has not read the viewport when the pref is false.  Instead 300 would always be used when the pref is false.  Please add comments to explain this additional logic.

Feel free to file a follow up bug to add a per-docshell API to control whether we read the viewport.  I think we **do** want to enable reading the viewport for the touch simulator, but we need more specific control over it per docshell (since a pref would enable it for all tabs, not just where the touch simulator is used) before we can turn it on for the touch simulator.

It seems fine to keep the existing logic you have that does check `getViewportInfo` though, since someone could choose to enable "dom.meta-viewport.enabled" manually if they wish to have more accurate touch simulation before we're able to add the per-docshell API for it.

Comment 54

[Dan Huang[:danhuang]]

Attachment: Bug920956_v2.patch

Hi Ryan,

Please help to feedback this patch. This patch check "dom.meta-viewport.enabled" in getDelayBeforeMouseEvent() and update test case for changing the value of "dom.meta-viewport.enabled".

Thanks!

Comment 55

[Dan Huang[:danhuang]]

> Feel free to file a follow up bug to add a per-docshell API to control
> whether we read the viewport.  I think we **do** want to enable reading the
> viewport for the touch simulator, but we need more specific control over it
> per docshell (since a pref would enable it for all tabs, not just where the
> touch simulator is used) before we can turn it on for the touch simulator.

The follow up bug: Bug 1267588

Comment 56

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8745277 [details] [diff] [review]
Bug920956_v2.patch

Review of attachment 8745277 [details] [diff] [review]:
-----------------------------------------------------------------

Great, thanks for working on this!  It looks good to me, feel free to land assuming try agrees.

If you don't have try access, set ni? to me and I can submit it for you.

Comment 57

[Dan Huang[:danhuang]]

Hi Ryan,

Thank for the review! 

I am dealing with try server these day, there had some test cases failed that I have no idea how to fix it. 

First, I run the mochitest test in my local OSX and Linux platform, all the test case passed.
(cmd: ./mach mochitest devtools/client/responsivedesign/test/browser_responsiceui_touch.js)

Second, I run the test on try server, and it got some failed cases[1].
So I add a new commit to initialize the test value, and push to try server again(for quick verify, I only run devtool related test case on linux platform). But these change still got intermittent error on try server[2] with e10s. 

Could you give some advice to fix these failed cases?

Thanks

[1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=04a870bcaaed
[2] https://treeherder.mozilla.org/#/jobs?repo=try&revision=4229b333331c

Comment 58

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Not sure what to suggest yet, I'll take another look tomorrow.

Comment 59

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8745277 [details] [diff] [review]
Bug920956_v2.patch

Review of attachment 8745277 [details] [diff] [review]:
-----------------------------------------------------------------

::: devtools/client/responsivedesign/test/touch.html
@@ +71,5 @@
> +    if (previousEvent === "touchend" && touchendTime !== 0) {
> +      let now = new Date().getTime();
> +      div.dataset.isDelay = ((now - touchendTime) >= 300) ? "true" : "false";
> +    } else {
> +      div.dataset.isDelay = false;

It would be better to use either strings or booleans for all paths (probably booleans?).

Comment 60

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

I tried to think about what race conditions there could be here, but I am not seeing anything yet...

Here's a wiki page[1] with lots of tips for trying to solve intermittents, especially to force them to appear locally.  I'd suggest trying at least `SimpleTest.testInChaosMode()`, which should randomize event ordering a bit more, possibly force it to occur.  Also the "--run-until-failure" option to run many times locally is a good idea as well.

I'll try to keep thinking about other tips.

[1]: https://wiki.mozilla.org/DevTools/Intermittents

Comment 61

[Dan Huang[:danhuang]]

Attachment: Error log on Linux

Comment 62

[Dan Huang[:danhuang]]

Thanks for providing these advices. It really helpful.
I had update the test case code, and there had one intermittent error(Lunux x64 opt dt6) left which related to browser_responsiveui_touch.js [1]

And I add some code in touch.html try to log the mouse event, but this time there is no intermittent error happened related to browser_responsiveui_touch.js [2]

Then I try to reproduce the intermittent error on Linux platform[3]. When using synthesizeMouseAtCenter() to move mouse into the div element, we expect it should trigger mouseenter/mouseover and mousemove event.  But it trigger unexpected mouseout/mouseleave event sometimes, and this cause the test failed. attachment 8750315 [details].

I still not figure out how this happen, do you have any idea about this?

[1] https://treeherder.mozilla.org/#/jobs?repo=try&revision=46ff31634236&selectedJob=20529302
[2] https://treeherder.mozilla.org/#/jobs?repo=try&revision=e6cec29ba365
[3] cmd: ./mach mochitest --run-until-failure --total-chunks 8 --this-chunk 6 devtools/

Comment 63

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

(In reply to Dan Huang[:danhuang] from comment #62)
> Thanks for providing these advices. It really helpful.
> I had update the test case code, and there had one intermittent error(Lunux
> x64 opt dt6) left which related to browser_responsiveui_touch.js [1]
> 
> And I add some code in touch.html try to log the mouse event, but this time
> there is no intermittent error happened related to
> browser_responsiveui_touch.js [2]
> 
> Then I try to reproduce the intermittent error on Linux platform[3]. When
> using synthesizeMouseAtCenter() to move mouse into the div element, we
> expect it should trigger mouseenter/mouseover and mousemove event.  But it
> trigger unexpected mouseout/mouseleave event sometimes, and this cause the
> test failed. attachment 8750315 [details].
> 
> I still not figure out how this happen, do you have any idea about this?

Hmm, that is suspicious!  I retriggered the intermittent on your try run a few times, and those all came back green.  I'd like to run it even more, but try is closed at the moment, so I don't think more retriggers will run.

I'll see if I can reproduce the Linux failure on my VM here.

Comment 64

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Okay, I am able to reproduce the issue when running:

mach mochitest devtools/client/responsivedesign --run-until-failure

which should at least finish more quickly.  It seems to fail about 50% of the time for me.

I added a "done" log to AsyncUtilsContent.js and it suggests the mouseout happens _after_ EventUtils.synthesizeMouseAtPoint completes, so some other part of the platform is firing these events.  Not sure why that is happening yet.

Comment 65

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

I tried to capture a trace of the problem with rr, but of course it works fine when I try that. :/

Comment 66

[Dan Huang[:danhuang]]

Attachment: patch for init test case parameter and wrap click event by waitForEvent

This patch is that mentioned in Comment 62, which update test case with initializing test parameter before test start and wrap synthesizeClick() by BrowserTestUtils.waitForEvent() to make sure click event fired.

Comment 67

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

After a lot of tears and printfs, I think I see the issue...

The layout system occasionally throws in its own synthesized mouse move events, and these are interfering with your test.

Luckily, there is an easy way to disable this behavior: set the pref "layout.reflow.synthMouseMove" to false for the entire test.  Since we don't care / want this platform behavior for this test, it seems fine to disable here.

Comment 68

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

I looked into the SynthMouseMove platform feature a bit more...  The control to disable it was added in bug 657844.

Looks like it's used for things like updating mouse state when the content underneath has changed.

Comment 69

[Dan Huang[:danhuang]]

Hi Ryan,

It's amazing! 
After setting the pref: "layout.reflow.synthMouseMove" to false, the try server result are all passed![1]

[1]https://treeherder.mozilla.org/#/jobs?repo=try&revision=7598463d5654

Comment 70

[Dan Huang[:danhuang]]

Attachment: Bug920956_v3.patch

This patch update test case as the following:
[1] set pref: layout.reflow.synthMouseMove to false in test 
[2] init test parameter before each test start
[3] wrap synthesizeClick() until click event finish
[4] add background-color change effect when mouseout/mouseleave happened

Comment 71

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8752651 [details] [diff] [review]
Bug920956_v3.patch

Your comment sounds great, I am excited to review it! :)

However, it seems like you attached an unrelated patch by mistake.

Comment 72

[Dan Huang[:danhuang]]

Attachment: Bug920956_v3.patch

Sorry for uploading the wrong patch.
Please help review this patch again.

Comment 73

[Dan Huang[:danhuang]]

Attachment: Bug920956_v3.patch

Comment 74

[J. Ryan Stinnett [:jryans] (Use needinfo, replies may be slow)]

Comment on attachment 8753161 [details] [diff] [review]
Bug920956_v3.patch

Review of attachment 8753161 [details] [diff] [review]:
-----------------------------------------------------------------

Great, this looks good to me!  Thanks for working on this. :) I know it got a bit complex with the test failures.

Please check out http://firefox-dev.tools for more bug ideas or feel free to ask me if there's an area you're interested in.

Comment 75

[Dan Huang[:danhuang]]

Thanks for your mentoring! I learned lots of thing from here. Your advices and tips always help get through the problems!
I will check the dev.tools bugs to see if anything I can help with!

Comment 76

[Ryan VanderMeulen [:RyanVM]]

Needs rebasing against fx-team tip.

Comment 77

[Dan Huang[:danhuang]]

Attachment: Bug920956_rebase.patch

Rebase fx-team

Comment 78

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/e12734113e68

Comment 79

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/e12734113e68