From 545c43d28d7c8fcabf7cab082e76676890d8b871 Mon Sep 17 00:00:00 2001
From: Andrey Lushnikov <aslushnikov@gmail.com>
Date: Tue, 19 May 2020 16:27:56 -0700
Subject: [PATCH] fix: better hittarget testing for clicking (#2217)

While checking for hittarget, we first bubble from a target element
up to find the first element without `pointer-events: none` style.

This bubbling does not make much sense: we risk desperately clicking
"body" element, when we were actually asked to click some deeply-nested
"span".

Additionally, in many cases the original intent is to click a button. In this
case, we should use the enclosing "button" as a hit target directly.

Fixes #2175
---
 src/injected/injectedScript.ts |  3 +--
 test/click.spec.js             | 21 +++++++++++++++++++--
 2 files changed, 20 insertions(+), 4 deletions(-)

diff --git a/src/injected/injectedScript.ts b/src/injected/injectedScript.ts
index 9096675700..7acd156efe 100644
--- a/src/injected/injectedScript.ts
+++ b/src/injected/injectedScript.ts
@@ -375,10 +375,9 @@ export default class InjectedScript {
 
   checkHitTargetAt(node: Node, point: types.Point): types.InjectedScriptResult<boolean> {
     let element = node.nodeType === Node.ELEMENT_NODE ? (node as Element) : node.parentElement;
-    while (element && window.getComputedStyle(element).pointerEvents === 'none')
-      element = element.parentElement;
     if (!element || !element.isConnected)
       return { status: 'notconnected' };
+    element = element.closest('button, [role=button]') || element;
     let hitElement = this._deepElementFromPoint(document, point.x, point.y);
     while (hitElement && hitElement !== element)
       hitElement = this._parentElementOrShadowHost(hitElement);
diff --git a/test/click.spec.js b/test/click.spec.js
index 464540f2b3..bc282080e5 100644
--- a/test/click.spec.js
+++ b/test/click.spec.js
@@ -503,9 +503,26 @@ describe('Page.click', function() {
     expect(await page.evaluate(() => window.result)).toBe('Was not clicked');
   });
 
-  it('should climb dom for pointer-events:none targets', async({page, server}) => {
-    await page.setContent('<button><label style="pointer-events:none">Click target</label></button>')
+  it('should climb dom for inner label with pointer-events:none', async({page, server}) => {
+    await page.setContent('<button onclick="javascript:window.__CLICKED=true;"><label style="pointer-events:none">Click target</label></button>');
     await page.click('text=Click target');
+    expect(await page.evaluate(() => window.__CLICKED)).toBe(true);
+  });
+  it('should climb up to [role=button]', async({page, server}) => {
+    await page.setContent('<div role=button onclick="javascript:window.__CLICKED=true;"><div style="pointer-events:none"><span><div>Click target</div></span></div>');
+    await page.click('text=Click target');
+    expect(await page.evaluate(() => window.__CLICKED)).toBe(true);
+  });
+  it('should wait for BUTTON to be clickable when it has pointer-events:none', async({page, server}) => {
+    await page.setContent('<button onclick="javascript:window.__CLICKED=true;" style="pointer-events:none"><span>Click target</span></button>');
+    const clickPromise = page.click('text=Click target');
+    // Do a few roundtrips to the page.
+    for (let i = 0; i < 5; ++i)
+      expect(await page.evaluate(() => window.__CLICKED)).toBe(undefined);
+    // remove `pointer-events: none` css from button.
+    await page.evaluate(() => document.querySelector('button').style.removeProperty('pointer-events'));
+    await clickPromise;
+    expect(await page.evaluate(() => window.__CLICKED)).toBe(true);
   });
   it('should update modifiers correctly', async({page, server}) => {
     await page.goto(server.PREFIX + '/input/button.html');