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

Added a high contrast mode to screenshot cases. #18080

Merged
merged 1 commit into from May 2, 2024

Conversation

sarahboyce
Copy link
Contributor

Branch description

To extend our accessibility tests, added screenshots in forced-colors mode.

Checklist

  • This PR targets the main branch.
  • The commit message is written in past tense, mentions the ticket number, and ends with a period.
  • I have added or updated relevant tests.

@sarahboyce sarahboyce added selenium Apply to have Selenium tests run on a PR screenshots 🖼️ labels Apr 16, 2024
@sarahboyce sarahboyce requested a review from a team April 16, 2024 07:41
@sarahboyce
Copy link
Contributor Author

Screenshots can be seen in the Artifacts here: https://github.com/django/django/actions/runs/8701792464/
They are the ones with forced_colors in the file name

Here are a couple of examples:
test_collapsible_fieldset_forced_colors-collapsed

test_first_field_focus_forced_colors-focus-multi-widget

This aims to aid the review process.
We may need to skip this on browsers that do not support forced-colors mode.

@knyghty
Copy link
Member

knyghty commented Apr 16, 2024

Super cool 👍🏻

@sarahboyce sarahboyce requested a review from ngnpope April 16, 2024 07:52
@ngnpope
Copy link
Member

ngnpope commented Apr 16, 2024

Hi @sarahboyce 👋🏻

Firstly, congratulations on your new role! 🥳 (Not crossed paths on any post or pull request recently, so sorry for being late to the party...)

This is great - it'll be helpful to have something for high contrast mode. This brings me to the point - you've named this forced_colors. It might be better to go for high_constrast instead, i.e. what we're generating rather than the mechanism by which it is achieved?

@sarahboyce sarahboyce force-pushed the forced-colors-screenshots branch 2 times, most recently from cdca435 to e6b9442 Compare April 16, 2024 09:14
@sarahboyce
Copy link
Contributor Author

Firstly, congratulations on your new role! 🥳

Thank you very much 😊

  • Renamed to high_contrast
  • This will still generate screenshots on other browsers but they will just be the same as the desktop (we only run screenshots for chrome in the CI)

@MHLut
Copy link
Contributor

MHLut commented Apr 16, 2024

FYI, I have added some screenshots in #17910 per @sarahboyce's suggestion.

Copy link
Sponsor Member

@sabderemane sabderemane left a comment

Choose a reason for hiding this comment

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

Look good to me, nice addition ❤️

@sarahboyce sarahboyce requested a review from a team May 2, 2024 07:37
Copy link
Member

@ngnpope ngnpope left a comment

Choose a reason for hiding this comment

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

Looks good. I have a little suggestion to skip the test on non-Chrome browsers which will work without affecting other screenshot cases as we're duplicating the test method.

Also need to update the PR title; perhaps:

Added a high contrast mode to screenshot cases.

django/test/selenium.py Outdated Show resolved Hide resolved
@sarahboyce sarahboyce changed the title Added a forced-colors mode screenshot_cases. Added a high contrast mode to screenshot cases. May 2, 2024
Thank you to Sarah Abderemane and Nick Pope for the reviews.
@sarahboyce sarahboyce merged commit 39828fa into django:main May 2, 2024
35 checks passed
@ngnpope ngnpope mentioned this pull request May 9, 2024
2 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
screenshots 🖼️ selenium Apply to have Selenium tests run on a PR
Projects
None yet
5 participants