Make focus visible (mostly CSS) #13415

gabalafou · 2022-11-14T21:39:56Z

References

I'm not sure there is a single issue that corresponds to this PR, but it's related to:

Accessibility Issues Needing Addressing for WCAG 2.1 compliance (As of Version 2.2.6) #9399

Code changes

The biggest change introduced by this PR is that I delete the bit of CSS that disables the browser's default focus-visible indicator. This is a major accessibility anti-pattern.

After restoring the focus-visible outline, I went about fixing parts of the UI that that change messed up.

I also search-replaced the CSS, replacing border with outline for focus-visible. For several reasons, outline is a better choice.

User-facing changes

Every element that can receive focus should now have a visible outline. The following videos show screen casts of me pressing the tab key in JupyterLab. Each element that receives focus has a visible indicator.

Safari

Screen.Recording.2022-11-14.at.16.01.20.mov

Chrome

Screen.Recording.2022-11-14.at.16.02.44.mov

Firefox

Screen.Recording.2022-11-14.at.16.03.28.mov

Backwards-incompatible changes

The only potentially backward-incompatible change is where I remove the tabindex attribute from the main area widget. All the other changes are just style tweaks.

jupyterlab-probot · 2022-11-14T21:39:59Z

Thanks for making a pull request to jupyterlab!
To try out this branch on binder, follow this link:

gabalafou

Done self-reviewing.

gabalafou · 2022-11-14T21:44:23Z

packages/ui-components/style/base.css

@@ -36,13 +36,6 @@ a:hover {
  color: unset;
 }

-/* Disable default focus outline */
-:focus {


There is a risk that this exposes the focus-visible indicator in ways that some users find ugly. But I would prefer to have an ugly focus-visible indicator that prompts aesthetic interventions from users and contributors rather than take away a key accessibility feature from users that need it.

@gabalafou this changes the settingseditor's input:focus appearance, replacing the light blue gradient with a strong dark blue, similar to the following diff (new on left):

Was that expected or just a consequence ?
I'm asking this to know if I should open a PR to fix this for the settingseditor.

No, that was not specifically intended. Sorry about that! (If you're short on time, just let me know, and I can submit a PR to fix that. I'm quite familiar now with configuring focus states in CSS, so it shouldn't take me much time.)

By the way, do you know why I cannot tab-key to the drop-downs in the settings editor?

Thanks, I had already something about that. I opened #13554.

By the way, do you know why I cannot tab-key to the drop-downs in the settings editor?

No idea, it seems to work on my end, if you mean reaching a drop-down with tab-key.

gabalafou · 2022-11-14T21:44:48Z

packages/apputils/src/mainareawidget.ts

@@ -67,7 +67,6 @@ export class MainAreaWidget<T extends Widget = Widget>
    if (!content.id) {
      content.id = DOMUtils.createDomID();
    }
-    content.node.tabIndex = 0;


@marthacryan could you take a look at this change?

After playing for some time with the JupyterLab UI, I couldn't find any good reason to give the main area widget a tab index. Every main area widget that I could find has some element within it that can receive focus, so there's no reason that I can think of to make the container receive focus. I thought maybe it had to do with widgets that only have content and no editing functions (like contextual help). I thought maybe putting a tab index on the container would help the user to use page up and page down keyboard keys to scroll the content, but in fact (on my Mac at least), when the container element has focus I cannot use the page up and page down keys to scroll.

I wonder why the visual tests fail (and updates cannot be generated https://github.com/jupyterlab/jupyterlab/actions/runs/3503291407). Since all other changes are CSS-only, my suspicion is that galata may depend on MainAreaWidget being focusable. This is usually required in order to receive keyboard events. Could we try with

content.node.tabIndex = -1;

?

@gabalafou I opened gabalafou#12. In particular tests hang on:

jupyterlab/galata/src/helpers/activity.ts

Lines 91 to 97 in 72f89c9

await tab.click();

await this.page.waitForFunction(

({ tab }) => {

return tab.classList.contains('jp-mod-current');

},

{ tab }

);

and this is visible in the UI as the tabs do not get the blue highlight the way they used to:

Oh sorry thought I responded here! If you can't find a good reason to set that tabindex to 0 that's totally fine to remove. I went through and tried to remove a bunch of tabindex being set to -1 and I may have been setting them to 0 for no good reason. Thanks for working on this!

gabalafou · 2022-11-14T21:57:38Z

packages/application/style/menus.css

+.lm-MenuBar-content:focus-visible {
+  outline-offset: -3px; /* this value is a compromise between Firefox, Chrome,
+    and Safari over this outline's visibility and discretion */
+}
+
+.lm-MenuBar:focus-visible {
+  outline: 1px solid var(--jp-accept-color-active, var(--jp-brand-color1));
+  outline-offset: -1px;
+}
+
+.lm-MenuBar-menu:focus-visible,
+.lm-MenuBar-item:focus-visible,
+.lm-Menu-item:focus-visible {
+  outline: unset;
+  outline-offset: unset;
+  -moz-outline-radius: unset;
+}
+


With the changes here, the tabbing/focus user experience for the Lumino menubar is not great. The menubar UX needs to be changed at the Lumino level. But my intention with this PR is not to fix all keyboard navigation issues. My intention with this PR is to make focus visible. There are elements that shouldn't receive focus but do, and vice versa. But even if the tabbing order or the set of elements that can receive focus doesn't make sense, that's not a reason to hide the focus indicator; it's a reason to fix which elements receive focus and in which order. At least that's how I see it.

Would this changes be applicable to lumino too therefore be better included as default lumino styles?

The menubar UX needs to be changed at the Lumino level.

Does jupyterlab/lumino#465 address the changes needed? Would you be able to help reviewing it?

Oh neat! I'll take a look at that PR today, thanks @krassowski!

The problem with the Lumino menubar is that it renders an HTML structure like this:

<div tabindex="0"> <div tabindex="0"> <ul> <li tabindex="0">

So the user has to tab through an outer container, then an inner container, and then finally each item in the menu bar (File, Edit, etc.). I think that ideally the menubar component should output a single DOM node with tabindex=0 so that the end user can navigate to the menubar, and then from there use arrow keys to access the different parts of the menubar. I'm hoping that's what jupyterlab/lumino#465 does.

I like the idea of simplifying the DOM output of menubar as it could help with performance too.

packages/launcher/style/base.css

krassowski · 2022-11-14T23:38:25Z

This surfaces an issue with the file browser ignoring focused elements. I think that we would either need to rewrite file browser listing to use browser focus (may be hard), or make the items unfocusable and handle it all programatically (as we currently do).

Actually since the file browser listing can have 100s of files, those should probably be only focused (regardless of implementation detail) only after explicit user confirmation. The alternative is adding a skip-link. I think that the former would be better UX.

gabalafou · 2022-11-15T13:46:54Z

@krassowski thanks! You are 100% spot on. My intention was to roll out a number of follow-up PRs to this one that address some of the issues that this PR surfaces (such as the file browser ignoring focussed elements). I wanted to keep this PR fairly small and focussed on making focus visible, separate from any other issues around focus. But do you think I should update this PR to address some of these other issues instead of doing it in a separate PR?

For the file browser in particular, my preference would be to make it possible for the user to tab to the directory listing, but then remove the ability to tab to individual files within the list. Instead the user would just use the arrow keys, which is already supported, whereas pressing the tab key would move the user away from the directory listing to another part of the JupyterLab UI. What do you think? Should I go ahead and make that change in this PR or in a follow-up?

krassowski · 2022-11-15T20:06:49Z

I like your plan for the file browser listing. I am happy to approve it as-is (though let's ensure the UI tests pass) as it is already an improvement.

No strong preference for a single large PR vs multiple smaller ones, especially if you are targeting 4.0.

krassowski · 2022-11-19T11:20:59Z

bot please update snapshots

fcollonval · 2022-11-28T10:08:32Z

bot please update snapshots

krassowski · 2022-11-28T22:46:20Z

I took liberty of switching milestone as I would love to see this in 3.6.

gabalafou#12 should solve the visual testing regression (which is relevant).

krassowski · 2022-11-28T23:37:54Z

bot please update snapshots

but exclude from keyboard navigation

krassowski · 2022-11-30T19:04:57Z

The documentation code needs updating to ensure there is no focus on irrelevant elements as that will confuse the documentation readers. It would also be nice if this PR did not change the dimensions of buttons (as seen on the popup dialog galata documentation screenshot)

fcollonval · 2022-12-06T09:54:43Z

In c5339c5, I updated the documentation test to remove noisy focus highlight for documentation snapshot. I did not modify the sizing change for the button as actually this fixes the jitter of the button switching to having a 0 border to 1px border when focus due to CSS rules like:

jupyterlab/packages/application/style/buttons.css

Line 31 in c5339c5

border: 0;

fcollonval · 2022-12-06T09:55:47Z

@meeseeksdev please backport to 3.6.x

lumberbot-app · 2022-12-06T09:56:07Z

Owee, I'm MrMeeseeks, Look at me.

There seem to be a conflict, please backport manually. Here are approximate instructions:

Checkout backport branch and update it.

git checkout 3.6.x
git pull

Cherry pick the first parent branch of the this PR on top of the older branch:

git cherry-pick -x -m1 e658b6f28e3501968e7fe8e211677c5378be14d0

You will likely have some merge/cherry-pick conflict here, fix them and commit:

git commit -am 'Backport PR #13415: Make focus visible (mostly CSS)'

Push to a named branch:

git push YOURFORK 3.6.x:auto-backport-of-pr-13415-on-3.6.x

Create a PR against branch 3.6.x, I would have named this PR:

"Backport PR #13415 on branch 3.6.x (Make focus visible (mostly CSS))"

And apply the correct labels and milestones.

Congratulations — you did some good work! Hopefully your backport PR will be tested by the continuous integration and merged soon!

Remember to remove the Still Needs Manual Backport label once the PR gets merged.

If these instructions are inaccurate, feel free to suggest an improvement.

…13543) * Backport PR #13415: Make focus visible (mostly CSS) * Fix binder script Co-authored-by: gabalafou <gabriel@fouasnon.com>

github-actions bot added Design System CSS pkg:application pkg:apputils pkg:filebrowser pkg:launcher pkg:ui-components tag:CSS For general CSS related issues and pecadilloes labels Nov 14, 2022

github-actions bot assigned gabalafou Nov 14, 2022

gabalafou force-pushed the fix-focus-visible branch from c30f59f to 9ee6d47 Compare November 14, 2022 21:50

gabalafou commented Nov 14, 2022

View reviewed changes

gabalafou changed the title ~~Fix focus visible (mostly CSS)~~ Make focus visible (mostly CSS) Nov 14, 2022

gabalafou mentioned this pull request Nov 14, 2022

Focus not always visible on JupyterLab initial page Quansight-Labs/jupyter-a11y-mgmt#167

Closed

krassowski reviewed Nov 14, 2022

View reviewed changes

packages/launcher/style/base.css Show resolved Hide resolved

fcollonval added bug tag:Accessibility labels Nov 15, 2022

fcollonval added this to the 4.0.0 milestone Nov 15, 2022

gabalafou mentioned this pull request Nov 17, 2022

Team update - Nov 16, 2022 Quansight-Labs/jupyter-a11y-mgmt#179

Closed

krassowski modified the milestones: 4.0.0, 3.6.0 Nov 28, 2022

foo and others added 2 commits November 29, 2022 09:52

Fix focus visible (mostly CSS)

6006dce

Allow main area widgets to receive focus again,

57fe723

but exclude from keyboard navigation

fcollonval force-pushed the fix-focus-visible branch from 1850e78 to dd944be Compare November 29, 2022 08:56

github-actions bot added the tag:Testing label Nov 29, 2022

gabalafou mentioned this pull request Nov 30, 2022

File browser has numerous keyboard issues, some newly surfaced Quansight-Labs/jupyter-a11y-mgmt#182

Closed

krassowski mentioned this pull request Dec 1, 2022

ui-components CSS should probably not change "global" elements CSS? #13493

Open

Update documentation snapshots

c5339c5

fcollonval force-pushed the fix-focus-visible branch from dd944be to c5339c5 Compare December 6, 2022 08:44

fcollonval approved these changes Dec 6, 2022

View reviewed changes

fcollonval merged commit e658b6f into jupyterlab:master Dec 6, 2022

lumberbot-app bot added the Still Needs Manual Backport label Dec 6, 2022

fcollonval pushed a commit to fcollonval/jupyterlab that referenced this pull request Dec 6, 2022

Backport PR jupyterlab#13415: Make focus visible (mostly CSS)

dac8974

fcollonval mentioned this pull request Dec 6, 2022

Backport PR #13415 on branch 3.6.x (Make focus visible (mostly CSS)) #13543

Merged

brichet mentioned this pull request Dec 7, 2022

Restores the appearance of the settingeditor's input focus #13554

Merged

fcollonval added a commit that referenced this pull request Dec 8, 2022

Backport PR #13415 on branch 3.6.x (Make focus visible (mostly CSS)) (#…

f209231

…13543) * Backport PR #13415: Make focus visible (mostly CSS) * Fix binder script Co-authored-by: gabalafou <gabriel@fouasnon.com>

gabalafou mentioned this pull request Dec 12, 2022

Make file browser respond to focussed elements #13577

Merged

fcollonval removed the Still Needs Manual Backport label Dec 15, 2022

trallard mentioned this pull request Dec 16, 2022

Fix tab traps in JupyterLab Quansight-Labs/jupyter-a11y-mgmt#134

Open

fcollonval mentioned this pull request Jan 27, 2023

JupyterLab 3.6.0rc1 - style changes #13870

Closed

krassowski mentioned this pull request Jan 28, 2023

Restore blueprint focus overrides on 3.x branch #13879

Merged

github-actions bot locked as resolved and limited conversation to collaborators Dec 16, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make focus visible (mostly CSS) #13415

Make focus visible (mostly CSS) #13415

gabalafou commented Nov 14, 2022 •

edited

jupyterlab-probot bot commented Nov 14, 2022

gabalafou left a comment

gabalafou Nov 14, 2022

brichet Dec 7, 2022

gabalafou Dec 7, 2022

brichet Dec 7, 2022

gabalafou Nov 14, 2022

krassowski Nov 19, 2022

krassowski Nov 28, 2022

marthacryan Nov 28, 2022

gabalafou Nov 14, 2022

krassowski Nov 14, 2022

gabalafou Nov 15, 2022

krassowski Nov 15, 2022

krassowski commented Nov 14, 2022

gabalafou commented Nov 15, 2022

krassowski commented Nov 15, 2022

krassowski commented Nov 19, 2022

fcollonval commented Nov 28, 2022

krassowski commented Nov 28, 2022

krassowski commented Nov 28, 2022

krassowski commented Nov 30, 2022

fcollonval commented Dec 6, 2022

fcollonval commented Dec 6, 2022

lumberbot-app bot commented Dec 6, 2022

	await tab.click();
	await this.page.waitForFunction(
	({ tab }) => {
	return tab.classList.contains('jp-mod-current');
	},
	{ tab }
	);

Make focus visible (mostly CSS) #13415

Make focus visible (mostly CSS) #13415

Conversation

gabalafou commented Nov 14, 2022 • edited

References

Code changes

User-facing changes

Safari

Chrome

Firefox

Backwards-incompatible changes

jupyterlab-probot bot commented Nov 14, 2022

gabalafou left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

krassowski commented Nov 14, 2022

gabalafou commented Nov 15, 2022

krassowski commented Nov 15, 2022

krassowski commented Nov 19, 2022

fcollonval commented Nov 28, 2022

krassowski commented Nov 28, 2022

krassowski commented Nov 28, 2022

krassowski commented Nov 30, 2022

fcollonval commented Dec 6, 2022

fcollonval commented Dec 6, 2022

lumberbot-app bot commented Dec 6, 2022

gabalafou commented Nov 14, 2022 •

edited