Fix `RangeSlider` semantics node size #114999

TahaTesser · 2022-11-09T16:39:18Z

Description

This PR fixes semantics node size for RangeSlider

Current behavior simply divides the node size in half. This doesn't match when testing on Android.

Android

Left	Right

Flutter

Before	After

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.

HansMuller · 2022-11-09T17:05:20Z

Does this all work correctly if the text direction is RTL? I expect that there should be a RTL version of the test.

TahaTesser · 2022-11-09T17:46:13Z

Does this all work correctly if the text direction is RTL? I expect that there should be a RTL version of the test.

Sure, we can add a semantics test for RTL

However, it looks like there is a bug in matchSemantics. Even if I match everything, it fails in RTL in the last node.
I will look into it further in an isolated test.

Semantics debug tree

SemanticsNode#5
 │ Rect.fromLTRB(0.0, 0.0, 800.0, 600.0)
 │ flags: scopesRoute
 │
 └─SemanticsNode#6
   │ Rect.fromLTRB(0.0, 0.0, 800.0, 600.0)
   │
   ├─SemanticsNode#1
   │   Rect.fromLTRB(526.4, 276.0, 574.4, 324.0)
   │   actions: decrease, increase
   │   flags: hasEnabledState, isEnabled, isSlider
   │   value: "10%"
   │   increasedValue: "15%"
   │   decreasedValue: "5%"
   │   textDirection: rtl
   │
   └─SemanticsNode#2
       Rect.fromLTRB(676.8, 276.0, 724.8, 324.0)
       actions: decrease, increase
       flags: hasEnabledState, isEnabled, isSlider
       value: "30%"
       increasedValue: "35%"
       decreasedValue: "25%"
       textDirection: rtl

test:

              matchesSemantics(
                isEnabled: true,
                isSlider: true,
                hasEnabledState: true,
                hasIncreaseAction: true,
                hasDecreaseAction: true,
                value: '10%',
                increasedValue: '15%',
                decreasedValue: '5%',
                rect: const Rect.fromLTRB(526.4, 276.0, 574.4, 324.0),
              ),
              matchesSemantics(
                isEnabled: true,
                isSlider: true,
                hasEnabledState: true,
                hasIncreaseAction: true,
                hasDecreaseAction: true,
                value: '30%',
                increasedValue: '35%',
                decreasedValue: '25%',
                rect: const Rect.fromLTRB(676.8, 276.0, 724.8, 324.0),
              ),

Result:

 Actual: SemanticsNode:<SemanticsNode#5(Rect.fromLTRB(0.0, 0.0, 800.0, 600.0), flags:
[scopesRoute])>
   Which: rect was: Rect.fromLTRB(676.8, 276.0, 724.8, 324.0)

Piinks · 2022-11-09T17:59:58Z

it looks like there is a bug in matchSemantics

Is there a bug filed for this? @chunhtai can you take a look and confirm?

TahaTesser · 2022-11-10T13:42:28Z

it looks like there is a bug in matchSemantics

Is there a bug filed for this? @chunhtai can you take a look and confirm?

I've investigated the problem and filed an issue #115079

Also fixed the tests here with a workaround.

HansMuller

Nice fix. Please add a TODO comment above the workaround for #115079 so that we'll remember to fix it.

* bdcacbf 1c1ad0e72 Roll Fuchsia Linux SDK from RNSA2Wp1MObtc7OHy... to mduclAELKkftJ-fwI... (flutter/engine#37495) (flutter/flutter#115088) * ca61652 Roll ios-deploy iOS artifact (flutter/flutter#115042) * b31b9dc 057c53a05 [Impeller] Remove duplicate code in 'TiledTextureContents' (flutter/engine#37492) (flutter/flutter#115093) * 19b351b f467eee51 Turned on performance-move-const-arg everywhere. (flutter/engine#37482) (flutter/flutter#115095) * 5a60045 Reland: Adds support for the Material Badge widget, BadgeTheme, BadgeThemeData (#114560) (flutter/flutter#115002) * 09a4f23 Prevent tests from producing dill files alongside the test file (flutter/flutter#115075) * 1f891a0 Fix `RangeSlider` semantics node size (flutter/flutter#114999) * c1ceda4 Include stdout in codesign failure output (flutter/flutter#115115) * 2e432ec chore: enable Flutter Android workflow on aarch64 (flutter/flutter#113829) * ad72a45 12dabe83d Felt analyze (flutter/engine#37481) (flutter/flutter#115102) * 4e7dbef Explicitly call out documentation links in "flutter create" (flutter/flutter#114181) * d413081 Fix shrinkwrap on animated list (flutter/flutter#115104) * bd00a93 Roll Flutter Engine from 12dabe83da19 to 4cb9c1e00a84 (5 revisions) (flutter/flutter#115118) * 8858c17 Roll Flutter Engine from 4cb9c1e00a84 to 30e2cedfa2ac (2 revisions) (flutter/flutter#115122) * 0f2f4b8 Roll Flutter Engine from 30e2cedfa2ac to cae1716611a4 (2 revisions) (flutter/flutter#115124) * c6561e6 53cfb94c5 Roll Skia from 65b60707772d to b50a2f714837 (46 revisions) (flutter/engine#37501) (flutter/flutter#115128) * 6e89042 0721c860e [Impeller] null check samplers (flutter/engine#37489) (flutter/flutter#115132) * 88e49ed Load assets in flutter_test without turning event loop. (flutter/flutter#115123) * f997281 Roll Flutter Engine from 0721c860e2c3 to cb78576e5fec (3 revisions) (flutter/flutter#115134) * 0d081f9 Roll Flutter Engine from cb78576e5fec to a41712cec63a (2 revisions) (flutter/flutter#115135) * 83cda7e 19ead2834 Roll Fuchsia Linux SDK from mduclAELKkftJ-fwI... to R3L54Ma3VT2TQ6RpI... (flutter/engine#37513) (flutter/flutter#115139) * c021d91 [flutter_tools] support github reporter (flutter/flutter#115137) * 413bb21 e32fc0044 Roll Skia from b50f741ef341 to 77620568f467 (5 revisions) (flutter/engine#37514) (flutter/flutter#115142) * 098aac7 7326e5b33 Roll Dart SDK from 3a022103e91d to 1e37edb1f6c1 (1 revision) (flutter/engine#37515) (flutter/flutter#115145)

* 19b351b f467eee51 Turned on performance-move-const-arg everywhere. (flutter/engine#37482) (flutter/flutter#115095) * 5a60045 Reland: Adds support for the Material Badge widget, BadgeTheme, BadgeThemeData (#114560) (flutter/flutter#115002) * 09a4f23 Prevent tests from producing dill files alongside the test file (flutter/flutter#115075) * 1f891a0 Fix `RangeSlider` semantics node size (flutter/flutter#114999) * c1ceda4 Include stdout in codesign failure output (flutter/flutter#115115) * 2e432ec chore: enable Flutter Android workflow on aarch64 (flutter/flutter#113829) * ad72a45 12dabe83d Felt analyze (flutter/engine#37481) (flutter/flutter#115102) * 4e7dbef Explicitly call out documentation links in "flutter create" (flutter/flutter#114181) * d413081 Fix shrinkwrap on animated list (flutter/flutter#115104) * bd00a93 Roll Flutter Engine from 12dabe83da19 to 4cb9c1e00a84 (5 revisions) (flutter/flutter#115118) * 8858c17 Roll Flutter Engine from 4cb9c1e00a84 to 30e2cedfa2ac (2 revisions) (flutter/flutter#115122) * 0f2f4b8 Roll Flutter Engine from 30e2cedfa2ac to cae1716611a4 (2 revisions) (flutter/flutter#115124) * c6561e6 53cfb94c5 Roll Skia from 65b60707772d to b50a2f714837 (46 revisions) (flutter/engine#37501) (flutter/flutter#115128) * 6e89042 0721c860e [Impeller] null check samplers (flutter/engine#37489) (flutter/flutter#115132) * 88e49ed Load assets in flutter_test without turning event loop. (flutter/flutter#115123) * f997281 Roll Flutter Engine from 0721c860e2c3 to cb78576e5fec (3 revisions) (flutter/flutter#115134) * 0d081f9 Roll Flutter Engine from cb78576e5fec to a41712cec63a (2 revisions) (flutter/flutter#115135) * 83cda7e 19ead2834 Roll Fuchsia Linux SDK from mduclAELKkftJ-fwI... to R3L54Ma3VT2TQ6RpI... (flutter/engine#37513) (flutter/flutter#115139) * c021d91 [flutter_tools] support github reporter (flutter/flutter#115137) * 413bb21 e32fc0044 Roll Skia from b50f741ef341 to 77620568f467 (5 revisions) (flutter/engine#37514) (flutter/flutter#115142) * 098aac7 7326e5b33 Roll Dart SDK from 3a022103e91d to 1e37edb1f6c1 (1 revision) (flutter/engine#37515) (flutter/flutter#115145)

* bdcacbf 1c1ad0e72 Roll Fuchsia Linux SDK from RNSA2Wp1MObtc7OHy... to mduclAELKkftJ-fwI... (flutter/engine#37495) (flutter/flutter#115088) * ca61652 Roll ios-deploy iOS artifact (flutter/flutter#115042) * b31b9dc 057c53a05 [Impeller] Remove duplicate code in 'TiledTextureContents' (flutter/engine#37492) (flutter/flutter#115093) * 19b351b f467eee51 Turned on performance-move-const-arg everywhere. (flutter/engine#37482) (flutter/flutter#115095) * 5a60045 Reland: Adds support for the Material Badge widget, BadgeTheme, BadgeThemeData (#114560) (flutter/flutter#115002) * 09a4f23 Prevent tests from producing dill files alongside the test file (flutter/flutter#115075) * 1f891a0 Fix `RangeSlider` semantics node size (flutter/flutter#114999) * c1ceda4 Include stdout in codesign failure output (flutter/flutter#115115) * 2e432ec chore: enable Flutter Android workflow on aarch64 (flutter/flutter#113829) * ad72a45 12dabe83d Felt analyze (flutter/engine#37481) (flutter/flutter#115102) * 4e7dbef Explicitly call out documentation links in "flutter create" (flutter/flutter#114181) * d413081 Fix shrinkwrap on animated list (flutter/flutter#115104) * bd00a93 Roll Flutter Engine from 12dabe83da19 to 4cb9c1e00a84 (5 revisions) (flutter/flutter#115118) * 8858c17 Roll Flutter Engine from 4cb9c1e00a84 to 30e2cedfa2ac (2 revisions) (flutter/flutter#115122) * 0f2f4b8 Roll Flutter Engine from 30e2cedfa2ac to cae1716611a4 (2 revisions) (flutter/flutter#115124) * c6561e6 53cfb94c5 Roll Skia from 65b60707772d to b50a2f714837 (46 revisions) (flutter/engine#37501) (flutter/flutter#115128) * 6e89042 0721c860e [Impeller] null check samplers (flutter/engine#37489) (flutter/flutter#115132) * 88e49ed Load assets in flutter_test without turning event loop. (flutter/flutter#115123) * f997281 Roll Flutter Engine from 0721c860e2c3 to cb78576e5fec (3 revisions) (flutter/flutter#115134) * 0d081f9 Roll Flutter Engine from cb78576e5fec to a41712cec63a (2 revisions) (flutter/flutter#115135) * 83cda7e 19ead2834 Roll Fuchsia Linux SDK from mduclAELKkftJ-fwI... to R3L54Ma3VT2TQ6RpI... (flutter/engine#37513) (flutter/flutter#115139) * c021d91 [flutter_tools] support github reporter (flutter/flutter#115137) * 413bb21 e32fc0044 Roll Skia from b50f741ef341 to 77620568f467 (5 revisions) (flutter/engine#37514) (flutter/flutter#115142) * 098aac7 7326e5b33 Roll Dart SDK from 3a022103e91d to 1e37edb1f6c1 (1 revision) (flutter/engine#37515) (flutter/flutter#115145)

* Fix `RangeSlider` semantics node size and add RTL semantics test * Add TODO comments

* bdcacbf 1c1ad0e72 Roll Fuchsia Linux SDK from RNSA2Wp1MObtc7OHy... to mduclAELKkftJ-fwI... (flutter/engine#37495) (flutter/flutter#115088) * ca61652 Roll ios-deploy iOS artifact (flutter/flutter#115042) * b31b9dc 057c53a05 [Impeller] Remove duplicate code in 'TiledTextureContents' (flutter/engine#37492) (flutter/flutter#115093) * 19b351b f467eee51 Turned on performance-move-const-arg everywhere. (flutter/engine#37482) (flutter/flutter#115095) * 5a60045 Reland: Adds support for the Material Badge widget, BadgeTheme, BadgeThemeData (#114560) (flutter/flutter#115002) * 09a4f23 Prevent tests from producing dill files alongside the test file (flutter/flutter#115075) * 1f891a0 Fix `RangeSlider` semantics node size (flutter/flutter#114999) * c1ceda4 Include stdout in codesign failure output (flutter/flutter#115115) * 2e432ec chore: enable Flutter Android workflow on aarch64 (flutter/flutter#113829) * ad72a45 12dabe83d Felt analyze (flutter/engine#37481) (flutter/flutter#115102) * 4e7dbef Explicitly call out documentation links in "flutter create" (flutter/flutter#114181) * d413081 Fix shrinkwrap on animated list (flutter/flutter#115104) * bd00a93 Roll Flutter Engine from 12dabe83da19 to 4cb9c1e00a84 (5 revisions) (flutter/flutter#115118) * 8858c17 Roll Flutter Engine from 4cb9c1e00a84 to 30e2cedfa2ac (2 revisions) (flutter/flutter#115122) * 0f2f4b8 Roll Flutter Engine from 30e2cedfa2ac to cae1716611a4 (2 revisions) (flutter/flutter#115124) * c6561e6 53cfb94c5 Roll Skia from 65b60707772d to b50a2f714837 (46 revisions) (flutter/engine#37501) (flutter/flutter#115128) * 6e89042 0721c860e [Impeller] null check samplers (flutter/engine#37489) (flutter/flutter#115132) * 88e49ed Load assets in flutter_test without turning event loop. (flutter/flutter#115123) * f997281 Roll Flutter Engine from 0721c860e2c3 to cb78576e5fec (3 revisions) (flutter/flutter#115134) * 0d081f9 Roll Flutter Engine from cb78576e5fec to a41712cec63a (2 revisions) (flutter/flutter#115135) * 83cda7e 19ead2834 Roll Fuchsia Linux SDK from mduclAELKkftJ-fwI... to R3L54Ma3VT2TQ6RpI... (flutter/engine#37513) (flutter/flutter#115139) * c021d91 [flutter_tools] support github reporter (flutter/flutter#115137) * 413bb21 e32fc0044 Roll Skia from b50f741ef341 to 77620568f467 (5 revisions) (flutter/engine#37514) (flutter/flutter#115142) * 098aac7 7326e5b33 Roll Dart SDK from 3a022103e91d to 1e37edb1f6c1 (1 revision) (flutter/engine#37515) (flutter/flutter#115145)

flutter-dashboard bot added f: material design flutter/packages/flutter/material repository. framework flutter/packages/flutter repository. See also f: labels. labels Nov 9, 2022

TahaTesser marked this pull request as ready for review November 9, 2022 16:39

TahaTesser requested a review from HansMuller November 9, 2022 16:39

TahaTesser changed the title ~~Fix RangeSlider semantics node rect~~ Fix RangeSlider semantics node size Nov 9, 2022

TahaTesser force-pushed the range_slider_semantic_nodes branch from cb5aba1 to 0bd7fd2 Compare November 9, 2022 16:40

TahaTesser requested a review from Piinks November 9, 2022 16:40

TahaTesser added the a: accessibility Accessibility, e.g. VoiceOver or TalkBack. (aka a11y) label Nov 9, 2022

TahaTesser force-pushed the range_slider_semantic_nodes branch from 0bd7fd2 to 0acf368 Compare November 10, 2022 13:02

Fix RangeSlider semantics node size and add RTL semantics test

adea3a0

TahaTesser force-pushed the range_slider_semantic_nodes branch from 0acf368 to adea3a0 Compare November 10, 2022 13:07

TahaTesser mentioned this pull request Nov 10, 2022

getSemantics.rect != matchesSemantics.rect #115079

Open

HansMuller approved these changes Nov 10, 2022

View reviewed changes

Add TODO comments

f4baed7

TahaTesser added the autosubmit Merge PR when tree becomes green via auto submit App label Nov 10, 2022

auto-submit bot merged commit 1f891a0 into flutter:master Nov 10, 2022

TahaTesser deleted the range_slider_semantic_nodes branch November 10, 2022 22:35

This was referenced Nov 11, 2022

Roll Flutter from c7d11549a51e to 098aac7ffeef (24 revisions) flutter/plugins#6692

Merged

Roll Flutter from b31b9dc05f99 to 098aac7ffeef (21 revisions) flutter/packages#2802

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/packages that referenced this pull request Nov 11, 2022

1f891a0 Fix RangeSlider semantics node size (flutter/flutter#114999)

844d966

engine-flutter-autoroll added a commit to engine-flutter-autoroll/plugins that referenced this pull request Nov 11, 2022

1f891a0 Fix RangeSlider semantics node size (flutter/flutter#114999)

76ced76

shogohida pushed a commit to shogohida/flutter that referenced this pull request Dec 7, 2022

Fix RangeSlider semantics node size (flutter#114999)

93368af

* Fix `RangeSlider` semantics node size and add RTL semantics test * Add TODO comments

gspencergoog pushed a commit to gspencergoog/flutter that referenced this pull request Jan 19, 2023

Fix RangeSlider semantics node size (flutter#114999)

936508f

* Fix `RangeSlider` semantics node size and add RTL semantics test * Add TODO comments

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Fix `RangeSlider` semantics node size #114999

Fix `RangeSlider` semantics node size #114999

Uh oh!

TahaTesser commented Nov 9, 2022 •

edited

Loading

Uh oh!

HansMuller commented Nov 9, 2022

Uh oh!

TahaTesser commented Nov 9, 2022 •

edited

Loading

Uh oh!

Piinks commented Nov 9, 2022

Uh oh!

TahaTesser commented Nov 10, 2022 •

edited

Loading

Uh oh!

HansMuller left a comment

Uh oh!

Uh oh!

Fix RangeSlider semantics node size #114999

Fix RangeSlider semantics node size #114999

Uh oh!

Conversation

TahaTesser commented Nov 9, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Android

Flutter

Pre-launch Checklist

Uh oh!

HansMuller commented Nov 9, 2022

Uh oh!

TahaTesser commented Nov 9, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Piinks commented Nov 9, 2022

Uh oh!

TahaTesser commented Nov 10, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

HansMuller left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Fix `RangeSlider` semantics node size #114999

Fix `RangeSlider` semantics node size #114999

TahaTesser commented Nov 9, 2022 •

edited

Loading

TahaTesser commented Nov 9, 2022 •

edited

Loading

TahaTesser commented Nov 10, 2022 •

edited

Loading