Skip to content
This repository has been archived by the owner on Feb 20, 2023. It is now read-only.

fixes #4193 - made close button for tabs more accessible. #4442

Merged
merged 2 commits into from
Sep 6, 2019

Conversation

mcarare
Copy link
Contributor

@mcarare mcarare commented Aug 1, 2019

set recommended minimum size for accessibility 48x48, while keeping image size the same
removed margin from button and text as it was not needed anymore
aligned close button in center of tab to be visual consistent with alignment of favicon and more visual accessible

Pull Request checklist

  • Quality: This PR builds and passes detekt/ktlint checks (A pre-push hook is recommended)
  • Tests: This PR includes thorough tests or an explanation of why it does not
  • Changelog: This PR includes a changelog entry or does not need one
  • Accessibility: The code in this PR follows accessibility best practices or does not include any user facing features

set recommended minimum size for accessibility 48x48, while keeping image size the same
removed margin from button and text as it was not needed anymore
aligned close button in center of tab to be visual consistent with alignment of favicon and more visual accessible
@mcarare mcarare requested a review from a team as a code owner August 1, 2019 13:29
@ekager
Copy link
Contributor

ekager commented Aug 3, 2019

Could you post a screenshot of the new tab for quick UX verification? :)

@sblatz
Copy link
Contributor

sblatz commented Aug 5, 2019

This is the new layout:

image

@colintheshots colintheshots added the needs:UX-feedback Needs UX Feedback label Aug 8, 2019
Copy link
Contributor

@sblatz sblatz left a comment

Choose a reason for hiding this comment

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

This still feels off to me, the top margin and the right margin on the close button do not match. Perhaps we just want to increase the 'clickableArea' as we do with other buttons?

@mcarare
Copy link
Contributor Author

mcarare commented Aug 14, 2019

The top and right margin cannot be equal, cause the parent view also has margin. Also, I'm not sure If the Talkback rectangle is actually increased when using TouchDelegate.

@sblatz
Copy link
Contributor

sblatz commented Sep 3, 2019

@mcarare I looked at this some more. Looks like increasing the touch size with a TouchDelegate does not make a larger selection square for TalkBack.

That being said, I was able to fix the visual margins on the button by just getting rid of the bottom constraint.

It now looks like this with TalkBack enabled and I think this is good to ship!

image

@mcarare
Copy link
Contributor Author

mcarare commented Sep 3, 2019

Looks good. I think we should still check for UI feedback, because the vertically centered placement for the close button has already been accepted and merged for collections, and it would seem weird to have them placed differently for tabs.

@mcarare mcarare requested a review from a team as a code owner September 3, 2019 15:46
@sblatz
Copy link
Contributor

sblatz commented Sep 3, 2019

@AmyYLee Are you able to take a look at this and let us know whether or not the close button on the tab should be centered vertically or should be aligned to the top?

Centered

image

Top

image

@sblatz
Copy link
Contributor

sblatz commented Sep 5, 2019

To clarify: we likely want consistent behavior here and should either revert the tab collection one to be top aligned or make this one centered :)

Tab Collection:

image

@lime124 lime124 assigned AmyYLee and sblatz and unassigned sblatz Sep 5, 2019
@AmyYLee
Copy link
Collaborator

AmyYLee commented Sep 5, 2019

I think these should be top aligned (collections and open tabs). @topotropic do you have feedback since you're working on the new collections design?

@topotropic
Copy link

no additional feedback at this point

@sblatz
Copy link
Contributor

sblatz commented Sep 6, 2019

Created #5161 to address the tab in collection

@sblatz sblatz self-requested a review September 6, 2019 17:28
@sblatz sblatz merged commit ed0b6bd into mozilla-mobile:master Sep 6, 2019
sblatz added a commit that referenced this pull request Sep 9, 2019
@mcarare mcarare deleted the 4193 branch October 28, 2019 08:52
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
needs:UX-feedback Needs UX Feedback
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants