[a11y][Glic] Add accessible labels to links in experimental opt-in dialog [chromium/src : main]
Min Qin (Gerrit)
Min Qin added 2 comments![]( "Open in Gerrit")
if (!safely_element.is_ok() || safely_element == base::Value()) {Returning DOM elements through `EvalJs` relies on V8's structured serialization, which yields an empty dictionary `{}` for DOM nodes. While comparing the result against `base::Value()` (which evaluates to type `NONE` for `null`) works as a null-check, it's safer and cleaner to evaluate a boolean expression over IPC.
Consider doing:
```cpp
if (EvalJs(guest_contents, "!!document.querySelector('a[href*=\"use-policy\"]')") != true) {
return false;
}
```
(The same applies to the other element existence checks below)
"//tools/typescript/definitions/tabs.d.ts",Is `tabs.d.ts` needed? It doesn't look like `chrome.tabs` is used in this file.
Related details
- David Trainor
- Rosie Yang
- Tommy Nyquist
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. |
Rosie Yang (Gerrit)
Rosie Yang added 2 comments![]( "Open in Gerrit")
if (!safely_element.is_ok() || safely_element == base::Value()) {Returning DOM elements through `EvalJs` relies on V8's structured serialization, which yields an empty dictionary `{}` for DOM nodes. While comparing the result against `base::Value()` (which evaluates to type `NONE` for `null`) works as a null-check, it's safer and cleaner to evaluate a boolean expression over IPC.
Consider doing:
```cpp
if (EvalJs(guest_contents, "!!document.querySelector('a[href*=\"use-policy\"]')") != true) {
return false;
}
```
(The same applies to the other element existence checks below)
Makes sense! Thanks! I have changed it to evaluate a boolean expression (!!document.querySelector(...)) directly in JS to make the check safer and avoid returning DOM nodes over IPC.
Is `tabs.d.ts` needed? It doesn't look like `chrome.tabs` is used in this file.
- David Trainor
- Min Qin
- Tommy Nyquist
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Tommy Nyquist (Gerrit)
Tommy Nyquist added 5 comments![]( "Open in Gerrit")
Reviewed with a focus on merging before translations are ready: structurally safe. All three labels are plain text with no placeholders, read via loadTimeData.getString and injected with JSON.stringify -> setAttribute('aria-label', ...), so there is no $N/substitution-count exposure, no span markers to go missing, and no sanitizer throw-path in any locale. Until translations land, non-English users will get English aria-labels over the server-localized link text — an acceptable interim given the flag doubles as a kill switch. Two landing-order coordination items and two Nits inline.
<message name="IDS_SETTINGS_GLIC_EXPERIMENTAL_TRIGGERING_CONSIDER_REVIEW_RISKS_LINK_LABEL" desc="Accessible name for the 'Review the risks' link in the Gemini Spark experimental triggering considerations list." is_accessibility_with_no_ui="true">Coordination: crrev.com/c/8104286 adds a message with this exact name (identical text, different desc) at the same anchor after CONSIDER_3. The second CL to land hits a grdp merge conflict, and a keep-both resolution would be a duplicate-name grit build failure. Worth agreeing which CL owns the definition; since the text is identical they would share one translation ID either way, which is the desired end state.
if (!safely_label.is_ok() || safely_label != "Use Gemini safely") {Nit: comparing against hard-coded English literals works on the en-US bots, but it will silently drift from the grdp on the next copy tweak and can't run locale-parameterized. Comparing against l10n_util::GetStringUTF16(IDS_...) would keep the test anchored to the message.
IDS_SETTINGS_GLIC_PERMISSIONS_WEB_ACTUATION_TOGGLE_CONSIDER_SAFELY_LINK_LABEL},Landing-order hazard: crrev.com/c/8117346 renames these two messages to IDS_SETTINGS_GLIC_PERMISSIONS_WEB_ACTUATION_TOGGLE_CONSIDER_SAFELY_ARIA_LABEL / ..._CONSIDER_UNEXPECTED_RESULTS_ARIA_LABEL (grdp-only; same text, so translations are preserved). Because the rename and this new consumer live in different files, git merges cleanly and whichever CL lands second only breaks at CQ compile time. If 8117346 goes in first, these references need to move to the _ARIA_LABEL names.
if (link.href.includes('use-policy')) {Nit: for an SVG <a> element, link.href is an SVGAnimatedString, so .includes() would throw and kill the rest of this MutationObserver batch. The Google-hosted page presumably has no SVG anchors today, but link.getAttribute('href') (or String(link.href)) would make this robust against a future page revision.
Related details
- David Trainor
- Min Qin
- Rosie Yang
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. |
Rosie Yang (Gerrit)
Rosie Yang added 4 comments![]( "Open in Gerrit")
<message name="IDS_SETTINGS_GLIC_EXPERIMENTAL_TRIGGERING_CONSIDER_REVIEW_RISKS_LINK_LABEL" desc="Accessible name for the 'Review the risks' link in the Gemini Spark experimental triggering considerations list." is_accessibility_with_no_ui="true">Coordination: crrev.com/c/8104286 adds a message with this exact name (identical text, different desc) at the same anchor after CONSIDER_3. The second CL to land hits a grdp merge conflict, and a keep-both resolution would be a duplicate-name grit build failure. Worth agreeing which CL owns the definition; since the text is identical they would share one translation ID either way, which is the desired end state.
Thanks for pointing this out! I will keep an eye on it and rebase to drop the duplicate definition if another CL lands first.
if (!safely_label.is_ok() || safely_label != "Use Gemini safely") {Nit: comparing against hard-coded English literals works on the en-US bots, but it will silently drift from the grdp on the next copy tweak and can't run locale-parameterized. Comparing against l10n_util::GetStringUTF16(IDS_...) would keep the test anchored to the message.
Done. I have included ui/base/l10n/l10n_util.h and updated the assertions to compare against l10n_util::GetStringUTF8(IDS_...) instead of hardcoded English strings
IDS_SETTINGS_GLIC_PERMISSIONS_WEB_ACTUATION_TOGGLE_CONSIDER_SAFELY_LINK_LABEL},Landing-order hazard: crrev.com/c/8117346 renames these two messages to IDS_SETTINGS_GLIC_PERMISSIONS_WEB_ACTUATION_TOGGLE_CONSIDER_SAFELY_ARIA_LABEL / ..._CONSIDER_UNEXPECTED_RESULTS_ARIA_LABEL (grdp-only; same text, so translations are preserved). Because the rename and this new consumer live in different files, git merges cleanly and whichever CL lands second only breaks at CQ compile time. If 8117346 goes in first, these references need to move to the _ARIA_LABEL names.
thanks for letting me know, will monitor it and rebase to rename the references to _ARIA_LABEL if it lands before this CL.
Nit: for an SVG <a> element, link.href is an SVGAnimatedString, so .includes() would throw and kill the rest of this MutationObserver batch. The Google-hosted page presumably has no SVG anchors today, but link.getAttribute('href') (or String(link.href)) would make this robust against a future page revision.
Sure, have changed to use link.getAttribute('href'), right now it returns a string (or null) for both HTMLAnchorElement and SVGAElement, avoiding the SVGAnimatedString TypeError.
Related details
- David Trainor
- Min Qin
- Tommy Nyquist
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Min Qin (Gerrit)
Min Qin voted Code-Review+1![]( "Open in Gerrit")
- David Trainor
- Rosie Yang
- Tommy Nyquist
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Tommy Nyquist (Gerrit)
Tommy Nyquist voted and added 1 comment![]( "Open in Gerrit")
Related details
- David Trainor
- Rosie Yang
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Min Qin (Gerrit)
Min Qin voted Code-Review+1![]( "Open in Gerrit")
- David Trainor
- Rosie Yang
- Tommy Nyquist
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Rosie Yang (Gerrit)
Rosie Yang voted Commit-Queue+2![]( "Open in Gerrit")
- David Trainor
- Tommy Nyquist
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Chromium LUCI CQ (Gerrit)
Chromium LUCI CQ submitted the change![]( "Open in Gerrit")
Change information
[a11y][Glic] Add accessible labels to links in experimental opt-in
dialog
* Add explicit aria-label attributes to the "safely",
"unexpected_results", and "Review the risks" links inside the
experimental opt-in dialog webview.
* Add a new localized accessible label string for "Review the risks"
to glic_strings.grdp.
* Pass the localized accessible label strings to the experimental
opt-in WebUI page.
* Inject a script using a MutationObserver into the webview on
content load to watch the DOM and apply aria-label attributes as
links are rendered/updated dynamically.
* Gate this accessibility fix behind a new feature flag
GlicOptInDialogLinkA11yFix, and register it in chrome://flags.
* Add a browser test case VerifyA11yLabelsInDialog in
glic_experimental_opt_in_dialog_browsertest.cc to verify that
link aria-labels are correctly applied to dynamic elements.
- M chrome/browser/about_flags.cc
- M chrome/browser/flag-metadata.json
- M chrome/browser/flag_descriptions.h
- M chrome/browser/glic/experimental_opt_in/glic_experimental_opt_in_dialog_browsertest.cc
- M chrome/browser/glic/experimental_opt_in/glic_experimental_opt_in_ui.cc
- M chrome/browser/glic/public/features.cc
- M chrome/browser/glic/public/features.h
- M chrome/browser/resources/glic/experimental_opt_in/BUILD.gn
- M chrome/browser/resources/glic/experimental_opt_in/experimental_opt_in.ts
- M chrome/common/chrome_features.cc
- M chrome/test/data/webui/glic/test_data/page.html
- M tools/metrics/histograms/enums.xml
Code-Review: +1 by Min Qin
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Rosie Yang (Gerrit)
Rosie Yang has created a revert of this change![]( "Open in Gerrit")
Related details
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Tommy Nyquist (Gerrit)
Tommy Nyquist added 2 comments![]( "Open in Gerrit")
Post-landing thought for a follow-up: now that the opt-in page (`https://gemini.google.com/glic/intro`) is content our team owns, these aria-labels are a much better fit in that page's own markup than injected from the Chrome client via `webview.executeScript`. A persistent `MutationObserver` patching a11y onto a page we control is easy to break silently (see the href-substring matching inline) and is invisible to anyone working on the intro page itself — the labels can't be maintained alongside the visible link text they describe.
Could we file a follow-up to add the three labels ("safely", "unexpected_results", "Review the risks") to the intro page markup directly, then remove this injection block and the `GlicOptInDialogLinkA11yFix` flag? The strings are currently pulled from Chrome's grdp; server-side they'd come from the intro page's own i18n. Happy to pair on it.
(For contrast, crrev.com/c/8114749 fixes the *dialog-level* a11y for this same surface natively in C++ — that's the right home for the pieces Chrome actually owns; the link labels are the piece the server owns.)
if (href.includes('use-policy')) {This href-substring matching (`use-policy` / `unexpected_results` / `gemini_spark_safety`) is coupled to server-configurable URL params — if any of those change, the labels silently drop with no failing test on the Chrome side. Since we own the intro page, authoring the aria-labels on the links in that page's markup (next to the visible text) avoids reconstructing them here from the URL entirely. See the patchset-level note.
Related details
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |