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

report: source-location for linkifying #9354

Merged
merged 64 commits into from
Nov 20, 2019
Merged

Conversation

connorjclark
Copy link
Collaborator

@connorjclark connorjclark commented Jul 11, 2019

Linkify source code in DevTools.

Fixes: #6436, second half of #5443,

I've done font-size here. I haven't explored what other audits this could be done for. font-size only applies to CSS, but the changes here should be sufficient for JS/HTML too.

I've also made the line/columns that font-size reports more accurate. Namely:

  1. For inline styles, the stylesheet's startColumn was always being add to the rule's column. However, this is only valid if the stylesheet and the rule begin on the same line in the HTML.
  2. For inline styles with a sourceURL magic comment, the line/column was relative to the HTML file. I've changed it to be relative to the beginning of the style tag, as if it were its own file. This may be worse for UX (harder to pinpoint where to look in the HTML?). Open to tweaking this - note that, the data {url: <source mapped url>, line: <line relative to script tag>, column: <ditto>} is necessary for DevTools to link to this file in the Source panel, so without this change that gets complicated.

LH report (outside DevTools) before/after ...

Before:

image

After:

http://misc-hoten.surge.sh/lh-reports/pr-9354.html
image

GIF of it working in DevTools:

lh-dt-ui-location-linkify

Associated Chromium changes: https://chromium-review.googlesource.com/c/chromium/src/+/1694144

Test it out:

node lighthouse-cli/ http://misc-hoten.surge.sh/lh-ui-location-font-size/ --only-audits=font-size --view

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.

this is awesome!! a few concerns with the new attribution method, but I think this approach puts us in a much better place to be able to handle those challenges 👍

@vercel vercel bot temporarily deployed to staging July 11, 2019 21:55 Inactive
@brendankenny
Copy link
Member

I've also made the line/columns that font-size reports more accurate.

can we deal with these separately? They seem related but with significantly different concerns (audit internals vs report rendering/devtools integration)

@connorjclark
Copy link
Collaborator Author

can we deal with these separately? They seem related but with significantly different concerns (audit internals vs report rendering/devtools integration)

Done! #9356

@vercel vercel bot temporarily deployed to staging July 12, 2019 05:27 Inactive
This reverts commit 30a7daf.
"value": "'window.webkitStorageInfo' is deprecated. Please use 'navigator.webkitTemporaryStorage' or 'navigator.webkitPersistentStorage' instead."
},
{
"source": "deprecation",
"value": "/deep/ combinator is no longer supported in CSS dynamic profile.It is now effectively no-op, acting as if it were a descendant combinator. /deep/ combinator will be removed, and will be invalid at M65. You should remove it. See https://www.chromestatus.com/features/4964279606312960 for more details."
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

this message is missing a space so i fixed it: https://chromium-review.googlesource.com/c/chromium/src/+/1790201

@paulirish
Copy link
Member

@brendankenny can you take a look

@paulirish
Copy link
Member

@brendankenny we're gonna merge tomorrow, FYI

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

Successfully merging this pull request may close these issues.

report(font-size): linkify selectors in devtools
6 participants