fix: don't clear canvas on mobile browsers' height change #66
Conversation
agilgur5
changed the title
There was a problem hiding this comment.
Thanks for looking into this issue and submitting this PR!
I haven't looked at this in detail yet, but signature_pad recommends listening to orientation change instead on mobile, which might be more optimal and resilient than ignoring height changes.
There is also szimek/signature_pad#446 that implements a different workaround to the resize handling here and may make the resize handling and all its various workarounds unnecessary.
| this._handleResize(() => { | ||
| if (!this.props.clearOnResize) { | ||
| return | ||
| } | ||
| this._resizeCanvas() | ||
| }) |
There was a problem hiding this comment.
These methods can get called very frequently during each frame of a resize (esp. as they're not throttled), so instead of creating anonymous closures each time, it's better to have another helper method for this. That optimization is also why the rest of the code is written as such
| @@ -3,6 +3,9 @@ import React, { Component } from 'react' | |||
| import SignaturePad from 'signature_pad' | |||
| import trimCanvas from 'trim-canvas' | |||
|
|
|||
| const userAgent = window.navigator.userAgent | |||
| const isTouchDevice = userAgent.indexOf("iPhone") >= 0 || userAgent.indexOf("iPad") >= 0 || userAgent.indexOf("Android") >= 0 | |||
There was a problem hiding this comment.
I don't think this is fully inclusive of all mobile browsers. This SO answer lists quite a bit more.
isMobile is also probably better naming as laptops can have touchscreens too.
But while I like the DX of auto-detection, I'm not sure this is foolproof enough to work consistently. A prop isMobile might make more sense... but the developer might use the same method to set that prop 😅
There was a problem hiding this comment.
detect-it can also be used to check device support for touch screen and if the touch screen is the primary input method (which should filter out laptop touchscreens I think? though I'm not sure the compatibility of both checks).
There are still some edge cases not covered there, for instance, multi-tasking mode on mobile where you split the screen... 😕
Adding a clearOnOrientationChange prop may make things more clear for developers and be fully backward-compatible as well. Then they can decide whether to use that for their users, e.g. clearOnResize={false} clearOnOrientationChange, and each developer should know their users best.
That's probably the easiest solution, though I do prefer the DX of auto-detection
|
Ok so I was thinking changing the According to caniuse though, |
|
This would also need some tests before it could be merged |
ScreenOrientation seems to be supported by all browsers (except IE) as of March 2023. 🎉 |
That's pretty recent though, so we'd still need a fallback for unsupported browsers 😕 |
The resize events kept being triggered when user scrolled/touched on the mobile browser like Safari on iOS 15,
as the window height changed a little bit if a user scrolled/touched, particularly on iOS Safari the navigation bar will keep expand/collapse as user scrolls.
Fixes #65
Related links for resize problem:
https://titanwolf.org/Network/Articles/Article?AID=488d4b0c-53df-4102-99af-6983a915ea8d
https://stackoverflow.com/questions/8898412/iphone-ipad-triggering-unexpected-resize-events