Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Add missing kHasImplicitScrolling enum value #8030

Merged
merged 1 commit into from
Mar 5, 2019
Merged

Add missing kHasImplicitScrolling enum value #8030

merged 1 commit into from
Mar 5, 2019

Conversation

cbracken
Copy link
Member

@cbracken cbracken commented Mar 5, 2019 •
edited
Loading

This brings the Dart and C++ semantics flag enums back in sync.

In #5941, implicit scrolling support was added to SemanticsFlag in
dart:ui, and to the Android embedder, but not to the SemanticsFlags enum
on the C++ side.

This also adds this flag to the embedder API.

goderbauer
goderbauer reviewed Mar 5, 2019
View reviewed changes
shell/platform/embedder/embedder.h Outdated
// to move focus to an offscreen child.
//
// For example, a ListView widget has implicit scrolling so that users can
// easily move to the next visible set of children. A TabBar widget does not
Copy link
Member

Choose a reason for hiding this comment

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

The TabBar widget itself should have implicit scrolling turned on. The view that shows the tab's content should have it turned off to prevent you from switching tabs by just moving a11y focus around.

Copy link
Member Author

@cbracken cbracken Mar 5, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

These comments are copied from the existing documentation for this field in the dart code. Do you have suggested wording? I can update both at the same time.

Copy link
Member Author

Choose a reason for hiding this comment

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

Updated.

@cbracken
Copy link
Member Author

cbracken commented Mar 5, 2019

I've got a followup PR for actions momentarily.

goderbauer
goderbauer reviewed Mar 5, 2019
View reviewed changes
shell/platform/embedder/embedder.h Outdated
// to move focus to an offscreen child.
//
// For example, a ListView widget has implicit scrolling so that users can
// easily move to the next visible set of children. A TabBar widget does not
Copy link
Member

Choose a reason for hiding this comment

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

"visible" is easy to misinterpreted here. The focus moves to the currently invisible set of children and makes those visible...

Copy link
Member Author

Choose a reason for hiding this comment

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

Updated.

chinmaygarde
chinmaygarde approved these changes Mar 5, 2019
View reviewed changes
Copy link
Member

@chinmaygarde chinmaygarde left a comment

Choose a reason for hiding this comment

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

Can we remove the static cast for enum values in the embedder while we are at it? I believe warn on switch fallthrough is already enabled in our Clang flags.

@cbracken cbracken force-pushed the implicit_scrolling branch from 9686c23 to 21cfb01 Compare March 5, 2019 01:20
goderbauer
goderbauer reviewed Mar 5, 2019
View reviewed changes
lib/ui/semantics.dart Outdated
/// easily move to the next visible set of children. A [TabBar] widget does
/// not have implicit scrolling, so that users can navigate into the tab
/// body when reaching the end of the tab bar.
// For example, a [ListView] widget has implicit scrolling so that users can
Copy link
Member

Choose a reason for hiding this comment

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

This one needs three ///

Copy link
Member Author

Choose a reason for hiding this comment

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

Gah -- that's what I get for updating the C++ side first ;) Done.

goderbauer
goderbauer reviewed Mar 5, 2019
View reviewed changes
shell/platform/embedder/embedder.h Outdated
@@ -141,6 +141,14 @@ typedef enum {
kFlutterSemanticsFlagHasToggledState = 1 << 16,
// If true, the semantics node is "on". If false, the semantics node is "off".
kFlutterSemanticsFlagIsToggled = 1 << 17,
/// Whether the platform can scroll the semantics node when the user attempts
Copy link
Member

Choose a reason for hiding this comment

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

Just two // here?

Copy link
Member Author

Choose a reason for hiding this comment

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

And the Dart side for the summary :) Done.

goderbauer
goderbauer approved these changes Mar 5, 2019
View reviewed changes
Copy link
Member

@goderbauer goderbauer left a comment

Choose a reason for hiding this comment

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

LGTM once /////// are correct :)

@cbracken cbracken force-pushed the implicit_scrolling branch 4 times, most recently from be9d7f2 to cb55580 Compare March 5, 2019 02:22
@cbracken
Add missing kHasImplicitScrolling enum value
7bd8585 
This brings the Dart and C++ semantics flag enums back in sync.

In flutter#5941, implicit scrolling support was added to SemanticsFlag in
dart:ui, and to the Android embedder, but not to the SemanticsFlags enum
on the C++ side.

This also clarifies/corrects the documentation for this value in dart:ui
and in the embedder API.
@cbracken cbracken force-pushed the implicit_scrolling branch from cb55580 to 7bd8585 Compare March 5, 2019 02:24
@cbracken
Copy link
Member Author

cbracken commented Mar 5, 2019 •
edited
Loading

@chinmaygarde unless any objection, I'm going to followup with a separate PR that kills the static casts for this, semantics actions, accessibility features, and text direction.

@chinmaygarde
Copy link
Member

Go for it.

