report: source-location for linkifying by connorjclark · Pull Request #9354 · GoogleChrome/lighthouse

connorjclark · 2019-07-11T18:34:19Z

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:

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.
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:

After:

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

GIF of it working in DevTools:

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

patrickhulce

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 👍

brendankenny · 2019-07-11T23:16:24Z

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 · 2019-07-12T00:03:05Z

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

Done! #9356

paulirish

works for me!

paulirish · 2019-08-29T19:35:10Z

+    element.classList.add('lh-source-location');
+    element.setAttribute('data-url', item.url);
+    // Should pass `line` to `data-line` as zero-indexed, as DevTools expects that for linkifying.
+    element.setAttribute('data-line', String(item.line));


i think .toString() is a little favored over using the constructor.

fyi this file uses String() a few times but never toString

fyi this file uses String() a few times but never toString

yeah, I don't know how that happened in here but generally good to stick with the ambient style

changed to toString()

note, if a plugin happens to provide a bad value (or no value) here, it would now error the renderer.

Sorry, I was agreeing with you that String() is fine in here since it's the established style

(there's only so much we can do against a plugin that refuses to use our types, though. you don't even need a plugin, you could just save a bad lhr to a gist and load it in the viewer)

oops, reverted.

brendankenny · 2019-08-31T17:24:53Z

+   * @protected
+   */
+  renderSourceLocation(item) {
+    if (!item.url) {


this shouldn't be possible now with the changes to make url required, right?

I'm guarding against an empty string here. Perhaps that's not necessary, and just allowing the url to be meaningless is better than showing nothing.

It may be possible to get an empty string from stylesheet.sourceURL if it came from a comment - I'd have to check. It is certainly possible for a plugin to provide an empty string.

brendankenny · 2019-08-31T17:49:38Z

+    }
+
+    element.classList.add('lh-source-location');
+    element.setAttribute('data-url', item.url);


already done in renderTextURL?

oh, didn't see dataset.url =

There is a difference - renderTextURL only sets the url if it is a valid URL. However, we can expect a sourceURL to not be a valid URL, yet it is still needed to resolve correctly with the devtools front end (it know about styles.css if there was a magic comment defining it, even if it isn't a full url).

maybe this should be data-source-url and data-source-line? would also prevent clash with the 3p filter.

brendankenny · 2019-08-31T17:52:53Z


    const deprecations = entries.filter(log => log.entry.source === 'deprecation').map(log => {
+      // HTML deprecations will have no url and no way to attribute to a specific line.
+      let source = null;


undefined should work (and hopefully allows getting rid of the null mentioned in another comment)

Co-Authored-By: Brendan Kenny <bckenny@gmail.com>

…location-details

This reverts commit 30a7daf.

connorjclark · 2019-09-06T18:55:25Z

                    },
                    {
-                        "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."


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

paulirish · 2019-09-20T01:21:49Z

@brendankenny can you take a look

paulirish · 2019-11-19T00:52:21Z

@brendankenny we're gonna merge tomorrow, FYI

connorjclark added 5 commits July 10, 2019 10:57

first pass

ba10b4d

next pass

4b139e5

comment

dd1a015

use source column

771f4d2

Merge remote-tracking branch 'origin/master' into ui-location-details

35ee4a8

connorjclark requested review from brendankenny, patrickhulce and paulirish as code owners July 11, 2019 18:34

googlebot added the cla: yes label Jul 11, 2019

patrickhulce reviewed Jul 11, 2019

View reviewed changes

un comment code

1aff81c

vercel Bot temporarily deployed to staging July 11, 2019 21:55 Inactive

delete old code

d2f5901

vercel Bot deployed to staging July 11, 2019 21:56 View deployment

connorjclark added 5.2 and removed 5.2 labels Jul 11, 2019

brendankenny added 5.3 and removed 5.3 labels Jul 11, 2019

connorjclark mentioned this pull request Jul 12, 2019

audits(font-size): calculate accurate line/column for inline styles #9356

Merged

add inline/attribute decl back

2b7c535

vercel Bot deployed to staging July 12, 2019 05:02 View deployment

add dynamic

d958a06

vercel Bot temporarily deployed to staging July 12, 2019 05:27 Inactive

just dynamic

b60ac0c

vercel Bot deployed to staging July 12, 2019 05:31 View deployment

connorjclark commented Jul 12, 2019

View reviewed changes

Comment thread lighthouse-core/audits/seo/font-size.js

connorjclark mentioned this pull request Jul 13, 2019

Unused CSS should more clearly identify a style as inline #5626

Open

paulirish approved these changes Aug 29, 2019

View reviewed changes

Merge remote-tracking branch 'origin/master' into ui-location-details

7c28493

brendankenny reviewed Aug 31, 2019

View reviewed changes

Update lighthouse-core/report/html/renderer/details-renderer.js

a62e34c

Co-Authored-By: Brendan Kenny <bckenny@gmail.com>

vercel Bot deployed to staging August 31, 2019 21:53 View deployment

connorjclark added 2 commits August 31, 2019 15:02

'ui-location-details' of github.com:GoogleChrome/lighthouse into ui-…

eae758c

…location-details

toString

30a7daf

vercel Bot deployed to staging August 31, 2019 22:05 View deployment

namespace data attrs

f9de084

vercel Bot deployed to staging August 31, 2019 22:16 View deployment

table null->undefined

f75e251

vercel Bot deployed to staging August 31, 2019 22:23 View deployment

comment

96c7d3e

vercel Bot deployed to staging August 31, 2019 22:27 View deployment

Revert "toString"

a8c5655

This reverts commit 30a7daf.

vercel Bot deployed to staging August 31, 2019 22:29 View deployment

fix jsdoc

386a654

vercel Bot deployed to staging August 31, 2019 22:31 View deployment

fix depr ts

b26d9c0

vercel Bot deployed to staging August 31, 2019 22:33 View deployment

connorjclark added 2 commits September 6, 2019 11:21

Merge remote-tracking branch 'origin/master' into ui-location-details

2274114

fix sample

6f38744

connorjclark commented Sep 6, 2019

View reviewed changes

Merge remote-tracking branch 'origin/master' into ui-location-details

56eb6c0

connorjclark mentioned this pull request Nov 19, 2019

Audits panel shouldn't scroll to top of page when swapping between tabs #9983

Closed

connorjclark commented Nov 19, 2019

View reviewed changes

Comment thread types/audit-details.d.ts

connorjclark mentioned this pull request Dec 30, 2019

audits(is-crawlable): line number for blocking directive in robots.txt #10154

Merged

connorjclark mentioned this pull request Aug 10, 2020

core(font-size): remove deprecated DOM.getFlattenedDocument #11248

Merged

Conversation

connorjclark commented Jul 11, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

patrickhulce left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

brendankenny commented Jul 11, 2019

Uh oh!

connorjclark commented Jul 12, 2019

Uh oh!

Uh oh!

paulirish left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

connorjclark Aug 31, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

paulirish commented Sep 20, 2019

Uh oh!

paulirish commented Nov 19, 2019

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

connorjclark commented Jul 11, 2019 •

edited

Loading

connorjclark Aug 31, 2019 •

edited

Loading