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

Remove CSS declarations which change default scrollbar width #6

Draft
wants to merge 2 commits into
base: sc_v1.11.86
Choose a base branch
from

Conversation

luixxiul
Copy link

@luixxiul luixxiul commented Dec 1, 2024

Fixes SchildiChat/schildichat-desktop#238

Setting the declaration "scrollbar-width: thin" has a legitimate accessibility concern, though doing so could have made sense previously for some reason. Let's prefer each OS and its desktop environments' setting over Element's aesthetic view.

See: https://developer.mozilla.org/en-US/docs/Web/CSS/scrollbar-width#accessibility

Before:

before.mp4

After:

after.mp4

Closes SchildiChat/matrix-react-sdk#24


  • I agree to release my changes under this project's license

Setting the declaration "scrollbar-width: thin" has a legitimate accessibility concern, though doing so could have made sense previously for some reason. Let's prefer each OS and its desktop environments' setting over Element's aesthetic view.

See: https://developer.mozilla.org/en-US/docs/Web/CSS/scrollbar-width#accessibility

Signed-off-by: Suguru Hirahara <[email protected]>
@SpiritCroc
Copy link
Member

Thanks!

For keeping merge conflicts flow, I'd prefer

  • Commenting out code instead of fully removing it (ideally add something like a "SC: disabled" comment at the top so we know it was us when looking at the code)
  • Not changing upstream comments if the benefit is negligible
  • Not changing copyright headers (but it's ok if you insist to have your name there)

@luixxiul
Copy link
Author

luixxiul commented Dec 1, 2024

The file has not been updated since more than four years so it will not be changed for a while IMHO, but I'll address them anyway.

Signed-off-by: Suguru Hirahara <[email protected]>
@su-ex
Copy link
Member

su-ex commented Dec 1, 2024

Watch out doing this. I wanted to do do this a while back, but this made some mess in Firefox or some other browser. So test with every browser first.

@luixxiul luixxiul marked this pull request as draft December 1, 2024 17:55
@luixxiul
Copy link
Author

luixxiul commented Dec 1, 2024

I indeed have tested with Firefox, and am not quite sure how removing a declaration which is not recommended breaks UI, but I marked this as draft.

@SpiritCroc
Copy link
Member

The file has not been updated since more than four years so it will not be changed for a while IMHO, but I'll address them anyway.

I don't have enough merging history to tell for desktop but on Android I was proven wrong often enough with this kind of assumption. So I think it's good to be consistent with aiming for the lowest merge conflict impact possible

html {
scrollbar-color: $scrollbar-thumb-color transparent;
}
/* SC: disabled */
Copy link
Member

Choose a reason for hiding this comment

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

I would put it right above the actual commented out section, and use the same /*. The first point further reduces merge conflict area, the second one is just matter of taste I guess

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.

3 participants