suitcss / base

CSS base styles for web apps (a thin layer on top of normalize.css)
http://suitcss.github.io/base/test/
MIT License
194 stars 24 forks source link

AMP Compatible. #48

Closed fkraefft closed 6 years ago

fkraefft commented 6 years ago

The '!important' tag in the outline of the base/lib/base.css file it's not compatible with the new ampproject to make websites faster. It's necessary to have it?

/**
 * Suppress the focus outline on elements that cannot be accessed via keyboard.
 * This prevents an unwanted focus outline from appearing around elements that
 * might still respond to pointer events.
 */

[tabindex="-1"]:focus {
  outline: none !important;
}
kristoforsalmin commented 6 years ago

Hi @fkraefft, I don't really know why !important is necessary. I think we just need to test that in browsers we support and see if it makes any difference.

kristoforsalmin commented 6 years ago

@fkraefft Alright, here's what I've discovered so far. We have a test CSS that uses IDs, hence !important. So it's ether because of the tests or just to make sure we don't ever get an outline.

In theory we could remove the test CSS alongside with !important and we'll be fine. Although, in that case, you'll be testing against default browser's styles.

@simonsmith What do you think?

simonsmith commented 6 years ago

Judging from this commit it seems it was added to override styles that might be inside components.

I'm open to removing !important if we think this is not necessary.

cc @giuseppeg

kristoforsalmin commented 6 years ago

@simonsmith Thanks for the feedback 👍 Now I'm actually thinking of removing the entire ruleset. I'll try to explain where I'm coming from: tabindex with a negative value doesn't really mean an element shouldn't have an outline, or any other styles for that matter. Basically, the element won't reachable via keyboard, but it still could be focused, programmatically for example, and it that case why shouldn't it have an outline? Of course it's up to a developer to think about potential accessibility concerns. A good use case can be found here https://govuk-elements.herokuapp.com/errors/example-form-validation-multiple-questions (try submitting the blank form).

I also checked Bootstrap and Pure CSS — none of them remove the outline.

What do you think?

cc @giuseppeg

giuseppeg commented 6 years ago

yeah elements with tabindex="-1" can still be focused programmatically so I don't see why they shouldn't get an outline.

giuseppeg commented 6 years ago

maybe @necolas has an explanation though

kristoforsalmin commented 6 years ago

@simonsmith Any thoughts on https://github.com/suitcss/base/issues/48#issuecomment-424601769?

simonsmith commented 6 years ago

@racse1 I think removing the ruleset makes sense. There isn't really a strong case to keep it. Are you up for opening a PR?

kristoforsalmin commented 6 years ago

@simonsmith Sure, will do 👍

simonsmith commented 6 years ago

Fixed in #49