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

core: save native HTMLElement.prototype.matches function #6283

Merged
merged 1 commit into from
Oct 19, 2018

Conversation

connorjclark
Copy link
Collaborator

@connorjclark connorjclark commented Oct 15, 2018

Summary

HTMLElement.prototype.matches was being overriden (by userland code), so we now store the native implementation at HTMLElement.prototype.__matches and use that instead.

Not sure how to make a test for this. If you use the sample HTML provided in the associated ticket, you'll see that the error no longer occurs.

Related Issues/PRs
Fixes #5934

Copy link
Collaborator

@patrickhulce patrickhulce left a comment

Choose a reason for hiding this comment

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

Nice! If you wanted to be extra secure with a test, you could throw a line that overrides matches to one of our smoke test files, probably the dbw_tester.html?

@connorjclark
Copy link
Collaborator Author

Done, thanks for pointing me towards the smoke tests.

@connorjclark
Copy link
Collaborator Author

connorjclark commented Oct 15, 2018

Paul found this err'ing sites:

https://www.fitreisen.de/
http://historie.brann.no/

I confirmed they work after these changes.

Well, the second one does. Looking into why the first one is failing.

Copy link
Member

@exterkamp exterkamp left a comment

Choose a reason for hiding this comment

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

LGTM!

@connorjclark
Copy link
Collaborator Author

connorjclark commented Oct 15, 2018

OK, the first link fails due to specific elements having a custom property descriptor (overriding matches), but the second one fails due to overriding matches somewhere on the prototype chain (I assume, did not confirm).

Switched to saving the method on window instead of the prototype, and calling it w/ the instance explicitly w/ .call.

@paulirish
Copy link
Member

patch looks great. some of the smoketests are being a problem. Perhaps there's an extra console error now?

@connorjclark
Copy link
Collaborator Author

yeah, seems that the two elements I added produces an extra log statement. It also makes for two fewer render blocking requests ...

I guess these smoke tests are pretty fragile? I'll update the new expected values, let me know if that is incorrect.

Output before changing expectations:

lighthouse git:(issue-5934-matches-is-not-a-fn) ✗ yarn smoke dbw
yarn run v1.10.1
$ node lighthouse-cli/test/smokehouse/run-smoke.js dbw
Running ONLY smoketests for: dbw

Smoketest batch: parallel-second
dbw smoketest starting…

dbw smoketest results:
Command failed: node lighthouse-cli/test/smokehouse/smokehouse.js --config-path=lighthouse-cli/test/smokehouse/dbw-config.js --expectations-path=dobetterweb/dbw-expectations.js

Doing a run of 'http://localhost:10200/dobetterweb/dbw_tester.html'...
$ node lighthouse-cli/index.js http://localhost:10200/dobetterweb/dbw_tester.html --config-path=/Users/cjamcl/src/lighthouse/lighthouse-cli/test/smokehouse/dbw-config.js --output-path=smokehouse-32337.report.json --output=json --quiet --port=0
Asserting expected results match those found. (http://localhost:10200/dobetterweb/dbw_tester.html)
  ✓ final url: http://localhost:10200/dobetterweb/dbw_tester.html

  ✘ difference at errors-in-console.details.items.length
              expected: 6
                 found: 7

          found result:
      {
        "id": "errors-in-console",
        "title": "Browser errors were logged to the console",
        "description": "Errors logged to the console indicate unresolved problems. They can come from network request failures and other browser concerns.",
        "score": 0,
        "scoreDisplayMode": "binary",
        "rawValue": 7,
        "details": {
          "type": "table",
          "headings": [
            {
              "key": "url",
              "itemType": "url",
              "text": "URL"
            },
            {
              "key": "description",
              "itemType": "code",
              "text": "Description"
            }
          ],
          "items": [
            {
              "source": "other",
              "description": "Application Cache Error event: Manifest fetch failed (404) http://localhost:10200/dobetterweb/clock.appcache",
              "url": "http://localhost:10200/dobetterweb/dbw_tester.html"
            },
            {
              "source": "network",
              "description": "Failed to load resource: the server responded with a status of 404 (Not Found)",
              "url": "http://localhost:10200/dobetterweb/dobetterweb/dbw_tester.css?delay=100"
            },
            {
              "source": "network",
              "description": "Failed to load resource: the server responded with a status of 404 (Not Found)",
              "url": "http://localhost:10200/dobetterweb/unknown404.css?delay=200"
            },
            {
              "source": "network",
              "description": "Failed to load resource: the server responded with a status of 404 (Not Found)",
              "url": "http://localhost:10200/dobetterweb/fcp-delayer.js?delay=5000"
            },
            {
              "source": "network",
              "description": "Failed to load resource: the server responded with a status of 404 (Not Found)",
              "url": "http://localhost:10200/favicon.ico"
            },
            {
              "source": "Runtime.exception",
              "description": "TypeError: Cannot set property 'media' of null\n    at setTimeout (http://localhost:10200/dobetterweb/dbw_tester.html:24:22)",
              "url": "http://localhost:10200/dobetterweb/dbw_tester.html"
            },
            {
              "source": "Runtime.exception",
              "description": "Error: An error\n    at http://localhost:10200/dobetterweb/dbw_tester.html:63:38",
              "url": "http://localhost:10200/dobetterweb/dbw_tester.html"
            }
          ]
        }
      }


  ✘ difference at render-blocking-resources.details.items.length
              expected: 7
                 found: 5

          found result:
      {
        "id": "render-blocking-resources",
        "title": "Eliminate render-blocking resources",
        "description": "Resources are blocking the first paint of your page. Consider delivering critical JS/CSS inline and deferring all non-critical JS/styles. [Learn more](https://developers.google.com/web/tools/lighthouse/audits/blocking-resources).",
        "score": 0.47,
        "scoreDisplayMode": "numeric",
        "rawValue": 986,
        "displayValue": "Potential savings of 990 ms",
        "details": {
          "type": "opportunity",
          "headings": [
            {
              "key": "url",
              "valueType": "url",
              "label": "URL"
            },
            {
              "key": "totalBytes",
              "valueType": "bytes",
              "label": "Size (KB)"
            },
            {
              "key": "wastedMs",
              "valueType": "timespanMs",
              "label": "Potential Savings (ms)"
            }
          ],
          "items": [
            {
              "url": "http://localhost:10200/dobetterweb/dobetterweb/dbw_tester.css?delay=100",
              "totalBytes": 171,
              "wastedMs": 377
            },
            {
              "url": "http://localhost:10200/dobetterweb/unknown404.css?delay=200",
              "totalBytes": 171,
              "wastedMs": 377
            },
            {
              "url": "http://localhost:10200/dobetterweb/dbw_tester.css?delay=2200",
              "totalBytes": 853,
              "wastedMs": 377
            },
            {
              "url": "http://localhost:10200/dobetterweb/dbw_partial_a.html?delay=200",
              "totalBytes": 768,
              "wastedMs": 377
            },
            {
              "url": "http://localhost:10200/dobetterweb/dbw_tester.css?delay=3000&capped",
              "totalBytes": 853,
              "wastedMs": 227
            }
          ],
          "overallSavingsMs": 986
        }
      }

  Correctly passed 15 assertions

15 passing
2 failing
smoketest-dbw: 9636.795ms
dbw smoketest complete.

We have 1 failing smoketests: dbw
error Command failed with exit code 1.

@connorjclark
Copy link
Collaborator Author

My assumption is that some sort of costly processing happens with each element, and it is throwing off the expected result of all those ?delays.

@patrickhulce
Copy link
Collaborator

@hoten yeah those smoketest errors definitely aren't expected. It looks like something is wrong with the HTML that's causing the rest of the elements to be dropped.

            {
              "source": "Runtime.exception",
              "description": "TypeError: Cannot set property 'media' of null\n    at setTimeout (http://localhost:10200/dobetterweb/dbw_tester.html:24:22)",
              "url": "http://localhost:10200/dobetterweb/dbw_tester.html"
            },

is an error that shouldn't be there. A different test can't find amp-style-link which is just a few lines down from the code you added. Maybe there's something squirrely with the <object>s or script tag?

Also for the smoketest I'm thinking we want to redefine matches, not just use it to make sure our workaround is working-around something :)

@paulirish
Copy link
Member

Maybe there's something squirrely with the s or script tag?

they're within the <head> tag right now. gotta move it down into the <body>

Copy link
Member

@brendankenny brendankenny left a comment

Choose a reason for hiding this comment

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

nj getting into the smoke tests!

<object id="5934a"></object>
<object id="5934b"></object>
<script>
document.getElementById("5934a").matches = "See #5934";
Copy link
Member

Choose a reason for hiding this comment

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

can you add a comment on why these lines are here so we aren't mystified at some point in the future :)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yup, done

@@ -1137,7 +1137,8 @@ class Driver {
async cacheNatives() {
await this.evaluateScriptOnNewDocument(`window.__nativePromise = Promise;
window.__nativeError = Error;
window.__nativeURL = URL;`);
window.__nativeURL = URL;
window.__ElementMatches = Element.prototype.matches;`);
Copy link
Member

Choose a reason for hiding this comment

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

👍👍 on the __ElementMatches instead of modifying the prototype

@connorjclark
Copy link
Collaborator Author

connorjclark commented Oct 16, 2018

objects in head was definitely the issue. Doh.

Also for the smoketest I'm thinking we want to redefine matches, not just use it to make sure our workaround is working-around something :)

@patrickhulce I'll add a line that also redefines HTMLElement.prototype.matches. I think we should keep the others, as my first attempt at a solution (HTMLElement.prototype.__matches = HTMLElement.prototype.matches) failed to fix scenarios where individual objects (not prototypes) have a matches property that hid access to HTMLElement.prototype.matches.

@patrickhulce
Copy link
Collaborator

Oh ya for sure didn't mean to suggest removing what you've already got 👍 :)

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.

5 participants