report: quick tab #9175
report: quick tab #9175
Conversation
| 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.'}; |
There was a problem hiding this comment.
why not just put this last in the array?
There was a problem hiding this comment.
Because this asserts that the variable will always be non-null (if find returns null, the || is the backup)
There was a problem hiding this comment.
seems like sort of a roundabout way of asserting that :)
There was a problem hiding this comment.
Fair - I refactored it.
|
|
||
| const startOfContentEl = this._dom.find(selector, this._document); | ||
| const quickFocusEl = this._dom.find('.lh-a11y-quick-focus', this._document); | ||
| quickFocusEl.innerText = label; |
| background-color: var(--color-blue); | ||
| color: var(--color-white); | ||
| padding: 20px; | ||
| clip: rect(1px, 1px, 1px, 1px); |
There was a problem hiding this comment.
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.
| </style> | ||
|
|
||
| <!-- This is disabled by default. Enabled in ReportUIFeatures. --> | ||
| <a href="#" class="lh-a11y-quick-focus disabled" tabindex="1">asd</a> |
There was a problem hiding this comment.
asd significant in any way?
| @@ -84,6 +84,7 @@ class ReportUIFeatures { | |||
| this._setupToolsButton(); | |||
| this._setupThirdPartyFilter(); | |||
| this._setUpCollapseDetailsAfterPrinting(); | |||
| this._setUpQuickTabFocus(); | |||
There was a problem hiding this comment.
lol this setUp typo has survived a full 2 years https://github.com/GoogleChrome/lighthouse/pull/2000/files
There was a problem hiding this comment.
ahhhhh now I can't unsee.
|
oops didn't mean to close. Also, I need to do i18n :) |
|
i read up a bit on "skip links" because it's been so long. https://webaim.org/techniques/skipnav/ the intent is for skipping over the navigation. in our case we just have the URL in the topbar, followed by the export/tools menu, then we get into the gauges. I'd consider the gauges the first "content" since they change on each run. Jumping past them doesn't seem to align with the idea of a "skip link" IMO. Given that, I don't think this feature brings much user value. :/ sorry. Relatedly, when I flip on voiceover on my mac, I do see that our tools menu items are focusable when invisible. The general keyboard interaction with the tools menu seems a little atypical. If we want to improve the a11y of browsing our report, I think that'd be a good fix. |
add it to #9183! |
If we don't want to stretch "content" into "first most interesting content", then OK we shouldn't do any of this.
It was done here: #9169. Maybe the PR was not synced to master when you looked at this now deployment? |
|
ahhhhhhh true. #9169 is so hot. |
Inspired by GitHub's quick tab feature:
I'll deal with the errors + add tests after feedback.