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.

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

Adding pointer-events_touch-events recipe. #3

Open
wants to merge 26 commits into
base: master
Choose a base branch
from

Conversation

jevery23
Copy link
Owner

@jevery23 jevery23 commented Nov 2, 2017

I have created a recipe which checks all places where we could find an instance of a pointerevent being listened to and counts each instance it finds to provide us with a running total.

The recipe checks for desired attributes on HTML elements, but this didn't work on the test page when I tried in Chrome, so could not test this, but it is possible to listen to events in this way.

Data collected using this recipe from a small set of URLs is here: https://cosmos15.osdinfra.net/cosmos/asimov.partner.osg/shares/asimov.prod.data/Public/Upload/DataGrid/WPT/Recipe-WORKER4/Recipe-WORKER4/1.0/2017/11/02/Recipe-WORKER4_2017_11_02_08-0.txt?property=info

@jevery23 jevery23 self-assigned this Nov 2, 2017
@jevery23 jevery23 requested a review from scottlow November 2, 2017 18:01
Recipe.min.js Outdated

var isExternalJSAndNotRecipeJS = (element.src !== undefined && element.src !== "" && !element.src.includes("Recipe.min.js"));
var xhr;
if (isExternalJSAndNotRecipeJS)
Copy link
Collaborator

Choose a reason for hiding this comment

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

Isn't this making XHRs for every single element with a src? (An image from an img tag, for example, would be pulled down as part of this). I think you'll want to do a check for nodeName == "SCRIPT" here as well.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Or move this into the block of code where we know for sure we have an external script

Copy link
Owner Author

@jevery23 jevery23 Nov 2, 2017

Choose a reason for hiding this comment

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

Ah that's a good spot, I did not think of that case at all.

Recipe.min.js Outdated
}

if (nodeName === "SCRIPT") {
// if inline script. ensure that it's not our recipe script and look for string of interest
Copy link
Collaborator

Choose a reason for hiding this comment

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

Update comment here since we know that it's not our recipe script at this point (iirc, the recipe script is injected as a script tag, right?)

Copy link
Owner Author

Choose a reason for hiding this comment

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

Yeah so I was finding that the recipe would check the recipe.min.js script before this check was put in. Will update the comments as a result of the changes. :)

Recipe.min.js Outdated
else if (isExternalJSAndNotRecipeJS)
{
if (xhr.status === 200 && xhr.responseText.indexOf(event) !== -1) {
var regex = new RegExp(event, 'g');
Copy link
Collaborator

Choose a reason for hiding this comment

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

NIT: Redundant code to the above. Could be pulled into a function.

Copy link
Owner Author

Choose a reason for hiding this comment

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

Yeah so I have packed all this stuff into the external script check if statement.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants