fix(page): fix Mouse.click method

[ptommasi]

page.click method relies on Mouse.click method for execution. Mouse.click triggers the move, down and up methods in parallel waiting for all of them to finish, when they should be called sequentially instead.

Issues: #6462 (related), #3347 (left a comment in it).

[mathiasbynens]

Good catch!

[ptommasi]

Branch deleted for clean up, big thanks for your work! :)

[ondrejvelisek]

It seems to me this fix could increas probability to unexpected buggy behaviour. Kind of race condition. Since click is not an atomic operation. There could be another external mouse.move call between mouse.move and mouse.down. In such case puppeteer would click on a unexpected and potentialy dangerous position. For example (but not exclusivley) in headful mode where user moves mouse over the screen manually. Am I wrong?

[ptommasi]

For example (but not exclusivley) in headful mode where user moves mouse over the screen manually. Am I wrong?

I think you are indeed correct, but out of the two implementations (Promise.all vs sequential await) the latter is more predictable and much safer, in my opinion. Using the Promise.all implementation would not ensure that mouse down will happen before mouse up, and will not ensure that either of them will happen after the move.

As a consequence, page.click might be ignored (e.g. sequence ends up being up, move, down), it could cause a drag (e.g. down, move, up) or any other combination which is not move -> down -> up would cause a problem. In the issues I saw, people were complaining about the unpredictability of page.click, and Promise.all would definitely explain this type of behaviour.

My two cents is that is better to have an implementation that works most of the time and that is observable rather than an unpredictable one which could or could not work (as the issues demonstrated).

[ondrejvelisek]

Agree. It is definitely better this way.

Anyway it is fair to say and post it here for the future.

To fix it Chrome would need to implement some kind of atomic click operation or a way to send multiple sequential operations together within one call. Which what I know it is not able. Until then i'm afraid Puppeteer has no way to fix it.

[ptommasi]

To my understanding, page.click reason to exist is to simulate user behaviour by marking the click event as trusted (based on this comment). Single soft clicks can be still sent to the browser using page.evaluate, which is indeed what I still do at the moment (since I don't know exactly when the fix will be pushed in the deployed version, and the website I'm scraping doesn't seem to make a difference between trusted and untrusted events). For your use case, I suppose you could easily create a util function based on page.$eval(selector, (element) => element.click()) (or page.evaluate).

Disclaimer: I'm not a front-end developer, I'm not a puppeteer developer, I'm barely a developer, I used puppeteer in a hobbyist fashion for one of my pet project, so just keep in mind that my comments are based on very little experience on the matter! 😣