@cbracken cbracken merged commit ed628da into flutter:master Mar 5, 2019
@cbracken cbracken mentioned this pull request Mar 5, 2019
Merged
cbracken added a commit to cbracken/flutter that referenced this pull request Mar 5, 2019
@cbracken
Roll engine 4f54a1d...2e97618
115a7d8 
flutter/engine@f4951df19 (upstream/master) Minor cleanups to CONTRIBUTING.md (flutter/engine#8043)
flutter/engine@effee2f80 remove extra statement (flutter/engine#8041)
flutter/engine@14e082fa0 (master) add lint script for Android with baseline (flutter/engine#8036)
flutter/engine@5fed42197 Roll src/third_party/skia 25bc174cf682..72542816cadb (14 commits) (flutter/engine#8040)
flutter/engine@e6a5201f0 Add missing values to semantics action enums (flutter/engine#8033)
flutter/engine@75d4db31d Roll src/third_party/skia fbc887df72ec..25bc174cf682 (7 commits) (flutter/engine#8039)
flutter/engine@c46226980 Roll src/third_party/skia 0a57971f0544..fbc887df72ec (1 commits) (flutter/engine#8037)
flutter/engine@ed628da00 Add missing kHasImplicitScrolling enum value (flutter/engine#8030)
flutter/engine@052774e1c Roll src/third_party/skia e3e80b7edfd1..0a57971f0544 (16 commits) (flutter/engine#8035)
flutter/engine@2cd9b41ba Select MPL instead of LGPL (flutter/engine#7991)
flutter/engine@629c0d836 Roll src/third_party/dart 7d8ffe0daf..571ea80e11 (3 commits)
flutter/engine@8f1fdcd19 Android Embedding PR 14: Almost done with FlutterFragment. (flutter/engine#8000)
flutter/engine@fb3e35d6a Android Embedding PR15: Add Viewport Metrics to FlutterView (flutter/engine#8029)
flutter/engine@36ca5740c (flutter/engine#8026)
flutter/engine@3d53e2026 Roll src/third_party/skia 5800f2e8a920..e3e80b7edfd1 (2 commits) (flutter/engine#8028)
flutter/engine@f3d4a7f71 Roll src/third_party/dart 7c70ab1817..7d8ffe0daf (77 commits)
flutter/engine@a2246c199 Start of linting Android embedding (flutter/engine#8023)
flutter/engine@48bf4803e [fuchsia] Fix snapshot BUILD.gn file for Fuchsia roll (flutter/engine#8024)
cbracken added a commit to flutter/flutter that referenced this pull request Mar 6, 2019
@cbracken
Roll engine 4f54a1d...2e97618 (#28899)
01a29b8 
flutter/engine@f4951df19 (upstream/master) Minor cleanups to CONTRIBUTING.md (flutter/engine#8043)
flutter/engine@effee2f80 remove extra statement (flutter/engine#8041)
flutter/engine@14e082fa0 (master) add lint script for Android with baseline (flutter/engine#8036)
flutter/engine@5fed42197 Roll src/third_party/skia 25bc174cf682..72542816cadb (14 commits) (flutter/engine#8040)
flutter/engine@e6a5201f0 Add missing values to semantics action enums (flutter/engine#8033)
flutter/engine@75d4db31d Roll src/third_party/skia fbc887df72ec..25bc174cf682 (7 commits) (flutter/engine#8039)
flutter/engine@c46226980 Roll src/third_party/skia 0a57971f0544..fbc887df72ec (1 commits) (flutter/engine#8037)
flutter/engine@ed628da00 Add missing kHasImplicitScrolling enum value (flutter/engine#8030)
flutter/engine@052774e1c Roll src/third_party/skia e3e80b7edfd1..0a57971f0544 (16 commits) (flutter/engine#8035)
flutter/engine@2cd9b41ba Select MPL instead of LGPL (flutter/engine#7991)
flutter/engine@629c0d836 Roll src/third_party/dart 7d8ffe0daf..571ea80e11 (3 commits)
flutter/engine@8f1fdcd19 Android Embedding PR 14: Almost done with FlutterFragment. (flutter/engine#8000)
flutter/engine@fb3e35d6a Android Embedding PR15: Add Viewport Metrics to FlutterView (flutter/engine#8029)
flutter/engine@36ca5740c (flutter/engine#8026)
flutter/engine@3d53e2026 Roll src/third_party/skia 5800f2e8a920..e3e80b7edfd1 (2 commits) (flutter/engine#8028)
flutter/engine@f3d4a7f71 Roll src/third_party/dart 7c70ab1817..7d8ffe0daf (77 commits)
flutter/engine@a2246c199 Start of linting Android embedding (flutter/engine#8023)
flutter/engine@48bf4803e [fuchsia] Fix snapshot BUILD.gn file for Fuchsia roll (flutter/engine#8024)
@cbracken cbracken mentioned this pull request Mar 7, 2019
Closed
@cbracken cbracken deleted the implicit_scrolling branch August 1, 2019 20:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@goderbauer goderbauer goderbauer approved these changes

@chinmaygarde chinmaygarde chinmaygarde approved these changes

@jonahwilliams jonahwilliams Awaiting requested review from jonahwilliams

Assignees
No one assigned
Labels
cla: yes
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@cbracken @chinmaygarde @goderbauer @googlebot