New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 708815 link

Starred by 9 users

Issue metadata

Status: Fixed
Owner:
Email to this user bounced
Closed: Jul 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux , Windows , Chrome , Mac
Pri: 3
Type: Bug

Blocked on:
issue 714860



Sign in to add a comment

Add count of bookmarks to "open all" menu commands

Project Member Reported by rpop@chromium.org, Apr 5 2017

Issue description

Follow-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.
 
04-open-all-bookmarks-mac.png
323 KB View Download

Comment 1 Deleted

Comment 2 by w...@chromium.org, Apr 7 2017

Owner: w...@chromium.org
Status: Assigned (was: Available)
Taking ownership on behalf of Anna, who will take a look at this.

Comment 3 by w...@chromium.org, Apr 11 2017

Cc: w...@chromium.org
Owner: paezagon@chromium.org
Status: Started (was: Assigned)
Project Member

Comment 5 by bugdroid1@chromium.org, 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

Cc: paezagon@chromium.org
Owner: rpop@chromium.org
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?
Cc: hwi@chromium.org srahim@chromium.org
+srahim@ +hwi@ for string guidance. 


Comment 8 by srahim@chromium.org, 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)".

Comment 9 by srahim@chromium.org, Apr 19 2017

Attaching the file referred to in #8
bookmarks-menu.png
69.8 KB View Download

Comment 10 by rpop@chromium.org, 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.

Comment 11 by hwi@chromium.org, Apr 19 2017

c8-10: oh my fault for overlooking the other bug. My apologies, Shimi. 

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!

Comment 13 by rpop@chromium.org, Apr 19 2017

Owner: paezagon@chromium.org
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.
Project Member

Comment 14 by bugdroid1@chromium.org, 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

Comment 15 by a...@chromium.org, Apr 24 2017

Blockedon: 714860

Comment 16 by sky@chromium.org, 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

Comment 17 by sky@chromium.org, May 22 2017

Cc: dmazz...@chromium.org sky@chromium.org
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.

Comment 19 by sky@chromium.org, May 25 2017

Sriham, could you comment on why you feel screen readers need more context than non-screen readers in this case?
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."
Cc: tsergeant@chromium.org
Labels: Proj-MaterialDesign-WebUI
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.

Comment 22 by sky@chromium.org, 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.
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.

Comment 24 by sky@chromium.org, 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?
Project Member

Comment 25 by bugdroid1@chromium.org, 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

Project Member

Comment 26 by bugdroid1@chromium.org, 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

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.
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.

Comment 29 by sky@chromium.org, 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.
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!
Cc: rbasuvula@chromium.org
Labels: TE-Verified-M61 TE-Verified-61.0.3141.0
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!
708815
325 KB View Download
Status: Fixed (was: Started)
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