-
-
Notifications
You must be signed in to change notification settings - Fork 1.9k
David/enhancement modebar #7429
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
base: master
Are you sure you want to change the base?
Conversation
So I'm wondering what are the configuration options for the modebar besides hover, and what actually changes the visibility during hover (is it just css? can we do the same thing for focus as hover). Also looks like your editor did a bunch of reformatting, you should undo that Also, if we use button, we don't need tabindex now, right? |
Yeah, so, we have two options:
I prefer 2 but we should do one or the other |
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.
what are the configuration options for the modebar? can we fix the formatting? can we choose one of two accessibility strategies?
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.
also we need to fix the dist/ build/ folders, ill figure out who I should cc
@gvwilson this is an accessibility which makes the modebar tab-able (some of the buttons require a mouse but not all of them). Can someone on your team clarify for us what we need to run to fix the build/ and dist/ folders to submit the PR properly? @davidangarita1, I'd like some help understand the modebar a little better. Do you understand how its configured and what its options are? |
I'm interested in changing the modebar through configuration |
@ayjayt here is the config an options: |
Yeah so let's just make sure no options interfere with the accessibility setup and vice versa |
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.
just some questions
@@ -25,6 +29,11 @@ | |||
position: relative; | |||
vertical-align: middle; | |||
white-space: nowrap; | |||
|
|||
a { |
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.
@davidangarita1 do we still use this?
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.
By converting the Anchor to Button they were misaligned, this style is to focus the Plotly logo into the mode bar
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.
i mean the anchor tag, is there still an anchor tag?
edit: ok, i get it
|
||
svg { | ||
position: relative; | ||
top: 2px; |
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.
@davidangarita1 why did we get rid of this?
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.
By converting the anchor to Button they are misaligned, this style is that the buttons icon is centered
Thank you for this work @davidangarita1 ! This is huge! In Chrome I was able to tab through the modebar buttons and use Enter/Space to activate the ones that don't require mouse input 🎉 Is there additional functionality I should try out? Couple things I noticed, you can let me know if you see this behavior as well:
BTW, in case it's helpful for testing, if you add this key/value pair to the plot's modeBarButtonsToAdd: ["sendDataToCloud", "editInChartStudio", "zoom2d", "pan2d", "select2d", "lasso2d", "drawclosedpath", "drawopenpath", "drawline", "drawrect", "drawcircle", "eraseshape", "zoomIn2d", "zoomOut2d", "autoScale2d", "resetScale2d", "hoverClosestCartesian", "hoverCompareCartesian", "zoom3d", "pan3d", "orbitRotation", "tableRotation", "resetCameraDefault3d", "resetCameraLastSave3d", "hoverClosest3d", "zoomInGeo", "zoomOutGeo", "resetGeo", "hoverClosestGeo", "hoverClosestGl2d", "hoverClosestPie", "resetViewSankey", "toggleHover", "resetViews", "toggleSpikelines", "resetViewMapbox", "resetViewMap", "zoomInMapbox", "zoomInMap", "zoomOutMapbox", "zoomOutMap"] |
@emilykl Thank you so much for the thoughtful feedback — I really appreciate it! Regarding the "Autoscale" and "Home" buttons: those should reset the chart when zooming or panning has occurred. So if you zoom into the plot, pressing those buttons (via Enter/Space or mouse) should restore the axes to their default ranges. Let me know if that's not happening for you! Great point as well about testing with all the modebar buttons. When enabling all of them, keep in mind that some — like "Draw closed freeform" — are not keyboard-accessible even if they receive focus. That’s because their functionality depends on direct mouse interaction to draw on the chart, so triggering them via keyboard won’t result in any visible action. As for Safari, I’ll look into compatibility with the version you mentioned (Safari 17) to see if recent changes are working properly there. Thanks again for testing this so thoroughly — it's super helpful! |
Hah, I swear I was already doing this, but in any case, it's working now! Have you been able to test in Safari? Still not working for me in Safari, but Chrome and Firefox work great (on Mac). I'll try updating to the latest Safari and see if that changes anything. @davidangarita1 @ayjayt Do you want me to look into the failing Jasmine tests? |
@emilykl, Davids on vacation at the moment, I'm burning the candle from both ends coming up for a launch, and all the accessibility stuff is pro bono (which david might do on vaca anyway), but any help is appreciated. we need to get some better mac testing in our office in general, so any help there is definitely appreciated since we have a gap. |
@davidangarita1 If you pull commit b6e4865 onto your branch it should fix the jasmine tests. @davidangarita1 I'm also wondering if you know what additional code would be required if we were to keep the modebar buttons as It's probably OK to make the change, and I'm a fan of the simplicity, but I'm just thinking about unintended consequences, for example breaking custom CSS that people may have added to their pages. So just want to make sure I understand the alternative. (I realize you're on vacation right now so no rush, this can wait until you get back!) |
@emilykl we built both, there's some javascript that more or less changes the <a> tag to imitate the <button> tag |
@ayjayt Oh cool! Would you mind pushing that code to a separate branch so I could take a look? Again I realize you're in crunch time so no rush on this one. |
Here's the diff to this branch:
It's here: https://github.com/geopozo/plotly.js/tree/andrew/anchor-not-button-aria I tried to visualize it (the diff) with github but github didn't do it well. |
I have like 8 remotes in my local right now and I want to nuke everything. |
Pull request to improve accessibility for the modebar.
This makes the modebar tab-able so that people using special interfaces can interact with the graph. Some functions require a mouse, but not all do.