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

Disallow selecting the text of buttons #19330

Merged
merged 4 commits into from
Apr 14, 2022

Conversation

delvh
Copy link
Member

@delvh delvh commented Apr 5, 2022

Currently, it is possible to select the text of buttons.
I don't see a reason why, though. A button should (by default) not allow selecting its text.

This PR changes this by adding a new style class that disallows text selection.
In the future, if you want to make something unselectable, simply give it the style class unselectable.
If you want every element of another CSS selector to be unselectable, simply use &:extend(.unselectable); inside the selector body.

Originated in #19007 but is thematically irrelevant.

This could even be backported as it is a non-breaking change that enhances UX.

@delvh delvh added type/enhancement An improvement of existing functionality topic/ui Change the appearance of the Gitea UI topic/ui-interaction Change the process how users use Gitea instead of the visual appearance labels Apr 5, 2022
@delvh delvh added this to the 1.17.0 milestone Apr 5, 2022
@silverwind
Copy link
Member

Why not just

button {
  user-select: none;
}

Reasons:

  1. Keep it simple. If something is not a <button>, it will have a11y problems and should be changed to such.
  2. Vendor prefixes are not needed, all browsers we support do support the unprefixed variant since many years now.

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Apr 5, 2022
@@ -1526,12 +1538,12 @@ a.ui.label:hover {
}

.lines-num {
&:extend(.unselectable);
Copy link
Member

Choose a reason for hiding this comment

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

user-select: none; will be more readable to someone unitiated to Less. Vendor prefixes are not needed.

@delvh
Copy link
Member Author

delvh commented Apr 5, 2022

CanIUse gives me only 75% unprefixed.
prefixed, this goes up to 94%.
(https://caniuse.com/?search=user-select)
This would mean that it would not work correctly for one out of 4 people.
I think that's a bit too much.

Also, there are a lot more pseudo buttons than you think there are.
According to my quick search (ag 'class=".*button.*"' | grep -v '<button' | wc -l ), there are 335 of those... Have fun fixing all of them.

@silverwind
Copy link
Member

Right, Safari still needs -webkit-, so that must be kept, sorry I missed it on MDN.

@silverwind
Copy link
Member

there are 327 of those

Maybe compromise on button, .button 😉

@delvh
Copy link
Member Author

delvh commented Apr 5, 2022

Yeah, I can definitely throw out the unneeded prefix rules for -khtml, -ms and -moz.

That presents me with a problem: There might be a lot more components that might want not to be selectable.
So, which of the following options would you recommend?

  • Remove the style class completely, adding its rules to each needing selector (pretty messy to write or maintain)
  • Extend selectors that should not be selectable with the style class (as it is now - easy* to write and easy* to maintain)
  • Remove the &:extend(.unselectable) and instead add the style class to every component that qualifies for it (messy to write but easy to maintain)
  • Did I forgot something?

My personal choice is the current approach because this syntax is not too obscure, and if I'm writing less and see a syntax I don't understand, I can easily google it. To give you an example:

will be more readable to someone unitiated to Less

That was my first time writing (not CSS compliant) less, I simply googled whether I can extend a styleclass in less and used the first StackOverflow result. So, it should be possible to understand even for newbies.
Also, extending has the benefit that users can directly see which attributes will apply without having to skim through the entire web_src/less directory for potentially counter-productive rules, i.e. because of deeply nested rules as it is currently.

@GiteaBot GiteaBot added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. and removed lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels Apr 6, 2022
@wxiaoguang
Copy link
Contributor

Generally L-G-T-M. I prefer to keep only user-select and -webkit-user-select. Other prefixes are so rare .....

@delvh
Copy link
Member Author

delvh commented Apr 8, 2022

@silverwind, I would like still an answer to the question above.

@GiteaBot GiteaBot added lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels Apr 8, 2022
@wxiaoguang
Copy link
Contributor

No more comment, will get this PR merged.

@wxiaoguang wxiaoguang merged commit 4dabc21 into go-gitea:main Apr 14, 2022
@delvh delvh deleted the unselectable-buttons branch April 14, 2022 08:58
zjjhot added a commit to zjjhot/gitea that referenced this pull request Apr 14, 2022
* giteaofficial/main:
  Disallow selecting the text of buttons (go-gitea#19330)
AbdulrhmnGhanem pushed a commit to kitspace/gitea that referenced this pull request Aug 24, 2022
Introduce a CSS class `.unselectable`
@go-gitea go-gitea locked and limited conversation to collaborators May 3, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. topic/ui Change the appearance of the Gitea UI topic/ui-interaction Change the process how users use Gitea instead of the visual appearance type/enhancement An improvement of existing functionality
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants