Make Markdown tables accessible; fix some broken CSS

[shawnbot]

This is an important update to our Markdown table element styles. The big, potentially breaking change is removing display: block, the presence of which causes accessibility tools to ignore most (all?) user-generated Markdown tables on GitHub. I've also tidied up the rest of the CSS:

• width: 100% is out, since it works with display: table to make tables full-width (it did nothing with display: block in the mix)

• overflow: auto is out, since it's incompatible with display: table (citation needed!)

• tr elements don't render CSS borders, so I've removed the border declaration

• The border declaration for td, th elements uses Primer SCSS variables. The border color will change from (aligned vertically for easy comparison):

  • #dfe2e5 (lighten($gray-300, 5%)) to
  • #d1d5da ($border-gray-dark → $gray-300)

• Cell padding is now ($spacer-2 - 2) ($spacer-3 - 2), which computes to 6px 14px rather than the previous (static) 6px 13px. Maybe it's time to rip off the band-aid and just make this $spacer-2 $spacer-3?? 🤷‍♂

Since visually this may be a breaking change, I'm queueing it up for our 14.0.0 release (no PR yet).

Fixes #924.

[vercel]

This pull request is being automatically deployed with ZEIT Now (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://zeit.co/primer/primer-css/laar6wjec
🌍 Preview: https://primer-css-git-markdown-table-refactor.primer.now.sh

[shawnbot]

I just noticed that we use the equivalent of $spacer-2 $spacer-3 in Doctocat. So maybe it's safe to do so here, too? Also, @colebemis we're probably going to want to propagate these changes back to Doctocat. 😁

[simurai]

This looks good. Just added some comments with potentially making changes to the border and padding.

width: 100% is out, since it works with display: table to make tables full-width (it did nothing with display: block in the mix)

Tested removing display: block; + width: 100%; in Chrome DevTools and no visual change is noticeable. So seems fine to remove. 🤷‍♂ Curious about the initial reason? Maybe it was added for a certain browser?

Cell padding is now ($spacer-2 - 2) ($spacer-3 - 2), which computes to 6px 14px rather than the previous (static) 6px 13px. Maybe it's time to rip off the band-aid and just make this $spacer-2 $spacer-3?? 🤷‍♂

Or just $spacer-2? Then a bit more columns would fit.

[simurai]

Ohhh.. wait.. it looks like removing display: block; + width: 100%; makes the overflow: auto; not work when there are long strings without white space. Like in this example:

Before	After
asdfasdfasdfasdfasdfasdfasdfasdfasdfasdfasdfasdfasdfasdfasdfasdfasdfasdfasdf	asdfasdfasdfasdfasdfasdfasdfasdfasdfasdfasdfasdfasdfasdfasdfasdfasdf

We could use word-break: break-word; or table-layout: fixed;. But that could also make it harder to read a table with long strings because they would wrap and create big cells. Maybe there is another way to force scrolling? Like render an extra wrapper that scrolls the table:

<div style="overflow-x: auto">
  <table>

[shawnbot]

Ah yes, overflow... This would definitely be a breaking change, huh? 😬

I noticed that .comment-body gets overflow: visible on github.com, which seems... not quite right to me? But that's probably so that popovers (edit: tooltips!) work properly.

We might need to hold off on this until we've sorted out our z-index situation. ☹️

[shawnbot]

@simurai after some investigation, it looks to me like we can only solve the overflow problem by:

1. Wrapping each table in a div with the overflow-x-auto class;
2. Adding overflow-x: auto to the .markdown-body styles; or
3. Selectively adding the overflow-x-auto class to every instance of markdown-body.

I'm looking into how feasible 1) is, given our... complicated array of markdown filters in dot-com. 2) and 3) would be "easier", but both carry a lot of risk and will almost certainly break existing tooltips and/or popovers.

[shawnbot]

If anyone comes across this and wants to help test out weird edge cases in the wild, you can do it by pasting the following into your browser's JavaScript console:

// insert style overrides 
const style = document.createElement('style')
style.textContent = `.markdown-body table {
  display: table !important;
  width: auto !important;
}`
document.head.appendChild(style)

// wrap every markdown table in <div class="overflow-x-auto">
for (const table of document.querySelectorAll('.markdown-body table')) {
  const div = document.createElement('div')
  div.className = 'overflow-x-auto'
  table.parentNode.insertBefore(div, table)
  div.appendChild(table)
}

[simurai]

1. Wrapping each table in a div with the overflow-x-auto class;

I think this will break the :last-child selector and add extra bottom margin if the table is last:

We could add mb-3 to the wrapper and remove table from here

css/src/markdown/markdown-body.scss

Line 65 in a0a99c8

table,

. But that only works for github/github and would break for all the other "Markdown to HTML" libraries. Is that a concern?

[shawnbot]

@simurai good catch. One fix for that could be to change the > *:last-child selector to > *:last-child, > *:last-child > table.

[simurai]

Closing as wontfix, see #924 (comment).

There is still "Convert markdown page to one big example" #1065 that got cherry picked to improve the docs.