report: enable jump to audit in nested details #5915

midzer · 2018-08-27T23:38:54Z

Hi,

Summary

In report.html jump to respective audit on page load if hash is provided in URL. Collapsed content is opened automatically. Element is highlighted via CSS selector

At first i wanted to use https://developer.mozilla.org/en-US/docs/Web/API/Element/scrollIntoView for scrolling to requested item, but somehow this didn't work. Any feedback is welcome.

Related Issues/PRs

Fixes #5640
Fixes #6668

patrickhulce

thanks for jumping on this so quickly @midzer!

lighthouse-core/report/html/renderer/report-ui-features.js

lighthouse-core/report/html/report-styles.css

lighthouse-core/report/html/renderer/report-ui-features.js

brendankenny

thanks for the fix!

lighthouse-core/report/html/renderer/report-ui-features.js

lighthouse-core/report/html/report-styles.css

lighthouse-core/report/html/renderer/report-ui-features.js

midzer · 2018-08-28T01:00:02Z

closest() returns an Element, so I can't use offsetTop any more. I replaced it by details.getBoundingClientRect().top + window.scrollY

Thanks for your feedback. Cya later

brendankenny

Sorry for the long delay!

This is looking good to me, just a few lint errors and style nits.

lighthouse-core/report/html/renderer/report-ui-features.js

brendankenny · 2018-09-14T20:46:29Z

closest() returns an Element, so I can't use offsetTop any more. I replaced it by details.getBoundingClientRect().top + window.scrollY

