feat(message): enable component to be programmatically focused - FE-5725 #6033
Conversation
|
This pull request is automatically built and testable in CodeSandbox. To see build info of the built libraries, click here or the icon next to each commit SHA. Latest deployment of this branch, based on commit 5422eb2:
|
Passing run #16513 ↗︎
Details:
This comment has been generated by cypress-bot as a result of this project's GitHub integration settings. |
|||||||||||||||
DipperTheDan
force-pushed
the
FE-5725_focus-message-container
branch
from
df18f69 to
157a95e
Compare
| ) => { | ||
| const messageRef = useRef<HTMLDivElement | null>(null); | ||
| const focusedElementBeforeOpening = useRef<HTMLElement | null>(null); | ||
| const [tabIndex, setTabIndex] = useState<number | undefined>(0); |
There was a problem hiding this comment.
What do you think about turning this state variable into a boolean flag, then we can set the tabIndex on the message container accordingly?
That way it might be clearer that we only want the tabIndex of the container to change from 0 to undefined when the container is blurred.
There was a problem hiding this comment.
I had thought about that but we do it this way in Toast.
https://github.com/Sage/carbon/blob/master/src/components/toast/toast.component.tsx#L80
I figured to keep things consistent, I'd do it this way. Happy to change this though as I'm good with either way.
There was a problem hiding this comment.
I've left this as is for now until a second reviewer appears on the PR.
There was a problem hiding this comment.
I was the one who implemented this in Toast, as well as in Dialog and friends. Tbh it's a horrible hack - the tabindex should be -1 and never have to change (so that it's programatically focusable but not in the tab order), the issue there was that it messed up with Cypress tests (I forget exactly how). Hence the state to manually switch between tabindex being 0 (so it's focusable and doesn't cause Cypress to fall over) and undefined (so the container isn't in the tab order if the message/toast sticks around on the screen).
A boolean state is probably a better way of doing this, that I didn't think of at the time 😀 But consistency is good (I initially extracted this to a custom hook but then was persuaded it wasn't worth it at the time, maybe it is now we've got a third component using this), perhaps this is an update we should make to all components at once at some future date?
[PS I am about to start a review but thought I'd comment on this separately first.]
There was a problem hiding this comment.
In fact, see my overall comment but having played with this PR a bit I'm now strongly of the opinion that we should just put tabindex="-1" on the container here and not bother with any state. The difference is that toasts/dialogs etc. are always launched in response to some user action (at least I hope so - certainly this is the usual case), whereas a Message might conceivably be on the screen anyway, or multiples might be, or at least this is the case in Storybook (which I know doesn't always simulate a real application very well!).
There was a problem hiding this comment.
So I'm not sure about the whole approach, I'm afraid - mostly because on the storybook docs where we have loads of open messages already, they all get tabIndex=0 and allow tabbing onto them, until you've blurred them. I think this is one case where the hack with the tabindex of 0 which changes to undefined on blur just causes problems and we should just add a tabindex of -1 to the container, and cope with anything that goes wrong in Cypress. (In hindsight, I probably shouldn't have used such a hacky implementation in other components, that potentially affects the user experience negatively, when it was only done to make test scripts happy...)
| const l = useLocale(); | ||
|
|
||
| let refToPass = messageRef; | ||
| if (ref && typeof ref === "object" && "current" in ref) { |
There was a problem hiding this comment.
I don't think this guard is needed, or wanted - it means that if consumers pass a callback ref then it will do absolutely nothing.
I think simply const refToPass = ref || messageRef; instead of all this seems to work fine to me (at least TS doesn't complain, I haven't tested it 😁 ) - if you do end up needing a type for refToPass it should be something like React.Ref<HTMLDivElement> as that accepts both objects with a current property and callback refs.
| ) => { | ||
| const messageRef = useRef<HTMLDivElement | null>(null); | ||
| const focusedElementBeforeOpening = useRef<HTMLElement | null>(null); | ||
| const [tabIndex, setTabIndex] = useState<number | undefined>(0); |
There was a problem hiding this comment.
In fact, see my overall comment but having played with this PR a bit I'm now strongly of the opinion that we should just put tabindex="-1" on the container here and not bother with any state. The difference is that toasts/dialogs etc. are always launched in response to some user action (at least I hope so - certainly this is the usual case), whereas a Message might conceivably be on the screen anyway, or multiples might be, or at least this is the case in Storybook (which I know doesn't always simulate a real application very well!).
|
|
||
| beforeEach(() => { | ||
| const element = document.createElement("div"); | ||
| const htmlElement = document.body.appendChild(element); |
There was a problem hiding this comment.
do we need the appendChild stuff here? Fair enough if we do, I know we use it elsewhere but I've never fully understood why it's sometimes needed, and I'd rather do without if we can. If it doesn't work without, feel free to leave it in.
| }); | ||
|
|
||
| afterEach(() => { | ||
| wrapper.unmount(); |
There was a problem hiding this comment.
we'll need to remove element here to clean up (which of course will mean moving it to a higher scope than the beforeEach)
| ); | ||
| }; | ||
|
|
||
| export const WithRef: ComponentStory<typeof Message> = () => { |
There was a problem hiding this comment.
At the moment, although this code demonstrates how to pass refs, it doesn't actually do anything with those refs, so doesn't show a use-case for using them. If we're sure we want to allow this (because it only occurs to me now that the original request for this was to allow consumers to focus the container manually, this might not be needed now we're doing it automatically on open?), then we should demonstrate this - perhaps with buttons that focus the containers (not that we have visible focus styles, so I dunno, maybe change their color or something? Go wild, really, it doesn't have to be sensible, just something that shows the use of a ref to do something you couldn't easily do without. Am open to ideas as there's nothing really obvious...)
There was a problem hiding this comment.
this might not be needed now we're doing it automatically on open?
In my opinion I think giving consumers the focus control to consumers would be better, as there might be a case where they might want to programmatically refocus on the Message that has previously been opened
DipperTheDan
force-pushed
the
FE-5725_focus-message-container
branch
4 times, most recently
from
ed9e7fd to
db8d37c
Compare
DipperTheDan
changed the title
DipperTheDan
force-pushed
the
FE-5725_focus-message-container
branch
2 times, most recently
from
5cb9f28 to
444f06e
Compare
| ); | ||
| }; | ||
|
|
||
| export const WithRef: ComponentStory<typeof Message> = () => { |
There was a problem hiding this comment.
this might not be needed now we're doing it automatically on open?
In my opinion I think giving consumers the focus control to consumers would be better, as there might be a case where they might want to programmatically refocus on the Message that has previously been opened
There was a problem hiding this comment.
Happy enough with this other than the minor things I've replied to.
Just wondering though if it's the right approach to just give consumers the ref and leave it up to them to discover and use this functionality - I feel like much of the time it will be appropriate to have these automatically focus when opening, and we should offer a boolean prop to automatically enable that behaviour (or make it the default and offer a prop to turn it off)?
I realise this would mean a fair rewrite at this stage, maybe this is best addressed in the upcoming work on this component regarding it being a live region - but I wanted to mention it anyway.
DipperTheDan
force-pushed
the
FE-5725_focus-message-container
branch
2 times, most recently
from
94802c9 to
5bc324c
Compare
DipperTheDan
force-pushed
the
FE-5725_focus-message-container
branch
from
5bc324c to
6d8e41b
Compare
To improve the accessibility of this component, the Message component should be focusable. fixes #5924
DipperTheDan
force-pushed
the
FE-5725_focus-message-container
branch
from
6d8e41b to
32140bd
Compare
|
🎉 This PR is included in version 119.2.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
Parsium
changed the title
fixes #5924
Proposed behaviour
Add a ref so that the
Messagecomponent can be focused.Current behaviour
Component is not currently focusable.
Checklist
d.tsfile added or updated if requiredQA
Additional context
Inconsistent screenreader behaviour will not be addressed in this ticket. This work is to provide a mechanism for consumers to focus the
Messagecomponent.Testing instructions
with focusexample: Message component should be focused when first opened and not display an outline.with focusexample: Screenreader should read out the message in the component.