[ntp-bookmarks] Replace bookmarks bar toggle with a dropdown in settings [chromium/src : main]
Jennifer Serrano (Gerrit)
Jennifer Serrano added 2 comments![]( "Open in Gerrit")
label="$i18n{showBookmarksBar}"i think we should map this label to the other dropdown menus and add a translation for "Bookmarks bar" and set it here and in line 180
Related details
- Bofeng Chen
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. |
Bofeng Chen (Gerrit)
Bofeng Chen added 1 comment![]( "Open in Gerrit")
i think we should map this label to the other dropdown menus and add a translation for "Bookmarks bar" and set it here and in line 180
- Jennifer Serrano
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
gwsq (Gerrit)
Message from gwsq![]( "Open in Gerrit")
From chrome/chromeos/assistant/assistive.gwsq:
The reviewer queue could not find an appropriate reviewer for this CL. Falling back to the team alias
Reviewer source(s):
xiaohuic is from group(xc-...@google.com)
Related details
- Jennifer Serrano
- Xiaohui Chen
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Tibor Goldschwendt (Gerrit)
Tibor Goldschwendt voted and added 1 comment![]( "Open in Gerrit")
Votes added by Tibor Goldschwendt
| Code-Review | +1 |
1 comment
Generally LGTM. But let's also loop somebody from //chrome/browser/resources/settings/OWNERS in
Related details
- Bofeng Chen
- Jennifer Serrano
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Lei Zhang (Gerrit)
Lei Zhang added 2 comments![]( "Open in Gerrit")
Generally LGTM. But let's also loop somebody from //chrome/browser/resources/settings/OWNERS in
I think you have ownership already?
`bookmarks::prefs::kBookmarkBarVisibilityState` preference. New
localized strings are added for these options, and the preference isUse active voice: "Add new localized strings for these options", then do the same for the rest of the paragraph.
Related details
- Bofeng Chen
- Jennifer Serrano
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. |
Bofeng Chen (Gerrit)
Bofeng Chen added 1 comment![]( "Open in Gerrit")
`bookmarks::prefs::kBookmarkBarVisibilityState` preference. New
localized strings are added for these options, and the preference isUse active voice: "Add new localized strings for these options", then do the same for the rest of the paragraph.
- Jennifer Serrano
- Lei Zhang
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Eshwar Stalin (Gerrit)
Eshwar Stalin voted Code-Review+1![]( "Open in Gerrit")
- Bofeng Chen
- Jennifer Serrano
- Lei Zhang
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Tibor Goldschwendt (Gerrit)
Tibor Goldschwendt voted and added 1 comment![]( "Open in Gerrit")
Votes added by Tibor Goldschwendt
| Code-Review | +1 |
Lei ZhangGenerally LGTM. But let's also loop somebody from //chrome/browser/resources/settings/OWNERS in
I think you have ownership already?
Yep via //chrome/OWNERS. But I'm not a dedicated owner of chrome://settings. So, I thought it would be good if somebody maintaining that page to take a look.
Related details
- Bofeng Chen
- Jennifer Serrano
- Lei Zhang
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. |
Lei Zhang (Gerrit)
Lei Zhang added 1 comment![]( "Open in Gerrit")
Lei ZhangGenerally LGTM. But let's also loop somebody from //chrome/browser/resources/settings/OWNERS in
Tibor GoldschwendtI think you have ownership already?
Yep via //chrome/OWNERS. But I'm not a dedicated owner of chrome://settings. So, I thought it would be good if somebody maintaining that page to take a look.
You want chrome/browser/resources/settings/OWNERS then, because I'm also in chrome/OWNERS.
Related details
- Bofeng Chen
- Jennifer Serrano
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. |
Lei Zhang (Gerrit)
Lei Zhang added 1 comment![]( "Open in Gerrit")
Lei ZhangGenerally LGTM. But let's also loop somebody from //chrome/browser/resources/settings/OWNERS in
Tibor GoldschwendtI think you have ownership already?
Lei ZhangYep via //chrome/OWNERS. But I'm not a dedicated owner of chrome://settings. So, I thought it would be good if somebody maintaining that page to take a look.
You want chrome/browser/resources/settings/OWNERS then, because I'm also in chrome/OWNERS.
- Bofeng Chen
- Demetrios Papadopoulos
- Jennifer Serrano
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Demetrios Papadopoulos (Gerrit)
Demetrios Papadopoulos added 6 comments![]( "Open in Gerrit")
suite('BookmarksBarVisibilitySettings', () => {Is a new suite() really needed? The suite below only consists of a single test() case. Could it be part of the suite above (and maybe also rename that suite to `AppearancePage`) ?
ntpSimplificationBookmarksBarEnabled: true,Do tests for the `false` case already exist in this file? If not can you add such a test?
setup(async () => {
loadTimeData.overrideValues({
ntpSimplificationBookmarksBarEnabled: true,
});
appearanceBrowserProxy = new TestAppearanceBrowserProxy();
AppearanceBrowserProxyImpl.setInstance(appearanceBrowserProxy);
createAppearancePage();
await microtasksFinished();
});```suggestion
setup(() => {
loadTimeData.overrideValues({
ntpSimplificationBookmarksBarEnabled: true,
});
appearanceBrowserProxy = new TestAppearanceBrowserProxy();
AppearanceBrowserProxyImpl.setInstance(appearanceBrowserProxy);
createAppearancePage();
return microtasksFinished();
});
```
teardown(function() {
appearancePage.remove();
});Unecessary. In createAppearancePage() the DOM is cleared anyway, no? Let's remove.
0, appearancePage.get('prefs.bookmark_bar.visibility_state.value'));Use getPref() from the PrefsMixin to directly access pref values in TypeScript.
(Here and elsewhere)
assertTrue(!!selectElement);This is unnecessary. Elements accessed via `$` are expected to always exist.
Related details
- Bofeng Chen
- Jennifer Serrano
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. |
Bofeng Chen (Gerrit)
Bofeng Chen added 7 comments![]( "Open in Gerrit")
suite('BookmarksBarVisibilitySettings', () => {Is a new suite() really needed? The suite below only consists of a single test() case. Could it be part of the suite above (and maybe also rename that suite to `AppearancePage`) ?
Thanks for the suggestion, removed the new suite and consolidate cases into the suite above, and, renamed it to `AppearancePage`.
Do tests for the `false` case already exist in this file? If not can you add such a test?
Added a test for the `false` (fallback) case.
setup(async () => {
loadTimeData.overrideValues({
ntpSimplificationBookmarksBarEnabled: true,
});
appearanceBrowserProxy = new TestAppearanceBrowserProxy();
AppearanceBrowserProxyImpl.setInstance(appearanceBrowserProxy);
createAppearancePage();
await microtasksFinished();
});```suggestion
setup(() => {
loadTimeData.overrideValues({
ntpSimplificationBookmarksBarEnabled: true,
});appearanceBrowserProxy = new TestAppearanceBrowserProxy();
AppearanceBrowserProxyImpl.setInstance(appearanceBrowserProxy);
createAppearancePage();
return microtasksFinished();
});
```
Acknowledged
teardown(function() {
appearancePage.remove();
});Unecessary. In createAppearancePage() the DOM is cleared anyway, no? Let's remove.
Done
0, appearancePage.get('prefs.bookmark_bar.visibility_state.value'));Use getPref() from the PrefsMixin to directly access pref values in TypeScript.
(Here and elsewhere)
Done
This is unnecessary. Elements accessed via `$` are expected to always exist.
- Demetrios Papadopoulos
- Jennifer Serrano
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |
Demetrios Papadopoulos (Gerrit)
Demetrios Papadopoulos added 4 comments![]( "Open in Gerrit")
This change introduces a new dropdown menu in the Appearance settings```suggestion
Introduce a new dropdown menu in the Appearance settings
```
functionality. Note: The mapping between the old and new bookmark
visibility pref is not set up.Is this done in a follow-up? Can you clarify?
<div class="flex cr-padded-text" aria-hidden="true">Why is this added?
{value: 0, name: loadTimeData.getString('bookmarksBarAlwaysShow')},Add a comment pointing out that these values need to match the C++ code for `BookmarkBarVisibilityState`. Mention the exact file.
Related details
- Bofeng Chen
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. |
Bofeng Chen (Gerrit)
Bofeng Chen added 4 comments![]( "Open in Gerrit")
This change introduces a new dropdown menu in the Appearance settings```suggestion
Introduce a new dropdown menu in the Appearance settings
```
Done
functionality. Note: The mapping between the old and new bookmark
visibility pref is not set up.Is this done in a follow-up? Can you clarify?
Yes, this will be done in a future CL. Link to the comment in another CL: https://chromium-review.git.corp.google.com/c/chromium/src/+/7871258/comment/3348b874_4c73a8f0/
<div class="flex cr-padded-text" aria-hidden="true">Why is this added?
I noticed `aria-hidden` is set to `true` for all dropdown menus in this page.
Below is explanation from Gemini:
`aria-hidden="true"` is required on the visible label <div> to prevent screen readers from announcing the label duplicate times.
Because settings-dropdown-menu passes its label attribute directly to `aria-label$="[[label]]"` on the interactive <select> element, a screen reader will announce "Bookmarks bar" when the user focuses the dropdown. If the visible label <div> did not have aria-hidden="true", screen readers navigating the page would read the static text "Bookmarks bar" immediately followed by the dropdown control also announcing "Bookmarks bar".
Hiding the static <div> from the accessibility tree ensures screen reader users experience a single, cleanly announced interactive control. This matches the established accessibility pattern used by every other dropdown setting on this page (such as tabStripPosition, defaultFontSize, and sidePanelPosition).
{value: 0, name: loadTimeData.getString('bookmarksBarAlwaysShow')},Add a comment pointing out that these values need to match the C++ code for `BookmarkBarVisibilityState`. Mention the exact file.
- Demetrios Papadopoulos
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. |
Demetrios Papadopoulos (Gerrit)
Demetrios Papadopoulos voted and added 3 comments![]( "Open in Gerrit")
Votes added by Demetrios Papadopoulos
| Code-Review | +1 |
3 comments
functionality. Note: The mapping between the old and new bookmark
visibility pref is not set up.Bofeng ChenIs this done in a follow-up? Can you clarify?
Yes, this will be done in a future CL. Link to the comment in another CL: https://chromium-review.git.corp.google.com/c/chromium/src/+/7871258/comment/3348b874_4c73a8f0/
Acknowledged
<div class="flex cr-padded-text" aria-hidden="true">Bofeng ChenWhy is this added?
I noticed `aria-hidden` is set to `true` for all dropdown menus in this page.
Below is explanation from Gemini:
`aria-hidden="true"` is required on the visible label <div> to prevent screen readers from announcing the label duplicate times.Because settings-dropdown-menu passes its label attribute directly to `aria-label$="[[label]]"` on the interactive <select> element, a screen reader will announce "Bookmarks bar" when the user focuses the dropdown. If the visible label <div> did not have aria-hidden="true", screen readers navigating the page would read the static text "Bookmarks bar" immediately followed by the dropdown control also announcing "Bookmarks bar".
Hiding the static <div> from the accessibility tree ensures screen reader users experience a single, cleanly announced interactive control. This matches the established accessibility pattern used by every other dropdown setting on this page (such as tabStripPosition, defaultFontSize, and sidePanelPosition).
Acknowledged
ntpSimplificationBookmarksBarEnabled: false,Reset this to a default value in setup() so that there is no state leaking between tests.
Related details
- Bofeng Chen
Bofeng Chen (Gerrit)
Bofeng Chen voted and added 1 comment![]( "Open in Gerrit")
Votes added by Bofeng Chen
| Commit-Queue | +1 |
1 comment
Reset this to a default value in setup() so that there is no state leaking between tests.
Code-Coverage
Code-Owners
Code-Review
Review-Enforcement
| Inspect html for hidden footers to help with email filtering. To unsubscribe visit settings. |