[Lens] (Accessibility) focus on adding/removing layers

[mbondyra]

Summary

Fixes #83595

1. When layer is reset (so there's just one) the focus goes to the layer.

2. When layer is removed, the focus goes to the layer before it.
   

3. When layer is added, the focus goes to the added layer.
   

Adding tabIndex={-1} caused the side effect - now when we click on the layer, it gets selected. I am not sure if it's a problem though

[elasticmachine]

Pinging @elastic/kibana-app (Team:KibanaApp)

[dej611]

Code looks ok, just a minor comment. Tested locally on Chrome and Firefox and it works as described in the PR description.

TBH I'm not 100% sure about the behaviour here, I understand the focus should go to the first tabbable element, but see that tooltip show up makes me wonder as user I did something wrong.
But if @myasonik is ok then I won't object.

To clarify my concerns are:

• while it makes sense to put the focus on the first tabbable element, having that tooltip show up confuses me a bit. It would be great to either have the focus without the tooltip or set the focus to the datasource selection maybe?
• when I have multiple layers and delete it, I would expect the focus to move to the N-1th layer rather than scroll all the way up.

[mbondyra]

I agree with both points @dej611 – it's quite unfortunate that the first focusable element in the layer is a tooltip. So far I just implemented it according it to the issue description, but this PR can be an intro to discussion.

[myasonik]

Awesome work @mbondyra!

I understand the annoyance with the tooltip so let's implement something slightly different that still achieves the goal.

For where to place focus

Instead of moving focus to the first tabbable element, you can also move focus to the layer panel overall. You'll need to add tabIndex={-1} so that you can programmatically set focus to it. I'd also recommend making the outermost element of the panel a <section> instead of a <div>. I don't see any support for this in EuiPanel so I'd just wrap it.

TL:DR some final-ish pseudocode of <LayerPanel /> looks like this:

<section tabIndex={-1} panelRef={ref}>
  {/* the contents of layer panel today in here */}
</section>

For delete

@dej611 said:

when I have multiple layers and delete it, I would expect the focus to move to the N-1th layer rather than scroll all the way up.

If y'all agree this would be the more common need, this would also be appropriate. Feel free to move focus to wherever you feel is the most expected next thing the person will need to interact with.

[mbondyra]

Thanks @myasonik! I've updated the PR, please take a look at the description above before checking the code :)

[mbondyra]

I am surprised that git analyzed this file this way. Line 140 is the only changed line in the whole return statement (I added the section container, so plus its closing).

[dej611]

Reviewed with this flag on and it's only 2 lines change:

[mbondyra]

Nice!!! Thanks for the tip, I forgot about this!

[ryankeairns]

CSS changes looks good.

Nice enhancement!

[myasonik]

LGTM!

If you move the lnsLayerPanel class to the <section>, you can add these styles to it to remove the focus ring for mouse clicks and it doesn't break anything else:

.lnsLayerPanel {
  margin-bottom: $euiSizeS;

  // disable focus ring for mouse clicks, leave it for keyboard users
  &:focus:not(:focus-visible) {
    animation: none !important;
  }
}

(Tested with old and new theme. Won't work if the browser doesn't support focus-visible but the fallback is it just always shows the focus ring so it's not bad.)

@ryankeairns Is that scss change fine?

[mbondyra]

@elasticmachine merge upstream

[mbondyra]

Hey @myasonik does that mean we don't want to show focus ring at all, both for mouse and keyboard users when user removes/adds a layer? I am ok with it, but just making sure because that's how it works for Chrome. For FF and Safari it doesn't give any effect.

[kibanamachine]

💚 Build Succeeded

• continuous-integration/kibana-ci/pull-request
• Commit: 240bccd

Metrics [docs]

Async chunks

Total size of all lazy-loaded chunks that will be downloaded as the user navigates the app

id	before	after	diff
lens	1.0MB	1.0MB	+3.7KB

Distributable file count

id	before	after	diff
default	47280	48040	+760

History

• 💚 Build #94831 succeeded a7b0e9f
• 💚 Build #94371 succeeded 6111c75
• 💚 Build #93940 succeeded 9082efc
• 💚 Build #92199 succeeded bedbfc4
• 💚 Build #91802 succeeded f35a289

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

[ryankeairns]

LGTM!

If you move the lnsLayerPanel class to the <section>, you can add these styles to it to remove the focus ring for mouse clicks and it doesn't break anything else:

.lnsLayerPanel {
  margin-bottom: $euiSizeS;

  // disable focus ring for mouse clicks, leave it for keyboard users
  &:focus:not(:focus-visible) {
    animation: none !important;
  }
}

(Tested with old and new theme. Won't work if the browser doesn't support focus-visible but the fallback is it just always shows the focus ring so it's not bad.)

@ryankeairns Is that scss change fine?

Yep, looks good. Thanks for walking me through the example.

[myasonik]

@mbondyra Just tested this locally and everything seems to work as expected for me