Skip to content
This repository was archived by the owner on Jun 13, 2024. It is now read-only.

[a11y-web] adding enabled to button semantics #219

Merged
merged 1 commit into from
Jul 17, 2020

Conversation

nturgut
Copy link
Contributor

@nturgut nturgut commented Jul 17, 2020

@nturgut nturgut requested review from yjbanov and goderbauer July 17, 2020 20:34
@nturgut
Copy link
Contributor Author

nturgut commented Jul 17, 2020

This is a quick fix for the gallery code before the july updates. However a better fix might be changing enabled to default true in this class: https://github.com/flutter/flutter/blob/master/packages/flutter/lib/src/widgets/basic.dart#L6634

@goderbauer shall I also send a PR to the framework repo?

@nturgut nturgut changed the title [a111y-web] adding enabled to button semantics [a11y-web] adding enabled to button semantics Jul 17, 2020
@nturgut nturgut merged commit 0bf5e30 into flutter:master Jul 17, 2020
@goderbauer
Copy link
Member

I don't think this is the right fix. The bug appears to be here:

https://github.com/flutter/engine/blob/74df174fa3d8d95fcfee654fec5cd37cc5d191a2/lib/web_ui/lib/src/engine/semantics/tappable.dart#L27-L29

Something should only be marked as disabled when the hasEnabledState is set and the isEnabled is not set. If hasEnabledState is not set, the element has no opinion of whether it is disabled or enabled.

@goderbauer
Copy link
Member

I would recommend reverting this change. It may have unintended a11y side-effects because (some of) the wrapped button already set this property: https://github.com/flutter/flutter/blob/5b9ce492f3da4a2b3a629ede34951bcac7d79a56/packages/flutter/lib/src/material/button.dart#L462

@nturgut
Copy link
Contributor Author

nturgut commented Jul 20, 2020

@goderbauer I see, so we were implementing it wrong on the engine side for a while. I'll revert this one.

@goderbauer
Copy link
Member

I took a closer look at where this PR is adding the enabled: true properties. It looks like it is doing this only in places where the semantics of the wrapped button are actually disabled via excludeSemantics. In these cases there's no chance of unintended side-effects (because the real button semantics are disabled) and the enabled: true is probably correct here.

Nevertheless, the web engine still has the bug that something is only disabled if hasEnabledState is set and isEnabled is not set.

clocksmith pushed a commit to clocksmith/gallery that referenced this pull request Jul 23, 2020
…run for Flutter Gallery (flutter#219)

* Add command to travis CI to analyze if update-code-segments has been run

Former-commit-id: 6c10ea3
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants