-
Notifications
You must be signed in to change notification settings - Fork 7
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[접근성개선] 스크린 리더가 태그 등록 메시지 안내 #727
Changes from 1 commit
bb91b6a
fcd4aa7
df0f3e6
bbf8693
0bcc07b
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,21 @@ | ||
const ScreenReader = () => ( | ||
<div | ||
id='screen-reader' | ||
aria-live='polite' | ||
aria-hidden='true' | ||
style={{ | ||
position: 'absolute', | ||
width: '1px', | ||
height: '1px', | ||
margin: '-1px', | ||
padding: '0', | ||
border: '0', | ||
overflow: 'hidden', | ||
clip: 'rect(0, 0, 0, 0)', | ||
whiteSpace: 'nowrap', | ||
wordWrap: 'normal', | ||
}} | ||
/> | ||
); | ||
|
||
export default ScreenReader; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,36 +1,19 @@ | ||
import { useMemo, useRef, CSSProperties } from 'react'; | ||
import { useCallback } from 'react'; | ||
|
||
export const useScreenReader = () => { | ||
const elementRef = useRef<HTMLDivElement | null>(null); | ||
const updateScreenReaderMessage = useCallback((message: string) => { | ||
const element = document.getElementById('screen-reader'); | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 한 페이지에 screen-reader 라는 id를 가진 DOM요소가 한 개 이상 있다면 메시지가 중복으로 읽히지 않을까라는 우려가 들긴 하네요..! 혹시 해당 부분 테스트가 완료 되었나요?! There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 한 화면에 동일한 id를 갖는 요소는 존재할 수 없으니, 개발자의 실수로 생성했다면 에러가 나지 않을까 생각합니당 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. document.getElementById() 메소드는 첫 번째로 찾은 요소만 반환하기 때문에 에러가 날 일은 없을 듯 싶습니다..! 다만 원하는대로 동작하게 하기 위해서는 올바르게 덧붙여 혹시 조금 더 리액트스럽게 ref 를 사용해보는 방향은 어떨까요? |
||
|
||
const updateScreenReaderMessage = (message: string) => { | ||
if (elementRef.current) { | ||
elementRef.current.textContent = message; | ||
} | ||
}; | ||
|
||
const visuallyHiddenStyle: CSSProperties = { | ||
position: 'absolute', | ||
width: '1px', | ||
height: '1px', | ||
margin: '-1px', | ||
padding: '0', | ||
border: '0', | ||
overflow: 'hidden', | ||
clip: 'rect(0, 0, 0, 0)', | ||
whiteSpace: 'nowrap', | ||
wordWrap: 'normal', | ||
}; | ||
if (element) { | ||
element.setAttribute('aria-hidden', 'false'); | ||
element.textContent = message; | ||
|
||
const visuallyHiddenProps = useMemo( | ||
() => ({ | ||
ref: elementRef, | ||
style: visuallyHiddenStyle, | ||
'aria-live': 'polite' as const, | ||
'aria-hidden': false, | ||
}), | ||
[], | ||
); | ||
setTimeout(() => { | ||
element.setAttribute('aria-hidden', 'true'); | ||
element.textContent = ''; | ||
}); | ||
} | ||
}, []); | ||
|
||
return { visuallyHiddenProps, updateScreenReaderMessage }; | ||
return { updateScreenReaderMessage }; | ||
}; |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -7,6 +7,7 @@ import { RouterProvider } from 'react-router-dom'; | |
|
||
import { AuthProvider, HeaderProvider, ToastProvider } from '@/contexts'; | ||
|
||
import { ScreenReader } from './components/index'; | ||
import router from './routes/router'; | ||
import GlobalStyles from './style/GlobalStyles'; | ||
import { theme } from './style/theme'; | ||
|
@@ -49,6 +50,7 @@ enableMocking().then(() => { | |
<HeaderProvider> | ||
<GlobalStyles /> | ||
<RouterProvider router={router} /> | ||
<ScreenReader /> | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 오호... 혹시 스크린리더 컴포넌트를 최상위에 둔 이유가 있나요? 현재 월하의 리펙토링을 보면 최상위에 마치 Toast 처럼 스크린리더 내용을 바꿔껴준다는 생각이 듭니다. 이런 방향은 생각하지 못한 방향이어서 굉장히 신선하네요! 다만 이렇게 전역적으로 사용하였을 때, 유의해야 할 점이 어떤게 있을지 함께 생각해봐도 좋을 것 같아요. 일단 제가 가장 먼저 생각나는 것은
등등 하나의 상태로 관리해도 우리가 원하는대로 잘 동작하는지, 이렇게 스크린 리더가 읽어줘야 할 부분들을 전역으로 관리하는 것이 괜찮을지 함께 이야기 나눠봐도 좋을 것 같아요!! 그런데 다시 한 번 말하지만 이렇게 전역에서 두는 것도 굉장히 신선한 아이디어네요!! 각 컴포넌트 내부에 별도의 태그들을 만들지 않아도 되는 점이 좋은 것 같아요 👍👍 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
전역 상태로 관리한 이유는 스크린 리더를 사용하는 컴포넌트마다 요소를 렌더링해야 하는데, 전역에서 관리하면 이를 회피할 수 있고, RenderProvider의 리렌더링에 영향을 미치지 않기 때문에 성능적으로도 문제 없다고 생각했기 때문입니다.
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ref를 사용하는 방법도 생각해보았는데, 전역으로 관리한다면 Ref를 context 없이 사용하기 어렵다고 생각하여서 이렇게 구현하였습니다ㅠㅠ 이 부분에 대해서 고견 듣고 싶습니다.. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ref 를 쓰게되면 아마 context 가 또 생기거나, 아니면 인자로 받아서 써야할텐데... 후자처럼 useScreenReader 훅에서 ref를 받아 메시지를 업데이트하는 방식으로 바꾸려면 아래와 같이 사용할 수 있다고 하네요..! (feat. 지선생님) import React, { forwardRef } from 'react';
const ScreenReader = forwardRef((props, ref) => (
<div
ref={ref}
id='screen-reader'
aria-live='polite'
aria-hidden='true'
style={{
position: 'absolute',
width: '1px',
height: '1px',
margin: '-1px',
padding: '0',
border: '0',
overflow: 'hidden',
clip: 'rect(0, 0, 0, 0)',
whiteSpace: 'nowrap',
wordWrap: 'normal',
}}
/>
));
export default ScreenReader; const screenReaderRef = useRef();
const { updateScreenReaderMessage } = useScreenReader(screenReaderRef); 그런데 이렇게 ref를 매번 생성해서 전달해주는 부분이 불필요하게 느껴진다는 점에서 좋은 변화는 아니라는 생각도 듭니다...😂😂 저도 컨텍스트를 만들어서 쓰는게 아니라면 더 좋은 생각이 나지 않네요... |
||
</HeaderProvider> | ||
</ToastProvider> | ||
</AuthProvider> | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
너무 작고 사소한 제안인데, 이름
ScreenReaderOnly
는 어떤가요..?ㅎㅎ 만약 바꾸게된다면 훅 이름도 함께useScreenReaderOnly
로 바꿔도 좋을 것 같아요..!지금 이름도 충분히 스크린 리더를 위한 것이라는 느낌이 들지만, only를 붙여서 눈에는 보이지 않고 오로지 스크린 리더기를 위한 것이라는 것을 조금 더 강조하고 싶어서 제안해봅니다!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
이 부분은 바로 반영하도록 하겠습니당