report: make tools menu focus-able #9169
Conversation
| @@ -366,12 +372,12 @@ | |||
| <a href="" class="lh-topbar__url" target="_blank" rel="noopener"></a> | |||
|
|
|||
| <div class="lh-tools"> | |||
| <div class="report-icon report-icon--share lh-tools__button" title="Export report"> | |||
| <button class="report-icon report-icon--share lh-tools__button" title="Export report"> | |||
There was a problem hiding this comment.
If this stays div, would have to 1) use tabindex="0" and 2) wire our own key event listener so space/enter would toggle the tools menu.
| @@ -262,6 +262,14 @@ | |||
| height: var(--lh-tools-icon-size); | |||
| cursor: pointer; | |||
| margin-right: 5px; | |||
| /* Override default button styles. */ | |||
There was a problem hiding this comment.
maybe add to the comment that this is an actual button element but we want to style it like a transparent div? :)
| .lh-tools__button.active + .lh-tools__dropdown { | ||
| opacity: 1; | ||
| clip: rect(-1px, 187px, 242px, -3px); | ||
| visibility: visible; |
There was a problem hiding this comment.
anything extra this does for us compared to opacity?
There was a problem hiding this comment.
visibility: none removes elements from focus consideration.
...can you be more specific? :) |
|
Oh sorry :) It boils down to adding aria attributes and keyboard access. Tabbing isn't the correct way to handle it if you follow the spec. You should be using arrow keys instead. More info here: I don't think it's worth the effort in this PR 👍 |
|
Ah, ok. We should fix that, though :) |
Fixes #9168.