Amphtml: Replace Element#matches() with helper in dom.js

[choumx]

The bug

Element#matches() is currently being used in a few places in the amp-story extension. The function, however, has a quirk on older browsers:

Several browsers implement this, prefixed, under the non-standard name matchesSelector().

https://developer.mozilla.org/en-US/docs/Web/API/Element/matches

The task

• [ ] Find and replace usages of Element#matches() with our matches() helper in dom.js.

Bonus points:

• [ ] Comparing performance of matches() with closest or closestBySelector and switching the implementation.
• [ ] Adding a new check in presubmit-checks.js that warns against usage of native matches().

Step by step

• [ ] Claim this issue by adding a comment below. Please only claim this bug if you plan on starting work in the next day or so. (If you join the AMP Project we'll be able to assign this issue to you after you've claimed it.)
• [ ] If you aren't too familiar with Git/GitHub, see the Getting Started End-to-End Guide for an intro to Git & GitHub, and how to get a copy of the code. You can also refer to the Quick Start Guide for the necessary setup steps with less explanation than the End-to-End guide.
• [ ] Follow the instructions for building AMP.
• [ ] [Create a Git branch](https://github.com/ampproject/amphtml/blob/master/contributing/getting-started-e2e.md#create-a-git-branch) for making your changes.
• [ ] [Sign the Contributor License Agreement](https://github.com/ampproject/amphtml/blob/master/CONTRIBUTING.md#contributor-license-agreement) before creating a Pull Request. (If you are contributing code on behalf of a corporation start this process as early as possible.)
• [ ] [Commit your changes](https://github.com/ampproject/amphtml/blob/master/contributing/getting-started-e2e.md#edit-files-and-commit-them) frequently.
• [ ] [Push your changes to GitHub](https://github.com/ampproject/amphtml/blob/master/contributing/getting-started-e2e.md#push-your-changes-to-your-github-fork).
• [ ] [Create a Pull Request](https://github.com/ampproject/amphtml/blob/master/contributing/getting-started-e2e.md#send-a-pull-request-ie-request-a-code-review). Write Fixes #11816 in the description and mention me (@choumx).
• [ ] [Respond to your reviewer's comments](https://github.com/ampproject/amphtml/blob/master/contributing/getting-started-e2e.md#respond-to-pull-request-comments) (if any).

Questions?

If you have questions ask in this issue or on your Pull Request (if you've created one) or see the How to get help section of the Getting Started guide.

[reznord]

Hi, I would like to take on this issue if no one is working on this issue.

[choumx]

Awesome, go for it @reznord! 😄

[choumx]

@reznord Have you started working on this yet? Still interested?

[reznord]

Hey, sorry got busy with some office work.

Will look into this today. Is there a place (slack or discord) where I can ask for help if I am stuck at some point.

[choumx]

Yes, we have a Slack channel (sign up required). More details here: https://github.com/ampproject/amphtml/blob/master/CONTRIBUTING.md#ongoing-participation

[sanjsanj]

Is this issue in progress or is it available please?

[choumx]

@sanjsanj Please take this issue since Anup seems busy. Thanks!

[sanjsanj]

@choumx Cheers, I'll have a look! :)

[sanjsanj]

@choumx I think I've done the basic task and would like it to be reviewed to get feedback, however I'd also like to have a go at the bonus tasks but have questions about them, should I wait til it's all done or would you like to have a look at a PR first please?

Bonus:

The first one about performance of matches() sorry maybe I'm being really dumb but I don't understand what that means, could you clarify please? I can see that matches() uses the appropriate native browser method, and that closestBySelector() can return closest() with matches() as the callback but I still don't get the question :)

I tried to run gulp presubmit before I start fiddling with the presubmit-checks but right off the bat I get quite a lot of errors, is this expected?

[choumx]

Let's start with the core fix in your first PR and we can follow up with another PR.

Sorry I could've phrased the bonus more clearly. Basically I think that closest() may be more performant than matches() in some cases, but I might be wrong.

I tried to run gulp presubmit before I start fiddling with the presubmit-checks but right off the bat I get quite a lot of errors, is this expected?

Shouldn't happen. Try creating your PR and I'll help take a look on the PR's build logs.

[sanjsanj]

My PR is in, when you get a chance pls.

The CI is still failing because it's finding references to .matches in gulp presubmit. Did you want it replacing throughout the project or just in amp-story?

There's an issue with my matcher that I'll fix as well, it shouldn't be picking up occurrences in amp-story, but that'll have to wait until after work tomorrow. And I can act on any other feedback you have then as well.

I notice in the CI it didn't have issues with gulp presubmit but I am having them locally. I'm pretty sure it's something simple I'm missing, it's complaining about import and restricted words.