[framework]Add semantics role to table rows. #163337
Conversation
|
hmmm looks like i should not simply remove |
Oof, that's a tricky problem. What's the typical value of |
cdd51ee to
f2fbc6c
Compare
f2fbc6c to
3ecbeba
Compare
|
@chunhtai I want to merge this framework changes PR first and the "removing intermediate container layers in web engine" will be a separate effort in another PR. |
9494077 to
50fcfe9
Compare
|
|
||
| final SemanticsConfiguration configuration = | ||
| SemanticsConfiguration() | ||
| ..indexInParent = i |
There was a problem hiding this comment.
is this going to be used by the row index? if so, does the cell also uses this to set the col index? we should unify the way to set these indices in web.
There was a problem hiding this comment.
updated indexInParent for cells
| children: const <TableRow>[ | ||
| TableRow(children: <Widget>[Text('AAAAAA'), Text('B'), Text('C')]), | ||
| ], | ||
| TableCell( |
There was a problem hiding this comment.
Is this needed for semantics check? do we need to worry about this being a breaking change?
There was a problem hiding this comment.
yes because the test case here before is a very corner case putting a table inside a tablerow child, so it became table-> row-> table and it failed the new a11y check, i think cases like this are very rare and they are just made up for tests
There was a problem hiding this comment.
no, this is a case i replicate from internal code.
Should the tableRow assign the TableCell to its children internally?
There was a problem hiding this comment.
Should the tableRow assign the TableCell to its children internally?
I'm already doing that in assembleSemanticsNode, i assign the role to its children if the children have no roles.
but if the children already have roles(in this case, it's table), do we want to override that role? or do we want to insert a semantics node with a cell role between them?
There was a problem hiding this comment.
I'm already doing that in assembleSemanticsNode
Assigning child property in assembleSemanticsNode seems a bit hacky. We should do it through the TableCell widget if possible.
do we want to insert a semantics node with a cell role between them
This would be ideal, but doing it in widget layer means the Table here needs to be wrap with a semantics container. I think for now, let's just override it without creating a container. As you said this may be a rare case, we can fix it if it really is an issue
There was a problem hiding this comment.
also looks like this pr will make the table to be a container so there may not be overriding role issue in this particular case
There was a problem hiding this comment.
Assigning child property in assembleSemanticsNode seems a bit hacky. We should do it through the TableCell widget if possible.
We can't because we don't enforce user to use TableCell widget in table, TableRow can take any widgets as its children. User can just assign a Text widget to TableRow's children. That's why I assign the role here.
I think for now, let's just override it without creating a container.
SGTM,
there may not be overriding role issue in this particular case
in This particular case, the test case was table-> row-> table- > row-> cell, If I don't override the second table role , it will fail the check because row's children shouldn't be a table, if i override the second table role to be a cell, it will become table-> row-> cell- > row-> cell, it will fail the check because row's parent shouldn't be a cell. So I see the only way to fix this test case is to insert a cell and it will become table-> row-> cell -> table- > row-> cell,
There was a problem hiding this comment.
This would be ideal
Updated this PR to insert a semantics node with a cell role between them.
|
I just realized I had some misunderstanding about semanticsNode rect before. I thought the tree dump rect is actual semanticsnode.rect, but it's actually semanticsnode.rect with semanticsnode.transform. Instead of only setting row's and cell's Updated PR. |
| children: const <TableRow>[ | ||
| TableRow(children: <Widget>[Text('AAAAAA'), Text('B'), Text('C')]), | ||
| ], | ||
| TableCell( |
There was a problem hiding this comment.
I'm already doing that in assembleSemanticsNode
Assigning child property in assembleSemanticsNode seems a bit hacky. We should do it through the TableCell widget if possible.
do we want to insert a semantics node with a cell role between them
This would be ideal, but doing it in widget layer means the Table here needs to be wrap with a semantics container. I think for now, let's just override it without creating a container. As you said this may be a rare case, we can fix it if it really is an issue
| @@ -2869,6 +2895,13 @@ class SemanticsNode with DiagnosticableTreeMixin { | |||
| /// {@endtemplate} | |||
| SemanticsRole get role => _role; | |||
| SemanticsRole _role = _kEmptyConfig.role; | |||
| set role(SemanticsRole value) { | |||
There was a problem hiding this comment.
I want to assign the role cell to the child in assembleSemanticsNode.
And i want to only change its role. So i think this is needed.
There was a problem hiding this comment.
This is removed after offline discussion.
we won't directly change semantics node's role, just wrap them with a new semantics node with a cell role.
| children: const <TableRow>[ | ||
| TableRow(children: <Widget>[Text('AAAAAA'), Text('B'), Text('C')]), | ||
| ], | ||
| TableCell( |
There was a problem hiding this comment.
also looks like this pr will make the table to be a container so there may not be overriding role issue in this particular case
I mean we wrap it internally in either Table widget or TableRow widget |
looking at the code, we shouldn't add semanticsRole.cell in TableCell. It should added internally in Table or TableRow, this also avoid forcing people to use TableCell to have the correct semantics |
If you mean like to wrap TableRow's every children with Semantics( role: cell: child: child), I tried that and it didn't work out because TableCell is a ParentDataWidget so it assumes its parent is a Table, insert a Semantics widget between TableCell and Table will crash it. So I chose to assign the semantics role to cells in assembleSemanticsNode although it looks a bit hack 😅 maybe there is a better way to do it ?
make sense, I will remove that. |
1. framework side:
This PR Create semantics node for rows in table's
assembleSemanticsNodefunction.2. web side:
I tested on my mac, and i need to remove the
<flt-semantics-container>between table and row, row and cell to traverse inside table, removing those transfom intermediate containers on web will be a bit of hassle and will be in another separate PR.For example this code can only announce table but can’t get into cells.
If I removed the in between
</flt-semantics-container>, the code comeAnd I can get into table cells.
3. Other aria-attributes:
aria-colcount ,aria-rowcount aria-colindex aria-rowindex
aria-rowspan , aria-colspan :
*we currently don't support row span and col span in our widgets. related issue: #21594
related: #162339
issue: #45205
Pre-launch Checklist
///).If you need help, consider asking for advice on the #hackers-new channel on Discord.