Add count of bookmarks to "open all" menu commands |
||||||||||||
Issue descriptionFollow-on from Issue 708813 : Remove the word "bookmarks" and add a count of URLs to be opened to the context menus in bookmarks bar and bookmarks manager. This applies to all desktop platforms. See top image in attachment.
,
Apr 7 2017
Taking ownership on behalf of Anna, who will take a look at this.
,
Apr 11 2017
,
Apr 11 2017
,
Apr 17 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/ae360640e5b0475e52f8cee25d755ce484faf6e4 commit ae360640e5b0475e52f8cee25d755ce484faf6e4 Author: paezagon <paezagon@chromium.org> Date: Mon Apr 17 17:49:09 2017 Working on making context menu display the number of bookmarks that will be opened by by Open All. Modified the strings to change based on a passed in count. Modified bookmark_context_menu_controller and bookmark_menu_bridge to pass in the count of bookmarks that will be opened. Added a helper function in bookmark_utils_desktop to determine the count. BUG= 708815 Review-Url: https://codereview.chromium.org/2809003002 Cr-Commit-Position: refs/heads/master@{#464949} [modify] https://crrev.com/ae360640e5b0475e52f8cee25d755ce484faf6e4/chrome/app/bookmarks_strings.grdp [modify] https://crrev.com/ae360640e5b0475e52f8cee25d755ce484faf6e4/chrome/browser/ui/bookmarks/bookmark_context_menu_controller.cc [modify] https://crrev.com/ae360640e5b0475e52f8cee25d755ce484faf6e4/chrome/browser/ui/bookmarks/bookmark_context_menu_controller.h [modify] https://crrev.com/ae360640e5b0475e52f8cee25d755ce484faf6e4/chrome/browser/ui/bookmarks/bookmark_utils_desktop.cc [modify] https://crrev.com/ae360640e5b0475e52f8cee25d755ce484faf6e4/chrome/browser/ui/bookmarks/bookmark_utils_desktop.h [modify] https://crrev.com/ae360640e5b0475e52f8cee25d755ce484faf6e4/chrome/browser/ui/cocoa/bookmarks/bookmark_menu_bridge.mm [modify] https://crrev.com/ae360640e5b0475e52f8cee25d755ce484faf6e4/chrome/browser/ui/cocoa/bookmarks/bookmark_menu_bridge_unittest.mm [modify] https://crrev.com/ae360640e5b0475e52f8cee25d755ce484faf6e4/chrome/browser/ui/cocoa/bookmarks/bookmark_menu_cocoa_controller.mm [modify] https://crrev.com/ae360640e5b0475e52f8cee25d755ce484faf6e4/chrome/browser/ui/views/bookmarks/bookmark_bar_view_test.cc [modify] https://crrev.com/ae360640e5b0475e52f8cee25d755ce484faf6e4/chrome/browser/ui/views/bookmarks/bookmark_context_menu_unittest.cc
,
Apr 17 2017
Submitted patch to add the count to the context menu. Have left in the word Bookmarks because in the case where there is a single bookmark, "Open Bookmark" is clearer than "Open" or "Open All (1)". Should "Bookmarks" still be removed? Should it possibly be left in for the case of a single bookmark?
,
Apr 17 2017
+srahim@ +hwi@ for string guidance.
,
Apr 19 2017
Actually, I would recommend keeping the noun "Bookmarks" even when there is >1. The first reason is that I chatted w/Hwi, and it sounds like the reason for this change was to make it clearer what "Open All Bookmarks" refers to. But I don't think the string is the problem. No matter what the string says, the commands are separated visually from the bookmarks by a horizontal rule. Did you consider moving the commands so that they are more closely connected to the objects they apply to? See attached. The other reason I'd keep the noun "Bookmarks" in all cases is for accessibility. Omitting the object that a command applies to is disconcerting for the screen reader user - they rely on clear strings to orient them, as it's so easy for focus to accidentally be moved elsewhere. "Open All (5)" doesn't tell the a11y user what's going to be opened or even that they are still navigating in a list of Bookmarks. All that said, if you move forward, I agree with paezagon@'s decision in #6 to keep the word "Bookmark" for one bookmark rather than "Open All (1)".
,
Apr 19 2017
Attaching the file referred to in #8
,
Apr 19 2017
#8: we're removing these commands from the Mac bookmarks menu as part of Issue 708813 and I'll fwd you the thread where that was discussed. Mac is the only platform that has these, and it's odd to put commands in the middle of the hierarchy of bookmarks. So this change to show counts will only apply to the commands in bookmarks bar folder -> context click and bookmarks manager -> context click. In both cases, I think it's clear that you're acting on a group of bookmarks. Let me know if you disagree, especially around the screenreader use case.
,
Apr 19 2017
c8-10: oh my fault for overlooking the other bug. My apologies, Shimi.
,
Apr 19 2017
Thanks for the background thread. Removing those commands alleviates my first concern in c#8. :-) For the a11y issue: I just tested navigating through the bookmarks bar folder in VoiceOver. With a long list of bookmarks in a folder, I still believe the user needs the word "bookmarks" in the "Open" command to orient them and clarify the action. It is possible to implement a custom screenreader string that includes the word "bookmarks" so you can remove it from the visual UI. I've told in the past that it's not easy to implement, though. But if the team wants to move forward with removing the word "bookmarks," I ask that we at least look at adding a custom screenreader string that includes "bookmarks."Thanks!
,
Apr 19 2017
paezagon, we're ready for you again. Please use "Open Bookmark" for the case where there's 1 bookmark. Please also investigate a custom screenreader string per #12.
,
Apr 20 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/c32cfc7c2ca9fbc0c289dba04d7878ea8a65d7e9 commit c32cfc7c2ca9fbc0c289dba04d7878ea8a65d7e9 Author: paezagon <paezagon@chromium.org> Date: Thu Apr 20 17:16:13 2017 Fixing display issue in bookmark manager. Splitting bookmark strings into two sets, the original strings and the new strings, because the original strings are still needed for the bookmark manager until how to get the pluralization to work is figured out. BUG= 708815 , 712995 Review-Url: https://codereview.chromium.org/2828873003 Cr-Commit-Position: refs/heads/master@{#466038} [modify] https://crrev.com/c32cfc7c2ca9fbc0c289dba04d7878ea8a65d7e9/chrome/app/bookmarks_strings.grdp [modify] https://crrev.com/c32cfc7c2ca9fbc0c289dba04d7878ea8a65d7e9/chrome/browser/ui/bookmarks/bookmark_context_menu_controller.cc [modify] https://crrev.com/c32cfc7c2ca9fbc0c289dba04d7878ea8a65d7e9/chrome/browser/ui/cocoa/bookmarks/bookmark_menu_bridge.mm [modify] https://crrev.com/c32cfc7c2ca9fbc0c289dba04d7878ea8a65d7e9/chrome/browser/ui/cocoa/bookmarks/bookmark_menu_bridge_unittest.mm
,
Apr 24 2017
,
May 22 2017
It seems surprising to me that we would consider adding different verbage for screen readers. Sriham, you say that we need 'bookmarks' for screen readers 'to orient them and clarify the action.' Why is that argument different for screen reader vs non-screen reader? +dmazzoni
,
May 22 2017
,
May 22 2017
I don't have a strong opinion on this particular change, that's a UI decision and judgement call. Bookmarks is somewhat implied by the context, but I can totally see users getting confused by "Open all (7)" so I like the idea of the accessible name being "Open all 7 bookmarks" instead. It's not unusual to have a slightly more descriptive accessible name for a UI element than its visual text. This is especially true when the context or relationship between UI elements is conveyed by spatial relationships between things. For a lot of UI elements, the tooltip works well as the accessible name. I don't think we have the option of tooltips for menu items on all platforms, but I think all platforms let us override the accessible name for menu items as needed.
,
May 25 2017
Sriham, could you comment on why you feel screen readers need more context than non-screen readers in this case?
,
Jun 5 2017
In the visual UX, the user can see that a bookmarks bar folder is a container for 1+ bookmarks. The hierarchy is visible. In the Mac screen reader experience, the hierarchy is not visible: VoiceOver verbalizes the folder only as "FolderName button," with no indication that the button is a container for 1+ bookmarks. Adding "bookmarks" to the right-click menu item helps the user understand *what* will open. Otherwise, it may not be clear what "open all (7)" refers to, i.e. all bookmarks in the right-clicked folder. Bookmarks bar is not a primary user task, which is why this request for custom screenreader text is not a P0. It's more an attempt to avoid introducing a usability problem that did not exist when the menu item included the noun "bookmarks."
,
Jun 6 2017
We came to consensus over email that we will only implement this in the Material Design bookmark manager (chrome://flags/#enable-md-bookmarks), and not in the old non-MD manager. Adding the relevant tags so that this issue is tracked as part of the MD work.
,
Jun 7 2017
tsergeant & srahim, could you please clarify what surface you are saying should get the behavior? The bookmark manager, or the bookmark bar? Srahim, you say "the hierarchy is visible," that isn't true of the bookmark bar, only an icon is shown. The user has no visual indication as to the depth of the tree.
,
Jun 13 2017
sky@, I was referring to the bookmark bar. By "the hierarchy is visible," I meant that in the bookmarks bar, the folder icon indicates that there is a container. You are correct that the depth of the tree is not indicated.
,
Jun 20 2017
srahim, don't screen readers differentiate between the two types? By that I mean wouldn't a screen reader read the folder name differently than the url type? In which case users using a screen reader and not using a screen reader have the exact same information?
,
Jun 21 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/64e2012a9e0278ff735674d16db1c44caefc91ae commit 64e2012a9e0278ff735674d16db1c44caefc91ae Author: calamity <calamity@chromium.org> Date: Wed Jun 21 09:57:14 2017 [MD Bookmarks] Add bookmark count to context menu for multiple items. This CL adds the bookmark count to the end of the 'Open in new tab' option for the context menu when multiple bookmarks will be opened. BUG= 708815 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Review-Url: https://codereview.chromium.org/2924253002 Cr-Commit-Position: refs/heads/master@{#481161} [modify] https://crrev.com/64e2012a9e0278ff735674d16db1c44caefc91ae/chrome/browser/resources/md_bookmarks/command_manager.html [modify] https://crrev.com/64e2012a9e0278ff735674d16db1c44caefc91ae/chrome/browser/resources/md_bookmarks/command_manager.js
,
Jun 21 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/64e2012a9e0278ff735674d16db1c44caefc91ae commit 64e2012a9e0278ff735674d16db1c44caefc91ae Author: calamity <calamity@chromium.org> Date: Wed Jun 21 09:57:14 2017 [MD Bookmarks] Add bookmark count to context menu for multiple items. This CL adds the bookmark count to the end of the 'Open in new tab' option for the context menu when multiple bookmarks will be opened. BUG= 708815 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:closure_compilation Review-Url: https://codereview.chromium.org/2924253002 Cr-Commit-Position: refs/heads/master@{#481161} [modify] https://crrev.com/64e2012a9e0278ff735674d16db1c44caefc91ae/chrome/browser/resources/md_bookmarks/command_manager.html [modify] https://crrev.com/64e2012a9e0278ff735674d16db1c44caefc91ae/chrome/browser/resources/md_bookmarks/command_manager.js
,
Jun 21 2017
Re: c#24, I only tested on Mac, and no, VoiceOver did not verbalize the difference between a folder and an item in a folder.
,
Jun 21 2017
It sounds like we should *also* describe bookmark folders differently from bookmark items in the bookmarks bar. That's a great idea, and definitely an oversight. In the Chrome menu from the toolbar, or the top-level Bookmarks menu on Mac, they are distinguished, because folders have submenus and urls don't. But in the bar we don't have that distinction. I filed crbug.com/735607 for that. Maybe that's a better fix? I don't think making the accessible name for "Open All" slightly more verbose is a *bad* idea, but I'm okay with skipping it if we think labeling the bookmark bar button is a more important fix.
,
Jun 21 2017
I would prefer not to add the complexity if we don't need it. It sounds to me like 735607 is the right fix.
,
Jun 22 2017
Ah, I wasn't aware that Chrome can change how VoiceOver reads folders and folder contents; I thought the lack of distinction was due to how VoiceOver handles the items we send them. So if we can fix the verbalization of hierarchy on our side, I agree that's preferable, because then we won't have to go through this again if another string is changed. ;-) Thanks, Dominic!
,
Jun 27 2017
Tested the issue on Windows-10, Ubuntu 14.04 and Mac OS 10.12.5 using chrome latest Dev M61-61.0.3141.0 by following steps mentioned in the original comment. Observed that "bookmarks" word removed and count of URLs are displaying as expected. Hence adding TE-Verified label. Note:Please find the screen shot for reference Thank you!
,
Jul 12 2017
Closing this bug as it has been verified, work appears to be complete, and followup work is being tracked in Issue 735607 |
||||||||||||
►
Sign in to add a comment |
||||||||||||
Comment 1 Deleted