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

Change the border color of tabs to --pst-color-border #1863

Closed
wants to merge 2 commits into from

Conversation

timhoffm
Copy link
Contributor

@timhoffm timhoffm commented Jun 5, 2024

#1555 added a border around tabs. It uses --pst-color-primary:
grafik

IMHO primary color gives too much weight on the border. In this PR, suggest to change the color to --pst-color-border:
grafik

@trallard
Copy link
Collaborator

trallard commented Jun 5, 2024

Hey @timhoffm thanks for taking the time to work on this. Though the primary colour border is intentional to signal that such a tab is in an active state. This brings this state in alignment with the rest of the theme where active states are marked with the primary colour and also helps us meet accessibility standards.

I will let @smeragoel chime in as she led the design, but considering that we are prioritising accessible design and that she did quite a fair amount of research and work to make interactive elements design more accessible I would prefer not making this change.

@trallard trallard added the needs: discussion Needs discussion before an implementation can be made label Jun 5, 2024
@timhoffm
Copy link
Contributor Author

timhoffm commented Jun 5, 2024

Thanks @trallard for the feedback. To me (as a non-expert on accessibility) the focus should be on the tab head not the not whole tab box. The box is essentially like any static box. The active part is the head. Maybe something like this can work:

image

Changes:

  • All borders to --pst-color-border
  • The top border of the active tab head is --pst-color-primary and has doubled width. This is in analogy to "current item" highlighting in top menu and side-bar, where some sort of bar next to the text is used for active elements
    image and image
  • Non-active tabs are reduced to normal fontweight.
  • Added underline for hovering over tabs - IMHO this should be added for consistency irrespective of the other changes.

This is just a suggestion. I'll leave it to you accessibility experts to decide what's best.

@smeragoel
Copy link
Contributor

Thank you for your work, @timhoffm!

I considered --pst-color-border when working on this. However, the problem with this is that it does not provide sufficient contrast against the background and the inactive tabs.

Contrast Comparison:

Color Background #FFFFFF Inactive Tabs #F3F4F5
--pst-color-border #D1D5DA 1.47:1 1.33:1
Primary Color #0A7D91 4.82:1 4.37:1

The goal behind this redesign was to make the active state clearer and the component more accessible. High contrast is necessary for making the tablist recognizable for all users, which naturally pulls more focus. You can find more details on this here.

Thank you for understanding and supporting our efforts to improve accessibility 😸

@timhoffm
Copy link
Contributor Author

timhoffm commented Jun 5, 2024

Thanks, understood.

