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

PanelChrome: Fixes issues with hover header and resizing panel above #71040

Merged
merged 6 commits into from
Jul 5, 2023

Conversation

torkelo
Copy link
Member

@torkelo torkelo commented Jul 4, 2023

Noticed an issue with the hover header preventing me from resizing the panel above. This was due to the fact that the hover header is always there just with opacity 0 (placed above panel overlapping the resize element).

  • Fixed by setting visibility to hidden when opacity is 0
  • Removed unused emotion css class "hidden"
  • Removed left over logic that removed show-on-hover class when menu was opened, this logic was not working as the setMenuOpen callback was never called as PanelMenu never called onVisibleChange (so removed this).

Also required changes to

  • Make PanelChrome itself have focus instead of VizLayout (this is to make sure the hover header actions & menu can be focused)

@torkelo torkelo requested a review from a team as a code owner July 4, 2023 12:39
@torkelo torkelo requested review from joshhunt, JoaoSilvaGrafana, kaydelaney and axelavargas and removed request for a team, kaydelaney, joshhunt and JoaoSilvaGrafana July 4, 2023 12:39
@torkelo torkelo added this to the 10.0.x milestone Jul 4, 2023
@torkelo torkelo changed the title PanelChrome: Fixes issues with hover header and sizing panel above PanelChrome: Fixes issues with hover header and resizing panel above Jul 4, 2023
@grafana grafana deleted a comment from grafana-delivery-bot bot Jul 4, 2023
Copy link
Member

@axelavargas axelavargas left a comment

Choose a reason for hiding this comment

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

Hey @torkelo 👋🏾 , I gave it a spin, and found out that the changes break a11y when using the keyboard 😢 , I am attaching the video with the current behavior, at the end of the video you can see how it looks in play grafana.

P.S The menu should be accessible using the keyboard, this issue was fixed here: #69861

A11yIssuesOnPanelHeader.mp4

@torkelo
Copy link
Member Author

torkelo commented Jul 4, 2023

@axelavargas I think that is because the keyboard navigation is done a bit strangly, the panel itself should be focused and then the menu should show up. This was how I described keyboard navigation (so you can focus any panel (independent of visualization)), preferably move focus around using arrow keys (once a panel has focus).

Anyway not sure I can solve that a11y without this big change to panel focus / keyboard navigation. Maybe we can open a follow-up PR and address that separately? as this bug is pretty big/annoying. we can fix it first.

@torkelo
Copy link
Member Author

torkelo commented Jul 4, 2023

@axelavargas sorry, I think your right in one thing there. It's strange that the non hover header mode change (cannot be tabbed to).

@torkelo
Copy link
Member Author

torkelo commented Jul 4, 2023

not it's the same problem there. We need to make the whole panel have focus, hmm.

@torkelo torkelo requested a review from a team as a code owner July 4, 2023 13:52
@torkelo
Copy link
Member Author

torkelo commented Jul 4, 2023

@axelavargas @kaydelaney I fixed the issue by making the PanelChrome be focusable (this is what I always thought makes the most sense, compared to VizLayout).

I was against making VizLayout be the element that has focus as it should be the whole panel not the inner layout element (#42244 (comment))

The keyboard navigation for the time range line still works after moving focus to PanelChrome, but the tooltip is not shown. will investigate that

@torkelo
Copy link
Member Author

torkelo commented Jul 4, 2023

pushed an update to KeyboardPlugin and TooltipPlugin so that they now look for the focus parent correctly, they now both work again

@leeoniya
Copy link
Contributor

leeoniya commented Jul 4, 2023

btw, we also need an "active grid item" class added for new portal-into-viz tooltip strategy to work correctly. (z-index setting):

https://github.com/grafana/grafana/pull/70872/files#diff-8b78102d87c552bbe06c1e3887b7bb6c1bf2742c92d94431433daf9e3aaa5899R109

cc @dprokop

@torkelo torkelo requested a review from a team as a code owner July 4, 2023 16:35
@torkelo torkelo modified the milestones: 10.0.x, 10.1.x Jul 4, 2023
Copy link
Member

@axelavargas axelavargas left a comment

Choose a reason for hiding this comment

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

Great job tackling this issue, all the a11y issues were resolved. It works as expected 🥳

@torkelo torkelo added the no-backport Skip backport of PR label Jul 5, 2023
@torkelo torkelo merged commit 7252c6d into main Jul 5, 2023
23 checks passed
@torkelo torkelo deleted the fix-hover-header-resize-issue branch July 5, 2023 12:17
polibb pushed a commit that referenced this pull request Jul 14, 2023
…71040)

* PanelChrome: Fixes issues with hover header and sizing panel above

* Update

* Make panel be focusable

* Fix tooltip when using keyboard nav

* Re-render grid when layout change to have dom positions match absolute css positions

* Fix clicking panel leaves hover header open
@ricky-undeadcoders ricky-undeadcoders modified the milestones: 10.1.x, 10.1.0 Aug 1, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

None yet

6 participants