twbs / bootstrap

The most popular HTML, CSS, and JavaScript framework for developing responsive, mobile first projects on the web.
https://getbootstrap.com
MIT License
170.5k stars 78.84k forks source link

Disabled button group radio item is selectable in 3.3.5 js #16703

Closed NogsMPLS closed 8 years ago

NogsMPLS commented 9 years ago

Disabling a button group item does not seem to work anymore in 3.3.5. This is using the button.js bootstrap plugin.

Here is a fiddle with 3.3.4: https://jsfiddle.net/k6a3j46o/28/

Clicking the disabled item does nothing, as expected.

Here is the same fiddle with 3.3.5: https://jsfiddle.net/k6a3j46o/27/

Clicking the disabled item actually adds .active to the label field, so you end up with an active disabled item. I believe this is not expected behavior.

There seems to be a disconnect on the disabled input's wrapper container behavior and the actual input behavior. I am pretty sure the disabled attribute on the input is correct and will work correctly, but if a label element has the disabled class, it shouldn't be able to add the active class.

This is bug is specifically for the click event I think, as there are still use cases of have an 'active disabled' item given from server or other means, but the user should not be able to select a disabled button group item with the click event as it should be read only.

twbs-lmvtfy commented 9 years ago

Hi @NogsMPLS!

You appear to have posted a live example (https://fiddle.jshell.net/k6a3j46o/28/show/light/), which is always a good first step. However, according to Bootlint, your example has some Bootstrap usage errors, which might potentially be causing your issue:

You'll need to fix these errors and post a revised example before we can proceed further. Thanks!

(Please note that this is a fully automated comment.)

twbs-lmvtfy commented 9 years ago

Hi @NogsMPLS!

You appear to have posted a live example (https://fiddle.jshell.net/k6a3j46o/27/show/light/), which is always a good first step. However, according to Bootlint, your example has some Bootstrap usage errors, which might potentially be causing your issue:

You'll need to fix these errors and post a revised example before we can proceed further. Thanks!

(Please note that this is a fully automated comment.)

NogsMPLS commented 9 years ago

Updated fiddles -

3.3.4: https://jsfiddle.net/k6a3j46o/30/

3.3.5: https://jsfiddle.net/k6a3j46o/29/

cvrebert commented 9 years ago

What browser and OS are you using?

NogsMPLS commented 9 years ago

Windows 7, reproduced in Chrome and Firefox

mike-zorn commented 9 years ago

I found that even just a disabled button will still send click events in 3.3.5 (Chrome OSX)

3.3.4 (expected behavior: no alert opens on click) 3.3.5 (disabled button shows alert)

NogsMPLS commented 9 years ago

hmmm...good point, my pull request just prevents the toggle from running, but it appears that what really needs to happen is a setting state to disabled. You'll notice that the 'Loading...' button, when set to disabled is properly disabled. I'll see if I can come up with a good solution for this.

mike-zorn commented 9 years ago

I'd guess this has something to do with twbs/bootstrap@c26ffd4. If you add pointer-events: none to the button's styling, the alert won't fire in my example.

NogsMPLS commented 9 years ago

Huh, interesting. That does seem like the main issue. Annoying this is though, putting pointer-event:none back into the css makes the 'No Action' symbol on hover go away, as hover is a pointer-event.

NogsMPLS commented 9 years ago

@apechimp, you're test with the button element specifically can be solved by adding the 'disabled' attribute, alongside the disabled class to the button:

http://jsfiddle.net/b4kgw6vn/2/

This however does not fix the <label><input/></label> style of buttons and button groups:

https://jsfiddle.net/k6a3j46o/29/

NogsMPLS commented 9 years ago

yeah, I don't think label elements are allowed to be truly 'disabled'. This might be a bigger issue in how bootstrap handles disabling checkbox and radio buttons.

The best solution so far is setting pointer-events: none but that prevents cursor: not-allowed from applying on hover.

I believe that to be a more than fair trade off in order to keep consistent functionality.

cvrebert commented 9 years ago

@apechimp When using an actual <button> like in your examples, you should use the disabled attribute rather than the .disabled class.

pointer-events: none isn't truly enough to prevent clicks. Although you can't click on the button in http://jsfiddle.net/7uLrsex3/1/ using your mouse, you can still press the Tab key to focus it and then press the spacebar to "click" it, resulting in an alert box. See also http://getbootstrap.com/css/#callout-buttons-disabled-anchor

mike-zorn commented 9 years ago

Ahhhh, thanks, @cvrebert. Sorry for hijacking this issue.

cvrebert commented 9 years ago

Confirmed via https://jsfiddle.net/Ldh1fzcj/1/ that this is a valid bug in the button.js plugin's checkbox/radio widget.

NogsMPLS commented 9 years ago

I'm not sure if my pull request #16707 correctly solves the core problem. All the pull request will do is prevent the Plugin.call($btn, 'toggle') from being called on click a .btn element has .disabled. The root issue is that an event gets fired at all on a disabled label.

Past versions seem to have relied on pointer-events: none;. While @cvrebert referenced http://jsfiddle.net/7uLrsex3/1/ as an example why pointer-events: none; is not fool-proof because of tabbing, if you combine it with the disabled attribute request referenced in twbs/bootlint#292 and make sure the label element also has a disabled attribute in addition to class, then I think we are 90% there.

For reference, this fiddle shows that disabled class, combined with disabled attribute and the pointer-events:none; CSS prevents click events and from the element being tabbed into (Tested: Chrome/FF, still need confirmation in IE versions): https://jsfiddle.net/Ldh1fzcj/3/

I suppose we could also prevent Plugin.call($btn, 'toggle') in addition to adding pointer-events: none; and documenting the disabled attribute+class requirement. I believe that would cover almost all use cases that we actually have within our power to control.

Thoughts? If people agree, I can get a PR up tonight/tomorrow.

NogsMPLS commented 9 years ago

i will note that a downside to my suggestion is that cursor: not-allowed will not work for disabled label buttons at the very least.

cvrebert commented 9 years ago

I think it should be possible to avoid relying on pointer-events at all by tweaking the event handler. We didn't intend to rely on it in previous versions.

NogsMPLS commented 9 years ago

okay, well, in that case, we can definitely prevent Plugin.call($btn, 'toggle') from being called, which is the only thing called inside that click event. current PR does that.

And we can prevent tabbing to the element by making sure disabled class and disabled attribute are together on the same element.

But what it will NOT do is prevent any inline onClicks from firing. Other than somehow unbinding or calling $.off(), but that would cause a toooon of issues and you'd be putting the burden of rebinding on the user.

I think we'd just have to say something in documentation that states that disabled will prevent bootstrap js events from firing on the element, but any custom events are not guaranteed to be prevented.

Agree/disagree?

alhuber1502 commented 8 years ago

Not sure if this is implied, but the same issue affects tabs, click event is sent despite .disabled

gviswanathan commented 8 years ago

Whats the work around for this issue?

ortonomy commented 8 years ago

Found this issue today. I've resorted to @NogsMPLS 's solution of "pointer-events: none". It stops the disabled "cursor:not-allowed" class, but until it's fixed, it will have to do for me.

Jargon64 commented 8 years ago

@geligelu @gviswanathan I've just discovered this issue and after some experimentation I've found that hooking into the click event for the label and returning false if the disabled attribute exists works well for me. Example:

$('#label-selector').click(function () {
    if ($(this).attr('disabled')) return false;
    return true;
});
karolyi commented 8 years ago

+1

http://stackoverflow.com/questions/29331261/disable-radio-button-in-bootstrap-3#comment59213857_29331323

mdo commented 8 years ago

Bootstrap 3 is no longer being officially developed or supported.

All work has moved onto our next major release, v4. As such, this issue or pull request is being closed as a "won't fix." For additional help and support, we recommend utilizing our community resources. Thanks for your understanding, and see you on the other side of v4!

<3, @mdo and team

karolyi commented 8 years ago

Is this for real? bootstrap v4 is still alpha, and the production version (v3) is not supported anymore?

karolyi commented 8 years ago

bump

rafalp commented 8 years ago

@karolyi this is for reals. You may google futhrer if you wish so, this was discussed in plenty of places like Reddit, HackerNews or even here in other tickets.

In a nutshell: bugfixes will happen to v3, but getting v4 out the door is priority and maintenance burden for v3 has proven to be too time consuming.

garygreen commented 8 years ago

@mdo erm, bootstrap 4 still alpha and so 3 get's no more bug fixes?!

This is a bug. You shouldn't be able to select a disabled field.

NetTecture commented 7 years ago

@mdo Great. 4 months later the bug is still open, with 3.3.7 and no fix because hey, one point in the future. Anyone else please use a version with other bugs in 3.3.4

patrickhlauke commented 7 years ago

"4 months later, and the volunteers working on bootstrap in their spare time still haven't fixed this..."

wirelessed commented 7 years ago

Hi,

This issue is now re-producable in v4 alpha 6. When using a button plugin with radio button group, and when you disable the button, it is still selectable.

Temp fix is to add pointer-events: none to .btn.disabled

Dooks123 commented 6 years ago

It is probably best to revert to bootstrap 3.3.5 then