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

Create keyboard mode for ui-ace editor #13339

Merged
merged 3 commits into from Aug 9, 2017

Conversation

Projects
None yet
4 participants
@timroes
Copy link
Contributor

commented Aug 4, 2017

Release Note: Our code editor in several places won't trap keyboard focus anymore, by using the Tab key. When navigating by keyboard, you first need to Enter editing mode in the code editor and you can leave it by pressing Escape.

Since the ui-ace editor create a keyboard trap, that you cannot tab away from anymore (since tab creates tab characters), this PR implements a keyboard mode for the editor.

If you click in the editor, everything works as beforehand. You can click in the editor start typing, and click outside to get out there. If you tab now towards the editor, an overlay will be showing, telling you to press Enter to start editing (and that you can get out with Escape again).

Pressing Enter will now jump towards the editor, and pressing escape there will bring you back to the overlay screen (from which you can freely tab away). The following screencast shows the behavior:

A visual screencast of the above described behavior

How to manually test?

If you want to test that behavior manually, click inside the editor to validate it still works and you can type. Also put the focus somewhere before the box and tab to it. Check that you cannot reach the actual editor that way and get trapped. Press Enter in the overlay to validate it enters editing mode and you cannot exit with tab anymore. Press Escape to validate you can leave the box that way again. Also validate, that Escape will still work even when entered with the mouse and Enter will also work, if you left beforehand with the mouse.

/cc @bhavyarm @LeeDr

Fixes

I've used that directive for all "simple" ui-ace usages:

  • In the query filter editor (shown above)
  • In the Saved Objects screen when editing a saved objects on all editors.

This fixes #12612

Due to the way Console initializes it's ACE editor, it will need some special treatment, and I would look into it once the general behavior is merged and okay for everyone.

@bhavyarm

This comment has been minimized.

Copy link
Contributor

commented Aug 4, 2017

Thanks @timroes. This is very helpful :)

@cjcenizal
Copy link
Contributor

left a comment

Awesome!! Just had a couple suggestions.

uiModules.get('kibana').directive('kbnUiAceKeyboardMode', () => ({
restrict: 'A',
link(scope, element) {
const uniqueId = `uiAceKeyboardHint-${scope.$id}-${Math.floor(Math.random() * 10000)}`;

This comment has been minimized.

Copy link
@cjcenizal

cjcenizal Aug 7, 2017

Contributor

Why is the random token needed? I assume the $id attribute comes from the ace editor -- is that value not guaranteed to be unique?

If it is necessary, how about we just store a global ID value and auto-increment it? Seems a little simpler, and guaranteed to be unique.

let aceKeyboardModeId = 0;

uiModules.get('kibana').directive('kbnUiAceKeyboardMode', () => ({
  restrict: 'A',
  link(scope, element) {
    const uniqueId = `uiAceKeyboardHint-${scope.$id}-${aceKeyboardModeId++)}`;

This comment has been minimized.

Copy link
@timroes

timroes Aug 8, 2017

Author Contributor

I used the same approach we used in several other places (date picker, typeahead). This brings us back to the topic, that we might want to create a universal HTML Id generation service, that we have a distinct way to create those ids :-)

But I will change it to your suggestion.

link(scope, element) {
const uniqueId = `uiAceKeyboardHint-${scope.$id}-${Math.floor(Math.random() * 10000)}`;

const hint = angular.element(`<div class="ui-ace-keyboard-hint" id="${uniqueId}" tabindex="0" role="application">

This comment has been minimized.

Copy link
@cjcenizal

cjcenizal Aug 7, 2017

Contributor

Could you leave a comment explaining why you chose role="application"?

This comment has been minimized.

Copy link
@cjcenizal

cjcenizal Aug 7, 2017

Contributor

Also, can we indent the markup to make it a little easier to read? We can also use some UI Framework components here, which will make it easier to change the font-family later.

    const hint = angular.element(`
      <div
        class="ui-ace-keyboard-hint"
        id="${uniqueId}"
        tabindex="0"
        role="application"
      >
        <p class="kuiText kuiVerticalRhythmSmall">
          Press Enter to start editing.
        </p>

        <p class="kuiText kuiVerticalRhythmSmall">
          When you&rsquo;re done, press Escape to stop editing.
        </p>
      </div>
    `);

This will make it look like this:

image

This comment has been minimized.

Copy link
@timroes

timroes Aug 8, 2017

Author Contributor

role=application - I found it the best fitting role. This element kind of introduces it's own navigation schema (entering, exiting with keys) and also when in the editor, you won't be able to use tab anymore, and arrow keys will navigate all within that editor. But I also thought a lot what it should have, and figured out it might be the best (in contrast to textbox, which I mentioned I guess in the ticket somewhere, which would read out to a textbox, but apparently the user cannot do what she can do in any other textbox, since you require some additional enter press before editing starts).

Regarding the text: I can modify it like that. I actually find the text way more beautiful and had something similar in my first draft. The reason I decided for the very short sentence was: I fear people would press Enter directly when they hear "Press Enter to start editing. " and miss the part, that they actually can exit by pressing Escape again, because the screen reader had a short pause between the sentences. That's why I tried to put it into one sentence. But I am glad to change it, if you feel like it's more reasonable. What do you think about people maybe missing the Escape mode?

This comment has been minimized.

Copy link
@cjcenizal

cjcenizal Aug 8, 2017

Contributor

I think you're right, role="application" sounds perfect for this widget. Thanks for the explanation!

That's a good point that people using screen readers may hit Enter without waiting to hear the entire prompt. But I think they're used to hearing multiple sentences, the same way we're used to reading them. And if they do skip ahead, I think they might be able to guess that "Esc" will let you exit this special interaction mode, so they won't be completely stuck.

opacity: 1;
border: 2px solid @globalColorBlue;
z-index: 1000;
}

This comment has been minimized.

Copy link
@cjcenizal

cjcenizal Aug 7, 2017

Contributor

Can we reformat this to use camel case per the style guide and to nest these states to be more efficient? I also added flex-direction and text-align to refine the appearance.

.uiAceKeyboardHint {
  position: absolute;
  top: 0;
  bottom: 0;
  right: 0;
  left: 0;
  background: rgba(255, 255, 255, 0.7);
  display: flex;
  flex-direction: column;
  justify-content: center;
  align-items: center;
  text-align: center;
  opacity: 0;

  &.uiAceKeyboardHint-isInactive {
    display: none;
  }

  &:focus {
    opacity: 1;
    border: 2px solid @globalColorBlue;
    z-index: 1000;
  }
}

This comment has been minimized.

Copy link
@timroes

timroes Aug 8, 2017

Author Contributor

I will reformat them to use camel case syntax. Why would nesting the states be more efficient? In CSS the :focus will anyway compile to the same, so no difference there. The isInactive state should actually be slower, since it has to check multiple classes instead of a single on on the element. Though since we have the example for states like this in our styleguide I will of course also fix it (though in my opinion giving states the full prefix (uiAceKeyboardHint-), makes nesting kind of unnecessary, but I won't fight the styleguide here :-)

This comment has been minimized.

Copy link
@cjcenizal

cjcenizal Aug 8, 2017

Contributor

Oh, sorry I meant efficient in terms of readability. By using &:focus it's a bit easier to scan the styles and grok the context of all the available selectors -- they're all related to the "uiAceKeyboardHint" component.

@timroes timroes added the review label Aug 8, 2017

@timroes

This comment has been minimized.

Copy link
Contributor Author

commented Aug 8, 2017

Ready for a new review :)

@cjcenizal
Copy link
Contributor

left a comment

🦈 LGTM!

@cjcenizal

This comment has been minimized.

Copy link
Contributor

commented Aug 9, 2017

@timroes Don't forget to put version labels of 6.1.0 and 7.0.0 on this, and backport to the 6.x branch.

@timroes timroes removed the review label Aug 9, 2017

@timroes timroes merged commit e66c1d2 into elastic:master Aug 9, 2017

2 checks passed

CLA Commit author has signed the CLA
Details
kibana-ci Build finished.
Details

@timroes timroes deleted the timroes:a11y-ace branch Aug 9, 2017

timroes added a commit that referenced this pull request Aug 9, 2017

Create keyboard mode for ui-ace editor (#13339)
* Add kbn-ui-ace-keyboard-mode directive

* Implemented PR feedback

* Fix broken tests
@timroes

This comment has been minimized.

Copy link
Contributor Author

commented Aug 9, 2017

Backport:

chrisronline added a commit to chrisronline/kibana that referenced this pull request Nov 20, 2017

Create keyboard mode for ui-ace editor (elastic#13339)
* Add kbn-ui-ace-keyboard-mode directive

* Implemented PR feedback

* Fix broken tests

chrisronline added a commit to chrisronline/kibana that referenced this pull request Dec 1, 2017

Create keyboard mode for ui-ace editor (elastic#13339)
* Add kbn-ui-ace-keyboard-mode directive

* Implemented PR feedback

* Fix broken tests
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.