[web] always add secondary role managers

[yjbanov]

Always add secondary role managers irrespective of the initial state of the semantic node, and have role manager decide whether it applies to the node or not.

Fixes flutter/flutter#130546

[ditman]

I don't have strong opinions on this, but the determination of what secondary role factories are needed is a little bit itchy :/

[ditman]

Is this because this update can be called multiple times? Does every update restore the roleFactories for this role manager? Is the role manager disposable?

[ditman]

This for loop looks a little bit weird to me. Wouldn't it be better that this knows what availableSecondaryRoles can be used, and the semanticsObject knows what configuredSecondaryRoles it has?

That way instead of this loop checking on every roleFactory if it can support the semanticsObject, an intersection would narrow down the actually needed Roles (which could have the roleFactory associated to the Role an immutable const Map<Role, RoleManagerFactory>, or whatever?)

[mdebbar]

Looks like these factories are always the same and have no dependency on the PrimaryRoleManager instance. Can we put them in a global map:

final Map<Role, RoleManagerFactory> secondaryRoleFactories = <Role, RoleManagerFactory> {
  Role.focusable: (SemanticsObject obj) => obj.isFocusable ? Focusable(obj) : null,
  ...
};

or combine them in a switch expression somewhere (I prefer this to make sure we exhaust all cases for Role):

extension RoleFactoryExtension on Role {
  RoleManager? createManager() {
    return switch (this) {
      Role.focusable => this.isFocusable ? Focusable(this) : null,
      ...
    }
  }
}

And the primary role manager only maintains a List<Role> and uses it during update() to decide what secondary role managers to create.

[mdebbar]

To clarify my suggestion above, here's how I'm imagining this method to look like:

    final List<Role> secondaryRoles = _secondaryRoles;
    for (Role role in secondaryRoles) {
      final RoleManager? secondaryRoleManager = role.createManager();
      if (secondaryRoleManager != null) {
        _secondaryRoleManagers ??= <RoleManager>[];
        _secondaryRoleManagers!.add(secondaryRoleManager);
        _secondaryRoles.remove(role); // Is this allowed inside the for loop?
      }
    }

[yjbanov]

Updated the design based on comments and discussion. We now always add secondary role managers, and have them figure out in their update() methods whether their functionality is needed or not.

[yjbanov]

PTAL.

[ditman]

I think I like this better now, with the relevant logic in the subclasses!

[mdebbar]

LGTM!

[mdebbar]

Do we need to changeFocus(false) when the node becomes unfocusable?

[yjbanov]

Good point. I also noticed that I unconditionally start managing the node in the constructor. Fixing.

[yjbanov]

Done.