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.

Sign up for GitHub

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

Jump to bottom

report: quick tab #9175

Closed
wants to merge 8 commits into from
Closed

report: quick tab #9175

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
e7560c6
report: quick tab
connorjclark Jun 10, 2019
3ccf02e
white
connorjclark Jun 10, 2019
55008b7
pr
connorjclark Jun 11, 2019
9a7df73
refactor
connorjclark Jun 11, 2019
8b49ef6
i18n
connorjclark Jun 11, 2019
15bfaf1
remove unneeded conditional
connorjclark Jun 11, 2019
cf10dd8
Merge remote-tracking branch 'origin/master' into report-quick-tab
connorjclark Jun 11, 2019
ccc083c
sample jsonn
connorjclark Jun 11, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
35 changes: 35 additions & 0 deletions lighthouse-core/report/html/renderer/report-ui-features.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -84,6 +84,7 @@ class ReportUIFeatures {
this._setupToolsButton();
this._setupThirdPartyFilter();
this._setUpCollapseDetailsAfterPrinting();
this._setUpQuickTabFocus();
Copy link
Collaborator

Choose a reason for hiding this comment

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

lol this setUp typo has survived a full 2 years https://github.com/GoogleChrome/lighthouse/pull/2000/files

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

ahhhhh now I can't unsee.

this._resetUIState();
this._document.addEventListener('keyup', this.onKeyUp);
this._document.addEventListener('copy', this.onCopy);
Expand Down Expand Up @@ -559,6 +560,40 @@ class ReportUIFeatures {
}
}

/**
* Tab + Enter for quick jump document.activeElement to the first interesting element.
*/
_setUpQuickTabFocus() {
const firstContentSelectors = [
{
selector: '.lh-audit--fail summary',
label: 'Jump to first failing audit.',
},
{
selector: '.lh-audit--average summary',
label: 'Jump to first average audit.',
},
{
selector: '.lh-audit--audit summary',
label: 'Jump to first audit.',
},
];

const {selector, label} = firstContentSelectors.find(({selector}) => {
const el = this._document.querySelector(selector);
return Boolean(el && el instanceof HTMLElement);
}) || {selector: '.lh-category-header .lh-gauge__wrapper', label: 'Jump to first category.'};
Copy link
Collaborator

Choose a reason for hiding this comment

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

why not just put this last in the array?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Because this asserts that the variable will always be non-null (if find returns null, the || is the backup)

Copy link
Collaborator

Choose a reason for hiding this comment

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

seems like sort of a roundabout way of asserting that :)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Fair - I refactored it.


const startOfContentEl = this._dom.find(selector, this._document);
const quickFocusEl = this._dom.find('.lh-a11y-quick-focus', this._document);
quickFocusEl.textContent = label;
quickFocusEl.classList.remove('disabled');
quickFocusEl.addEventListener('click', (e) => {
startOfContentEl.focus();
e.preventDefault();
});
}

/**
* Returns the html that recreates this report.
* @return {string}
Expand Down
24 changes: 24 additions & 0 deletions lighthouse-core/report/html/templates.html
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -352,8 +352,32 @@
margin-left: 0;
}
}

.lh-a11y-quick-focus {
background-color: var(--color-blue);
color: var(--color-white);
padding: 20px;
clip: rect(1px, 1px, 1px, 1px);
Copy link
Collaborator

Choose a reason for hiding this comment

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

why clip?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good question. Here's a bad answer: I took it straight from GitHub.

display: none -> block won't work, because you can't focus onto a display: none element. opacity: 0 -> 1 does work, though. I wonder if there might be screen readers that will ignore 0 opacity elements.

margin: 0;
height: 1px;
width: 1px;
overflow: hidden;
position: absolute;
}
.lh-a11y-quick-focus.disabled {
visibility: hidden;
}
.lh-a11y-quick-focus:focus {
clip: auto;
height: auto;
width: auto;
z-index: 10000; /* lol */
}
</style>

<!-- This is disabled by default. Enabled in ReportUIFeatures. -->
<a href="#" class="lh-a11y-quick-focus disabled" tabindex="1"></a>

<div class="lh-topbar">
<!-- Flat Lighthouse logo. -->
<svg class="lh-topbar__logo" viewBox="0 0 192 192">
Expand Down