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

Too strong selectors #24140

Closed
joneff opened this issue Sep 27, 2017 · 19 comments
Closed

Too strong selectors #24140

joneff opened this issue Sep 27, 2017 · 19 comments
Labels

Comments

@joneff
Copy link
Contributor

joneff commented Sep 27, 2017

The following selector Is a notch too strong (0, 2, 1) and overrides normal class names:

bootstrap/scss/_reboot.scss

Lines 208 to 220 in 6dd3d91

a:not([href]):not([tabindex]) {
color: inherit;
text-decoration: none;
@include hover-focus {
color: inherit;
text-decoration: none;
}
&:focus {
outline: 0;
}
}

It could be

a[name]:not([href]):not([tabindex]): {
  color: inherit;
  text-decoration: none;


  @include hover-focus {
    color: inherit;
    text-decoration: none;
  }


  &:focus {
    outline: 0;
  }
}

Note: Updated snippet to better reflect the purpose of the styling i.e. named anchors. Original selector was a:not([href]):not([tabindex]):not([class])

@joneff joneff changed the title To strong selectors Too strong selectors Sep 27, 2017
@prateekgoel
Copy link
Contributor

I agree @joneff .
Can I submit a PR to this in case you're not working on this?

@joneff
Copy link
Contributor Author

joneff commented Sep 27, 2017

It's all yours.

@prateekgoel
Copy link
Contributor

Thanks @joneff

@mdo
Copy link
Member

mdo commented Oct 1, 2017

What about anchors without a name attribute? Also, apparently name is obsolete in HTML 5 per the MDN docs: https://developer.mozilla.org/en-US/docs/Web/HTML/Element/a.

@prateekgoel
Copy link
Contributor

Yes, you are right @mdo. Can we use something like a:not(:link):not([tabindex])? However, this seems less specific and may be overwritten by other CSS.

@joneff
Copy link
Contributor Author

joneff commented Oct 2, 2017

@mdo, yes named (as in a[name]) anchors are obsolete, but are still widely used. The rest of cases use id attribute on the nearest element. In the case of github issue comments, per say, the naming is done with ID attribute on div elements.

For those who use the standard approach i.e. <element id="some_id">...</element> the current selector is not an issue. However, those who have <a class="my-class">...</a>, the current selector is quite strong.

As an example of such links, if you have a slider widget with the slider part and two buttons for changing the value, you don't need tabindex nor href attribute on those because, per spec, you can only focus the widget itself and it manages the focus on its child items.

To me, assuming that anyone using named anchors is doing so by using the name attribute and not the id attribute has much smaller impact than assuming that anyone not having href or tabindex attribute is using named anchor and in terms forcing them to do something about the color and / or text decoration.

@XhmikosR
Copy link
Member

XhmikosR commented Oct 3, 2017

Definitely against name personally. We ought to support proper markup and not obsolete stuff.

@joneff
Copy link
Contributor Author

joneff commented Oct 3, 2017

@XhmikosR, if that's the case, then I am not sure why the above code exists anyway :) Using named anchors with id attribute negates the need for the snippet.

@mdo
Copy link
Member

mdo commented Oct 3, 2017

Context is in the issue that code resolved: #19402.

@XhmikosR
Copy link
Member

XhmikosR commented Oct 4, 2017

I think the current implementation, which is what GitHub is using too judging from @mdo's linked issue, seems fine.

Anyway, just my 2 cents.

@prateekgoel
Copy link
Contributor

Okay. So, I will close the PR for this.

@prateekgoel
Copy link
Contributor

I think this issue can be closed now.

@bmaehr
Copy link

bmaehr commented Feb 7, 2019

I'm not sure why this is happening but this change creates a problem for our application.

We have an application using https://github.com/vsymguysung/ember-cli-summernote 1.5.0 and after upgrading to 1.5.1 some styles from the application have changed.

The reason is, that in the CSS used by the browser there is
a:not([href]):not([tabindex]) {
color: inherit;
}
quite far on the top of the list. It overrides a
.btn-primary {
color: #fff;
}
which is further down and the color black is used instead of white.

Also other visual problems occur after upgrading the version.

@orionseye
Copy link

Who had this idea anyway? supposed that a frameworks makes (my) life easier

@willimaese
Copy link

I have the same issue as @bmaehr .
"a:not([href]):not([tabindex]), a:not([href]):not([tabindex]):focus, a:not([href]):not([tabindex]):hover" selector is too strong and overrides our app styles.

@imoark
Copy link

imoark commented Aug 1, 2019

Same issue as @bmaehr
Can confirm that a:not([href]):not([tabindex]) is basically too strong, that it overrides a class selector.
Any good workaround??

@Spawnrad
Copy link

Same issue :( the overwritten is too strong

@bmaehr
Copy link

bmaehr commented Apr 28, 2020

A link requires the href="" attribute.

As far as I know there is no HTML standard which defines href as mandatory.

The examples above do not have a href="" but adding it fixes the problem.

Have you ever thought, that an application not only uses bootstrap but also other libraries where it is not so easy to just add something?

@ffoodd
Copy link
Member

ffoodd commented Apr 28, 2020

You probably want to read @mdo's last comment above for context.

Links without href are valid HTML— however those are considered placeholder links, not hyperlinks.

Styling both differently is considered a best practice, since they do not behave the same. Placeholder links litteraly do nothing and should not look like hyperlinks.

Would you have any valid use case demonstrating this could be wrong?

pat270 added a commit to pat270/clay that referenced this issue May 21, 2020
…ight` on the `html` element already set on `body`

fix(@clayui/css): Reboot don't set `text-align: left` on `body` due to RTL issues. See liferay#1743

fix(@clayui/css): Reboot remove `:focus-visible` future-proof rule

fix(@clayui/css): Reboot remove uses of Bootstrap's `font-size` and `hover` mixin, we don't support it in Clay CSS

fix(@clayui/css): Reboot remove Bootstrap's `text-align` reset on `th`. See liferay#2219

fix(@clayui/css): Reboot remove Bootstrap's "hand" cursor to non-disabled button elements. We don't support `$enable-pointer-cursor-for-buttons`. We can change the cursor to whatever we want in Clay CSS.

fix(@clayui/css): Reboot use selector `[href]` for styling anchor links instead of `a` so we don't need to overwrite placeholder anchor styles with `a:not([href])`. Styling `<a tabindex="0">a link</a>` should be done with a class like `link-primary`. Trying to avoid going down twbs/bootstrap#24140
pat270 added a commit to pat270/clay that referenced this issue Jun 29, 2020
…ight` on the `html` element already set on `body`

fix(@clayui/css): Reboot don't set `text-align: left` on `body` due to RTL issues. See liferay#1743

fix(@clayui/css): Reboot remove `:focus-visible` future-proof rule

fix(@clayui/css): Reboot remove uses of Bootstrap's `font-size` and `hover` mixin, we don't support it in Clay CSS

fix(@clayui/css): Reboot remove Bootstrap's `text-align` reset on `th`. See liferay#2219

fix(@clayui/css): Reboot remove Bootstrap's "hand" cursor to non-disabled button elements. We don't support `$enable-pointer-cursor-for-buttons`. We can change the cursor to whatever we want in Clay CSS.

fix(@clayui/css): Reboot use selector `[href]` for styling anchor links instead of `a` so we don't need to overwrite placeholder anchor styles with `a:not([href])`. Styling `<a tabindex="0">a link</a>` should be done with a class like `link-primary`. Trying to avoid going down twbs/bootstrap#24140
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

10 participants