From a77d64e5c291132ab971b0d1d1dd237e3d1a5f85 Mon Sep 17 00:00:00 2001 From: Daniel Freedman Date: Thu, 11 Jan 2018 17:54:59 -0800 Subject: [PATCH 1/4] Fix label tap by checking matched label pairs Fixes #4717 --- lib/utils/gestures.html | 23 +++++++++++++++++++++++ test/smoke/label-click.html | 25 +++++++++++++++++++++++++ 2 files changed, 48 insertions(+) create mode 100644 test/smoke/label-click.html diff --git a/lib/utils/gestures.html b/lib/utils/gestures.html index 738940f406..b29a7a4d4e 100644 --- a/lib/utils/gestures.html +++ b/lib/utils/gestures.html @@ -97,6 +97,10 @@ /** @type {(function(MouseEvent): void | undefined)} */ GestureRecognizer.prototype.click; + // keep track of any labels hit by tghe mouseCanceller + /** @type {!Array} */ + const clickedLabels = []; + // touch will make synthetic mouse events // `preventDefault` on touchend will cancel them, // but this breaks `` focus and link clicks @@ -115,14 +119,31 @@ mouseEvent[HANDLED_OBJ] = {skip: true}; // disable "ghost clicks" if (mouseEvent.type === 'click') { + let labels = []; + let clickFromLabel = false; let path = mouseEvent.composedPath && mouseEvent.composedPath(); if (path) { for (let i = 0; i < path.length; i++) { + if (path[i].nodeType === Node.ELEMENT_NODE) { + if (path[i].localName === 'label') { + labels.push(path[i]); + } else if (path[i].labels) { + let ownerLabels = path[i].labels; + for (let j = 0; j < ownerLabels.length; j++) { + clickFromLabel = clickFromLabel || clickedLabels.indexOf(ownerLabels[j]) > -1; + } + } + } if (path[i] === POINTERSTATE.mouse.target) { + // commit tracked labels to "clickedLabels" + clickedLabels.push(...labels); return; } } } + if (clickFromLabel) { + return; + } mouseEvent.preventDefault(); mouseEvent.stopPropagation(); } @@ -137,6 +158,8 @@ for (let i = 0, en; i < events.length; i++) { en = events[i]; if (setup) { + // reset clickLabels array + clickedLabels.length = 0; document.addEventListener(en, mouseCanceller, true); } else { document.removeEventListener(en, mouseCanceller, true); diff --git a/test/smoke/label-click.html b/test/smoke/label-click.html new file mode 100644 index 0000000000..c2ca4d3fa1 --- /dev/null +++ b/test/smoke/label-click.html @@ -0,0 +1,25 @@ + + + + + + + + + + + + + + + + + \ No newline at end of file From 70edf1f8dc7a764dc065ba29b299d13da39f5366 Mon Sep 17 00:00:00 2001 From: Daniel Freedman Date: Wed, 21 Feb 2018 16:06:41 -0800 Subject: [PATCH 2/4] Add tests Workaround IE11 not having `element.labels` and Safari not populating `element.labels` in shadowroots --- lib/utils/gestures.html | 55 ++++++++++++++++++++++++++++++-- test/unit/gestures-elements.html | 15 +++++++++ test/unit/gestures.html | 23 +++++++++++++ 3 files changed, 90 insertions(+), 3 deletions(-) diff --git a/lib/utils/gestures.html b/lib/utils/gestures.html index b29a7a4d4e..94cd964dbd 100644 --- a/lib/utils/gestures.html +++ b/lib/utils/gestures.html @@ -97,10 +97,59 @@ /** @type {(function(MouseEvent): void | undefined)} */ GestureRecognizer.prototype.click; - // keep track of any labels hit by tghe mouseCanceller + // keep track of any labels hit by the mouseCanceller /** @type {!Array} */ const clickedLabels = []; + /** @type {!Map} */ + const labellable = { + 'button': true, + 'input': true, + 'keygen': true, + 'meter': true, + 'output': true, + 'textarea': true, + 'progress': true, + 'select': true + }; + + /** + * @param {HTMLElement} el Element to check labelling status + * @return {boolean} element can have labels + */ + function canBeLabelled(el) { + return labellable[el.localName] || false; + } + + /** + * @param {HTMLElement} el Element that may be labelled. + * @return {!Array} Relevant label for `el` + */ + function matchingLabels(el) { + /** @type {!Array} */ + let labels = el.labels; + // IE doesn't have `labels` and Safari doesn't populate `labels` if element is in a shadowroot. + if (!labels || !labels.length) { + labels = []; + let root = el.getRootNode(); + // find all labels that are ancestors + let cur = el.parentNode; + while (cur !== root) { + if (cur.localName === 'label') { + labels.push(/** @type {!HTMLLabelElement} */(cur)); + } + } + // if there is an id on `el`, check for all labels with a matching `for` attribute + if (el.id) { + let matching = root.querySelectorAll(`label[for = ${el.id}]`); + for (let i = 0; i < matching.length; i++) { + labels.push(/** @type {!HTMLLabelElement} */(matching[i])); + } + } + } + return labels; + } + // touch will make synthetic mouse events // `preventDefault` on touchend will cancel them, // but this breaks `` focus and link clicks @@ -127,8 +176,8 @@ if (path[i].nodeType === Node.ELEMENT_NODE) { if (path[i].localName === 'label') { labels.push(path[i]); - } else if (path[i].labels) { - let ownerLabels = path[i].labels; + } else if (canBeLabelled(path[i])) { + let ownerLabels = matchingLabels(path[i]); for (let j = 0; j < ownerLabels.length; j++) { clickFromLabel = clickFromLabel || clickedLabels.indexOf(ownerLabels[j]) > -1; } diff --git a/test/unit/gestures-elements.html b/test/unit/gestures-elements.html index 777f2de556..ccf03f36ea 100644 --- a/test/unit/gestures-elements.html +++ b/test/unit/gestures-elements.html @@ -212,3 +212,18 @@ }); + + + + + diff --git a/test/unit/gestures.html b/test/unit/gestures.html index 4c8a92b4cb..b3ad058953 100644 --- a/test/unit/gestures.html +++ b/test/unit/gestures.html @@ -559,6 +559,29 @@ assert.equal(count, 1); Polymer.Gestures.remove(window, 'tap', increment); }); + + test('native label click', function() { + let el = document.createElement('x-native-label'); + document.body.appendChild(el); + let target = el.$.label; + let touches = [{ + clientX: 0, + clientY: 0, + identifier: 1, + // target is set to the element with `addEventListener`, which is app + target + }]; + let touchstart = new CustomEvent('touchstart', {bubbles: true, composed: true}); + touchstart.changedTouches = touchstart.touches = touches; + target.dispatchEvent(touchstart); + let touchend = new CustomEvent('touchend', {bubbles: true, composed: true}); + touchend.touches = touchend.changedTouches = touches; + target.dispatchEvent(touchend); + let click = new MouseEvent('click', {bubbles: true, composed: true}); + target.dispatchEvent(click); + assert.equal(el.$.check.checked, true); + document.body.removeChild(el); + }); }); From e1df166204ad9dc87605184e89ae91fd2db23de3 Mon Sep 17 00:00:00 2001 From: Daniel Freedman Date: Wed, 21 Feb 2018 16:43:36 -0800 Subject: [PATCH 3/4] Add docs and cleanup matchingLabels Remove broken `while` loop, as that case is already handled by the mouseCancellor code --- lib/utils/gestures.html | 20 ++++++++------------ test/unit/gestures.html | 7 +++++-- 2 files changed, 13 insertions(+), 14 deletions(-) diff --git a/lib/utils/gestures.html b/lib/utils/gestures.html index 94cd964dbd..ca38ec3173 100644 --- a/lib/utils/gestures.html +++ b/lib/utils/gestures.html @@ -128,17 +128,13 @@ function matchingLabels(el) { /** @type {!Array} */ let labels = el.labels; - // IE doesn't have `labels` and Safari doesn't populate `labels` if element is in a shadowroot. + // IE doesn't have `labels` and Safari doesn't populate `labels` + // if element is in a shadowroot. + // In this instance, finding the non-ancestor labels is enough, + // as the mouseCancellor code will handle ancstor labels if (!labels || !labels.length) { labels = []; let root = el.getRootNode(); - // find all labels that are ancestors - let cur = el.parentNode; - while (cur !== root) { - if (cur.localName === 'label') { - labels.push(/** @type {!HTMLLabelElement} */(cur)); - } - } // if there is an id on `el`, check for all labels with a matching `for` attribute if (el.id) { let matching = root.querySelectorAll(`label[for = ${el.id}]`); @@ -168,28 +164,28 @@ mouseEvent[HANDLED_OBJ] = {skip: true}; // disable "ghost clicks" if (mouseEvent.type === 'click') { - let labels = []; let clickFromLabel = false; let path = mouseEvent.composedPath && mouseEvent.composedPath(); if (path) { for (let i = 0; i < path.length; i++) { if (path[i].nodeType === Node.ELEMENT_NODE) { if (path[i].localName === 'label') { - labels.push(path[i]); + clickedLabels.push(path[i]); } else if (canBeLabelled(path[i])) { let ownerLabels = matchingLabels(path[i]); + // check if one of the clicked labels is labelling this element for (let j = 0; j < ownerLabels.length; j++) { clickFromLabel = clickFromLabel || clickedLabels.indexOf(ownerLabels[j]) > -1; } } } if (path[i] === POINTERSTATE.mouse.target) { - // commit tracked labels to "clickedLabels" - clickedLabels.push(...labels); return; } } } + // if one of the clicked labels was labelling the target element, + // this is not a ghost click if (clickFromLabel) { return; } diff --git a/test/unit/gestures.html b/test/unit/gestures.html index b3ad058953..465abf2fe6 100644 --- a/test/unit/gestures.html +++ b/test/unit/gestures.html @@ -564,11 +564,12 @@ let el = document.createElement('x-native-label'); document.body.appendChild(el); let target = el.$.label; + // simulate the event sequence of a touch on the screen let touches = [{ clientX: 0, clientY: 0, identifier: 1, - // target is set to the element with `addEventListener`, which is app + // target is set to the element with `addEventListener`, which is `target` target }]; let touchstart = new CustomEvent('touchstart', {bubbles: true, composed: true}); @@ -577,9 +578,11 @@ let touchend = new CustomEvent('touchend', {bubbles: true, composed: true}); touchend.touches = touchend.changedTouches = touches; target.dispatchEvent(touchend); + // simulate a mouse click on the label let click = new MouseEvent('click', {bubbles: true, composed: true}); target.dispatchEvent(click); - assert.equal(el.$.check.checked, true); + // check that the mouse click on the label will activate the checkbox + assert.equal(el.$.check.checked, true, 'checkbox should be checked'); document.body.removeChild(el); }); }); From c11c99b22bef5891959aade11df8fe1863ad5dfd Mon Sep 17 00:00:00 2001 From: Daniel Freedman Date: Wed, 21 Feb 2018 17:05:46 -0800 Subject: [PATCH 4/4] add test case for nested label --- test/unit/gestures-elements.html | 16 +++++++ test/unit/gestures.html | 77 ++++++++++++++++++++++---------- 2 files changed, 69 insertions(+), 24 deletions(-) diff --git a/test/unit/gestures-elements.html b/test/unit/gestures-elements.html index ccf03f36ea..534b4edfbf 100644 --- a/test/unit/gestures-elements.html +++ b/test/unit/gestures-elements.html @@ -227,3 +227,19 @@ customElements.define(XNativeLabel.is, XNativeLabel); + + + + + \ No newline at end of file diff --git a/test/unit/gestures.html b/test/unit/gestures.html index 465abf2fe6..30866d06fa 100644 --- a/test/unit/gestures.html +++ b/test/unit/gestures.html @@ -560,30 +560,59 @@ Polymer.Gestures.remove(window, 'tap', increment); }); - test('native label click', function() { - let el = document.createElement('x-native-label'); - document.body.appendChild(el); - let target = el.$.label; - // simulate the event sequence of a touch on the screen - let touches = [{ - clientX: 0, - clientY: 0, - identifier: 1, - // target is set to the element with `addEventListener`, which is `target` - target - }]; - let touchstart = new CustomEvent('touchstart', {bubbles: true, composed: true}); - touchstart.changedTouches = touchstart.touches = touches; - target.dispatchEvent(touchstart); - let touchend = new CustomEvent('touchend', {bubbles: true, composed: true}); - touchend.touches = touchend.changedTouches = touches; - target.dispatchEvent(touchend); - // simulate a mouse click on the label - let click = new MouseEvent('click', {bubbles: true, composed: true}); - target.dispatchEvent(click); - // check that the mouse click on the label will activate the checkbox - assert.equal(el.$.check.checked, true, 'checkbox should be checked'); - document.body.removeChild(el); + suite('native label click', function() { + + test('native label click', function() { + let el = document.createElement('x-native-label'); + document.body.appendChild(el); + let target = el.$.label; + // simulate the event sequence of a touch on the screen + let touches = [{ + clientX: 0, + clientY: 0, + identifier: 1, + // target is set to the element with `addEventListener`, which is `target` + target + }]; + let touchstart = new CustomEvent('touchstart', {bubbles: true, composed: true}); + touchstart.changedTouches = touchstart.touches = touches; + target.dispatchEvent(touchstart); + let touchend = new CustomEvent('touchend', {bubbles: true, composed: true}); + touchend.touches = touchend.changedTouches = touches; + target.dispatchEvent(touchend); + // simulate a mouse click on the label + let click = new MouseEvent('click', {bubbles: true, composed: true}); + target.dispatchEvent(click); + // check that the mouse click on the label will activate the checkbox + assert.equal(el.$.check.checked, true, 'checkbox should be checked'); + document.body.removeChild(el); + }); + }); + + test('label click with nested element', function() { + let el = document.createElement('x-native-label-nested'); + document.body.appendChild(el); + let target = el.$.label; + // simulate the event sequence of a touch on the screen + let touches = [{ + clientX: 0, + clientY: 0, + identifier: 1, + // target is set to the element with `addEventListener`, which is `target` + target + }]; + let touchstart = new CustomEvent('touchstart', {bubbles: true, composed: true}); + touchstart.changedTouches = touchstart.touches = touches; + target.dispatchEvent(touchstart); + let touchend = new CustomEvent('touchend', {bubbles: true, composed: true}); + touchend.touches = touchend.changedTouches = touches; + target.dispatchEvent(touchend); + // simulate a mouse click on the label + let click = new MouseEvent('click', {bubbles: true, composed: true}); + target.dispatchEvent(click); + // check that the mouse click on the label will activate the checkbox + assert.equal(el.$.check.checked, true, 'checkbox should be checked'); + document.body.removeChild(el); }); });