[A11y] Updated lighthouse categories group [devtools/devtools-frontend : main]
Guangyue Xu (Gerrit)
Guangyue Xu added 1 comment![]( "Open in Gerrit")
Hi Benedikt, could you help review this A11y bug fix in the lighthouse panel.
Thanks,
Gavin
Related details
- Benedikt Meurer
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Benedikt Meurer (Gerrit)
Benedikt Meurer voted Code-Review+1![]( "Open in Gerrit")
- Guangyue Xu
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Guangyue Xu (Gerrit)
Guangyue Xu added 1 comment![]( "Open in Gerrit")
Hi Kim, could you help review this A11y fix in the lighthouse panel. Need two +1s to land this fix. Thanks.
Related details
- Kim-Anh Tran
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Kim-Anh Tran (Gerrit)
Kim-Anh Tran added 3 comments![]( "Open in Gerrit")
Hi Guangyue,
Thanks for the CL! I've added a comment 😊
const checkboxFragment = UI.Fragment.Fragment.build`We don't want to use `fragment`s anymore, please use LitHTML if you need HTML templates 😊.
UI.Tooltip.Tooltip.install(inputElement, tooltip);So as far as I understand, you are trying to set the tooltip on the input element. Instead of creating an input element directly, let's try to use what we have.
If we can't do that out of the box, maybe we need to change the `ToolbarSettingCheckbox` instead, to set the tooltip on top of the actual input element. It looks like `ToolbarSettingCheckbox` uses `CheckboxLabel` underneath, which has a create method. If we allow for setting a tooltip (via create), we can pass the tooltip to the input element, instead of installing it on the `ToolbarSettingCheckbox`. Would that work? The default tooltip can still be the same as the `text` as before, if no tooltip was passed (https://source.chromium.org/chromium/chromium/src/+/main:third_party/devtools-frontend/src/front_end/ui/legacy/UIUtils.ts;l=1185;drc=b033422e587017e016b52f96d3ac02ed7b2f8dc2)
Related details
- Guangyue Xu
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Guangyue Xu (Gerrit)
Guangyue Xu added 1 comment![]( "Open in Gerrit")
if (title !== undefined) {Guangyue XuWe should also set the tooltip, if the `title` is undefined and the `tooltip` is defined
Updated to set checkbox title from either tooltip or title inputs.
Related details
- Kim-Anh Tran
- Simon Zünd
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Kim-Anh Tran (Gerrit)
Kim-Anh Tran added 4 comments![]( "Open in Gerrit")
We don't want to use `fragment`s anymore, please use LitHTML if you need HTML templates 😊.
Kim-Anh Tranremoved fragment. No need to use LitHTML since we have updated `ToolbarCheckbox`.
Done
So as far as I understand, you are trying to set the tooltip on the input element. Instead of creating an input element directly, let's try to use what we have.
If we can't do that out of the box, maybe we need to change the `ToolbarSettingCheckbox` instead, to set the tooltip on top of the actual input element. It looks like `ToolbarSettingCheckbox` uses `CheckboxLabel` underneath, which has a create method. If we allow for setting a tooltip (via create), we can pass the tooltip to the input element, instead of installing it on the `ToolbarSettingCheckbox`. Would that work? The default tooltip can still be the same as the `text` as before, if no tooltip was passed (https://source.chromium.org/chromium/chromium/src/+/main:third_party/devtools-frontend/src/front_end/ui/legacy/UIUtils.ts;l=1185;drc=b033422e587017e016b52f96d3ac02ed7b2f8dc2)
Kim-Anh TranUpdated `ToolbarCheckbox` and `CheckboxLabel` to install tooltip.
Done
if (inputTooltip !== undefined) {nit: `if (inputTooltip)`
element.#textElement.title = inputTooltip;This is not entirely correct, the `textElement` should be the `title` foremost, and secondary only the `tooltip`. If both exist, you'd take the tooltip here. For the tooltip, it's the other way round.
Related details
- Guangyue Xu
- Simon Zünd
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Guangyue Xu (Gerrit)
Guangyue Xu added 2 comments![]( "Open in Gerrit")
if (inputTooltip !== undefined) {nit: `if (inputTooltip)`
updated as suggested.
element.#textElement.title = inputTooltip;This is not entirely correct, the `textElement` should be the `title` foremost, and secondary only the `tooltip`. If both exist, you'd take the tooltip here. For the tooltip, it's the other way round.
- Kim-Anh Tran
- Simon Zünd
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Kim-Anh Tran (Gerrit)
Kim-Anh Tran voted and added 4 comments![]( "Open in Gerrit")
Votes added by Kim-Anh Tran
| Code-Review | +1 |
4 comments
Guangyue XuWe should also set the tooltip, if the `title` is undefined and the `tooltip` is defined
Updated to set checkbox title from either tooltip or title inputs.
Done
const textTooltip = title ?? tooltip;Looking at this again, actually.. if we don't have a title, the tooltip shouldn't replace the title. I think that would be unexpected behavior. I.e. removing all. 1189-1193 should be enough and then good to go. Sorry for that.
if (inputTooltip !== undefined) {Guangyue Xunit: `if (inputTooltip)`
updated as suggested.
- Guangyue Xu
- Simon Zünd
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Guangyue Xu (Gerrit)
Guangyue Xu added 1 comment![]( "Open in Gerrit")
const textTooltip = title ?? tooltip;Looking at this again, actually.. if we don't have a title, the tooltip shouldn't replace the title. I think that would be unexpected behavior. I.e. removing all. 1189-1193 should be enough and then good to go. Sorry for that.
- Kim-Anh Tran
- Simon Zünd
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Kim-Anh Tran (Gerrit)
Kim-Anh Tran voted and added 3 comments![]( "Open in Gerrit")
Votes added by Kim-Anh Tran
| Code-Review | +1 |
3 comments
const textTooltip = title ?? tooltip;Guangyue XuLooking at this again, actually.. if we don't have a title, the tooltip shouldn't replace the title. I think that would be unexpected behavior. I.e. removing all. 1189-1193 should be enough and then good to go. Sorry for that.
updated as suggested.
Done
element.#textElement.title = inputTooltip;Guangyue XuThis is not entirely correct, the `textElement` should be the `title` foremost, and secondary only the `tooltip`. If both exist, you'd take the tooltip here. For the tooltip, it's the other way round.
Updated as suggested. Thanks.
- Guangyue Xu
- Simon Zünd
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Simon Zünd (Gerrit)
Simon Zünd voted Code-Review+1![]( "Open in Gerrit")
| Code-Review | +1 |
Related details
- Guangyue Xu
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Guangyue Xu (Gerrit)
Guangyue Xu voted Commit-Queue+2![]( "Open in Gerrit")
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Devtools-frontend LUCI CQ (Gerrit)
Devtools-frontend LUCI CQ submitted the change![]( "Open in Gerrit")
Change information
[A11y] Updated lighthouse categories group
When navigate to the "performance" checkbox of the categories group
in the lighthouse panel, narrator announces
Before:
How long does this app take to show content and become usable group,
performance checkbox checked,
How long does this app take to show content and become usable
After:
Categories group,
performance checkbox checked,
How long does this app take to show content and become usable
- M front_end/panels/lighthouse/LighthouseStartView.ts
- M front_end/panels/lighthouse/lighthouseStartView.css
- M front_end/ui/legacy/Toolbar.ts
- M front_end/ui/legacy/UIUtils.ts
Code-Review: +1 by Simon Zünd, +1 by Kim-Anh Tran
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |