Skip to content
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

[a11y] 11.1.4.11 Non-text contrast #4433

Merged
merged 20 commits into from
Jul 22, 2024
Merged

[a11y] 11.1.4.11 Non-text contrast #4433

merged 20 commits into from
Jul 22, 2024

Conversation

Aitorbp
Copy link
Contributor

@Aitorbp Aitorbp commented Jul 3, 2024

Related Issues

App:

  • Add changelog files for the fixed issues in folder changelog/unreleased. More info here
  • Add feature to Release Notes in ReleaseNotesViewModel.kt creating a new ReleaseNote() with String resources (if required)

QA

QA checks:

#4433 (comment)

@Aitorbp Aitorbp linked an issue Jul 3, 2024 that may be closed by this pull request
3 tasks
@Aitorbp
Copy link
Contributor Author

Aitorbp commented Jul 3, 2024

Changes to SearchView have been made on the following screens:

  • Search file main list
  • Search space in Space view
  • Search space in Select folder
  • Picture upload path -> Choose destination folder
  • Video upload path -> Choose destination folder
  • Receive external files --> choose destination folder

No changes will be made to the Users and group SearchView in the Share view.

Now, the searchbar includes a new resource with rounded edges, using the same background color (brandable) as the containing toolbar: rounded_search_view.xml

The new color #BDBDBD of the hint text meets the contrast requirements of the issue:

hint_contrast

The search cross will be in white color to meet the contrast requirements:

cross_contrast

@Aitorbp Aitorbp force-pushed the feature/non_text_contrast branch 3 times, most recently from 778f050 to 37a0516 Compare July 3, 2024 13:37
@Aitorbp Aitorbp self-assigned this Jul 3, 2024
@Aitorbp Aitorbp force-pushed the feature/non_text_contrast branch from 1d0fb58 to 57dce08 Compare July 5, 2024 11:33
@Aitorbp Aitorbp requested a review from JuancaG05 July 5, 2024 13:07
Copy link
Collaborator

@JuancaG05 JuancaG05 left a comment

Choose a reason for hiding this comment

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

This needs a general recheck @Aitorbp, not valid to move it to QA yet

@Aitorbp Aitorbp requested a review from JuancaG05 July 10, 2024 08:21
Copy link
Collaborator

@JuancaG05 JuancaG05 left a comment

Choose a reason for hiding this comment

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

Some more changes requested here @Aitorbp

@Aitorbp Aitorbp requested a review from JuancaG05 July 11, 2024 07:37
Copy link
Collaborator

@JuancaG05 JuancaG05 left a comment

Choose a reason for hiding this comment

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

LGTM now

@Aitorbp Aitorbp mentioned this pull request Jul 11, 2024
3 tasks
@Aitorbp
Copy link
Contributor Author

Aitorbp commented Jul 11, 2024

Research:
First of all, we need to know that there are two toolbars currently working in the application: RootToolbar and StandardToolbar.

The RootToolbar is displayed in the following views:

  • Main list of files root view
  • Shares view
  • Spaces root view
  • Uploads view
  • Available offline view

The StandardToolbar appears in the following views:

  • Main list of files (non-root) view
  • Spaces (non-root) view
  • Set target path for auto-uploads
  • Move and copy operations
  • Sharing files with ownCloud from external apps (Share with)
  • File previews
  • Settings
  • And more...

In this PR we are trying to unify the common appearance (colors, size...) of both toolbars in a single place. We are moving the general behaviour of the toolbar from the child classes to the ToolbarActivity, avoiding repeated code. Regarding the StandardToolbar, when we don't want the main_menu (which includes Search, Select all and Share buttons) to be displayed, we override the onCreateOptionsMenu method to not showing it, which is the default behaviour provided in ToolbarActivity.

This way, the following classes don't need to display the main_menu so we override this method to return false (won't be displayed):

  • UploadListActivity ( Uploads view)
  • ShareActivity (Shares view)
  • PreviewImageActivity ( Image preview)
  • PreviewVideoActivity (Video preview)

We need to do this now because these classes extend from ToolbarActivity and main_menu is attached to it.

On the other hand, we have fixed a bug. Now, when we are in the spaces view, the text hint of the SearchView is Search spaces.

@Aitorbp Aitorbp force-pushed the feature/non_text_contrast branch from 03e856d to 701c8f7 Compare July 11, 2024 08:49
@jesmrec
Copy link
Collaborator

jesmrec commented Jul 17, 2024

QA checks:

Personal Space

Edit shared Link

  • Adjust contrast for switches (if necessary) NA

Spaces

  • "Search folder" input field and the button for deleting search entries - See comment on Personal Space

Toolbar checks

  • Main list of files root view
  • Shares view -> glitch in landscape
  • Spaces root view
  • Uploads view -> glitch, hidden avatar
  • Available offline view
  • Main list of files (non-root) view
  • Spaces (non-root) view
  • Set target path for auto-uploads
  • Move and copy operations
  • Sharing files with ownCloud from external apps (Share with)
  • File previews
  • Settings
  • Search spaces

New brandable fields

Background color of the searchview:
<color name="search_view_background_color">@color/actionbar_start_color</color>

@jesmrec
Copy link
Collaborator

jesmrec commented Jul 17, 2024

(1) [FIXED]

With a branded search_view_background_color

  1. Browse into a non-root folder
  2. Click in the lens icon

It looks like this:

Screenshot 2024-07-17 at 14 25 00

IMO, it shouldn't overlap the share and 3-dot icons

NOTE: same in every folder without the avatar, f. ex, in the root of any space except personal

Pixel 2, Android 11
701c8f75b

@jesmrec
Copy link
Collaborator

jesmrec commented Jul 17, 2024

About (1), i notice that the regression behaviour is that one. The point is, if the new bubble makes it a bit weird. With the same color it looks good, with two colors... maybe not. Open to discussion.

@jesmrec
Copy link
Collaborator

jesmrec commented Jul 17, 2024

There is a non-regressive problem in the search bar: it stops working after changing device orientation. It will be addressed in a separate issue

@jesmrec
Copy link
Collaborator

jesmrec commented Jul 18, 2024

Problem with filtering after rotating: #4441
Problem with bubble taking place in the top bar: WONT FIX

@jesmrec
Copy link
Collaborator

jesmrec commented Jul 18, 2024

About the search hint color

<color name="search_view_hint_text">#BDBDBD</color>

That default color was added to the color's list, but is not brandable for the search hint. Is that right? i'd go for the first approach, but we should not forget that the bubble background and the top bar are brandable.

@Aitorbp
Copy link
Contributor Author

Aitorbp commented Jul 18, 2024

About the search hint color

<color name="search_view_hint_text">#BDBDBD</color>

That default color was added to the color's list, but is not brandable for the search hint. Is that right? i'd go for the first approach, but we should not forget that the bubble background and the top bar are brandable.

We've left it in the search_view_background_color. There's no reason to make the color of the text hitn brandable as well. We could do that, so we'd give customers more freedom to play with color contrasts. But by adjusting the search_view_background_color they could do it.

@Aitorbp
Copy link
Contributor Author

Aitorbp commented Jul 18, 2024

(1) READY TO TEST

With a branded search_view_background_color

  1. Browse into a non-root folder
  2. Click in the lens icon

It looks like this:

Screenshot 2024-07-17 at 14 25 00

IMO, it shouldn't overlap the share and 3-dot icons

NOTE: same in every folder without the avatar, f. ex, in the root of any space except personal

Pixel 2, Android 11 701c8f75b

I would leave it as it is now, but adding a margin end to the toolbar. In all the applications I have seen (Drive, Instagram, WhatsApp, LinkedIn) when you click on the searchview, the search element expands the entire screen, removing the other elements that were previously there.
If we also take a look at the Material documentation, the examples follow the same design. https://m3.material.io/components/search/guidelines

@jesmrec
Copy link
Collaborator

jesmrec commented Jul 18, 2024

Approved on my side.

A problem was detected, as no regression was moved to another place: #4441

@Aitorbp Aitorbp force-pushed the feature/non_text_contrast branch from 86f1cd8 to 663ea64 Compare July 22, 2024 09:36
@Aitorbp Aitorbp merged commit ecf51e1 into master Jul 22, 2024
7 checks passed
@Aitorbp Aitorbp deleted the feature/non_text_contrast branch July 22, 2024 09:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[a11y] 11.1.4.11 Non-text contrast
3 participants