Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix touch event issue #594

Closed
wants to merge 7 commits into from
Closed

Fix touch event issue #594

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
212d142
fix: Add touch event listeners directly to node
jaehwan-moon Mar 5, 2019
34a3fc8
test: Add a mock ref to tests
jaehwan-moon Mar 5, 2019
1354a63
test: Update snapshots
jaehwan-moon Mar 5, 2019
9785fe7
chore: Add comments for reason
jaehwan-moon Mar 5, 2019
d1afdd5
fix: Remove focus from active element when sliders are chosen
jaehwan-moon Mar 5, 2019
f5cf913
Revert "fix: Remove focus from active element when sliders are chosen"
jaehwan-moon Mar 5, 2019
c5358d0
fix: Remove touch event listeners when unmounted
jaehwan-moon Mar 7, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 0 additions & 4 deletions src/components/alpha/__snapshots__/spec.js.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -92,8 +92,6 @@ exports[`Alpha renders correctly 1`] = `
/>
<div
onMouseDown={[Function]}
onTouchMove={[Function]}
onTouchStart={[Function]}
style={
Object {
"height": "100%",
Expand Down Expand Up @@ -232,8 +230,6 @@ exports[`Alpha renders vertically 1`] = `
/>
<div
onMouseDown={[Function]}
onTouchMove={[Function]}
onTouchStart={[Function]}
style={
Object {
"height": "100%",
Expand Down
3 changes: 3 additions & 0 deletions src/components/alpha/spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@ import React from 'react'
import renderer from 'react-test-renderer'
import { mount } from 'enzyme'
import color, { red } from '../../helpers/color'
import { createNodeMock } from '../../../support/createNodeMock'
// import canvas from 'canvas'

import Alpha from './Alpha'
Expand All @@ -13,6 +14,7 @@ import AlphaPointer from './AlphaPointer'
test('Alpha renders correctly', () => {
const tree = renderer.create(
<Alpha { ...red } />,
{ createNodeMock },
).toJSON()
expect(tree).toMatchSnapshot()
})
Expand Down Expand Up @@ -43,6 +45,7 @@ test('Alpha onChange events correctly', () => {
test('Alpha renders vertically', () => {
const tree = renderer.create(
<Alpha { ...red } width={ 20 } height={ 200 } direction="vertical" />,
{ createNodeMock },
).toJSON()
expect(tree).toMatchSnapshot()
})
Expand Down
6 changes: 0 additions & 6 deletions src/components/chrome/__snapshots__/spec.js.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39,8 +39,6 @@ exports[`Chrome renders correctly 1`] = `
>
<div
onMouseDown={[Function]}
onTouchMove={[Function]}
onTouchStart={[Function]}
style={
Object {
"MozBorderRadius": undefined,
Expand Down Expand Up @@ -274,8 +272,6 @@ exports[`Chrome renders correctly 1`] = `
<div
className="hue-horizontal"
onMouseDown={[Function]}
onTouchMove={[Function]}
onTouchStart={[Function]}
style={
Object {
"MozBorderRadius": undefined,
Expand Down Expand Up @@ -431,8 +427,6 @@ exports[`Chrome renders correctly 1`] = `
/>
<div
onMouseDown={[Function]}
onTouchMove={[Function]}
onTouchStart={[Function]}
style={
Object {
"height": "100%",
Expand Down
5 changes: 4 additions & 1 deletion src/components/chrome/spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,17 +4,18 @@ import React from 'react'
import renderer from 'react-test-renderer'
import color, { red } from '../../helpers/color'
import { mount } from 'enzyme'
import { createNodeMock } from '../../../support/createNodeMock'

import Chrome from './Chrome'
import ChromeFields from './ChromeFields'
import ChromePointer from './ChromePointer'
import ChromePointerCircle from './ChromePointerCircle'
import { Alpha } from '../common'


test('Chrome renders correctly', () => {
const tree = renderer.create(
<Chrome { ...red } />,
{ createNodeMock },
).toJSON()
expect(tree).toMatchSnapshot()
})
Expand Down Expand Up @@ -72,13 +73,15 @@ test('ChromePointerCircle renders correctly', () => {
test('Chrome renders custom styles correctly', () => {
const tree = renderer.create(
<Chrome styles={{ default: { picker: { boxShadow: 'none' } } }} />,
{ createNodeMock },
).toJSON()
expect(tree.props.style.boxShadow).toBe('none')
})

test('Chrome renders correctly with width', () => {
const tree = renderer.create(
<Chrome width={300} />,
{ createNodeMock },
).toJSON()
expect(tree.props.style.width).toBe(300)
});
16 changes: 14 additions & 2 deletions src/components/common/Alpha.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,10 +5,24 @@ import * as alpha from '../../helpers/alpha'
import Checkboard from './Checkboard'

export class Alpha extends (PureComponent || Component) {
constructor(props) {
super(props)

this.container = null
}

componentWillUnmount() {
this.unbindEventListeners()
}

componentDidMount() {
// e.preventDefault() does not work properly on React synthetic touch event.
// As a workaround, touch events have to be added to directly to node
// https://github.com/facebook/react/issues/9809#issuecomment-414072263
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

follow up here, the comment you're referencing doesn't actually show the solution.

You need to update your use to one of these:

Note if you use the { passive: false } option route, you should probably do the check to see if it is supported too.

Copy link
Contributor Author

@jaehwan-moon jaehwan-moon Mar 14, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@hartzis I have used the second option. I think the diffrence is that el.addEventListener('touchstart') is used instead of el.ontouchstart. Aren't those same? Please correct me if I'm wrong.

Thanks for following up!

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

They are different. Pretty helpful explanation: https://stackoverflow.com/a/35093997

In order to gain the ability to call preventDefault you will need to add also add the { passive: false } option for chrome.

this.container.addEventListener('touchstart', this.handleChange, {passive: false})

or do what dan recomended

this.container.ontouchstart = this.handleChange

Let me know if this helps, i just stumbled across this PR since it referenced a facebook issue i was looking at and i'm dealing with a lot of the same issues as the maintainer for https://github.com/dogfessional/react-swipeable

Copy link
Contributor Author

@jaehwan-moon jaehwan-moon Mar 15, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@hartzis Thanks for the information. After looking into this issue more, I found that ontouchstart is currently an experimental feature. https://developer.mozilla.org/en-US/docs/Web/API/GlobalEventHandlers/ontouchstart. Also in Chrome, the flag touch-event needs to be enabled to use ontouchstart API.
I'm not sure why dan used ontouchstart...

Regarding passive option, I think it does not need to be set to false unless the event handler is attached to document level e.g. window, document or body. Document level touch event handlers are treated as passive

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@cokencode That is an incredibly helpful link, thank you for sharing. I either totally missed or forgot the window, document or body part of that change. You rock!

this.container.addEventListener('touchstart', this.handleChange)
this.container.addEventListener('touchmove', this.handleChange)
Copy link

@fsubal fsubal Mar 7, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems they should be removeEventListener-ed inside componentWillUnmount or unbindEventListeners.

Copy link
Contributor Author

@jaehwan-moon jaehwan-moon Mar 7, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@fsubal It's done. Thanks for reviewing 😄

Aren't they GCed when unmounted as they are attached to the node in the component? But I guess it is safe to remove them explicitly in componentWillUnmount.

}

handleChange = (e) => {
const change = alpha.calculateChange(e, this.props.hsl, this.props.direction, this.props.a, this.container)
change && typeof this.props.onChange === 'function' && this.props.onChange(change, e)
Expand Down Expand Up @@ -96,8 +110,6 @@ export class Alpha extends (PureComponent || Component) {
style={ styles.container }
ref={ container => this.container = container }
onMouseDown={ this.handleMouseDown }
onTouchMove={ this.handleChange }
onTouchStart={ this.handleChange }
>
<div style={ styles.pointer }>
{ this.props.pointer ? (
Expand Down
16 changes: 14 additions & 2 deletions src/components/common/Hue.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,20 @@ import reactCSS from 'reactcss'
import * as hue from '../../helpers/hue'

export class Hue extends (PureComponent || Component) {
constructor(props) {
super(props)

this.container = null
}

componentDidMount() {
// e.preventDefault() does not work properly on React synthetic touch event.
// As a workaround, touch events have to be added to directly to node
// https://github.com/facebook/react/issues/9809#issuecomment-414072263
this.container.addEventListener('touchstart', this.handleChange)
this.container.addEventListener('touchmove', this.handleChange)
}

componentWillUnmount() {
this.unbindEventListeners()
}
Expand Down Expand Up @@ -72,8 +86,6 @@ export class Hue extends (PureComponent || Component) {
style={ styles.container }
ref={ container => this.container = container }
onMouseDown={ this.handleMouseDown }
onTouchMove={ this.handleChange }
onTouchStart={ this.handleChange }
>
<style>{ `
.hue-horizontal {
Expand Down
14 changes: 11 additions & 3 deletions src/components/common/Saturation.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,11 +7,21 @@ export class Saturation extends (PureComponent || Component) {
constructor(props) {
super(props)

this.container = null

this.throttle = throttle((fn, data, e) => {
fn(data, e)
}, 50)
}

componentDidMount() {
// e.preventDefault() does not work properly on React synthetic touch event.
// As a workaround, touch events have to be added to directly to node
// https://github.com/facebook/react/issues/9809#issuecomment-414072263
this.container.addEventListener('touchstart', this.handleChange)
this.container.addEventListener('touchmove', this.handleChange)
}

componentWillUnmount() {
this.throttle.cancel()
this.unbindEventListeners()
Expand Down Expand Up @@ -87,9 +97,7 @@ export class Saturation extends (PureComponent || Component) {
<div
style={ styles.color }
ref={ container => this.container = container }
onMouseDown={ this.handleMouseDown }
onTouchMove={ this.handleChange }
onTouchStart={ this.handleChange }
onMouseDown={ this.handleMouseDown }
>
<style>{`
.saturation-white {
Expand Down
6 changes: 0 additions & 6 deletions src/components/common/__snapshots__/spec.js.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,8 +82,6 @@ exports[`Alpha renders correctly 1`] = `
/>
<div
onMouseDown={[Function]}
onTouchMove={[Function]}
onTouchStart={[Function]}
style={
Object {
"height": "100%",
Expand Down Expand Up @@ -206,8 +204,6 @@ exports[`Hue renders correctly 1`] = `
<div
className="hue-horizontal"
onMouseDown={[Function]}
onTouchMove={[Function]}
onTouchStart={[Function]}
style={
Object {
"MozBorderRadius": undefined,
Expand Down Expand Up @@ -279,8 +275,6 @@ exports[`Hue renders correctly 1`] = `
exports[`Saturation renders correctly 1`] = `
<div
onMouseDown={[Function]}
onTouchMove={[Function]}
onTouchStart={[Function]}
style={
Object {
"MozBorderRadius": undefined,
Expand Down
4 changes: 4 additions & 0 deletions src/components/common/spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
import React from 'react'
import renderer from 'react-test-renderer'
import { red } from '../../helpers/color'
import { createNodeMock } from '../../../support/createNodeMock'
// import canvas from 'canvas'

import Alpha from './Alpha'
Expand All @@ -15,6 +16,7 @@ import Swatch from './Swatch'
test('Alpha renders correctly', () => {
const tree = renderer.create(
<Alpha { ...red } />,
{ createNodeMock },
).toJSON()
expect(tree).toMatchSnapshot()
})
Expand Down Expand Up @@ -50,13 +52,15 @@ test('EditableInput renders correctly', () => {
test('Hue renders correctly', () => {
const tree = renderer.create(
<Hue { ...red } />,
{ createNodeMock },
).toJSON()
expect(tree).toMatchSnapshot()
})

test('Saturation renders correctly', () => {
const tree = renderer.create(
<Saturation { ...red } />,
{ createNodeMock },
).toJSON()
expect(tree).toMatchSnapshot()
})
Expand Down
4 changes: 0 additions & 4 deletions src/components/hue/__snapshots__/spec.js.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,8 +35,6 @@ exports[`Hue renders correctly 1`] = `
<div
className="hue-horizontal"
onMouseDown={[Function]}
onTouchMove={[Function]}
onTouchStart={[Function]}
style={
Object {
"MozBorderRadius": "2px",
Expand Down Expand Up @@ -140,8 +138,6 @@ exports[`Hue renders vertically 1`] = `
<div
className="hue-vertical"
onMouseDown={[Function]}
onTouchMove={[Function]}
onTouchStart={[Function]}
style={
Object {
"MozBorderRadius": "2px",
Expand Down
4 changes: 4 additions & 0 deletions src/components/hue/spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,27 +3,31 @@
import React from 'react'
import renderer from 'react-test-renderer'
import { red } from '../../helpers/color'
import { createNodeMock } from '../../../support/createNodeMock'

import Hue from './Hue'
import HuePointer from './HuePointer'

test('Hue renders correctly', () => {
const tree = renderer.create(
<Hue { ...red } />,
{ createNodeMock },
).toJSON()
expect(tree).toMatchSnapshot()
})

test('Hue renders vertically', () => {
const tree = renderer.create(
<Hue { ...red } width={ 20 } height={ 200 } direction="vertical" />,
{ createNodeMock },
).toJSON()
expect(tree).toMatchSnapshot()
})

test('Hue renders custom styles correctly', () => {
const tree = renderer.create(
<Hue { ...red } styles={{ default: { picker: { boxShadow: '0 0 10px red' } } }} />,
{ createNodeMock },
).toJSON()
expect(tree.props.style.boxShadow).toBe('0 0 10px red')
})
Expand Down
4 changes: 0 additions & 4 deletions src/components/photoshop/__snapshots__/spec.js.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -69,8 +69,6 @@ exports[`Photoshop renders correctly 1`] = `
>
<div
onMouseDown={[Function]}
onTouchMove={[Function]}
onTouchStart={[Function]}
style={
Object {
"MozBorderRadius": undefined,
Expand Down Expand Up @@ -211,8 +209,6 @@ exports[`Photoshop renders correctly 1`] = `
<div
className="hue-vertical"
onMouseDown={[Function]}
onTouchMove={[Function]}
onTouchStart={[Function]}
style={
Object {
"MozBorderRadius": undefined,
Expand Down
3 changes: 3 additions & 0 deletions src/components/photoshop/spec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
import React from 'react'
import renderer from 'react-test-renderer'
import { red } from '../../helpers/color'
import { createNodeMock } from '../../../support/createNodeMock'

import Photoshop from './Photoshop'
import PhotoshopButton from './PhotoshopButton'
Expand All @@ -14,13 +15,15 @@ import PhotoshopPreviews from './PhotoshopPreviews'
test('Photoshop renders correctly', () => {
const tree = renderer.create(
<Photoshop { ...red } onAccept={ () => {} } onCancel={ () => {} } />,
{ createNodeMock },
).toJSON()
expect(tree).toMatchSnapshot()
})

test('Photoshop renders custom styles correctly', () => {
const tree = renderer.create(
<Photoshop { ...red } styles={{ default: { picker: { boxShadow: '0 0 10px red' } } }} />,
{ createNodeMock },
).toJSON()
expect(tree.props.style.boxShadow).toBe('0 0 10px red')
})
Expand Down
6 changes: 0 additions & 6 deletions src/components/sketch/__snapshots__/spec.js.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,8 +34,6 @@ exports[`Sketch renders correctly 1`] = `
>
<div
onMouseDown={[Function]}
onTouchMove={[Function]}
onTouchStart={[Function]}
style={
Object {
"MozBorderRadius": undefined,
Expand Down Expand Up @@ -199,8 +197,6 @@ exports[`Sketch renders correctly 1`] = `
<div
className="hue-horizontal"
onMouseDown={[Function]}
onTouchMove={[Function]}
onTouchStart={[Function]}
style={
Object {
"MozBorderRadius": undefined,
Expand Down Expand Up @@ -359,8 +355,6 @@ exports[`Sketch renders correctly 1`] = `
/>
<div
onMouseDown={[Function]}
onTouchMove={[Function]}
onTouchStart={[Function]}
style={
Object {
"height": "100%",
Expand Down
Loading