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

Add and apply focusStyle function #726

Merged
merged 1 commit into from
Mar 8, 2023

Conversation

owi92
Copy link
Member

@owi92 owi92 commented Mar 1, 2023

This adds and where possible utilizes a function to apply a default, but somewhat customizable focus outline to elements. This function should be used in place of a manual :focus declaration to ensure a uniform focus style that is easily maintainable to Tobira.

Important note:
As noted in #724, Safari does not apply a border radius to outlines. This means, that in Safari, the focus outlines will always be rectangular. But I think this is reasonable enough (see my justification for this below).

I replaced the previously used box-shadow with an outline for two reasons:

  • box-shadow in combination with outline: none can be problematic with high contrast modes that use forced-colors, which forces box-shadow: none (for instance Windows high contrast mode). This could be fixed by using sth like outline: 1px solid transparent but:
  • outline is more flexible than box-shadow, mainly by allowing to set a specific offset. While box-shadow has the inset keyword to show it inside the focused element, it does not allow for an outside offset without using multiple shadows.

Closes: #654

@github-actions github-actions bot temporarily deployed to test-deployment-pr726 March 1, 2023 12:09 Destroyed
Copy link
Member

@LukasKalbertodt LukasKalbertodt left a comment

Choose a reason for hiding this comment

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

Good to see someone cleaning up this mess that we had before!

One general question: why 2.5 as width? As someone with 200% scaling on all my devices, I don't mind that as it will result in 5 clean physical pixels. But generally one would try to stay with whole pixels to have a better chance of hitting the pixel grid on most devices, resulting in a sharper looking image. Or not? Are browsers free to round to the pixel grid anyway?

frontend/src/layout/header/UserBox.tsx Show resolved Hide resolved
frontend/src/layout/header/Logo.tsx Show resolved Hide resolved
frontend/src/ui/Button.tsx Outdated Show resolved Hide resolved
frontend/src/ui/Input.tsx Outdated Show resolved Hide resolved
frontend/src/ui/index.tsx Outdated Show resolved Hide resolved
frontend/src/ui/index.tsx Outdated Show resolved Hide resolved
@owi92
Copy link
Member Author

owi92 commented Mar 5, 2023

why 2.5 as width?

Nadine mentioned in the corresponding issue that the focus border is a little too thin and I do agree with that. I tried 3px and actually think that would work well, but might be perceived as a little too thick by some.
So I went with the middle ground without really thinking about it too much, as it worked on my (admittedly relatively high ppi) display. On that it appears that at least firefox and chrome can display even smaller fractions of a pixel, and safari can handle half pixels without sacrificing any sharpness of the image (in my perception).
I couldn't find any super recent and/or reliable information about this general topic, so I think you might have a point and we should stick with whole numbers for now.

TLDR: I increased this to 3px for now though I wouldn't be totally opposed to stick with 2px instead.

Edit: the latest commit (inset for menu item hover) is more of a proposal and I'm not super convinced about that myself, but I'm interested to hear your opinion. Basically I think the "cutoff" between the white arrow tip and greyish menuitem looks just a little weird.
Bildschirm­foto 2023-03-05 um 12 21 27

Bildschirm­foto 2023-03-05 um 12 26 01

@github-actions github-actions bot temporarily deployed to test-deployment-pr726 March 5, 2023 09:50 Destroyed
@github-actions github-actions bot temporarily deployed to test-deployment-pr726 March 5, 2023 11:24 Destroyed
@LukasKalbertodt
Copy link
Member

Regarding the non-integer width: I did some experiments aaaand it seems like browsers round to the nearest pixel to get sharp lines! Here are screenshots from Firefox and Chrome for a 2.5px border:

border-width

You likely have to download and then open in a viewer/program where you can zoom with nearest neighbor scaling. My desktop scaling was at 200% for all screenshots, I just zoomed the browser to either 100% or 50%. And well: on the 100% scaling, the border is exactly 2px wide, on the 200% scaling it's 5px wide.

So... feel very free to put it back to 2.5px :D
Sorry, sometimes I should rather do the research before I spout some comments into your PRs!

@LukasKalbertodt
Copy link
Member

the latest commit (inset for menu item hover) is more of a proposal and I'm not super convinced about that myself, but I'm interested to hear your opinion. Basically I think the "cutoff" between the white arrow tip and greyish menuitem looks just a little weird.

Yes it does look a bit weird, but I don't think it's too much of a problem. Your solution is an interesting one, but with a smaller area changing color and with the bright hover grey, I think the hover effect is too subtle. But we can certainly ask others before deciding on this.

@github-actions github-actions bot added the status:conflicts This PR has conflicts that need to be resolved label Mar 6, 2023
@github-actions
Copy link

github-actions bot commented Mar 6, 2023

This pull request has conflicts ☹
Please resolve those so we can review the pull request.
Thanks.

@github-actions github-actions bot removed the status:conflicts This PR has conflicts that need to be resolved label Mar 7, 2023
@github-actions github-actions bot temporarily deployed to test-deployment-pr726 March 7, 2023 12:17 Destroyed
frontend/src/layout/header/Search.tsx Outdated Show resolved Hide resolved
frontend/src/layout/header/UserBox.tsx Outdated Show resolved Hide resolved
frontend/src/ui/Blocks/Series.tsx Outdated Show resolved Hide resolved
frontend/src/ui/Input.tsx Outdated Show resolved Hide resolved
frontend/src/ui/index.tsx Outdated Show resolved Hide resolved
@github-actions github-actions bot temporarily deployed to test-deployment-pr726 March 7, 2023 19:27 Destroyed
This adds and where possible utilizes a function to apply
a default, but somewhat customizable focus outline to elements.
This function should be used in place of a manual `:focus` declaration
to ensure a uniform focus style that is easily maintainable to Tobira.
@github-actions github-actions bot temporarily deployed to test-deployment-pr726 March 7, 2023 19:33 Destroyed
@LukasKalbertodt LukasKalbertodt merged commit ab6df75 into elan-ev:master Mar 8, 2023
@LukasKalbertodt LukasKalbertodt added the changelog:user User facing changes label May 15, 2023
@owi92 owi92 deleted the fix-tab-navigation branch March 4, 2024 16:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changelog:user User facing changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Tab-Navigation
2 participants