[Pagination] Second iteration #19612
Conversation
oliviertassinari
added
new feature
|
@material-ui/lab: parsed: -0.31% 😍, gzip: -0.25% 😍 Details of bundle changes.Comparing: e534b94...06fa79c
|
oliviertassinari
force-pushed
the
pagination-polish
branch
2 times, most recently
from
4d1c6e7
to
94902e8
Compare
Hmm, yeah, that came from making It would be more helpful if the test explained why this is an error, but if I had to guess, I suspect it's do do with list items needing an immediate parent with a role of list? On a semi-related note, I wonder if it should in fact be an
The only issue I see is with the visibility of the active item for a disable pagination in dark mode. |
|
@mbrookes I couldn't find why the ul + role="navigation" is considered an error. I don't know about ol, I haven't seen materials on the subject. Most seem to use a ul.
I have used an opacity based approach, but maybe we should go back to a color + background approach for the disabled state? I like how the use of the opacity simplifies the logic. |
I couldn't either, and I spent ages earlier trying to navigate the byzantine HTML spec, but while looking into ul vs ol just now, I came across this:
https://developer.mozilla.org/en-US/docs/Web/HTML/Element/ul and this: https://w3c.github.io/html-aria/#el-ul Not a comprehensive explanation as to why the role is restricted, but at least a pointer as to why it's considered an error. Also of interest is:
Regarding ul vs. ol – for ol screenreaders announce the number of the item, while for ul they refer to it as a bullet, so ul does appear to be the better choice. |
|
Oops, I was cleaning my fork. |
| </li> | ||
| ))} | ||
| </ul> | ||
| <ul className={classes.ul}> |
There was a problem hiding this comment.
| <ul className={classes.ul}> | |
| <ul role="list" className={classes.ul}> |
?
There was a problem hiding this comment.
It seems that the <ul> element has a default role of "list".
There was a problem hiding this comment.
Default roles should never be used.
[...] default implicit ARIA semantics is unnecessary and not recommended as these properties are already set by the browser.
-- https://www.w3.org/TR/html52/dom.html#do-not-set
I would prefer that over a blog post (even if it comes from an editor). Unless there's some issue on the spec concerning this.
There was a problem hiding this comment.
Default roles should never be used.
That isn't what the sentence says (it says "not recommended").
I would prefer that over a blog post
Blog post? I quoted https://w3c.github.io/html-aria/#el-ul
Unless there's some issue on the spec concerning this.
Strict conformance to the spec (which is actually only a recommendation in this case) over pragmatic choices that help make content more accessible seems like the wrong tradeoff to me.
There was a problem hiding this comment.
I hadn't this principle of "past experience" in mind (that I love BTW). I was proposing to leverage the "experience"/"exposure" of others. I think that the probability of doing it right is in their favor; not ours.
There was a problem hiding this comment.
I think that the probability of doing it right is in their favor; not ours.
You mean like Google using a table for pagination? :)
No, the guidance on this is pretty clear cut, it's just that nobody is following it. Following the crowd in this case isn't about others being correct, but about not bucking the trend. Subtly different.
Incidentally, there's no consistency with the use of <nav> to wrap the <ul>, and for those that do, whether or not to add role="navigation", so we still have that to debate. 😄
There was a problem hiding this comment.
I can't see clearly through these conflicting signals. Maybe the following would help: What happens if we set a default role while we shouldn't? How can it go bad?
There was a problem hiding this comment.
Worst case:
-
Validator warnings
But see also: https://www.scottohara.me/blog/2019/03/26/a-warning-about-warnings.html
"Sometimes you may come across gaps in the assertions these tools are based on. Or a best practice may dictate that you don’t double up ARIA roles on their native elements, when that’s absolutely what you need to do to ensure cross-browser and assistive technology feature parity."
-
User "surprise" (for Safari with Voiceover users) when the experience is not consistent with other pagination implementations they are used to. (Which use
<ul>withlist-style: nonebut don't addrole="list").
For the latter reason I'm suggesting (last paragraph here: #19612 (comment)) that we go with the flow rather than "fix" it. If we get feedback that it causes problems, we can revisit it.
There was a problem hiding this comment.
Ok awesome! So we try no role and we learn from it.
oliviertassinari
force-pushed
the
pagination-polish
branch
4 times, most recently
from
2b6c887
to
f267ddb
Compare
oliviertassinari
force-pushed
the
pagination-polish
branch
from
f267ddb
to
c81c837
Compare
| @@ -43,6 +43,10 @@ export const light = { | |||
| disabled: 'rgba(0, 0, 0, 0.26)', | |||
| // The background color of a disabled action. | |||
| disabledBackground: 'rgba(0, 0, 0, 0.12)', | |||
| disabledOpacity: 0.38, | |||
There was a problem hiding this comment.
Were these keys first introduced here in v4.9.3? Is it stable (i.e. can I use it when styling)?
There was a problem hiding this comment.
We plan to use theses keys more broadly. I think that it's safe to use.
This is a follow-up on the previous pull request. I have tried to explore the following dimensions:
Muiprefix of the PaginationItem style sheet. This is important for global overrides.BTW, I wonder if we shouldn't upgrade all the usePagination tests to higher level tests for the Pagination component?
Closes #19653