[a11y][clarity] Addressing launch a11y feedback for anchored messages [chromium/src : main]
Kaan Alsan (Gerrit)
Kaan Alsan added 3 comments![]( "Open in Gerrit")
void AnchoredMessageBubbleView::OnKeyEvent(ui::KeyEvent* event) {Not sure if this analysis is correct, but worth double checking:
Please fix this WARNING reported by autoreview issue finding: Is this `OnKeyEvent` override actually needed in production code?\n\nTypically, when a `views::MenuRunner` is open, the menu's own widget/controller receives and handles the `Escape` key to close the menu, so the bubble view shouldn't receive it.\n\nIf this was added because the `FocusRestoredToMenuButtonAfterMenuDismissed` unit test was failing to close the menu on `Escape`, the issue in the test is likely that the `ui::test::EventGenerator` is constructed using the bubble's root window (`views::GetRootWindow(view->GetWidget())`). This causes the simulated `Escape` event to bypass the menu's popup widget and go straight to the bubble view.
tooltip_text += u" - " + anchored_message_showing;This assumes all locales use this sentence structure, which may not be the case for all languages. We should define a new message in the .grd file, leveraging placeholders.
go/concat-considered-harmful
kTestText + u" - Anchored message showing");Let's use resource ID so that this doesn't fail when changing the text or running the test using a different locale.
Related details
- Dmytro Yeroshkin
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Dmytro Yeroshkin (Gerrit)
Dmytro Yeroshkin added 1 comment![]( "Open in Gerrit")
void AnchoredMessageBubbleView::OnKeyEvent(ui::KeyEvent* event) {Not sure if this analysis is correct, but worth double checking:
Please fix this WARNING reported by autoreview issue finding: Is this `OnKeyEvent` override actually needed in production code?\n\nTypically, when a `views::MenuRunner` is open, the menu's own widget/controller receives and handles the `Escape` key to close the menu, so the bubble view shouldn't receive it.\n\nIf this was added because the `FocusRestoredToMenuButtonAfterMenuDismissed` unit test was failing to close the menu on `Escape`, the issue in the test is likely that the `ui::test::EventGenerator` is constructed using the bubble's root window (`views::GetRootWindow(view->GetWidget())`). This causes the simulated `Escape` event to bypass the menu's popup widget and go straight to the bubble view.
The test was passing even without this. This was done to address a weird behaviour in a local build when testing the initial simpler fix: escaping out of the menu would close it, focus would go to the menu button for a fraction of a second, then move away from it.
I'm very much open to other idea for how to fix this.
Related details
- Kaan Alsan
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Kaan Alsan (Gerrit)
Kaan Alsan added 1 comment![]( "Open in Gerrit")
void AnchoredMessageBubbleView::OnKeyEvent(ui::KeyEvent* event) {Dmytro YeroshkinNot sure if this analysis is correct, but worth double checking:
Please fix this WARNING reported by autoreview issue finding: Is this `OnKeyEvent` override actually needed in production code?\n\nTypically, when a `views::MenuRunner` is open, the menu's own widget/controller receives and handles the `Escape` key to close the menu, so the bubble view shouldn't receive it.\n\nIf this was added because the `FocusRestoredToMenuButtonAfterMenuDismissed` unit test was failing to close the menu on `Escape`, the issue in the test is likely that the `ui::test::EventGenerator` is constructed using the bubble's root window (`views::GetRootWindow(view->GetWidget())`). This causes the simulated `Escape` event to bypass the menu's popup widget and go straight to the bubble view.
The test was passing even without this. This was done to address a weird behaviour in a local build when testing the initial simpler fix: escaping out of the menu would close it, focus would go to the menu button for a fraction of a second, then move away from it.
I'm very much open to other idea for how to fix this.
Acknowledged - In that case I don't think it's a big deal to leave this override
Related details
- Dmytro Yeroshkin
Code-Coverage
Code-Owners
Code-Review
No-Unresolved-Comments
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Dmytro Yeroshkin (Gerrit)
Dmytro Yeroshkin added 2 comments![]( "Open in Gerrit")
This assumes all locales use this sentence structure, which may not be the case for all languages. We should define a new message in the .grd file, leveraging placeholders.
go/concat-considered-harmful
Done. I kept the existing formatting for the tooltip text `{page action tooltip} - Anchored message showing`, but now it's using a formatted grd string.
Let's use resource ID so that this doesn't fail when changing the text or running the test using a different locale.
- Kaan Alsan
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |