Skip to content
This repository has been archived by the owner on May 29, 2019. It is now read-only.

fix(modal) Implement W3-compliant tabbing on the modal - no tabbing into the background document #2461

Closed
wants to merge 3 commits into from

Conversation

jmwolfe
Copy link

@jmwolfe jmwolfe commented Jul 14, 2014

This PR enables the bootstrap modal to use tabbing in forms without the tab cycle taking you to the background document. It implements the full standard W3 specification for browser tabbing. Demo is updated to demonstrate tabbing. May want to scale that back if it's "too much about tabbing." But I think it makes for a strong demo and test of the feature. Added a red outline

jmwolfe added 3 commits July 6, 2014 00:28
this is the initial implementation... doesn't do shift-tab wrap around
the other way. Tests also aren't working because simulated tab keypress
event isn't changing document.activeElement.
Updates of modal and testing updates to support W3 specification for
tabbing through form elements.
Changed the focus ring color to black to raise less questions when users
see it.
@pkozlowski-opensource
Copy link
Member

@jmwolfe thnx for the PR! Could you squash all the commits into one so it is easier to review? Thnx!

@jmwolfe
Copy link
Author

jmwolfe commented Jul 14, 2014

Not sure what you mean... how do I do that? do another branch, copy files in from the first-round branch and recommit?

@pkozlowski-opensource
Copy link
Member

The easiest (?) way would be (from top of my head):

  • interactive rebase: git rebase -i HEAD~3
  • force-push afterwards:git push --force

@jmwolfe
Copy link
Author

jmwolfe commented Jul 14, 2014

OK... got this:
C:\Users\tqgyso0\Documents\GitHub\bootstrap [modal_bg_notab_again +0 1 -0]> git
rebase -i HEAD
3
Successfully rebased and updated refs/heads/modal_bg_notab_again.
C:\Users\tqgyso0\Documents\GitHub\bootstrap [modal_bg_notab_again]> git push --f
orce
Everything up-to-date
C:\Users\tqgyso0\Documents\GitHub\bootstrap [modal_bg_notab_again]>

@jmwolfe jmwolfe changed the title (modal) Implement W3-compliant tabbing on the modal - no tabbing into the background document fix(modal) Implement W3-compliant tabbing on the modal - no tabbing into the background document Jul 14, 2014
@jmwolfe
Copy link
Author

jmwolfe commented Jul 17, 2014

I tried consolidating... what's the next step? is it stuck in pull-purgatory? ⌚

@mbcooper
Copy link

Any notion when this might appear in a stable build of Angular-ui/bootstrap?

@jmwolfe
Copy link
Author

jmwolfe commented Jul 27, 2014

bump @pkozlowski-opensource -- do I need to do something else to get this in? I used a good chunk of my holiday to get this ready as there seemed to be some healthy demand for it. Do I really need to create a new branch, pull all my changes over to it, and do it in only a single commit? The few commits i have aren't that complicated I don't think.

@aaronkaka
Copy link

+1

3 similar comments
@mbcooper
Copy link

+1

@gindulis
Copy link

+1

@db6edr
Copy link

db6edr commented Aug 11, 2014

+1

@mbcooper
Copy link

Any idea when this might be merged?

@aaronkaka
Copy link

^^^ this

@mbcooper
Copy link

+1
On Aug 11, 2014 4:53 AM, "Dirk Räder" notifications@github.com wrote:

+1


Reply to this email directly or view it on GitHub
#2461 (comment).

@kevinoid
Copy link
Contributor

kevinoid commented Sep 1, 2014

Thanks for working on this! I'm looking forward to seeing this fixed.

However, as a non-contributor, I'm a little concerned that this approach reimplements the sequential focus navigation algorithm and takes over focus navigation from the browser. Although the implementataion is very good, the approach seems very error prone and likely to need updating whenever new focus targets are added to one of the HTML standards. Also, as noted in the source comments, it doesn't handle dynamic changes to focusable elements (e.g. if readonly/disabled/visibility/etc. are dynamic).

Is there a reason why the upstream Bootstrap approach of listening for focusin and calling focus() whenever the target is not a child of the top modal was not used? Granted it doesn't currently appear to handle Shift-Tab cycling correctly, but that's probably solvable.

@jdanyow
Copy link

jdanyow commented Sep 2, 2014

+1

1 similar comment
@omniscient
Copy link

+1

@chrisirhc
Copy link
Contributor

@jmwolfe Sorry, I'm not sure about this PR in its current state because it is very different from what I discussed with you on #738 .
@kevinoid raised a valid point, is there a reason why we're not using the simple focusin handler approach? Why do we need to reimplement "tab" events?

@jmwolfe
Copy link
Author

jmwolfe commented Sep 6, 2014

OK. I see what happened. I looked into the focusin approach, and actually started that way. but bootstrap 3's implementation of this seemed really JQuery heavy. I wasn't sure about trying to engineer it without JQuery. I went off of @hall5714 's comment ( #738 (comment)) and did a tab-override approach. I noted that @chrisirhc noted we could ignore tabindex and made a reference to focusin but I must have read it too quickly and didn't notice the fact I wasn't dealing with focusin. I asked for feeback on the approach, but nobody had anything to say about it (unfortunately until after it was polished and tested, etc. :( ).

I guess it's back to the drawing board. I have more DOM event experience at this point; perhaps I can build it again around focusin.

Until then, if anyone really needs an implementation, the one on the named fork works perfectly. :) And really, how soon will the tabbing specification change?

Closing this PR.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet