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 outline:none for input field #87

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

suryanisaac
Copy link

@suryanisaac suryanisaac commented Nov 18, 2020

Outlines on the input look bad and don't go with the themes (amazing themes btw)
I'll add pictures for comparison in a comment.

Edit: This does not affect tab navigation for accessibility

Outlines on the input look bad and don't go with the themes (amazing themes btw)
I'll add pictures for comparison in a comment.
@suryanisaac
Copy link
Author

suryanisaac commented Nov 18, 2020

Eclipse

When focused:
Screenshot 2020-11-18 at 10 29 57

When focused with no outline:
Screenshot 2020-11-18 at 10 30 16

9009

Default:
Screenshot 2020-11-18 at 10 13 24

When focused:
Screenshot 2020-11-18 at 10 13 31

When focused with no outline:
Screenshot 2020-11-18 at 10 15 15

1976

When Focused:
Screenshot 2020-11-18 at 10 15 34

When focused with no outline:
Screenshot 2020-11-18 at 10 15 44

Solarized Dark

When Focused:
Screenshot 2020-11-18 at 10 16 07

When focused with no outline:
Screenshot 2020-11-18 at 10 16 00

@jakergrossman
Copy link
Contributor

jakergrossman commented Nov 19, 2020

Edit: This does not affect tab navigation for accessibility

While this may not affect tab navigation, this does affect accessibility for those who have a visual impairment. A better solution (IMHO) is to fade the outline after a period of time, so as to still indicate focus. Maybe something like:

@keyframes fade-outline {
  to { outline-color: rgba(0,0,0,0); }
}

#input-field:focus {
  animation: fade-outline 0.375s linear 2s both;
  outline-style: solid;
}

potatoes

@suryanisaac
Copy link
Author

I never thought of that, it solves both problems I worried about.

I don't think it would be too much work to sync the outline colour to a colour included in the theme and fade it. That would look good too, but i'm not sure how it would work for accessibility.

I've never coded for accessibility because most of my projects are for my own use. If you think that it doesn't work, then the blue fade is probably fine too.

@jakergrossman
Copy link
Contributor

jakergrossman commented Nov 19, 2020

You could customize the outline color, but seeing that the themes are disorganized CSS stylesheets, that would require either:

  1. Modifying every theme to include the outline color for #input-field:focus, or

  2. Grabbing an appropriate color from the page via JavaScript.

These aren't impossible hurdles by any means, but I just opted to let the user agent stylesheet define the color for input:focus.


One solution that would require only modification of CSS stylesheets without messing with the JavaScript implementation of theme selections would be to refactor the themes to use custom CSS properties that are semantically named.

This can be seen in action in the implementation of themes for my portfolio, where each color of the theme is attached to a named custom CSS property (--background-color, --caret-color, etc.). Then it would be as simple as:

#input-field:focus {
  animation: fade-outline 0.375s linear 2s both;
  outline-color: var(--foo-bar);
  outline-style: solid;
}

Approaching the problem from a JavaScript perspective, you could set the outline-color property of #input-field when inside of the setTheme() function (maybe pull from .highlight class). This seems like the best "design first" solution.

That said, by far the simplest solution is to let the browser define the outline-color property. This would also let users who have custom focus styles (specific colors to assist with colorblindness, for example) continue to use those styles.

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.

2 participants