-
Notifications
You must be signed in to change notification settings - Fork 364
login: Clarify semantics on page #1968
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.
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
base: main
Are you sure you want to change the base?
Conversation
660f0d7 to
c3996fc
Compare
orDivider semantic labelorDivider
c3996fc to
d7c2822
Compare
orDivider
chrisbobbe
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks! Comments below.
assets/l10n/app_en.arb
Outdated
| "loginMethodDividerSemanticLabel": "Log-in alternatives", | ||
| "@loginMethodDividerSemanticLabel": { | ||
| "description": "Semantic label for OrDivider in login page." | ||
| }, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: indentation
assets/l10n/app_en.arb
Outdated
| }, | ||
| "loginMethodDividerSemanticLabel": "Log-in alternatives", | ||
| "@loginMethodDividerSemanticLabel": { | ||
| "description": "Semantic label for OrDivider in login page." |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"OrDivider" is about something in the code, and we don't expect our volunteer translators to read the code. We should describe the string in a way that will make sense to the translators.
lib/widgets/login.dart
Outdated
| onPressed: widget.loginPageState._inProgress ? null : _submit, | ||
| child: Text(zulipLocalizations.loginFormSubmitLabel)), | ||
| ]))); | ||
| return Semantics( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
How did you decide this Semantics was necessary? From a quick skim of the implementation of the upstream Form widget, it looks like this is already taken care of:
return Semantics(
container: true,
explicitChildNodes: true,
role: SemanticsRole.form,
child: form,
);There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
noted.
lib/widgets/login.dart
Outdated
| final externalAuthenticationMethods = widget.serverSettings.externalAuthenticationMethods; | ||
|
|
||
| final loginForm = Column(mainAxisAlignment: MainAxisAlignment.center, children: [ | ||
| final loginOption = Column(mainAxisAlignment: MainAxisAlignment.center, children: [ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't understand the reason for this renaming. The commit message says the new name is "more explicit". What's less explicit about the old name? Also, the new name is confusing: it sounds like it's about only one login method ("option"), but the variable configures the UI for multiple login methods: username-password, plus any others the server offers, such as Google or GitHub.
I guess the old name could be confusing because it's too specific: only part of its job is about a "form", i.e., the username-password form. The third-party auth buttons aren't part of any form. If you want to change the name, I think "content" would be fine. It encompasses all of the variable's job, and it reads nicely alongside its ancestor widgets that are about how the content is presented (ConstrainedBox, SingleChildScrollView, SafeArea, etc.).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also a commit-message nit: renaming this variable seems unrelated to #1961. Let's move the Fixes line to the earlier commit that actually fixes that issue.
d7c2822 to
a01fc84
Compare
|
Thank you @chrisbobbe for the review, pushed the new changed. PTAL. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request overview
This PR improves accessibility on the login page by adding a semantic label to the OrDivider widget that separates username/password login from third-party authentication options. The changes ensure screen readers provide meaningful context about the alternative login methods.
- Added
Semanticswidget to the OrDivider withexcludeSemantics: trueto prevent child widgets from being read separately - Introduced new localization string
loginMethodDividerSemanticLabelacross all supported languages - Renamed
loginFormvariable tologinContentfor better clarity
Reviewed changes
Copilot reviewed 2 out of 20 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| lib/widgets/login.dart | Wraps OrDivider content in Semantics widget with new semantic label; renames loginForm to loginContent |
| lib/generated/l10n/zulip_localizations.dart | Adds abstract getter for loginMethodDividerSemanticLabel with documentation |
| assets/l10n/app_en.arb | Defines base English semantic label "Log-in alternatives" with description |
| lib/generated/l10n/zulip_localizations_*.dart (16 files) | Implements loginMethodDividerSemanticLabel getter for each language (currently untranslated) |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
a01fc84 to
cc96dfd
Compare
Fixes #1961.
Changes Displayed using SemanticsDebugger widget (Updated)
### Semantic Label of orDivider changed
Other than the
orDivider, all the semantic label seems to be in ideal condition in login page as tested and is visually represented in the semantic tree in the screenshot provided.