pkp/pkp-lib#5196 CSS Pseudo selectors creating a duplicated link announcement on screenreaders

[israelcefrin]

Fix the duplicated announcement of link galleys by screenreaders removing the extra space between closing / opening list item tags, i.e.: </li><li>. The required visual gap is fixed via CSS/LESS.

[asmecher]

@NateWr, could you review this one? Thanks!

[NateWr]

Thanks @israelcefrin. Can you help me understand why the change you proposes fixes the problem? It doesn't look like it touches any pseudo selector. Also, why was the added margin necessary?

[israelcefrin]

Thanks @israelcefrin. Can you help me understand why the change you proposes fixes the problem? It doesn't look like it touches any pseudo selector. Also, why was the added margin necessary?

Hi @NateWr I've ran a screen reader test to navigate to this page to review the issue reported on the audit. It turns out that the extra spaces this code outputs generates a :after element between <li> elements which is rendered by some screen readers as a

element and it announces "list , first element of three" when you have only 2 elements. It doesn't happen on every browser, but I noticed it on MacOs.

Also, when I removed the extra space I noticed that the buttons to galley got closer to each other.
See the adjusted HTML:

<ul class="value galleys_links">
<li><a class="obj_galley_link pdf" href="#">PDF</a></li><li><a class="obj_galley_link file" href="#">HTML</a></li>
</ul>

And see the result:

It seems this white space was also generating a visual gap between the elements and I've fixed it by adding a margin to separate the clicking area of each button. Otherwise, people with low dexterity (or thick fingers) might run into trouble when they attempt to click on one of these buttons.

[NateWr]

I think we should try to solve this by removing the :after element. Can you find the source of that and see if we really need it?

[israelcefrin]

This is the issue, the :after element is generated or rendered by assistive technologies from white spaces. The element itself is not in the code. Some screen readers render it as a DOM element and even announce it as a list item which doesn't exist in fact.

See how FF can show the DOM element created (espaços = white spaces):

And see how Chrome renders it:

Chrome and Safari don't show it on the Element Inspector, but Voice Over renders and announces it from the code.

[NateWr]

Hmm, I think we need to investigate this more. If that's indeed what happens, it wouldn't just happen here but everywhere that we use <li> elements. I found almost 200 uses of </li> in the code base in a quick search.

Removing white space between <li> elements really mangles the HTML and makes it difficult to read. I'm not in favor of doing this everywhere and it's not something we could reliably enforce anyway.

I think we need an alternative solution of some kind. I did a quick Google search but nothing came up. If the issue was so common, I'd expect to see it talked about more. That leads to me to think there may be other factors at play here.

[NateWr]

Quick thought: I wonder if it's because the <li> elements have display: inline-block. Maybe it only effects inline-block elements? If it is the inline-block aspect, then we should probably change that to make use of flexbox instead. The browser support is very wide now so I think we can start adopting flexbox in our themes.

[israelcefrin]

I will run tests using flexbox and check whether the screenreader reads it correctly if we change the CSS approach to it.

[israelcefrin]

@NateWr I've tested the page against VoiceOver with a CSS where I removed the display: inline-block from LI elements and implemented the flexbox to the UL component. It is announcing the total of elements correctly. I've updated the PR. Do you need me to squash it or you can merge the latest push only?

[NateWr]

This looks good, thanks @israelcefrin.

Instead of margin-right: 0.2em can you use margin-inline-end: 0.25rem? This will ensure it works ok for right-to-left languages and using 0.25rem will ensure the spacing is consistent with the base spacing (one quarter of the spacing).

I noticed this only fixes the problem on the article details page. But the same pattern is used on the article summary (issue lists and search results).

Also, once that is done, I think you'll need to apply this to OPS's default theme as well.

[israelcefrin]

@NateWr I've adjusted the CSS selector to margin-inline-end: 0.25rem and included the same adjustment to the other places you've mentioned. However, I noticed a &:last-child { margin-right: 0; } element which I didn't remove, but it is likely it will not be necessary to keep it since we are using the margin-inline-end now. Do you think we should leave as it is or remove it too ?

[NateWr]

This is probably cleaning up the margin after the last element. I do this sometimes because in certain cases the margin will cause the last element to go to a new line, even though it would fit without the margin.

I think you can fix this with:

&:last-child { margin-inline-end: 0; }

[NateWr]

This should be 0.25rem, not 1.25rem.

[NateWr]

Same below.

[israelcefrin]

I've adjusted the margin due to the fact when I had changed to flexbox the parent container, the LI elements decreased the space between the buttons.

See the current distance (non flexbox):

If I keepet 0.25rem (with flexbox)

And using 1.25rem (with flexbox)

I thought that this distance should be preserved for keeping enough room between elements and presenting easier user interaction (clicking) area. Should I change back to the original value?

[NateWr]

Oh my mistake. I thought that the previous margin was 0.25rem but it was 1em. I'd suggest leaving it the way that it was, 1em. Sorry that was my mistake.

[israelcefrin]

No worries!
I've made all adjustments. Could you please review it and let me know if any other is required to merge?

[NateWr]

This needs to be set back to 1rem too. Sorry!

[NateWr]

Just the one comment. Plus we need a PR for OPS.

[israelcefrin]

I've added the OPS PR in the original github issue: pkp/pkp-lib#5196 (comment)