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

Why not merge changes back into bootstrap itself? #1

Closed
patrickhlauke opened this issue Jan 29, 2014 · 21 comments
Closed

Why not merge changes back into bootstrap itself? #1

patrickhlauke opened this issue Jan 29, 2014 · 21 comments

Comments

@patrickhlauke
Copy link

Just wondering, as the approach seems to be that this plugin retrofits accessibility on top of bootstrap, why it was not instead submitted as patches directly into core bootstrap project itself?

@poorgeek
Copy link

👍 had the same thought myself

@vick08
Copy link
Contributor

vick08 commented Jan 30, 2014

Thanks for asking. This is coming once we get the community play with the plugin and give us suggestions on improvements and refinements. We already received good feedback and are happy we didn’t rush with pull requests. HTH!

On Jan 30, 2014, at 3:17 PM, Justin Stockton [email protected] wrote:

had the same thought myself


Reply to this email directly or view it on GitHub.

@ddprrt
Copy link

ddprrt commented Jan 31, 2014

👍 to @patrickhlauke request. Accessibility should be not an option but standard.

Great work, though! It's about time somebody takes care on that issue

@vick08
Copy link
Contributor

vick08 commented Jan 31, 2014

Thanks. Point taken. :)

On Jan 31, 2014, at 2:02 AM, Stefan Baumgartner [email protected] wrote:

to @patrickhlauke request. Accessibility should be not an option but standard.

Great work, though! It's about time somebody takes care on that issue


Reply to this email directly or view it on GitHub.

@zacechola
Copy link

👍 for merging into core Bootstrap. I'm glad this project exists, though, so I'll be using it/providing feedback as is. Thanks!

@zacechola
Copy link

Looking through some of the styles, @mdo would not merge in some changes. Changing the .alert color contrast to simply use black would be immediately rejected. twbs 3.1 alert contrast currently meets WCAG 2.0 Level AA, but that took several tries before someone found a color combination the core team liked enough to merge in.

Here's the last change: twbs/bootstrap#11432 You can follow the references from there.

@vick08
Copy link
Contributor

vick08 commented Feb 7, 2014

The large chunk of changes have to do with keyboard and ARIA support. I am sure we can reconcile the visual style differences.

On Feb 7, 2014, at 9:35 AM, Zac Echola [email protected] wrote:

Looking through some of the styles, @mdo would not merge in some changes. Changing the .alert color contrast to simply use black would be immediately rejected. twbs 3.1 alert contrast currently meets WCAG 2.0 Level AA, but that took several tries before someone found a color combination the core team liked enough to merge in.

Here's the last change: twbs/bootstrap#11432 You can follow the references from there.


Reply to this email directly or view it on GitHub.

@mdo
Copy link

mdo commented Feb 7, 2014

We've always been on board with improving the accessibility features of Bootstrap. The biggest problem has been coordination and scope of changes. I have yet to dive into this more extensively, but we obviously welcome any and all practical improvements that can be made.

<3

@cvrebert
Copy link
Contributor

Seems like a good chunk of this requires only simple changes to the static HTML in Bootstrap's docs (rather than invasive JS magic):

Selector Add attribute
[data-toggle=dropdown] aria-haspopup="true"
.modal-content role="document"
[data-toggle="collapse"] role="tab"
[data-slide="prev"] [data-slide="next"] role="button"
.nav-tabs role="tablist"
.nav-tabs > li role="presentation"
.nav-tabs > [data-toggle="tab"] .nav-tabs > [data-toggle="pill"] role="tab"

@superscott
Copy link

+1 for Merging.

janwalther added a commit to janwalther/bootstrap-accessibility-plugin that referenced this issue Apr 2, 2014
This patch fixes a JS error when the element with the attribute data-toggle="collapse" has a URL href (instead of a jQuery selector) and additionally a data-target attribute with the jQuery selector for the collapsible element. This is useful if you want to offer the opportunity to open the link in a new window (or even when JS is disabled).
Example: 
<a data-toggle="collapse" href="http://www.example.org/" data-target="#collapsibleElement>
  Collapsible Group Item paypal#1
</a>
@weboverhauls
Copy link
Collaborator

@cvrebert These roles are only needed when JS is running, therefore they should be added with the JS. For example, if they were added in the HTML, and a screen-reader user doesn't have access to JS, he/she will be very confused because the functionality described is missing.

@cvrebert
Copy link
Contributor

@weboverhauls Bootstrap already pretty much assumes (at least for the widgets in question) that JS is running.

@patrickhlauke
Copy link
Author

@weboverhauls I think that train already left the station a long time ago...devs adding roles directly to their HTML, and not generating it via JS, is a fairly common practice. I'd say any screenreader user without JS running is going to have a surprising/bad time nowadays on most sites.

@weboverhauls
Copy link
Collaborator

Sorry to hear that Patrick (and Chris). I thought you had a much higher appreciation for inclusive design and progress enhancement. Code quality is key. Please move this discussion to Twitter if you'd like.

@cvrebert
Copy link
Contributor

I'm just stating the facts about how Bootstrap is currently architected, not any opinion on the merits of that architecture.

Please move this discussion to Twitter if you'd like.

No thanks. Intelligent discussions don't fit into 140 characters.

@zacechola
Copy link

I think it should be noted that WebAIM has studied the usage of JavaScript in screen readers and found the overwhelming majority have JavaScript enabled. Granted, this is a survey without a control group, so a big grain of salt. Similar results have been found in previous surveys, though.

@weboverhauls
Copy link
Collaborator

The screen reader example was just an obvious use case. The much bigger issue was outlined in a recent study in UK which showed that .9% of ALL users had JS enabled but didn't receive it. http://bit.ly/RTzBVk

@weboverhauls
Copy link
Collaborator

Code from the plugin continues to be ported into core, so closing issue.

@cvrebert
Copy link
Contributor

cvrebert commented Oct 3, 2014

Is the plugin's code going to be slimmed down accordingly, as things get ported into core?

@vick08
Copy link
Contributor

vick08 commented Oct 3, 2014

The plugin is smart to overwrite the core features if we feel our implementation is better. In other words, we have been able to avoid conflicts so far.
On Oct 3, 2014, at 2:33 PM, Chris Rebert [email protected] wrote:

Is the plugin's code going to be slimmed down accordingly, as things get ported into core?


Reply to this email directly or view it on GitHub.

@weboverhauls
Copy link
Collaborator

I would like to see the plugin slimmed down.

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

No branches or pull requests

9 participants