Skip to content

Conversation

jzaefferer
Copy link
Member

Resurrects a patch from Marius Stefan Bethge (@bethge), too many intermediate changes prevented even cherry-picks.

Replaces gh-1524
Fixes #4143

Successfully tested all interactions+slider demos on Chrome/OSX, Chrome/Android 5.x, Mobile Safari/iOS 9 Simulator, successfully tested interactions+slider default demos on BrowserStack on IE9/Win7, IE11/Win 8.1, Edge12/Win 10.

TODOs:

  • Do we bundle PEP in the standard build?
  • Deal with IE8, pep won't load here. Do we just not support it? Adding es5-shim helps, but then we run into other (unrelated) issues, apparently demos/bootstrap.js uses document.head, which IE8 doesn't support. Switching to document.body "works", but then it gets stuck somewhere else. Haven't yet been able to figure out where. We won't support IE8, IE9 and IE10 in 1.13
  • Windows Phone 8.1 on a BrowserStack Nokia Lumia 930 loads, but draggable doesn't work at all; slider is usable, but the handle doesn't move while dragging, only updates when stopping. I tried the pep/paint sample on the same "device" and it works, so PEP itself seems to be okay.
  • PhantomJS 1.x doesn't have Function#bind, which is annoying, but easy enough to work around. It also doesn't have MutationObservers, which is a bigger issue (PointerEventsPolyfill: MutationObservers not found, touch-action will not be dynamically detected). PhantomJS 2.x is still not usable; this seems to be the best issue to follow.
  • Running tests/unit/all.html on Chrome/OSX has failures on dialog and draggable, but both also happen on master. Datepicker and effects tests also had failures on master, but those were easy to fix. Fix for draggable is in Draggable: Fix options tests #1618, can't reproduce dialog issue anymore.
  • Interactions+slider pass on Chrome/OSX and Chrome/Android 5.x. On Mobile Safari/iOS Simulator, draggable has lots of failures, and gets stuck on "connectToSortable, dragging clone into sortable" test. Droppable has 2 failures. Sortable has lots of failures. Slider has some failures, one due to a missing pointermove event (slider's slide event isn't triggered by .simulate( "drag", { dx: 10, dy: 10 } ) call. Part of this is due to window.scrollTo behaving differently on iOS, see below.

@jzaefferer
Copy link
Member Author

@arschmitz could you take a look at my TODO list above? No need to dig into anything in detail, just some highlevel feedback on each item.

@jzaefferer
Copy link
Member Author

@bethge would you be interested in working with me on this? Getting draggable to work in Windows Phone and fixing any of the failing unit tests would be great.

@bethge
Copy link
Member

bethge commented Oct 6, 2015

Yes, sounds great! - I'll dive in.

@arschmitz
Copy link
Member

A few quick comments on the list above

Deal with IE8, pep won't load here. Do we just not support it? Adding es5-shim helps, but then we run into other (unrelated) issues, apparently demos/bootstrap.js uses document.head, which IE8 doesn't support. Switching to document.body "works", but then it gets stuck somewhere else. Haven't yet been able to figure out where.

We already chose not to support ie8 in 1.13

Windows Phone 8.1 on a BrowserStack Nokia Lumia 930 loads, but draggable doesn't work at all; slider is usable, but the handle doesn't move while dragging, only updates when stopping.

need to dig deeper but probably a touch-action issues

PhantomJS 1.x doesn't have Function#bind, which is annoying, but easy enough to work around. It also doesn't have MutationObservers, which is a bigger issue (PointerEventsPolyfill: MutationObservers not found, touch-action will not be dynamically detected). PhantomJS 2.x is still not usable; this seems to be the best issue to follow.

Function#bind is easy to work around most people just load a polyfill for it in tests only. Mutation observers is a different story. this effects versions of android and ie9 as well. There is an in the works function that will make this work a bit better. we are adding a function that will update the current touch-action map so that if you have dynamic content you can call this function on updates

@jzaefferer
Copy link
Member Author

We already chose not to support ie8 in 1.13

That makes it a lot easier.

need to dig deeper but probably a touch-action issues

Might be of help to @bethge.

There is an in the works function that will make this work a bit better

That sounds promising. Will see if thats available before Phantom 2.x.

@bethge
Copy link
Member

bethge commented Oct 12, 2015

On Mobile Safari/iOS Simulator, draggable has lots of failures

I went for draggable: core: element types and plastered it with timeouts, to go nice and slow through each drag. With this, the 72 asserts pass.

Just to get it right: We have MouseEvents simulated by jquery.simulate.js, converted to PointerEvents by pep.js, which trigger draggable.js while in a browser using TouchEvents.

I will try to figure out why those drags on iOS won't run synchronously and without timeouts.

@jzaefferer
Copy link
Member Author

Just to get it right: We have MouseEvents simulated by jquery.simulate.js, converted to PointerEvents by pep.js, which trigger draggable.js while in a browser using TouchEvents.

As far as I can tell, we can't trigger PointerEvents unless the browser supports them natively, so it seems like triggering MouseEvents is the only choice we have, even with pep.js in place.

I will try to figure out why those drags on iOS won't run synchronously and without timeouts.

Thanks, let me know what you find.

@jzaefferer jzaefferer changed the title WIP: Mouse: Switch to pointer events, via PEP [WIP] Mouse: Switch to pointer events, via PEP Oct 17, 2015
Resurrects a patch from Marius Stefan Bethge (@bethge), too many
intermediate changes prevented even cherry-picks.

Replaces gh-1524
Fixes #4143
@bethge
Copy link
Member

bethge commented Oct 19, 2015

I have been able to make some progress on the issue:

pageX/Y and clientX/Y are diverging during a drag on iOS. On Chrome, Chrome - Mobile Dev Mode and OS X Safari they never differ [in the draggable test suite].

jquery.simulate.js uses clientX/Y for simulating events, but draggable.js::_generatePosition uses pageX/Y. So while the distance form pointer-down to pointer-up is correct in clientX/Y-space, it is off in pageX/Y-space. draggable.js uses the latter to position the element with css properties left and top.

Now I feel like the whole pageX/Y vs. clientX/Y is another pandora's box.

On quirksmode.org I found:

On iPhone, Firefox, and BlackBerry clientX/Y is equal to pageX/Y

On developer.apple.com it says:

For the purposes of Safari and WebKit, clientX and clientY are document relative, as are pageX and pageY (which are thus always equal to clientX and clientY).

For Safari and iPhone I get quite different values for page and client, as soon as I am not at the top of the page (just as shown here: stackoverflow.com). I feel like I am missing something.

I am not quite sure in which direction to head further. Using clientX/Y in draggable.js seems like more than just a replaceAll. MouseEvents are created with clientX/Yas per spec and pageX/Y is not officially standardised but everyone uses it, iiuc.

@arschmitz
Copy link
Member

This may be helpful for sorting this out its from the mobile touch events. https://github.com/jquery/jquery-mobile/blob/master/js/events/touch.js#L117-L143

@mikesherov
Copy link
Member

Ideally, we'd rewrite draggable before jamming PEP in there. I can't start to look at this soon.

@jzaefferer
Copy link
Member Author

There is no jamming going on here, there aren't even any draggable changes necessary to make it work with pointer events. I'd much rather see a release with PEP support a few months after 1.12 than a draggable rewrite in another year...

@jzaefferer
Copy link
Member Author

If we continue to ignore testing on mobile devices, we could just land this asap (after 1.12 is done), since it passes our current browser support and works much much better on various mobile devices.

@scottgonzalez
Copy link
Member

We've had a plan to add support for pointer events separately from the interaction rewrites for quite a while now. I'm not sure why the two would need to be intertwined.

@bethge
Copy link
Member

bethge commented Dec 3, 2015

Getting back on:

I will try to figure out why those drags on iOS won't run synchronously and without timeouts.

I noticed that iOS Safari's window.scrollTo() behaves differently than that of Chrome, FF, and OS X Safari:

window.scrollTo(0, -100);
window.pageYOffset;                       // iOS Safari: -100  -  Others: 0
setTimeout(() => window.pageYOffset, 1);  // iOS Safari: 0     -  Others: 0

For draggable: core: element types, the elements are in far outer negative space. When simulating a drag on them, each mousemove triggers a scroll https://github.com/jquery/jquery-ui/blob/master/ui/widgets/draggable.js#L966 .
Since the scrolls are towards nothingness the other browsers disregard it, no scrolling occurs and the element gets dragged 50px. Safari iOS actually scrolls the predefined 20px for each mousemove, for the duration of the synchronous execution. which in turn changes the client/page-locations.

@jzaefferer
Copy link
Member Author

Great work digging into that! Reading about scrollTo on iOS reminded me of running into that same issue before: https://github.com/jzaefferer/pitfalls-examples/blob/d95a75d63c008eb6cb1601ced27a1b3f0df5a443/app/gallery/gallery.js#L9-L11

Adding timeouts everywhere is hardly a welcome solution (and based on your description, I'm not sure if that's even correct). I wonder if we should instead adjust the tests to drag the element inside the viewport, or at least in a non-negative location (would that help?).

@mikesherov any thoughts on this?

@bethge
Copy link
Member

bethge commented Dec 25, 2015

Apologies for the delay.

I agree, timeouts should be avoided. I hoped that each asynchronous function call would reset the page offset, but that doesn't seem to be the case (see example). Only with higher timeouts does the page offset go back to 0. :

[1,2,3,4,5].forEach(() => 
  setTimeout(() => {
    window.scrollTo(0, window.pageYOffset - 10);
    console.log(window.pageYOffset);                // -10, -20, -30, -40, -50
  }, 1)
);

Iiuc: As soon as one scroll into negative space takes place, the testing environment is off, causing issues for subsequent tests in the suite.

Relocating all drags into view would, imho, solve the issue. A number of elements would need to be repositioned such that they do not cause unwanted scrolling during drags (this time in all browsers).

Are there specifications on minimum height and width of the viewport, that the tests should run in? qunit-fixture currently measures a whooping 6000px in height. Relocating it to top: 100px; left: 100px also puts it on top of the interface of the Test Suite Runner. I will see if I can reduce those dimensions and rather dynamically add and remove the elements in the fixture.

@jzaefferer
Copy link
Member Author

Feel free to try any values. As long as the fixture isn't visible at the end of the run, it doesn't matter much. Better to have reliable and correct tests with some visual clutter while running the tests.

I'm not sure what the iframe size for TestSwarm is, nor what the default dimensions of the browserstack-runner viewport are. I guess 800x600 is pretty safe.

@bethge
Copy link
Member

bethge commented Jan 2, 2016

Windows Phone 8.1 on a BrowserStack Nokia Lumia 930 loads, but draggable doesn't work at all; slider is usable, but the handle doesn't move while dragging, only updates when stopping. I tried the pep/paint sample on the same "device" and it works, so PEP itself seems to be okay.

Got by chance my hands on a Nokia Lumia 950 with Windows 10 Mobile: All draggable demos work well.

How do we unit testing browsers with native pointer events support, like IE11, Edge and FF (with flag)? Since PEP is noop'ing out, and we only simulate mouse events, not much is happening.
We could directly simulate pointer events for these browser in one way or another. Or inject PEP particularly to convert simulated mouse events to pointer events.

@jzaefferer
Copy link
Member Author

We could directly simulate pointer events for these browser in one way or another. Or inject PEP particularly to convert simulated mouse events to pointer events.

Neither sounds particularly attractive to me, but I don't have a better idea either.

@scottgonzalez @mikesherov thoughts, ideas?

@scottgonzalez
Copy link
Member

I'd say there are really only two viable options:

  • Rewrite tests to use WebDriver and simulate actual drags.
  • Update jquery-simulate to detect which event types are supported and trigger either Pointer, Touch, or Mouse appropriately.

Both are a fair amount of work. Updating jquery-simulate has a smaller delta though.

@arschmitz
Copy link
Member

I think using web driver is the right direction moving forward however it is probably more work.

@mgol mgol deleted the branch jquery:master February 19, 2021 19:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

8 participants