feat: i shortcut on workspace gives overview#9677
feat: i shortcut on workspace gives overview#9677mikeharv wants to merge 3 commits intoRaspberryPiFoundation:v13from
Conversation
BenHenning
left a comment
There was a problem hiding this comment.
Thanks @mikeharv! Took a first pass and has some comments, PTAL.
Note that I'm not requesting changes in case you want to switch to a different reviewer since I'm actually out until next Wednesday.
|
|
||
| registerDefaultShortcuts(); | ||
| registerKeyboardNavigationShortcuts(); | ||
| registerScreenReaderShortcuts(); |
There was a problem hiding this comment.
I think we actually want to keep this out, for now. The idea is we'll use an optional plugin to enable these based on user prompting.
There was a problem hiding this comment.
Ah, okay. With this shortcut, I was thinking it reasonable to always include since it won't really do much if you're using a screen reader.
There was a problem hiding this comment.
I don't think this is quite true. You shouldn't need a plugin just to enable a feature that's already in core.
We haven't really fleshed out exactly what will be enabled by default yet, but I think everything that doesn't impact the experience for non-screenreader users should be enabled by default, and then a select few things are added via a settings toggle (the sounds as you navigate the workspace, disable looping are the ones I can think of). That way a screenreader user who doesn't discover the additional features still gets as much utility as possible.
That being said feel free to keep this method and not call it yet, we can call it or not later.
There was a problem hiding this comment.
The problem I see is that this still uses up the keyboard binding, and I’m assuming makes populating the keyboard navigation help menu a bit trickier since it presumably needs to account for only showing these when the “screen reader mode” thing is enabled. My thinking was gating the actual registration of the shortcuts makes both cases easier, but perhaps you’re thinking of this differently?
There was a problem hiding this comment.
I am writing a document now about what we'll do about default options, so we don't need to debate it in this PR :) I'll share it with you when ready.
packages/blockly/msg/json/en.json
Outdated
| "ARIA_WORKSPACE_BLOCKS_MANY": "%1 stacks of blocks%2 in workspace.", | ||
| "ARIA_WORKSPACE_BLOCKS_ONE": "One stack of blocks%2 in workspace.", | ||
| "ARIA_WORKSPACE_BLOCKS_ZERO": "No blocks%2 in workspace.", | ||
| "ARIA_WORKSPACE_COMMENTS_MANY": " and %1 comments", | ||
| "ARIA_WORKSPACE_COMMENTS_ONE": " and one comment" |
There was a problem hiding this comment.
I'm finding this a bit hard to grok, and I suspect a translator might as well. Perhaps we could be slightly repetitive to simplify things. What if we avoided the double concatenation and had four different variables for the label:
- "%1 stacks of blocks and %2 comments in workspace."
- "One stack of blocks and %2 comments in workspace."
- "%1 stacks of blocks and one comment in workspace."
- "One stack of blocks and one comment in workspace."
Separately, I'm not sure about the Msg name here. These aren't actual ARIA labels or properties, they're just strings that happen to be used as a status message to trick the screen reader into announcing them I do recognize that we will want to standardize on this, though, so perhaps @maribethb might have a suggestion. One thought might be to do something like: WORKSPACE_MANY_BLOCKS_AND_MANY_COMMENTS_SCREEN_READER_ANNOUNCEMENT_LABEL but I'm not sure sure just how specific we want to get with these.
There was a problem hiding this comment.
Yeah, I think going with the 4 separate messages is more straightforward for translators. I might add a 5th one which is "Workspace is empty" for that case. You mentioned you wrestled with a couple different options, what made you not choose this one?
Agree I wouldn't use Aria in the name, but I think we can come up with a shorter name than Ben's suggestion. The fact that it's primarily a label for screenreader announcement can be explained in the message comment, the same way we do for e.g. "Tooltip" and "Dropdown menu option". So I would suggest something like WORKSPACE_CONTENTS_MANY_BLOCKS_MANY_COMMENTS etc
There was a problem hiding this comment.
Actually, we discussed a bit more in real time and one of the things that the original proposal gives us that the 4 separate comments doesn't is the ability to leave off information about comments entirely. We actually don't include workspace comments by default in Blockly, as you have to enable the context menu item that lets you add them separately. Some products don't use them at all (e.g. code.org). So it would be confusing for those users to always be told that there are no workspace comments when they aren't even aware that those might exist.
So I think if we add more examples in the message description that the original design is workable. Also, Ben, had you been able to read the message descriptions when you commented here? Since you commented on en.js you don't see the descriptions in that file, but a translator would see them in the console.
There was a problem hiding this comment.
I admittedly hadn’t looked at the descriptions since I reviewed this a bit quickly and figured they would change if we changed the format itself.
I agree with the benefit of omitting comments though the current implementation doesn’t make this distinction. We could perhaps just not include the word ‘comments’ if there are zero to account for that case and still benefit from having the four simpler messages. Could that work?
Pedagogically I don’t think specifying 0 comments is actually all that useful since comments are very much a secondary or tertiary user flow/concept.
There was a problem hiding this comment.
Pedagogically I don’t think specifying 0 comments is actually all that useful since comments are very much a secondary or tertiary user flow/concept.
Agreed. That's actually the main motivator of the current implementation here.
The current implementation skips the phrase and n comments entirely if there aren't any. Whether there are comments or not, we still need to be able to report that there are zero/one/many stacks. There could also be zero/one/many stacks. (Note that it's advantageous to avoid grouping 'zero' with the plural version, because not all languages work that way.)
I believe these are the permutations we'd need to support, assuming we're aligned with dropping the comments part entirely when there are zero:
[0, 0]"Workspace is empty" (or "No blocks on workspace")[1, 0]"1 stack of blocks on workspace"[2+,0]"2 stack of blocks on workspace"[0, 1]"No blocks and 1 comment on workspace"[1, 1]1 stack of block and 1 comment on workspace"[2+,1]"2 stacks of blocks and 1 comment on workspace"[0, 2+]"No blocks and 2 comments on workspace"[1, 2+]"1 stack of blocks and 2 comments on workspace"[2+,2+]"2 stacks of blocks and 2 comments on workspace"
It's not clear to me how we could do this with four simpler messages.
There was a problem hiding this comment.
The current implementation does make this distinction, as you can leave off the "and x comments" message.
We could perhaps just not include the word ‘comments’ if there are zero to account for that case and still benefit from having the four simpler messages. Could that work?
That's what the current implementation does, but I'm not sure how you could do it with the messages you suggested earlier. Can you clarify?
edit: sorry did not see Mike's comment above when I wrote this
There was a problem hiding this comment.
Yeah you’re both right that this can’t be done easier. Well I’m certainly not going to be insistent on it being simpler since what we’ll be doing for block labels is going to be much more complex than this and translators hopefully will be able to figure it out. Splitting up seems reasonable.
The basics
The details
Resolves
Fixes #9622
Proposed Changes
This adds a new 'i' shortcut for workspaces. If the workspace is focused, the shortcut will trigger an announcement of the current total of block stacks and workspace comments. New translation strings were added to facilitate communicating the number of stacks and comments in a grammatically accurate way. The new strings were intentionally phrased to front-load the most important information (ie. actual quantity) over contextual description (ie. "on the workspace").
See tests for sample announcement strings created using the new messages.
Reason for Changes
From linked issue:
Test Coverage
The main new test performs a round trip through the different permutations of messages. The other test checks that the precondition logic works as expected (the workspace must be focused).
Documentation
N/A
Additional Information
I expect we'll want to make sure this new shortcut gets added to the keyboard navigation help menu/shortcuts list once that has been ported over.