[web][a11y]Delete _childContainerElement #163662
Merged
+157
−261
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This comment was marked as outdated.
This comment was marked as outdated.
9 tasks
819734c to
ea98f32
Compare
yjbanov
reviewed
Mar 8, 2025
|
|
||
| /// Computes the size and position of [element] and, if this element | ||
| /// [hasChildren], of [getOrCreateChildContainer]. | ||
| /// [hasChildren], Computes the parent adjustment for each child. |
| _clearSemanticElementTransform(containerElement); | ||
| /// Computes the size and position of children. | ||
| void updateChildrenPositionAndSize() { | ||
| for (var childOrder in _childrenInTraversalOrder!) { |
There was a problem hiding this comment.
nit: s/var childOrder/final childIndex/
| </sem>'''); | ||
| } | ||
|
|
||
| semantics().semanticsEnabled = false; | ||
| }); | ||
|
|
||
| test('container nodes are transparent and leaf children are opaque hit-test wise', () async { |
There was a problem hiding this comment.
I think we still need this test. The "container nodes" in the description does not refer to the intermediate container elements that you are removing. It refers to SemanticsNode whose only purpose is to contain children and affect their transforms. We want those nodes to be transparent hit-test wise, and we should continue testing for that.
| semantics().semanticsEnabled = false; | ||
| }); | ||
|
|
||
| test('containers can be opaque if tappable', () async { |
There was a problem hiding this comment.
This test is still useful for the same reason.
| semantics().semanticsEnabled = false; | ||
| }); | ||
|
|
||
| test('container can be opaque if it is a text field', () async { |
yjbanov
reviewed
Mar 12, 2025
| @@ -1661,7 +1626,7 @@ class SemanticsObject { | |||
| // Trivial case: previous list was empty => just populate the container. | |||
| if (_currentChildrenInRenderOrder == null || _currentChildrenInRenderOrder!.isEmpty) { | |||
| for (final SemanticsObject child in childrenInRenderOrder) { | |||
| containerElement!.append(child.element); | |||
| element!.append(child.element); | |||
There was a problem hiding this comment.
Without the container element could we have a problem with _scrollOverflowElement used by scrollables? This algorithm assumes that it controls all the children of the container. However, SemanticScrollable appends _scrollOverflowElement that this algorithm is unaware of. I think we're OK, since here we only use containerElement.append/insertBefore. For example, we don't use containerElement.children or containerElement.firstChild. However, it might be worth paying attention to. Previously, it was OK, because all children went into the container, and _scrollOverflowElement went into the root element.
There was a problem hiding this comment.
So _scrollOverflowElement was previously a sibling of containerElement and is now a sibling of children.
I think this is OK since, as you said, we are only using containerElement.append/insertBefore but not containerElement.children.
c7265ef to
e0b4cc3
Compare
yjbanov
approved these changes
Mar 14, 2025
fe49219 to
0950aef
Compare
52fb340 to
c829e2b
Compare
add back tests
update tests Update semantics_test.dart Update semantics_text_test.dart lint
c829e2b to
35fdfb9
Compare
9 tasks
|
A reason for requesting a revert of flutter/flutter/163662 could |
|
Reason for revert: google 3 failure |
auto-submit bot
pushed a commit
that referenced
this pull request
Mar 18, 2025
This reverts commit 6e74ee4.
9 tasks
github-merge-queue bot
pushed a commit
that referenced
this pull request
Mar 18, 2025
<!-- start_original_pr_link --> Reverts: #163662 <!-- end_original_pr_link --> <!-- start_initiating_author --> Initiated by: hannah-hyj <!-- end_initiating_author --> <!-- start_revert_reason --> Reason for reverting: google 3 failure <!-- end_revert_reason --> <!-- start_original_pr_author --> Original PR Author: hannah-hyj <!-- end_original_pr_author --> <!-- start_reviewers --> Reviewed By: {yjbanov} <!-- end_reviewers --> <!-- start_revert_body --> This change reverts the following previous change: delete _childContainerElement , add the rect compensate and scrolling adjustment to the children ## Pre-launch Checklist - [ ] I read the [Contributor Guide] and followed the process outlined there for submitting PRs. - [ ] I read the [Tree Hygiene] wiki page, which explains my responsibilities. - [ ] I read and followed the [Flutter Style Guide], including [Features we expect every widget to implement]. - [ ] I signed the [CLA]. - [ ] I listed at least one issue that this PR fixes in the description above. - [ ] I updated/added relevant documentation (doc comments with `///`). - [ ] I added new tests to check the change I am making, or this PR is [test-exempt]. - [ ] I followed the [breaking change policy] and added [Data Driven Fixes] where supported. - [ ] All existing and new tests are passing. If you need help, consider asking for advice on the #hackers-new channel on [Discord]. <!-- Links --> [Contributor Guide]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#overview [Tree Hygiene]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md [test-exempt]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#tests [Flutter Style Guide]: https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md [Features we expect every widget to implement]: https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md#features-we-expect-every-widget-to-implement [CLA]: https://cla.developers.google.com/ [flutter/tests]: https://github.com/flutter/tests [breaking change policy]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#handling-breaking-changes [Discord]: https://github.com/flutter/flutter/blob/main/docs/contributing/Chat.md [Data Driven Fixes]: https://github.com/flutter/flutter/blob/main/docs/contributing/Data-driven-Fixes.md <!-- end_revert_body --> Co-authored-by: auto-submit[bot] <flutter-engprod-team@google.com>
9 tasks
engine-flutter-autoroll
added a commit
to engine-flutter-autoroll/packages
that referenced
this pull request
Mar 20, 2025
github-merge-queue bot
pushed a commit
that referenced
this pull request
Mar 22, 2025
Reland #163662 with some changes 1. when a node's role changes and reparenting it, append children to it 2. Merge optimization from /pull/165352 fix: #45205 ## Pre-launch Checklist - [ ] I read the [Contributor Guide] and followed the process outlined there for submitting PRs. - [ ] I read the [Tree Hygiene] wiki page, which explains my responsibilities. - [ ] I read and followed the [Flutter Style Guide], including [Features we expect every widget to implement]. - [ ] I signed the [CLA]. - [ ] I listed at least one issue that this PR fixes in the description above. - [ ] I updated/added relevant documentation (doc comments with `///`). - [ ] I added new tests to check the change I am making, or this PR is [test-exempt]. - [ ] I followed the [breaking change policy] and added [Data Driven Fixes] where supported. - [ ] All existing and new tests are passing. If you need help, consider asking for advice on the #hackers-new channel on [Discord]. <!-- Links --> [Contributor Guide]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#overview [Tree Hygiene]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md [test-exempt]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#tests [Flutter Style Guide]: https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md [Features we expect every widget to implement]: https://github.com/flutter/flutter/blob/main/docs/contributing/Style-guide-for-Flutter-repo.md#features-we-expect-every-widget-to-implement [CLA]: https://cla.developers.google.com/ [flutter/tests]: https://github.com/flutter/tests [breaking change policy]: https://github.com/flutter/flutter/blob/main/docs/contributing/Tree-hygiene.md#handling-breaking-changes [Discord]: https://github.com/flutter/flutter/blob/main/docs/contributing/Chat.md [Data Driven Fixes]: https://github.com/flutter/flutter/blob/main/docs/contributing/Data-driven-Fixes.md
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
delete _childContainerElement , add the rect compensate and scrolling adjustment to the children
Pre-launch Checklist
///).If you need help, consider asking for advice on the #hackers-new channel on Discord.