Skip to content
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

ナビゲーションバーをキーボード操作可能に #4025

Merged
merged 15 commits into from
Apr 26, 2024

Conversation

mehm8128
Copy link
Member

@mehm8128 mehm8128 commented Jul 17, 2023

プレビュー環境
prod: https://4025-prod.traq-preview.trapti.tech/
dev: https://4025-dev.traq-preview.trapti.tech/

思ったより変更点が多くなってしまいました、分からないところあれば聞いてください

やったこと

  • チャンネルボタンをdivではなくてrouter-link
  • aria-属性をつけた
  • 逆にaria-selectedをつけても意味のないroleのHTMLタグについているaria-selecteddata-is-selectedに書き換え
    • linkroleにはaria-currentをつけるべき
      • つけたけどスクリーンリーダーには上手く読み上げられなかった、スクリーンリーダーによるのかな
  • タブにtab tablistなどのroleをつけた
    • tabpanelもつけるべきだけど、Vueのコンポーネントにつけたら型エラーになったので一旦諦めた
    • tabroleはaria-selectedつけていいやつ
  • ボタンなのにdivタグで書かれていたところをbuttonタグを使うように
  • buttonタグにしたりしてフォーカスできるようになったところの:focus:focus-visible:hoverと同じCSSをつけた

確認してほしいこと

  • 今まで通り動くか
    • もちろんスマホサイズでも
    • 特にChannelElementはHTMLタグの構造も変えたのでスタイルが崩れていないか確認してほしいです
  • キーボードで操作できるか
    • ホームタブ以外はフォーカス時のCSSが効いていないところあり
  • (できれば)スクリーンリーダーでもいい感じに読み上げられるか

スクリーンリーダーとかは自己満足なので、余裕なければ最低限上2つを確認してもらえれば大丈夫です

やること

  • div→buttonにしたらなぜかチャンネルのCSSが崩壊した
  • フォーカス中のデザインを依頼
    • 別PRでやる

遷移先のページとかモーダルとか難しそうなポップアップナビゲーターは別PRで

@mehm8128 mehm8128 self-assigned this Jul 17, 2023
@codecov
Copy link

codecov bot commented Jul 17, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 86.35%. Comparing base (2075787) to head (3dd6840).
Report is 47 commits behind head on master.

❗ Current head 3dd6840 differs from pull request most recent head bf23f13. Consider uploading reports for the commit bf23f13 to get more accurate results

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #4025      +/-   ##
==========================================
- Coverage   86.36%   86.35%   -0.01%     
==========================================
  Files          66       66              
  Lines        4722     4719       -3     
  Branches      565      564       -1     
==========================================
- Hits         4078     4075       -3     
  Misses        638      638              
  Partials        6        6              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@mehm8128 mehm8128 marked this pull request as ready for review August 18, 2023 13:51
Copy link
Contributor

@nokhnaton nokhnaton left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

チャンネルリストなどで、tabを押しても#の部分にしかフォーカスが当たらないのが少し気になりました。
現状よりは改善されてるので、とりあえずこのままマージでもいいと思います!

:is-noticeable="notificationState.isNoticeable"
:unread-count="notificationState.unreadCount"
/>
</router-link>
<channel-element-hash
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NITS
<router-link><channel-element-hash>, <channel-element-name><channel-element-unread-badge>の順番は逆のほうが自然な気がします

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

<router-link><channel-element-hash>は確かにそうですね
<channel-element-name><channel-element-unread-badge>は逆だと表示がおかしくなる気がするのですが、なぜですか?(フォーカス順には特に影響しない気がする)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

あ、<channel-element-unread-badge>が指してるもの勘違いしてました。このままで大丈夫です

@mehm8128
Copy link
Member Author

tabを押しても#の部分にしかフォーカスが当たらない

これがよく分からなかったのですが、どんな感じですか?
僕の手元だと↓の動画みたいに、一通りフォーカスが当たってenterで開閉までできるようになっています

chrome_7D8NXhGZSO.mp4

@mehm8128 mehm8128 requested a review from nokhnaton April 25, 2024 11:50
@nokhnaton
Copy link
Contributor

手元のブラウザ(Vivaldi)だとこうなってたんですけど、Chromeで動かしてみると特に問題なかったので、大きな問題にはならない気がします。
https://github.com/traPtitech/traQ_S-UI/assets/104210327/67a1fb80-5396-445d-ad5e-e6653b532e6c

Copy link
Contributor

@nokhnaton nokhnaton left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

マージしちゃって大丈夫だと思います!

@mehm8128
Copy link
Member Author

chromeとfirefoxでは動いているのでvivaldiの問題っぽいですね🤔
とりあえずマージしちゃいます

@mehm8128 mehm8128 merged commit 4b11357 into master Apr 26, 2024
9 checks passed
@mehm8128 mehm8128 deleted the feat/navigation_bar_a11y branch April 26, 2024 02:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants