Repository navigation
Move geometric sort for semantics to the framework side. - #14539
Conversation
There was a problem hiding this comment.
please assert containerTextDirection is not null
There was a problem hiding this comment.
Thanks, I did need to document this.
Actually, we don't need an assert because I do handle the null case properly. I added a bunch of comments to clarify. A null value is allowed, because in that case we want to fall back to ordering by Y position, and then child insertion order.
There was a problem hiding this comment.
trivial nit: generally we prefer rtl to be first
There was a problem hiding this comment.
Ahh, I was going alphabetically, and L < R. Is there a rationale for that preference?
There was a problem hiding this comment.
There was a problem hiding this comment.
what if there's no Directionality widget? e.g. with a Stack rather than a Column.
There was a problem hiding this comment.
Well, actually...
- A Column doesn't need directionality
- You can't use a Stack without directionality (it'll assert).
- Just adding a label on a Semantics node will assert without a directionality.
I've tried to add a test that will check that widgets that don't have any directionality end up with child order as a result, but really it's hard to do without a horizontal layout widget that ignores directionality, since I do still sort vertically in that case.
There was a problem hiding this comment.
You can use a stack without directionality, just set its alignment to something non-directional.
There was a problem hiding this comment.
Oooh! OK, I'll try that!
There was a problem hiding this comment.
OK, see below.
c46f8bf to
b9fe5ed
Compare
|
So there's good news and bad news. 👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there. 😕 The bad news is that it appears that one or more commits were authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request. Note to project maintainer: This is a terminal state, meaning the |
There was a problem hiding this comment.
did you mean to do this in this PR?
e962ea7 to
771b812
Compare
| // coordinate is also ignored when doing the comparison in that case, so | ||
| // this isn't actually expressing a directionality opinion. | ||
| globalStartCorner = _transformPoint( | ||
| containerTextDirection == TextDirection.rtl ? node.rect.topRight : node.rect.topLeft, |
There was a problem hiding this comment.
pass in topRight when direction is null, so we're more likely to catch regressions where someone starts depending on it
|
|
This adds geometric sort ordering back in for semantics nodes that don't have a sort order defined. With this change, widgets that either have no sort order, or have an equivalent sort order, will be compared geometrically. The comparison is between the two starting corners, so it is TextDirection-aware: parent nodes that are set to have LTR text will compare upper left corners of their children, and upper right when set to RTL. Also fixed a bug in the Transform widget that didn't mark modified nodes as needing a semantics update.
3 participants
This adds geometric sort ordering back in for semantics nodes that don't have a sort order defined.
With this change, widgets that either have no sort order, or have an equivalent sort order, will be compared geometrically. The comparison is between the two starting corners, so it is TextDirection-aware: parent nodes that are set to have LTR text will compare upper left corners of their children, and upper right when set to RTL.
Also fixed a bug in the Transform widget that didn't mark modified nodes as needing a semantics update.