Conversation
eef3c2b to
0226fc2
Compare
client/src/global/components/Annotation/Annotation/TextContent/FromNodes/Wrapper.js
Show resolved
Hide resolved
7e7d963 to
222d409
Compare
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
7ad700f to
78050e1
Compare
cba23d9 to
1c16aae
Compare
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
8d7a495 to
34d00b7
Compare
|
@dananjohnson This PR is ready for review. There's one focus issue left, but the rest of the issues in this PR are ready |
dananjohnson
left a comment
There was a problem hiding this comment.
Thanks for addressing all these things, @jen-castiron! I noted a few things in my comments. There's also an issue with the flag modal that @1aurend should take a look at.
client/src/global/components/Annotation/Annotation/TextContent/FromNodes/Wrapper.js
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
@1aurend I'm seeing some issues with the focus trap for the flag modal.
- If you tab through to the Cancel button, tabbing again doesn't move focus any further. It just stays on Cancel.
- If you tab in reverse, the trap does correctly cycle through the dialog elements, but it also focuses on the
bodyelement as part of the loop.
There was a problem hiding this comment.
There are also some warnings in the console about missing required props, id and dialogId.
There was a problem hiding this comment.
I pushed a fix for the ids. It looked like the prop just got missed when the dialog was switched to the new global component. However, for the focus, the behavior I'm seeing matches what's in these discussions:
whatwg/html#8339
(https://github.com/w3c/wcag/issues/4185
It seems like you're expecting something different?
There was a problem hiding this comment.
Yeah, I don't think this is quite right, at least in Chrome and Safari. Firefox cycles focus between the interactive elements in the modal and the browser chrome. That seems consistent with modal dialogs on other sites that I checked. In Chrome and Safari, when focus moves out of the modal, it gets stuck on the body element and won't come back to the modal. Oddly, that's a bit different than what I saw when I first reported the issue. 🤔 Neither matches other implementations, though, or what I'd expect, which makes me wonder whether the underlying drawer's focus trap is interfering. Does it need to be paused? I can show you what I'm seeing at confab if it'd be helpful.
0798d80 to
05ca9f5
Compare
dananjohnson
left a comment
There was a problem hiding this comment.
Jen, everything is looking good. Once @1aurend addresses the flag modal issue, this PR should be good to go.
|
@jen-castiron @dananjohnson fixed my goof with the helper export. Sorry about that! I think reverting to not using a named export will be smoothest for rebasing resources. |
- Separates button from content - Adds focus to content when opened - Adds show/hide label - Wrap annotations in blockquotes, include cite tag
- Resolves issue where the active and hover states were too similar
- If there's only one entry in the list, the list should have role=presentation
-- Uses dialog popup (non-modal dialog) a11y
- Row 23, Audit 1
- Fixes issue where focus on drawer close might not be where users expect it - Some users may expect the focus to return to the Notes button, while some may want the focus to return to the annotation - This adds a 'go to passage' button that will close the drawer and focus on the annotation
Changes
All changes below were to the annotations drawer or annotation popup
[B] Add uuid to "Annotate this passage…" textarea (Row 30)
[B] Set annotation textarea id & label to uuid (Row 28)
[B] Fix collapsible content wrapper in annotation dialog (Row 32 & 48)
-- Separates button from content
-- Adds focus to content when opened
-- Adds show/hide label
-- Wrap annotations in blockquotes, include cite tag
[B] Change underlined annotation tag from span to mark (Row 48)
[B] Use bright hover and focus color on the annotation menu, add underline (Row 47)
-- Resolves issue where the active and hover states were too similar
[B] Switch report flag modal to native dialog; handle esc (lauren commit)
[B] Split into new global modal, fix up new dialog modal styles
[B] Fix list markup for annotation comments (Row 44)
-- If there's only one entry in the list, the list should have role=presentation
[B] Fix restore focus after removing a highlight
-- Cherry-picked from Lauren's work
[B] Fix a11y and return focus on edit annotation (Row 35)
-- Uses dialog popup (non-modal dialog) a11y
[B] Add role=group around options on annotation submenu
-- Row 23, Audit 1
[E] Add 'go to passage' to reader annotation drawer
-- Fixes issue where focus on drawer close might not be where users expect it
-- Some users may expect the focus to return to the Notes button, while some may want the focus to return to the annotation
-- This adds a 'go to passage' button that will close the drawer and focus on the annotation