-
Notifications
You must be signed in to change notification settings - Fork 3.4k
Breaking change flutter/flutter#134921 #9469
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Breaking change flutter/flutter#134921 #9469
Conversation
|
Just a heads up - The cloud build check (flutter-website-staging) can be ignored, it is something I am working on (#9468) that I accidentally triggered for all presubmit PRs. It has been turned off! |
|
Hi @chunhtai could you take a look and see if the semantics tree change makes sense? |
chunhtai
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the doc still valid? I remember we had a conversation whether the message should introduce a node at all.
| As a result, the generated semantics tree does not put `Tooltip.message` | ||
| immediately after `Tooltip.child`. | ||
|
|
||
| You may see accessibility test failures when a tooltip message is visible. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
avoid you in public doc
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What's the reason for avoiding using "you"? I see plenty of "you"s when i searched for it in this directory.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you are right, It shouldn't apply for design doc
|
|
||
| ## Migration guide | ||
|
|
||
| Update your failing accessibility tests. For the following widget tree: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe
The accessibility test before the migration:
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changed the wording to emphasize the "test" setting.
| You may see accessibility test failures when a tooltip message is visible. | ||
| This is because this update moved the tooltip message in the semantics tree. | ||
|
|
||
| ## Migration guide |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe we should mention the old behavior is no longer supported.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure we should say the current behavior is never making a comeback? The new behavior makes more sense as the default but maybe in the future it will be made configurable to allow developers to specify who the accessibility parent is, the overlay or the overlayportal?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
but for now there is no way to get back to original behavior, and I think we should communicate it more clearly. If we were to update the behavior later, we can update this doc with a new revision.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I found it pretty hard to describe what "the old behavior" is so I added "adopt the new semantics order in your tests".
ae45ea0 to
24f5354
Compare
chunhtai
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
parlough
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for this migration guide!
A few minor fixes:
| * [Windows: External windows should notify Flutter engine of lifecycle changes][] | ||
| * [Windows build path changed to add the target architecture][] | ||
| * [Deprecated API removed after v3.13][] | ||
| * [Accessibility traversal order of tooltip changed][] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You'll need to add the link definition for this link below.
Co-authored-by: Parker Lougheed <parlough@gmail.com>
…ooong/website into tooltip-message-accessibility
|
PTAL @atsansone |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you!
I'm going to merge this, but if anyone wants to see further technical or copy changes, please feel free to open a PR or an issue. Thanks!
Breaking change announcement for flutter/flutter#134921
Presubmit checklist