Adding buttonFocus option

[quasipickle]

Added a buttonFocus option so user can specify which button (or none)
to focus by default.

I know this PR won't get accepted yet as I haven't successfully managed to write the tests, nor have I grunted - I just want to share and get the code green lighted before I go though with that.

[quasipickle]

I'm having trouble figuring out how to write a test for this.

I want to find out what the focused element is, but document.activeElement is referencing the document body, not the focused button. If I explicitly call document.getElementById('alertify-ok').focus(); first, then document.activeElementreferences the expected element.

I don't understand why .focus() in the library isn't having the same effect.

Update
I think QUnit might be buggering something up. This code works as expected and outputs "A" (the focused element's tag name) to the console: http://jsfiddle.net/t4eRJ/

This test writes "BODY" to the console.

test("default OK", function(){
    expect(1);

    alertify.confirm("Test");
    this.ok     = document.getElementById("alertify-ok");
    this.cancel = document.getElementById("alertify-cancel");

    console.log(document.activeElement.tagName);

    equal(document.activeElement,this.ok,'OK focused');
});

[fabien-d]

Functional testing is far from up to date in alertify - so I wouldn't necessarily worry about it. I'm still new to testing so I've been focusing more on unit testing than functional testing. If you can sort it out, and more functional test can be based off of it that would be fantastic.

I'm assuming since PhantomJS uses WebKit, it most likely detects the transitionend property and focus isn't set when you attempt to grab the active element. Since in the test there is no trigger element, you get back the body element.

In the jsfiddle you sent, the focus element is the trigger element and not the OK button. I'm assuming it's the same issue, focus isn't set until the transition is done, and therefore the active element remains the trigger button.

[fabien-d]

=== instead of ==
" instead of '

The only issue I have is with null. If focus isn't set to either button, than a keyboard user will have to tab through the entire page to reach the button of the DOM and enter the dialog element before being able to click a button. (or resort to using the mouse)

Could add another empty <a> in the dialog and assign focus to it? Seems a bit hacky...

[quasipickle]

If the desire is to have nothing focused, then the desire is to use the mouse anyway - the idea being to slow the user down and have them really consider what the dialog is saying.

I'm unable to tab the buttons into focus anyway - either when using my updated code or the original.

[fabien-d]

Overall I like it... a couple of minor code styling. My biggest concern is the focus state when set to none. If we can sort that out, I'll merge this in.

[fabien-d]

Tying to issue #86

[fabien-d]

I've update to 0.3.4 to resolve another issue. Just an FYI that you'll need to merge master into your branch so it can be automatically merged.

[quasipickle]

I'll merge once I've figured out how to get the /lib/ files generated.

As far as the focus goes, the whole point of setting the focus to neither button is to remove the possibility for someone to inadvertently hit the enter button. So having no focus is kind of the point.

With that said, if the user tabs the button into focus, they're obviously making a conscious effort, so the button wouldn't be accidentally triggered in that case. However, I'm unable to tab the button into focus, even when using the original library code (ie, the OK button gets focused by default, then I hit tab a bunch, but the OK button never regains focus).

[fabien-d]

The inability to tab is a browser setting which (I believe) is off (mac webkit only?) by default.

Safari Preferences

Press Tab option

Chrome Settings

Pressing Tab option

I don't have an issue tabbing through the links in the dialogs. The idea to not set the focus inside the dialog means that as a keyboard user (I rarely use the mouse) I have to tab through the entire page. I understand that the idea is to avoid (as much as possible) a user from doing something they didn't want.

But I won't sacrifice accessibility for it. I like the concept here - setting which button has focus, but still need a solution for null.

Also as an alternative, have you considered changing the labels on the button to be more descriptive? E.g. "Disconnect Account" and "Cancel" instead of a simple "OK", "Cancel" to re-enforce the action they will be taking?

[quasipickle]

Good point - duh. I was testing in Firefox, Chrome didn't have the problem.

I see there's an #alertify-resetFocus element that gets added to the end of .alertify-dialog. Is there a reason it's at the end, or could we put it at the beginning of the dialog and focus that?

I have changed the wording of the buttons. My personal concern is not that my clients will be confused as to what will happen when the button is clicked, but that they might hit [Enter] without realizing it will execute the highlighted action (I'm not working with completely savvy clients).

[fabien-d]

The resetFocus element is at the end to keep the focus inside the dialog. When it receives focus, it automatically resets it to the first element so you can tab all you want and never leave the dialog

[quasipickle]

Hmm, then ya - the only solutions I can think of are:

1. As you suggested create a blank at the beginning of the dialog when the option is set to "none"
2. Give focus to the last focusable element on the page before the dialog.

#2 would be tricky because there's no easy way to query the page for focusable elements. One could use document.querySelectorAll('a,input,button,select,textarea') and focus on the last element, but that requires IE8+

I just updated my commit/PR with code that does #1. It only adds 2 more lines.

[fabien-d]

👍

[fabien-d]

Option 1 works for me. Also for commit 077b28b, did you by chance remove the wrong file?

[quasipickle]

Crap, yes I did remove the wrong file. Everything should be up-to-date now (except lib/*, I'll get to that when I have a lull in my work)

[fabien-d]

it seems like the 2 test files have been removed...Make sure these are still included and that you merge with master. I've made a few updates since you started this branch and it can no longer be automatically merged.

[quasipickle]

I believe I have done all that now.

[quasipickle]

Sorry, I tried to squash my commits, but I'm new to actually Git, so I don't think my rebase worked. Do I have to send a new PR with my now merged master branch?

[fabien-d]

one last nitpick... space between if (

[fabien-d]

Actually that may be better - just link it to this one so the conversation doesn't get lost. And make sure you pull my master into your branch so it can be merged automatically.

Also, send the PR to the branch staging. I'm going to update documentation, that way I can do a code review, merge, pull down that branch, do the testing (without setting new remotes to pull down the PR branch) and so on... Just going to be a new process.

[quasipickle]

Again..., I'm new - by "pull your master", do you want me to clone your 0.4.0 branch, then merge my master into that newly cloned branch?

[fabien-d]

This is off the top of my head, so may want to just research and read up and confirm the syntax:

Setup an upstream, a link so you can access my repo
git remote add upstream git://github.com/fabien-d/alertify.js.git

then you can fetch, which will pull down the latest code from this repo
git fetch upstream

and then you can pull the changes from this repos master into your branch (may want to create a test branch to do this just to see what happens)
git pull upstream master

You can read a bit on it here... http://gun.io/blog/how-to-github-fork-branch-and-pull-request/ under Make Your Fork Track the Original Upstream Repo

The article suggests doing a git merge upstream/master (from your master branch) which should make your repos master match my repos master... and then you would be able switch back to your dev branch, and do a git pull origin master and it should pull my changes into it.

Again, I would suggest you try it out in a test branch, so if anything goes wrong, you can just trash and move on.

[quasipickle]

Maybe it's just that I'm really new at Git, but this seems like a lot of work just to submit a simple patch. At this point I've spent more time figuring out (unsuccessfully) how to work with Git than writing actual code. If you want to accept this PR, cool. If not, I've got a copy of the source that's working for my needs and I'll just leave it at that.

[fabien-d]

The reason I can't accept it now is that it can't be automatically merged. The whole process of setting up an upstream doesn't normally get into play when submitting a quick patch, the problem here is that I've made some updates to master since you created your branch.

Currently your code base is out of date with mine, and some of your changes are conflicting with the updated master branch in my repo, so git doesn't know how to merge. Setting up an upstream simply allows you to pull in the latest update from my repo into yours, so that your pull request can be automatically merged.

I appreciate your patience, and if you can set it up into another PR which includes the latest code from my master branch so it can be automatically merged, I'll merge, otherwise I can't... actually can't, I don't get the merge button.

It can sound like a lot, but it's really not that bad when you get a hang of git.