[web] better class names for semantics #54070

yjbanov · 2024-07-24T04:58:35Z

A few non-functional clean-ups in semantics:

Rename RoleManager to SemanticBehavior.
Rename PrimaryRoleManager to SemanticRole.
Remove the Role enum. Move the enum docs into the respective classes.

Why?

Previous naming was confusing. It's not clear what the difference is between a "role manager" and a "primary role manager". The word "manager" is a meaningless addition; the Semantic* prefix is much more meaningful. The Role enum was only used for tests, but tests can just use SemanticRole.runtimeType.

New state of the world

After this PR the semantics system has "objects" (class SemanticsObject), "roles" (class SemanticRole), and "behaviors" (class SemanticBehavior).

A semantic object is an object attached to the framework-side SemanticNode. It lives as long as the semantic node does, and provides basic functionality that's common across all nodes.
A semantic object has exactly one semantic role. This role is determined from the flags set on the semantic node. Flags can change, causing a semantic object to change its role, which is why these are two separate classes. If an object had just one permanent role, we could combine these classes into one (maybe one day we'll do it, as changing roles dynamically is weird, but that needs major changes in the framework).
A semantic role may have zero or more semantic behaviors. A behavior supplies a piece of functionality, such as focusability, clickability/tappability, live regions, etc. A behavior can be shared by multiple roles. For example, both Button and Checkable roles use the Tappable behavior. This is why there's a many-to-many relationship between roles and behaviors.

Or in entity relationship terms:

---
title: Semantic object relationships
---
erDiagram
    SemanticsNode ||--|| SemanticsObject : managed-by
    SemanticsObject ||--o{ SemanticRole : has-a
    SemanticRole }o--o{ SemanticBehavior : has

…o classes

ditman

I understand the SemanticObject is the glue that controls the rest, but there's a lot of magic that is happening in semantic object (checking type, need for updates, etc...) that probably should be delegated to the SemanticRole that it's wrapping? Isn't the _getSemanticRoleId() method a little bit weird, or having an updateSelf that grows potentially unbounded (at least linearly with the types of semantics?)

That's probably a bigger refactor. The renaming LGTM other than what we've discussed!

ditman · 2024-07-24T20:25:06Z

lib/web_ui/lib/src/engine/semantics/checkable.dart

-class Checkable extends PrimaryRoleManager {
+class Checkable extends SemanticRole {


This is indeed much better. It's a shame that the name of the top-level classes sound like java interfaces (*-able), but the alternative (CheckableSemanticRole) is probably too verbose?

Made all concrete role type names use the Semantic* prefix. Looks much better now.

ditman · 2024-07-24T20:27:03Z

lib/web_ui/lib/src/engine/semantics/dialog.dart

@@ -39,7 +37,7 @@ class Dialog extends PrimaryRoleManager {

  void _setDefaultFocus() {
    semanticsObject.visitDepthFirstInTraversalOrder((SemanticsObject node) {
-      final PrimaryRoleManager? roleManager = node.primaryRole;
+      final SemanticRole? roleManager = node.semanticRole;


Consider doing a pass renaming these roleManager names. They don't map very well to what they contain anymore, now that the "Manager" name is going away from this class hierarchy.

ditman · 2024-07-24T20:29:40Z

lib/web_ui/lib/src/engine/semantics/image.dart

-      : super.blank(PrimaryRole.image, semanticsObject) {
-    // The following secondary roles can coexist with images. `LabelAndValue` is
-    // not used because this role manager uses special auxiliary elements to
+class ImageSemanticRole extends SemanticRole {


I understand that Image clashes with other 47 classes in the engine, but... should all the extends SemanticRole be SomethingSomethingSemanticRole, just for consistency of everything inside "engine/semantics"?

Done (using the Semantic* prefix)

ditman · 2024-07-24T21:04:16Z

lib/web_ui/lib/src/engine/semantics/semantics.dart


-  PrimaryRole _getPrimaryRoleIdentifier() {
+  SemanticRoleId _getSemanticRoleId() {


Should this be a SemanticRole.kind that every SemanticRole needs to override?

Done. Made it SemanticRoleKind.

ditman · 2024-07-24T21:09:14Z

lib/web_ui/lib/src/engine/semantics/semantics.dart

-  DomElement get element => primaryRole!.element;
+  DomElement get element => semanticRole!.element;


The class SemanticObject looks a little bit too complicated, shouldn't each SemanticRole/Behavior know how to update and identify itself? Is this class doing too much, does it need love? Is it needed?

SemanticRole can change, but a SemanticObject is persistent for the entire lifecycle of the node. So for now we need both. However, we can revisit who's responsible for what, since both objects are 1:1 with the node. For now, I'm keeping this PR to non-functional changes only, just renames and deletion of dead code.

ditman · 2024-07-24T21:11:46Z

lib/web_ui/lib/src/engine/semantics/semantics.dart

    }
  }

-  PrimaryRoleManager _createPrimaryRole(PrimaryRole role) {
+  SemanticRole _createSemanticRole(SemanticRoleId role) {


This is probably only used from within the class, but this probably should be factory (or at least static)? (Even though it needs "this" to build itself, ouch)

Can't be static. It uses this. But we could revisit this. Maybe sealed classes can help clean this up.

…ions) (#152305) Manual roll requested by zra@google.com flutter/engine@eb8fac2...7473782 2024-07-25 yjbanov@google.com [web] better class names for semantics (flutter/engine#54070) 2024-07-25 jonahwilliams@google.com Disable FlutterMetalLayer by default. (flutter/engine#54095) 2024-07-25 skia-flutter-autoroll@skia.org Roll Dart SDK from 0b3c00feefb1 to 693848f200d7 (1 revision) (flutter/engine#54092) 2024-07-25 gspencergoog@users.noreply.github.com Remove incorrect line (flutter/engine#54021) 2024-07-24 zanderso@users.noreply.github.com [et] Better RBE defaults (flutter/engine#54059) 2024-07-24 skia-flutter-autoroll@skia.org Roll Skia from 55ecdde3a5fa to 746d444f3efd (2 revisions) (flutter/engine#54091) 2024-07-24 jonahwilliams@google.com [iOS] Switch to FlutterMetalLayer by default. (flutter/engine#54086) 2024-07-24 skia-flutter-autoroll@skia.org Roll Skia from 54d1434637a1 to 55ecdde3a5fa (3 revisions) (flutter/engine#54089) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-engine-flutter-autoroll Please CC jonahwilliams@google.com,rmistry@google.com,zra@google.com on the revert to ensure that a human is aware of the problem. To file a bug in Flutter: 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

…ions) (flutter#152305) Manual roll requested by zra@google.com flutter/engine@eb8fac2...7473782 2024-07-25 yjbanov@google.com [web] better class names for semantics (flutter/engine#54070) 2024-07-25 jonahwilliams@google.com Disable FlutterMetalLayer by default. (flutter/engine#54095) 2024-07-25 skia-flutter-autoroll@skia.org Roll Dart SDK from 0b3c00feefb1 to 693848f200d7 (1 revision) (flutter/engine#54092) 2024-07-25 gspencergoog@users.noreply.github.com Remove incorrect line (flutter/engine#54021) 2024-07-24 zanderso@users.noreply.github.com [et] Better RBE defaults (flutter/engine#54059) 2024-07-24 skia-flutter-autoroll@skia.org Roll Skia from 55ecdde3a5fa to 746d444f3efd (2 revisions) (flutter/engine#54091) 2024-07-24 jonahwilliams@google.com [iOS] Switch to FlutterMetalLayer by default. (flutter/engine#54086) 2024-07-24 skia-flutter-autoroll@skia.org Roll Skia from 54d1434637a1 to 55ecdde3a5fa (3 revisions) (flutter/engine#54089) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-engine-flutter-autoroll Please CC jonahwilliams@google.com,rmistry@google.com,zra@google.com on the revert to ensure that a human is aware of the problem. To file a bug in Flutter: 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

yjbanov added 4 commits July 23, 2024 21:42

rename RoleManager to SemanticBehavior

42cb58e

remove the Role enum that was only used in tests; move their docs int…

e939472

…o classes

rename PrimaryRoleManager to SemanticRole

0f85a56

obsess about docs a little more

06e8909

github-actions bot added the platform-web Code specifically for the web engine label Jul 24, 2024

yjbanov requested a review from ditman July 24, 2024 18:41

ditman approved these changes Jul 24, 2024

View reviewed changes

yjbanov mentioned this pull request Jul 24, 2024

[web:a11y] switch header from role "heading" to role "banner" flutter/flutter#152268

Closed

yjbanov added 2 commits July 24, 2024 19:30

role ID => role kind

fea58ea

consistent naming of concrete role types

29a8c76

yjbanov added the autosubmit Merge PR when tree becomes green via auto submit App label Jul 25, 2024

auto-submit bot merged commit 7473782 into flutter:main Jul 25, 2024
29 checks passed

engine-flutter-autoroll mentioned this pull request Jul 25, 2024

Roll Flutter Engine from a57655cccb55 to 74737820a8ee (2 revisions) flutter/flutter#152295

Closed

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jul 25, 2024

74737820a [web] better class names for semantics (flutter/engine#54070)

1b93cfc

engine-flutter-autoroll mentioned this pull request Jul 25, 2024

Manual roll Flutter Engine from eb8fac2b1703 to 74737820a8ee (8 revisions) flutter/flutter#152305

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jul 25, 2024

74737820a [web] better class names for semantics (flutter/engine#54070)

e5a332f

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[web] better class names for semantics #54070

[web] better class names for semantics #54070

yjbanov commented Jul 24, 2024

ditman left a comment

ditman Jul 24, 2024

yjbanov Jul 25, 2024

ditman Jul 24, 2024

ditman Jul 24, 2024

yjbanov Jul 25, 2024

ditman Jul 24, 2024

yjbanov Jul 25, 2024

ditman Jul 24, 2024

yjbanov Jul 25, 2024

ditman Jul 24, 2024

yjbanov Jul 25, 2024

		class Checkable extends PrimaryRoleManager {
		class Checkable extends SemanticRole {


		PrimaryRole _getPrimaryRoleIdentifier() {
		SemanticRoleId _getSemanticRoleId() {

		DomElement get element => primaryRole!.element;
		DomElement get element => semanticRole!.element;

[web] better class names for semantics #54070

[web] better class names for semantics #54070

Conversation

yjbanov commented Jul 24, 2024

Why?

New state of the world

ditman left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment