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

chore: hide scrollbars in dev pages when motion is disabled on Safari #3303

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

jperals
Copy link
Member

@jperals jperals commented Feb 20, 2025

Description

When updating Safari from 15 to 16 in our visual regression Safari tests, scrollbars appear and they happen to be flaky —with variable opacity, and sometimes not appearing at all, causing unexpected screenshot diffs. Therefore bringing back scrollbars hiding for Safari in our dev pages.

After this change, scrollbars are not visible when using Safari and motion is disabled.

Note: I noticed that for the setting to have effect, motion needs to be disabled when the page is loaded. So in order to switch scrollbars on or off manually, it is necessary not only to toggle the motion disabled option (either via the checkbox or via the URL parameters) but also to reload the page. This seems to be a browser limitation and does not affect our tests.

This reverts commit f9aeff8 "in essence", but with two differences:

  • Hides page-level scrollbars as well, not only those inside the body
  • Hides scrollbars only in Safari, does not affect Chrome in MacOs (which is what most devs use to manually test)

Other options considered:

  • Disabling scrollbars via feature flags. No such option found for Safari / Browserstack
  • Adding a delay in test initialization. This would affect the tests for all browsers and for the sake of covering scrollbars in tests, they area already covered in Chrome tests.

How has this been tested?

Ran in my pipeline. Scrollbars are consistently hidden in Safari components tests.

Review checklist

The following items are to be evaluated by the author(s) and the reviewer(s).

Correctness

  • Changes include appropriate documentation updates.
  • Changes are backward-compatible if not indicated, see CONTRIBUTING.md.
  • Changes do not include unsupported browser features, see CONTRIBUTING.md.
  • Changes were manually tested for accessibility, see accessibility guidelines.

Security

Testing

  • Changes are covered with new/existing unit tests?
  • Changes are covered with new/existing integration tests?

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

@jperals jperals changed the title Revert "chore: Do not hide scrollbars in dev pages in Mac OS (#3168)" chore: hide scrollbars in dev pages in Mac OS when motion is disabled (again) Feb 20, 2025
Copy link

codecov bot commented Feb 20, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 96.42%. Comparing base (22d7bca) to head (8175d31).

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #3303   +/-   ##
=======================================
  Coverage   96.42%   96.42%           
=======================================
  Files         791      791           
  Lines       22603    22603           
  Branches     7814     7814           
=======================================
  Hits        21795    21795           
  Misses        754      754           
  Partials       54       54           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@jperals jperals changed the title chore: hide scrollbars in dev pages in Mac OS when motion is disabled (again) chore: hide scrollbars in dev pages when motion is disabled on Safari Feb 21, 2025
@jperals jperals marked this pull request as ready for review February 21, 2025 19:27
@jperals jperals requested a review from a team as a code owner February 21, 2025 19:27
@jperals jperals requested review from orangevolon and removed request for a team February 21, 2025 19:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant