`OverlayPortal.overlayChild` contributes semantics to `OverlayPortal` instead of `Overlay` #134921

LongCatIsLooong · 2023-09-18T07:28:43Z

Fixes #134456

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.
All existing and new tests are passing.

If you need help, consider asking for advice on the #hackers-new channel on Discord.

goderbauer · 2023-09-18T17:15:14Z

Looks like google testing is unhappy.

LongCatIsLooong · 2023-09-18T17:20:44Z

Looks like an infra thing I'll squash the commits.

chunhtai · 2023-09-19T19:30:12Z

packages/flutter/lib/src/rendering/object.dart

semanticsParent/semanticsChild are confusing names. maybe something like parentWithInterestingFragment or childWithInterestingFragment

I think even if a render object generated a ContainerFragment it is still going to be in this list.

what about parentInSemanticsFragmentOrder and childInSemanticsFragmentOrder

renamed to parent/childFragmentOwner

chunhtai · 2023-09-19T22:41:23Z

packages/flutter/lib/src/widgets/overlay.dart

It took me awhile to understand this only traverse child added from overlayportal. Can you add some doc to visitChildrenOfOverlayEntry or maybe rename it to something like visitOverlayPortalChildren?

chunhtai · 2023-09-19T22:50:51Z

packages/flutter/lib/src/rendering/object.dart

I thought it would still come to this case if there are rendering object that creates _ContainerSemanticsFragment inbetween semanticsChild and semanticsParent ?

I ran existing tests with a print statement and it seems that isn't happening in tests. In _getSemanticsForParent, every fragment in the sibling merge/merge up groups calls addAncestor(this).

chunhtai · 2023-09-19T22:55:16Z

packages/flutter/lib/src/rendering/object.dart

what happen if this loop past semanticsParent?

At least in the case of OverlayPortal, the final ancestor will be an ancestor of semanticsParent. Unfortunately we don't cache the clipping results from semanticsParent and semanticsChild's common ancestor so we can't utilize the previously computed clipping rects.

yes I understood the logic now, makes sense

chunhtai

LGTM, thanks for fixing this!

chunhtai · 2023-09-29T16:33:02Z

packages/flutter/lib/src/rendering/object.dart

nits: format

chunhtai · 2023-09-29T16:45:46Z

packages/flutter/lib/src/rendering/object.dart

yes I understood the logic now, makes sense

chunhtai · 2023-09-29T16:46:57Z

packages/flutter/lib/src/rendering/object.dart

what about parentInSemanticsFragmentOrder and childInSemanticsFragmentOrder

chunhtai · 2023-09-29T16:48:08Z

packages/flutter/lib/src/widgets/overlay.dart

Suggested change

// A convenience method for accessing `paintOrderIterator` using a

// A convenience method for accessing `paintOrderIterator` with a

LongCatIsLooong · 2023-09-30T06:12:59Z

Added a Semantics(container: true) to the tooltip on the overlay to prevent the semantics of tooltip's text from merging with that of Tooltip.child.

LongCatIsLooong · 2023-10-18T21:53:25Z

Hi @gspencergoog does the semantics tree change in menu_anchor_tests.dart make sense to you?

BEFORE

AFTER

SemanticsNode#0
 │ Rect.fromLTRB(0.0, 0.0, 2400.0, 1800.0)
 │
 └─SemanticsNode#1
   │ Rect.fromLTRB(0.0, 0.0, 800.0, 600.0) scaled by 3.0x
   │ textDirection: ltr
   │
   ├─SemanticsNode#2
   │ │ Rect.fromLTRB(0.0, 0.0, 800.0, 600.0)
   │ │ sortKey: OrdinalSortKey#8299f(order: 0.0)
   │ │
   │ └─SemanticsNode#3
   │   │ Rect.fromLTRB(0.0, 0.0, 800.0, 600.0)
   │   │ flags: scopesRoute
   │   │
   │   └─SemanticsNode#4
   │     │ merge boundary ⛔️
   │     │ Rect.fromLTRB(356.0, 276.0, 444.0, 324.0)
   │     │ flags: hasExpandedState, isExpanded
   │     │
   │     └─SemanticsNode#5
   │         merged up ⬆️
   │         Rect.fromLTRB(0.0, 0.0, 88.0, 48.0)
   │         actions: tap
   │         flags: isFocused, hasEnabledState, isEnabled, isFocusable
   │         label: "ABC"
   │         textDirection: ltr
   │
   └─SemanticsNode#6
     │ Rect.fromLTRB(356.0, 316.0, 476.0, 380.0)
     │ thickness: 3.0
     │
     └─SemanticsNode#7
       │ Rect.fromLTRB(0.0, 8.0, 120.0, 56.0)
       │ flags: hasImplicitScrolling
       │ scrollExtentMin: 0.0
       │ scrollPosition: 0.0
       │ scrollExtentMax: 0.0
       │ elevation: 3.0
       │
       └─SemanticsNode#8
         │ merge boundary ⛔️
         │ Rect.fromLTRB(0.0, 0.0, 120.0, 48.0)
         │
         └─SemanticsNode#9
             merged up ⬆️
             Rect.fromLTRB(0.0, 0.0, 120.0, 48.0)
             actions: tap
             flags: hasEnabledState, isEnabled, isFocusable
             label: "Item 0"
             textDirection: ltr

SemanticsNode#0
 │ Rect.fromLTRB(0.0, 0.0, 2400.0, 1800.0)
 │
 └─SemanticsNode#1
   │ Rect.fromLTRB(0.0, 0.0, 800.0, 600.0) scaled by 3.0x
   │ textDirection: ltr
   │
   └─SemanticsNode#2
     │ Rect.fromLTRB(0.0, 0.0, 800.0, 600.0)
     │ sortKey: OrdinalSortKey#bfee0(order: 0.0)
     │
     └─SemanticsNode#3
       │ Rect.fromLTRB(0.0, 0.0, 800.0, 600.0)
       │ flags: scopesRoute
       │
       ├─SemanticsNode#4
       │ │ merge boundary ⛔️
       │ │ Rect.fromLTRB(356.0, 276.0, 444.0, 324.0)
       │ │ flags: hasExpandedState, isExpanded
       │ │
       │ └─SemanticsNode#5
       │     merged up ⬆️
       │     Rect.fromLTRB(0.0, 0.0, 88.0, 48.0)
       │     actions: tap
       │     flags: isFocused, hasEnabledState, isEnabled, isFocusable
       │     label: "ABC"
       │     textDirection: ltr
       │
       └─SemanticsNode#6
         │ Rect.fromLTRB(356.0, 316.0, 476.0, 380.0)
         │ thickness: 3.0
         │
         └─SemanticsNode#7
           │ Rect.fromLTRB(0.0, 8.0, 120.0, 56.0)
           │ flags: hasImplicitScrolling
           │ scrollExtentMin: 0.0
           │ scrollPosition: 0.0
           │ scrollExtentMax: 0.0
           │ elevation: 3.0
           │
           └─SemanticsNode#8
             │ merge boundary ⛔️
             │ Rect.fromLTRB(0.0, 0.0, 120.0, 48.0)
             │
             └─SemanticsNode#9
                 merged up ⬆️
                 Rect.fromLTRB(0.0, 0.0, 120.0, 48.0)
                 actions: tap
                 flags: hasEnabledState, isEnabled, isFocusable
                 label: "Item 0"
                 textDirection: ltr

gspencergoog · 2023-10-18T22:02:02Z

Honestly, I don't see any difference in those two trees.

LongCatIsLooong · 2023-10-18T22:17:59Z

Honestly, I don't see any difference in those two trees.

Oops sorry copied the wrong tree. Updated the comment.

gspencergoog · 2023-10-18T22:30:07Z

Okay, if I understand that correctly, it sounds OK to me.

… instead of `Overlay`

…yPortal` instead of `Overlay` (flutter/flutter#134921)

flutter/flutter@823e083...5e8b5f4 2023-10-23 [email protected] Update `ColorScheme.fromSwatch` docs for Material 3 (flutter/flutter#136816) 2023-10-23 [email protected] Reland "Fixes ability to call nextFocus() on a node to focus its descâ�¦ (flutter/flutter#136898) 2023-10-23 [email protected] `OverlayPortal.overlayChild` contributes semantics to `OverlayPortal` instead of `Overlay` (flutter/flutter#134921) 2023-10-23 [email protected] Roll Packages from be915be to 4bf5114 (12 revisions) (flutter/flutter#137062) 2023-10-23 [email protected] Roll Flutter Engine from 8820a419c800 to 1dc66e8f2fba (1 revision) (flutter/flutter#137055) 2023-10-23 [email protected] Roll Flutter Engine from e2abdeb650f8 to 8820a419c800 (1 revision) (flutter/flutter#137051) 2023-10-23 [email protected] Roll Flutter Engine from 39f78ce1884d to e2abdeb650f8 (1 revision) (flutter/flutter#137050) 2023-10-23 [email protected] Roll Flutter Engine from 4899344c34d9 to 39f78ce1884d (1 revision) (flutter/flutter#137048) 2023-10-23 [email protected] Roll Flutter Engine from 9d05b061e245 to 4899344c34d9 (1 revision) (flutter/flutter#137046) 2023-10-23 [email protected] Roll Flutter Engine from cde34b8182b1 to 9d05b061e245 (1 revision) (flutter/flutter#137044) 2023-10-22 [email protected] Add timeline events for post frame callbacks (flutter/flutter#136435) 2023-10-22 [email protected] Roll Flutter Engine from b8d702c0e0d7 to cde34b8182b1 (1 revision) (flutter/flutter#137038) 2023-10-22 [email protected] Roll Flutter Engine from f3c7a50bc83f to b8d702c0e0d7 (1 revision) (flutter/flutter#137035) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-packages Please CC [email protected],[email protected],[email protected] on the revert to ensure that a human is aware of the problem. To file a bug in Packages: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://issues.skia.org/issues/new?component=1389291&template=1850622 Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md

Breaking change announcement for flutter/flutter#134921 --------- Co-authored-by: Parker Lougheed <[email protected]>

LongCatIsLooong changed the title ~~OverlayPortal.overlayChild contributes semantics to OverlayPortal instead of Overlay/~~ OverlayPortal.overlayChild contributes semantics to OverlayPortal instead of Overlay Sep 18, 2023

github-actions bot added the framework flutter/packages/flutter repository. See also f: labels. label Sep 18, 2023

LongCatIsLooong force-pushed the overlayportal-semantics branch from 951f09a to 7279a5d Compare September 18, 2023 07:32

LongCatIsLooong marked this pull request as ready for review September 18, 2023 08:11

LongCatIsLooong requested review from chunhtai and goderbauer September 18, 2023 08:11

LongCatIsLooong force-pushed the overlayportal-semantics branch 2 times, most recently from 709d8c2 to 546ccb0 Compare September 19, 2023 19:53

chunhtai reviewed Sep 19, 2023

View reviewed changes

LongCatIsLooong requested a review from chunhtai September 26, 2023 17:27

LongCatIsLooong force-pushed the overlayportal-semantics branch from 69bfcbd to d8352f1 Compare September 26, 2023 22:08

chunhtai approved these changes Sep 29, 2023

View reviewed changes

github-actions bot added the f: material design flutter/packages/flutter/material repository. label Sep 30, 2023

LongCatIsLooong added a commit to LongCatIsLooong/website that referenced this pull request Sep 30, 2023

Breaking change flutter/flutter#134921

e66966f

LongCatIsLooong added a commit to LongCatIsLooong/website that referenced this pull request Sep 30, 2023

Breaking change flutter/flutter#134921

ae95bba

LongCatIsLooong force-pushed the overlayportal-semantics branch from 455813a to e3b7e19 Compare September 30, 2023 10:53

LongCatIsLooong mentioned this pull request Sep 30, 2023

Breaking change flutter/flutter#134921 flutter/website#9469

Merged

3 tasks

LongCatIsLooong requested a review from gspencergoog October 18, 2023 21:54

OverlayPortal.overlayChild contributes semantics to OverlayPortal…

c59fe2c

… instead of `Overlay`

LongCatIsLooong force-pushed the overlayportal-semantics branch from d8ce12e to c59fe2c Compare October 19, 2023 00:38

LongCatIsLooong added a commit to LongCatIsLooong/website that referenced this pull request Oct 19, 2023

Breaking change flutter/flutter#134921

24f5354

LongCatIsLooong added the autosubmit Merge PR when tree becomes green via auto submit App label Oct 23, 2023

auto-submit bot merged commit af129b6 into flutter:master Oct 23, 2023

LongCatIsLooong deleted the overlayportal-semantics branch October 23, 2023 15:46

engine-flutter-autoroll mentioned this pull request Oct 23, 2023

Roll Flutter from 823e08353dd8 to 5e8b5f4ea293 (13 revisions) flutter/packages#5208

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Oct 23, 2023

af129b6 OverlayPortal.overlayChild contributes semantics to `Overla…

fae6dd5

…yPortal` instead of `Overlay` (flutter/flutter#134921)

parlough added a commit to flutter/website that referenced this pull request Oct 23, 2023

Breaking change flutter/flutter#134921 (#9469)

3f9f5cb

Breaking change announcement for flutter/flutter#134921 --------- Co-authored-by: Parker Lougheed <[email protected]>

HansMuller mentioned this pull request Nov 1, 2023

Flutter Framework Team: Notable Commits #121415

Closed

	// A convenience method for accessing `paintOrderIterator` using a
	// A convenience method for accessing `paintOrderIterator` with a

OverlayPortal.overlayChild contributes semantics to OverlayPortal instead of Overlay #134921

OverlayPortal.overlayChild contributes semantics to OverlayPortal instead of Overlay #134921

Uh oh!

Conversation

LongCatIsLooong commented Sep 18, 2023

Pre-launch Checklist

Uh oh!

goderbauer commented Sep 18, 2023

Uh oh!

LongCatIsLooong commented Sep 18, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

LongCatIsLooong Sep 24, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

chunhtai left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

LongCatIsLooong commented Sep 30, 2023

Uh oh!

LongCatIsLooong commented Oct 18, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gspencergoog commented Oct 18, 2023

Uh oh!

LongCatIsLooong commented Oct 18, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gspencergoog commented Oct 18, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

`OverlayPortal.overlayChild` contributes semantics to `OverlayPortal` instead of `Overlay` #134921

`OverlayPortal.overlayChild` contributes semantics to `OverlayPortal` instead of `Overlay` #134921

LongCatIsLooong Sep 24, 2023 •

edited

Loading

LongCatIsLooong commented Oct 18, 2023 •

edited

Loading

LongCatIsLooong commented Oct 18, 2023 •

edited

Loading