It does return an Element, but we know more specifically that it's an HTMLElement since we asked for a 'details', so there's no harm in treating it as such (in fact, the typescript checker even narrows it down to HTMLDetailsElement | null for us, and the conditional rules out the null

brendankenny

so trying it now, there are still a couple of more cases to handle:

as @patrickhulce noted, some of these are nested more than one <details> element deep (e.g. passed accessibility audits), so will need to do multiple steps up the DOM tree using el.closest('.lh-container details') (.lh-container will also prevent going outside our own subtree)
some audits aren't nested at all, don't have a details ancestor, so we can't scroll to that point. We'll have to handle scrolling for those cases too (e.g. try out jumping to the SEO #meta-description on a report for example.com)

midzer · 2018-09-16T18:41:10Z

First of all, many thanks for your insight testing and review @brendankenny . I tried to include proposed suggestions in latest commits. Somehow, tests are broken now though. Can't figure out right now whats going havoc there^^

Regarding scrolling to element I've played around with element.scrollIntoView() again which does the job as current window.scroll() implementation. Too bad, it does not work in all cases yet: for example #meta-description element is hidden below .lh-scores.header after scroll took place. Manual additions/subtractions to offsetTop seem not to have any effect. So I'm kinda stuck on further progress right now.

…etting window.location there

midzer · 2018-11-19T15:02:28Z

Just to bump this one to the top. I have time to help bring it to merge.

Is this PR out of focus now?

brendankenny

hi @midzer!

I think there is definitely still value in updating and landing this. I added some new comments on this version.

Two other things:

it would be great to get a test/some tests. e.g. report-ui-features-test could create a report, manually update the jsdom location and call _jumpToAudit, then check that all the <details> containing that audit are open. Not sure if there's an easier way to set the URL for jsdom so _jumpToAudit would be called as in a real report, or if there's a way to also test the scroll location
You mentioned scrolling to the audit with the big score header in the way. That's definitely an issue. Since the scrolling is being done in JS, there's no reason we couldn't do a little manual math to make sure the audit is visible. But maybe there are other options. cc @paulirish (who implemented proper permalinks for categories way back in Report: denser styling #2706)

lighthouse-core/report/html/renderer/report-ui-features.js

lighthouse-core/test/report/html/renderer/report-ui-features-test.js

…TMLDetailsElement

lighthouse-core/report/html/renderer/report-ui-features.js

patrickhulce

very sorry for the crazy long review cycles :(

patrickhulce · 2019-01-08T04:06:55Z

lighthouse-core/report/html/renderer/report-ui-features.js

+      audit.firstElementChild.open = true;
+    }
+    this._openClosestDetails(audit);
+    if (audit instanceof HTMLElement) {


seems like this will always be true by this point, no? 🤔

Without this check, typescript throws Property 'offsetTop' does not exist on type 'Element'. in my IDE. I could not find another way around this issue

ah ok just for ts then?

let's flip it around then and add a comment to explain

// This should always be an HTMLElement by this point by ts doesn't know that if (!(audit instanceof HTMLElement)) return;

unless @brendankenny has better ideas?

ah ok just for ts then?

yeah, it's a tsc thing (technically it's correct, but we know if this particular document.querySelector response isn't null it's an HTMLElement).

We can either do your suggestion or do a cast above.

/** @type {?HTMLElement} */ const audit = this._document.querySelector(hash); if (!audit || !audit.classList.contains('lh-audit')) { return; } // ...

either one sounds good to me

I'm fairly new to TypeScript, so I did not know how to do the cast. Thanks for your suggestion @brendankenny . pushed.to.branch

patrickhulce · 2019-01-08T04:14:01Z

lighthouse-core/report/html/report-styles.css

@@ -337,6 +337,10 @@
  display: none;
 }

+.lh-audit:target {


I'm not convinced we need this. It's a bit odd given that the only thing that will make it go away is clicking another category, not just another audit.

This has been requested by @paulirish here #5640 (comment)

Oh! OK :)

@paulirish I'm not convinced we need this. It's a bit odd given that the only thing that will make it go away is clicking another category, not just another audit.

patrickhulce · 2019-01-08T04:15:34Z

lighthouse-core/report/html/renderer/report-ui-features.js

+    if (audit instanceof HTMLElement) {
+      const scoresWrapper = this._dom.find('.lh-scores-wrapper', this._document);
+      window.scroll({
+        top: audit.offsetTop + scoresWrapper.clientHeight,


I think this needs to be full height of .lh-header-container not just .lh-scores-wrapper. Also don't we need - not +? The audits are all offscreen for me on navigation.

…nto jumpToAuditViaAnchor

brendankenny

LGTM (with selector fix)

over to @patrickhulce and @paulirish

lighthouse-core/test/report/html/renderer/report-ui-features-test.js

brendankenny · 2019-01-08T20:39:14Z

lighthouse-core/report/html/renderer/report-ui-features.js

+      audit.firstElementChild.open = true;
+    }
+    this._openClosestDetails(audit);
+    const headerContainer = this._dom.find('lh-header-container', this._document);


'.lh-header-container'

Ooooops :) thanks

paulirish · 2019-01-10T00:56:56Z

This scrolling is pretty flaky and I tried experimenting with different combos of rAF/setTimeout/etc and thought I got something good: https://github.com/midzer/lighthouse/pull/1

And then I put my test page online and its no longer scrolling 100% reliably. https://envious-cave.surge.sh/cnn.html#js-libraries (try with reload and also with pasting into a new tab)

I dont think we can guarantee an animated scroll (until some browser bug is fixed?), but we can at least guarantee the page is scrolled to the right spot.

I do like setting focus on the <summary> instead of using :target though. Seems like a nice solution to the CSS discussion above: https://github.com/midzer/lighthouse/blob/7ce0f63cff7ccd3676d3c6c582b3ab790f7849a7/lighthouse-core/report/html/renderer/report-ui-features.js#L324-L327

brendankenny · 2019-04-05T01:23:08Z

Thanks for all your work @midzer.

I'm going to close for now since the scrolling logic never entirely panned out here due to how long it takes the report to construct the entire DOM after load. We're gearing up for a report UI refresh as we close in on May, and I'm hoping we can restructure report rendering and the DOM a little bit to make scrolling to individual audits more feasible. Hopefully then we can revisit this PR and make it work!

introduce jumpToAudit

74e7048

midzer requested a review from paulirish as a code owner August 27, 2018 23:38

patrickhulce suggested changes Aug 27, 2018

View reviewed changes

lighthouse-core/report/html/renderer/report-ui-features.js Outdated Show resolved Hide resolved

lighthouse-core/report/html/report-styles.css Outdated Show resolved Hide resolved

lighthouse-core/report/html/renderer/report-ui-features.js Outdated Show resolved Hide resolved

brendankenny reviewed Aug 27, 2018

View reviewed changes

lighthouse-core/report/html/renderer/report-ui-features.js Outdated Show resolved Hide resolved

lighthouse-core/report/html/renderer/report-ui-features.js Outdated Show resolved Hide resolved

lighthouse-core/report/html/report-styles.css Outdated Show resolved Hide resolved

paulirish reviewed Aug 27, 2018

View reviewed changes

lighthouse-core/report/html/renderer/report-ui-features.js Outdated Show resolved Hide resolved

lighthouse-core/report/html/renderer/report-ui-features.js Outdated Show resolved Hide resolved

patrickhulce changed the title ~~introduce jumpToAudit~~ report: enable jump to audit in nested details Aug 28, 2018

improve code and more suitable CSS selector

e7389f5

brendankenny requested changes Sep 14, 2018

View reviewed changes

fix some lint errors and improve code style

c2dcea1

brendankenny requested changes Sep 14, 2018

View reviewed changes

midzer added 2 commits September 16, 2018 17:39

open all closest <details> elements recursively

a7a17ea

fix test regression

1816171

midzer added 3 commits October 9, 2018 15:32

fix tsc-check and throw Error on fail

598b9c2

satisfy eslint

099f22b

fix jest tests by adding report-ui-features-test.js from master and s…

f4f918f

…etting window.location there

brendankenny requested changes Nov 19, 2018

View reviewed changes

midzer added 5 commits November 27, 2018 11:34

don't throw Error if no closest details element found recursively

45dc394

update to latest master

403ecff

move window.scroll to end of jumpToAudit() and check for instanceof H…

57b0e08

…TMLDetailsElement

put HTMLDetailsElement and HTMLElement to globals block

7dc027a

fix Cannot read property 'hash' of undefined test

8fe8b5c

brendankenny mentioned this pull request Nov 27, 2018

HTML Report: support deep linking #6668

Closed

3 tasks

ebidel reviewed Nov 27, 2018

View reviewed changes

lighthouse-core/report/html/renderer/report-ui-features.js Outdated Show resolved Hide resolved

ebidel reviewed Nov 27, 2018

View reviewed changes

lighthouse-core/report/html/renderer/report-ui-features.js Outdated Show resolved Hide resolved

midzer added 2 commits November 30, 2018 17:28

take scoresWrapper height into account for scroll to audit

796a444

prefer shorthand 'open' attribute

b6aa711

patrickhulce suggested changes Jan 8, 2019

View reviewed changes

midzer added 4 commits January 8, 2019 14:21

take lh-header-container into account for scroll position

9db9069

Merge branch 'master' of https://github.com/GoogleChrome/lighthouse i…

76eac93

…nto jumpToAuditViaAnchor

cast audit variable

fa847f2

prefer check over cast

4594435

brendankenny approved these changes Jan 8, 2019

View reviewed changes

fix selector

9724004

set focus instead of CSS :target selector

c02af00

brendankenny closed this Apr 5, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

report: enable jump to audit in nested details #5915

report: enable jump to audit in nested details #5915

midzer commented Aug 27, 2018 •

edited by paulirish

Loading

patrickhulce left a comment

brendankenny left a comment

midzer commented Aug 28, 2018

brendankenny left a comment

brendankenny commented Sep 14, 2018 •

edited

Loading

brendankenny left a comment

midzer commented Sep 16, 2018

midzer commented Nov 19, 2018

brendankenny left a comment

patrickhulce left a comment

patrickhulce Jan 8, 2019

midzer Jan 8, 2019

patrickhulce Jan 8, 2019

brendankenny Jan 8, 2019

midzer Jan 8, 2019

patrickhulce Jan 8, 2019

midzer Jan 8, 2019

patrickhulce Jan 8, 2019

patrickhulce Jan 8, 2019

brendankenny left a comment •

edited

Loading

brendankenny Jan 8, 2019

midzer Jan 8, 2019

paulirish commented Jan 10, 2019

brendankenny commented Apr 5, 2019

report: enable jump to audit in nested details #5915

report: enable jump to audit in nested details #5915

Conversation

midzer commented Aug 27, 2018 • edited by paulirish Loading

patrickhulce left a comment

Choose a reason for hiding this comment

brendankenny left a comment

Choose a reason for hiding this comment

midzer commented Aug 28, 2018

brendankenny left a comment

Choose a reason for hiding this comment

brendankenny commented Sep 14, 2018 • edited Loading

brendankenny left a comment

Choose a reason for hiding this comment

midzer commented Sep 16, 2018

midzer commented Nov 19, 2018

brendankenny left a comment

Choose a reason for hiding this comment

patrickhulce left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brendankenny left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

paulirish commented Jan 10, 2019

brendankenny commented Apr 5, 2019

midzer commented Aug 27, 2018 •

edited by paulirish

Loading

brendankenny commented Sep 14, 2018 •

edited

Loading

brendankenny left a comment •

edited

Loading