Semi-OT questions:

  • Is the font weight on the tabs intentionally left bold? That setting comes from sphinx-design. I suppose they use it to set their otherwise completely flat tabs apart from regular text (c.f. https://sphinx-design.readthedocs.io/en/latest/tabs.html#tabs). With the grey background here, bold font seems unnecessary.
  • Should there be an underline when hovering over tabs? On other navigation elements, we change color and underline. On tabs we only change color.
  • Should --pst-color-border be darkened? It's used with cards and also doesn't fulfill the 3:1 contrast limit. Or do cards not count because they are not interactive elements?

@trallard
Copy link
Collaborator

trallard commented Jun 6, 2024

Is the font weight on the tabs intentionally left bold? That setting comes from sphinx-design. I suppose they use it to set their otherwise completely flat tabs apart from regular text (c.f. sphinx-design.readthedocs.io/en/latest/tabs.html#tabs). With the grey background here, bold font seems unnecessary.

Yes, this is intentional. To provide a bit more context, there are at least 4 WCAG success criteria that touch on colour and how/when each applies to a given component is highly dependent on context/purpose/use.
One of such criteria is "Use of colour" which involves not using colour as the only way to indicate meaning or convey information. This ensures that components remain accessible for folks using accessibility settings (for example high contrast, screenshot added), assistive tech, and for folks with a variety of visual impairments/disabilities.

PST tabbed element in high contrast mode

Hence using a sufficiently different font weight (from the surrounding body and code text) helps with meeting such criteria.

Should there be an underline when hovering over tabs? On other navigation elements, we change color and underline. On tabs we only change color.

Yes or some other sort of indicator, we noted this already as it seems to have gone missing during implementation. So we should be fixing this soon.

Should --pst-color-border be darkened? It's used with cards and also doesn't fulfill the 3:1 contrast limit. Or do cards not count because they are not interactive elements?

This is a case where the contrast requirement is not critical for accessibility. Since these are not user interface (control) components, the border is not per se a means to convey information (such as state: hover, active, etc.). So, having a relatively low contrast does not hinder accessibility in any way; basically, the border can be considered purely decorative/visual styling.
There is, however, a special case of clickable cards, though the Sufficient contrast criteria are met at the interaction point (hover, focus) by changing the border (colour, thickness) and pointer indicator.

@timhoffm
Copy link
Contributor Author

timhoffm commented Jun 6, 2024

Thanks for the detailed explanation. 👍

@timhoffm timhoffm closed this Jun 6, 2024
@timhoffm timhoffm deleted the tab-border branch June 6, 2024 12:10
@trallard
Copy link
Collaborator

trallard commented Jun 6, 2024

Thanks for taking the time and effort to help us make the theme better @timhoffm 💜

@joelostblom
Copy link

Changes to improve accessibility are great! Unfortunately, for me personally the change in #1555 have made the content of tabs less accessible. There is something about the bright blue tab border that makes it hard to focus on the content in the tab and my eyes tend to be drawn to the border instead. The change that @timhoffm suggests above makes tabbed content noticeably more readable for me as I can more easily focus on what is in the tab instead of focusing on the border of the tab. Although I appreciate the utility of indicating where a tab ends wit ha border, there is something about the bright blue border color that makes my eyes/brain think that the border itself is more important/attention grabbing than its content.

Here is an example where I think it is quite difficult to focus the code content in the tab:

image

Changing the border to the primary border color as suggested by @timhoffm makes the content noticeably easier to focus for me:

image

I'm not an accessibility export, so I'm only speaking of my personal experience above. When I tried to read up on accessibility guidelines to learn more, I noticed that in the link posted above by @smeragoel they avoid using an active color like blue for the tab border (they use black instead), and it seems like the W3 accessibility guidelines for tabbed content also follows a similar convention and uses an inactive color for the outline (they use grey). I would be curious to take part of articles talking about using an active color for the border, so please share some resources if you have them handy.

Is there an option in the pydata sphinx theme (or a CSS configuration) to use the grey "inactive" color from --primary-border-color for the border of all tabs (both the tab content and the tab heading)?

@trallard
Copy link
Collaborator

Perhaps, if we replace the bright teal with a darker/lighter shade of this instead (for light/dark modes)? That would remove some of the stark contrast described above, thus making this less distracting.

Otherwise, it is probably an in-between design with a dark/light border around the whole tabbed component with an alternative indicator. @smeragoel WDYT?

@drammock
Copy link
Collaborator

drammock commented Aug 19, 2024

Perhaps, if we replace the bright teal with a darker/lighter shade of this instead (for light/dark modes)? That would remove some of the stark contrast described above, thus making this less distracting.

I'm open to this, but see below.

Otherwise, it is probably an in-between design with a dark/light border around the whole tabbed component with an alternative indicator. @smeragoel WDYT?

Sorry if I missed it in the above discussion @trallard @smeragoel, but what speaks against the proposal/screenshot by @timhoffm in this comment? AFAICT, it would still comply with the "use of color" guideline, because (1) the active tab is the only tab with a heavy overline, and (2) the inactive tabs have a different background shade. (but maybe there are other guidelines it doesn't comply with?)

EDIT: yep, I missed it. @smeragoel said:

the problem with this is that it does not provide sufficient contrast against the background and the inactive tabs.

so in that case I'm +1 on a medium gray that does provide sufficient contrast, or a less intense form of the primary color.

@timhoffm
Copy link
Contributor Author

On a side note, is the current value of --primary-border-color an issue in itself? If it is too low contrast here, then it may be too low contrast in other use cases too. Or are there semantic differences and one would need two variants of border colors, one defining content borders, which are accessibility-relevant and one purely stylistic, which does not have the contrast constraints.

@trallard
Copy link
Collaborator

trallard commented Aug 21, 2024

There is indeed a distinction between purely decorative (or stylistic) items and those that convey meaning. The former one can be exempt from meeting some WCAG success criteria but the latter are not.
For example, the border around the active/current Tabbed element is expected to convey meaning (the tabbed element is active in this case). So this should meet a number of criteria like use of colour, contrast minimum and others. And for consistency (which also helps with usability and accessibility including accessibility for cognitive disabilities) we went with the same colour we are using for other active/current states, our primary colour.

Other components like some cards for example, should be fine with the lighter gray border, even if it does not meet WCAG AA as this border is purely decorative or rather not essential to convey meaning of any sort.

So technically speaking having a border colour that does not meet WCAG AA does not always equate to an accessibility failure but it is highly contextual and purpose based. A way to avoid these uncertainties would be to always ensure every colour used meets the contrast requirements but then it's also likely we'd end up with a lot of instances like this one where components or elements like borders draw too much attention or are distracting (which is particularly problematic for folks with cognitive disabilities). That is one of the things that makes accessibility work quite tricky, sometimes what seems "ideal" for a group of folks might not be accessible for others. So we need to think in terms of progressive enhancements to accommodate for feedback and the likes.

We've been going through all the components and noting where we need to make accessibility improvements, I believe we have now addressed all instances where colour contrast might be an issue.
But if not we can fix that.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs: discussion Needs discussion before an implementation can be made
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants