MouseTracker Overhaul 2020 #1872
Conversation
MouseTracker Overhaul 2020 Preview, Beta Testers Needed!As I clean up a punch list of small issues I found along the way, if anyone would like to test it out it would help immensely! Links below... Demo SitesOpenSeadragon Imaging ImagingHelper Demo Preview Buildsopenseadragon.js Source Code (OpenSeadragon Repository Branch) |
|
I just tested in my web application (which uses frames), and the rendering is perfect. It works much better than before. Thanks a lot for the patch! |
|
Support on Firefox is a bit better than on Webkit. In Chrome, the zoom movement stops when the mouse is outside the frame area. |
Chrome running on what platform? Edit: Also, does it recover - in other words, can you still do stuff in the frame if you release outside of the frame or does it lock up? Thank you for testing!! Really appreciate the feedback! |
How so? I started the process of submitting a Webkit bug two days ago, wondering if it's related. It's possible to work around, and I tried to get at least the built-in OpenSeadragon event handling to work around it, but I only have so many devices/browsers available to test on (which makes feedback like yours invaluable!) |
|
I am using Windows 10. Note I use frames (not an iframe). I want to stop using frames, but it's a fairly large application, requiring a lot of code changes. In your demo, the bug does not exist, the movement of the zoom works well. |
Absolutely. In Chrome, if I release the mouse button outside the frame area, it locks on the zoom. I must then close the zoom and reopen it (sorry for my poor English :-) |
Just to clarify...this is Chrome on Windows 10, correct? And the new patched code stops zooming when button released outside the frame the button press occurred in? |
# Conflicts: # changelog.txt
changelog.txt
Outdated
| @@ -7,6 +7,26 @@ OPENSEADRAGON CHANGELOG | |||
| * Documentation fix (#1814 @kenanchristian) | |||
| * Better cleanup on destruction, to avoid memory leaks (#1832 @JoFrMueller) | |||
| * Miscellaneous code cleanup (#1840 @msalsbery) | |||
| * Improved browser sniffing - detect EDGE and CHROMEEDGE browsers | |||
There was a problem hiding this comment.
We are actually putting the pull request number, not the issue number, on items in the change log, and including the username of the patch author. I know it may feel a little silly, but I think it's worth listing all of these changeling items separately like you're doing, but including the same PR number and author at the end of each one.
There was a problem hiding this comment.
Feel free to officially bump up the in progress number at the top to 2.5.0, BTW :-)
There was a problem hiding this comment.
We are actually putting the pull request number, not the issue number, on items in the change log, and including the username of the patch author. I know it may feel a little silly, but I think it's worth listing all of these changeling items separately like you're doing, but including the same PR number and author at the end of each one.
Changelog is next on my punch list since you mentioned the format in the last PR that landed ;) ...fixing
changelog.txt
Outdated
| * MouseTracker: added overHandler/outHandler options for handling corresponding pointerover/pointerout events | ||
| * MouseTracker: changed enterHandler/leaveHandler to use DOM pointerenter/pointerleave events instead of simulating using pointerover/pointerout | ||
| * All internal uses of MouseTracker use pointerenter/pointerleave events instead of pointerover/pointerout events for more consistent pointer tracking | ||
| * DEPRECATION: MouseTracker exitHandler deprecated for name change to leaveHandler for consistency with DOM event names |
There was a problem hiding this comment.
We should put deprecations and breaking changes at the top of the change log so they are easy to spot.
There was a problem hiding this comment.
Looking through the code it appears that the exitHandler is entirely removed, not just deprecated. I would call that a breaking change. We should be able to just deprecate it, though… Continue to provide the hook for it, but don't document it (or document it as deprecated). If the user attaches to it, put a warning in the console (but allow it to continue to work).
There was a problem hiding this comment.
Looking through the code it appears that the exitHandler is entirely removed, not just deprecated. I would call that a breaking change. We should be able to just deprecate it, though… Continue to provide the hook for it, but don't document it (or document it as deprecated). If the user attaches to it, put a warning in the console (but allow it to continue to work).
It's still there, documented as deprecated, and called if a handler provided.
(added a locator comment in the mousetracker.js diff)
|
|
||
| // Leave (doesn't bubble and not cancelable) | ||
| // Note: exitHandler is deprecated, replaced by leaveHandler | ||
| if ( tracker.leaveHandler || tracker.exitHandler ) { |
There was a problem hiding this comment.
@iangilman Here's where the deprecated exitHandler is handled. Pondering if I should use an else-if so only one handler is called...and maybe emit a warning to the console if it's exitHandler?
There was a problem hiding this comment.
Thank you for pointing it out here! I think it's probably fine if both handlers are called (typically if you have an exit handler you wouldn't have a leave handler anyway). I think it would be good to console.warn if it's the exit handler, though.
|
@iangilman Update: Ready for review! I removed a lot of redundant code, as well as old IE version support code, from MouseTracker. The added lines number should be much more reasonable now ;) |
There was a problem hiding this comment.
@msalsbery exciting!
Thank you for the updated CodePen… Exciting to see us now able to pass the event through directly! It might be nice to add some sugar for this scenario at some point, but just being able to do it at all is a big step :-)
Man, this PR is a behemoth! I looked it over, and it seems reasonable from what I can tell. I think at this point we should merge it and have folks test! What do you think?
That would be great! |
|
@iangilman On #1863 we agreed to get that one in the 2.5.0 milestone as well. Not necessary, as the majority of the old IE support was probably in MouseTracker and is now gone, but I can make a pass on the rest of the code to finish #1863 before we release a 2.5.0... Would be easier to do if this one gets merged first ;-) |
* master: Changleg for #1878 Separate properties for buttonGroup and customButtons Better handle destruction when navigator in custom location # Conflicts: # src/viewer.js
|
Awesome… Merging! @msalsbery, thank you for taking on this Herculean task! As for finishing up #1863, I'd certainly be happy for it if you wanted to. I figure we should give this patch some testing time on master before we release it, so there is certainly time for other stuff in the meantime. |
|
@msalsbery, would you mind going through and closing all of the issues fixed by this patch? :-) |
Yay thanks! For #1863 there's probably not a whole lot to do...a lot of the legacy support was in MouseTracker |
Will do thanks |
|
@msalsbery excellent, thank you :-) Now that this patch has landed, our master build is failing some of its unit tests: https://travis-ci.org/github/openseadragon/openseadragon/builds/761987114 Seems odd, since this branch on its own passed the tests just fine. I've already restarted the build once, thinking it might be an intermittent error, but failed the second time too. Can you take a look? I can try restarting the build again if you'd like… |
Ugh! While removing old IE code I found the legacy mouse shim used by the tests needed updating so I may have just fixed it already. Is weird though, as I did run tests...just not after last few commits darnit. Looking into it... Edit: See PR #1949 |
MouseTracker Overhaul 2020
Finally finishing up this major MouseTracker overhaul! This should fix many of the mouse/touch/pen device issues we've seen over the past several years! Also added improved capabilities for controlling event traversal through the DOM tree, and support for using OpenSeadragon viewers in iframe and shadow DOM should be much improved!
Main Goals
Change Log