Skip to content

Conversation

@LongCatIsLooong
Copy link
Contributor

Breaking change announcement for flutter/flutter#134921

Presubmit checklist

@drewroengoogle
Copy link
Contributor

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!

@LongCatIsLooong
Copy link
Contributor Author

Hi @chunhtai could you take a look and see if the semantics tree change makes sense?

@atsansone atsansone added the st.WIP Issue in progress label Oct 7, 2023
Copy link
Contributor

@chunhtai chunhtai left a 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.
Copy link
Contributor

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

Copy link
Contributor Author

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.

Copy link
Contributor

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:
Copy link
Contributor

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:

Copy link
Contributor Author

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
Copy link
Contributor

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.

Copy link
Contributor Author

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?

Copy link
Contributor

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.

Copy link
Contributor Author

@LongCatIsLooong LongCatIsLooong Oct 19, 2023

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".

@LongCatIsLooong LongCatIsLooong force-pushed the tooltip-message-accessibility branch from ae45ea0 to 24f5354 Compare October 19, 2023 17:24
@LongCatIsLooong LongCatIsLooong marked this pull request as ready for review October 19, 2023 17:40
Copy link
Contributor

@chunhtai chunhtai left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@LongCatIsLooong LongCatIsLooong removed the st.WIP Issue in progress label Oct 19, 2023
@parlough parlough added the review.copy Awaiting Copy Review label Oct 22, 2023
Copy link
Member

@parlough parlough left a 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][]
Copy link
Member

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.

LongCatIsLooong and others added 3 commits October 22, 2023 15:11
Co-authored-by: Parker Lougheed <parlough@gmail.com>
…ooong/website into tooltip-message-accessibility
@domesticmouse
Copy link
Contributor

PTAL @atsansone

Copy link
Member

@parlough parlough left a 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!

@parlough parlough removed the review.copy Awaiting Copy Review label Oct 23, 2023
@parlough parlough merged commit 3f9f5cb into flutter:main Oct 23, 2023
@LongCatIsLooong LongCatIsLooong deleted the tooltip-message-accessibility branch October 23, 2023 17:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants