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

Don't focus close button on modal open/Try a modal appear animation #9900

Merged
merged 4 commits into from Oct 6, 2018

Conversation

Projects
None yet
7 participants
@jasmussen
Contributor

jasmussen commented Sep 14, 2018

This is the first in a series of tiny micro interaction animations I hope to add across the UI. It adds a "fade & appear" animation to any modal you open. It's fast, but it's smooth, and it's non intrusive.

sep-14-2018 11-57-10

Please try checking out the branch — the framerate of this GIF doesn't do it justice, so please don't give your animation feedback based solely on this GIF, please please try it first.

The animation includes movement, downwards up, because this helps suggest that the modal is a card that always exists at this elevation, it just sits outside the viewport, in this case below the screen and it's literally brought up when you ask for it.

However this animation also brings with us a challenge that needs solving — right now the "Close dialog" tooltip shows every time a modal is opened. This is a problem on its own because it sends mixed signals to the user: I just opened this dialog, should I close it again?

But specifically in this case, the tooltip appears mid-animation which means it's not correctly aligned.

How do we fix this? Here are some thoughts:

  • Don't focus the close button. Focus the header, or the modal itself instead, so a single press of the "Tab" button focuses it. I volunteer to make a giant in-your-face focus style for the modal, a dashed line inside the entire thing, if we can do this.
  • Don't show the tooltip when the close button is focused on modals.

What other solutions can you think of? It really is a bad experience for sighted users as it is.

@jasmussen jasmussen self-assigned this Sep 14, 2018

@jasmussen jasmussen requested a review from WordPress/gutenberg-core Sep 14, 2018

@mtias

This comment has been minimized.

Show comment
Hide comment
@mtias

mtias Sep 14, 2018

Contributor

Have you considered something like this for the micro-animations? https://github.com/Automattic/wp-calypso/tree/master/client/components/animate

Contributor

mtias commented Sep 14, 2018

Have you considered something like this for the micro-animations? https://github.com/Automattic/wp-calypso/tree/master/client/components/animate

@jasmussen

This comment has been minimized.

Show comment
Hide comment
@jasmussen

jasmussen Sep 14, 2018

Contributor

I have not, but I can probably refactor to use that for the modal here as a first.

The benefit is that it's easier to reuse the animations, and make them dedicated parts of the components themselves as opposed to "CSS sugar on top", correct?

This presumably still wouldn't fix the issue with tooltip appearing, though right?

Contributor

jasmussen commented Sep 14, 2018

I have not, but I can probably refactor to use that for the modal here as a first.

The benefit is that it's easier to reuse the animations, and make them dedicated parts of the components themselves as opposed to "CSS sugar on top", correct?

This presumably still wouldn't fix the issue with tooltip appearing, though right?

@kjellr

This comment has been minimized.

Show comment
Hide comment
@kjellr

kjellr Sep 14, 2018

Contributor

This is looking good! I have one possibly weird suggestion: Have you tried slowing down the white overlay on the page?

The speed at which the modal appears feels great, but the slight white overlay transition felt a little jarring to me — I had the sense that something changed on the page, but I wasn't able to quite figure out what it was at first. Felt a little like a glitch to me. Slowing that down just a little bit might make the transition feel a little more relaxed.

(It's also possible I'm just reacting to the fact that the white overlay is very subtle in general — in which case this won't help. 😄)

Contributor

kjellr commented Sep 14, 2018

This is looking good! I have one possibly weird suggestion: Have you tried slowing down the white overlay on the page?

The speed at which the modal appears feels great, but the slight white overlay transition felt a little jarring to me — I had the sense that something changed on the page, but I wasn't able to quite figure out what it was at first. Felt a little like a glitch to me. Slowing that down just a little bit might make the transition feel a little more relaxed.

(It's also possible I'm just reacting to the fact that the white overlay is very subtle in general — in which case this won't help. 😄)

@cristian-e

This comment has been minimized.

Show comment
Hide comment
@cristian-e

cristian-e Sep 16, 2018

Contributor

I tried the animation and it's working fine.
Another solution is to focus the close button after the modal appear animation.

Contributor

cristian-e commented Sep 16, 2018

I tried the animation and it's working fine.
Another solution is to focus the close button after the modal appear animation.

@afercia

This comment has been minimized.

Show comment
Hide comment
@afercia

afercia Sep 16, 2018

Contributor

Re: the tooltip appearing on initial focus, there's already a specific issue created a couple weeks ago, see #9410

Contributor

afercia commented Sep 16, 2018

Re: the tooltip appearing on initial focus, there's already a specific issue created a couple weeks ago, see #9410

@jasmussen

This comment has been minimized.

Show comment
Hide comment
@jasmussen

jasmussen Sep 17, 2018

Contributor

Just pushed 4e3c4ec that fixes #9410. GIF:

modal enhancements

Note that the animation is not visible in this GIF because Licecap doesn't have sufficient framerate to show the .1s animation, but it's there.

The modal itself now has a scrollable area that itself is focused when initially opened. Aside from solving the issue of the tooltip showing on open, this makes the scrollable region accessible to the keyboard (arrow keys, page up, page down).

Contributor

jasmussen commented Sep 17, 2018

Just pushed 4e3c4ec that fixes #9410. GIF:

modal enhancements

Note that the animation is not visible in this GIF because Licecap doesn't have sufficient framerate to show the .1s animation, but it's there.

The modal itself now has a scrollable area that itself is focused when initially opened. Aside from solving the issue of the tooltip showing on open, this makes the scrollable region accessible to the keyboard (arrow keys, page up, page down).

@keyframes modal-appear {
from {
margin-top: $grid-size * 4;

This comment has been minimized.

@jasmussen

jasmussen Sep 17, 2018

Contributor

Animating the margin is potentially not as performant as translate, but we can't use translate as we use that for responsive positioning at responsive breakpoints. @kjellr do you have any insights as to whether we can make this more performant? Can we use will-change: transform; yet? Or are we still using translateZ(0) for this?

@jasmussen

jasmussen Sep 17, 2018

Contributor

Animating the margin is potentially not as performant as translate, but we can't use translate as we use that for responsive positioning at responsive breakpoints. @kjellr do you have any insights as to whether we can make this more performant? Can we use will-change: transform; yet? Or are we still using translateZ(0) for this?

This comment has been minimized.

@kjellr

kjellr Sep 17, 2018

Contributor

Hmm, I see what you mean — we're sort of backed into animating the margin here because of the other transform properties we're using. As far as I can tell, we could theoretically use translate if we declared separate, super-specific animations for each of the two breakpoints:

@keyframes modal-appear-small {
	from {
		transform: translate(-50%, 10%);
	}
	to {
		transform: translate(-50%, 0);
	}
}

@mixin modal_appear_small() {
	animation: modal-appear-small 0.1s ease-out;
	animation-fill-mode: forwards;
}

@keyframes modal-appear-medium {
	from {
		transform: translate(-50%, -20%);
	}
	to {
		transform: translate(-50%, -30%);
	}
}

@mixin modal_appear_medium() {
	animation: modal-appear-medium 0.1s ease-out;
	animation-fill-mode: forwards;
}

And then declarations like these in /modal/style.css:

@include break-small() {
	...

	// Animate appearance.
	@include modal_appear_small();
}
@include break-medium () {
	...

	// Animate appearance.
	@include modal_appear_medium();
}

... but that feels like a bit of a hack (and a longwinded one at that), and I'm not 100% sure that it gains us a lot in terms of performance. So maybe we just stick to animating the margin?

Regarding will-change: I'm not totally familiar with that property, but I'm getting mixed signals about its usefulness when reading the MDN page. Browser support aside, they list a lot of caveats — based on what I'm reading there, I'm not sure an animation this small actually merits the use of it.

@kjellr

kjellr Sep 17, 2018

Contributor

Hmm, I see what you mean — we're sort of backed into animating the margin here because of the other transform properties we're using. As far as I can tell, we could theoretically use translate if we declared separate, super-specific animations for each of the two breakpoints:

@keyframes modal-appear-small {
	from {
		transform: translate(-50%, 10%);
	}
	to {
		transform: translate(-50%, 0);
	}
}

@mixin modal_appear_small() {
	animation: modal-appear-small 0.1s ease-out;
	animation-fill-mode: forwards;
}

@keyframes modal-appear-medium {
	from {
		transform: translate(-50%, -20%);
	}
	to {
		transform: translate(-50%, -30%);
	}
}

@mixin modal_appear_medium() {
	animation: modal-appear-medium 0.1s ease-out;
	animation-fill-mode: forwards;
}

And then declarations like these in /modal/style.css:

@include break-small() {
	...

	// Animate appearance.
	@include modal_appear_small();
}
@include break-medium () {
	...

	// Animate appearance.
	@include modal_appear_medium();
}

... but that feels like a bit of a hack (and a longwinded one at that), and I'm not 100% sure that it gains us a lot in terms of performance. So maybe we just stick to animating the margin?

Regarding will-change: I'm not totally familiar with that property, but I'm getting mixed signals about its usefulness when reading the MDN page. Browser support aside, they list a lot of caveats — based on what I'm reading there, I'm not sure an animation this small actually merits the use of it.

@@ -270,12 +270,3 @@ body.gutenberg-editor-page {
.ephox-snooker-resizer-bar-dragging {
background: $blue-medium-500;
}
// This style is defined here instead of the modal component

This comment has been minimized.

@jasmussen

jasmussen Sep 17, 2018

Contributor

I'm not exactly sure what this means — the height of the modal is defined at responsive breakpoints already, this did not seem to account for that. But regardless of that, I may have been able to refactor this out by removing the need for calc here. Can you take a look, @youknowriad?

@jasmussen

jasmussen Sep 17, 2018

Contributor

I'm not exactly sure what this means — the height of the modal is defined at responsive breakpoints already, this did not seem to account for that. But regardless of that, I may have been able to refactor this out by removing the need for calc here. Can you take a look, @youknowriad?

This comment has been minimized.

@youknowriad

youknowriad Sep 18, 2018

Contributor

It means we shouldn't assume we're in the context of the WP Admin page when tweaking the style of the modal in the style.scss of the modal component. Which means we can't use things like:

  • $admin-bar-height
  • body.is-fullscreen-mode

And as far as I can tell, this PR rewrites the modal component to be shown in the same way regardless of the pagee it's used in (editor or not), so I think we're probably fine removing those.

@youknowriad

youknowriad Sep 18, 2018

Contributor

It means we shouldn't assume we're in the context of the WP Admin page when tweaking the style of the modal in the style.scss of the modal component. Which means we can't use things like:

  • $admin-bar-height
  • body.is-fullscreen-mode

And as far as I can tell, this PR rewrites the modal component to be shown in the same way regardless of the pagee it's used in (editor or not), so I think we're probably fine removing those.

@jasmussen

This comment has been minimized.

Show comment
Hide comment
@jasmussen

jasmussen Sep 17, 2018

Contributor

I can use the dialog role, but then should it be removed from the parent components-modal__frame container?

Contributor

jasmussen commented Sep 17, 2018

I can use the dialog role, but then should it be removed from the parent components-modal__frame container?

@afercia

This comment has been minimized.

Show comment
Hide comment
@afercia

afercia Sep 17, 2018

Contributor

Just pushed 4e3c4ec that fixes #9410

Not sure it really "fixes" #9410 as discussion there is still ongoing. Not opposed to exploration though.

The main drawbacks I see in the current state of this PR are:

  • a certain excess of labelling: there are two wrappers, both labelled, they will be announced by screen readers. Screen readers already announce "dialog" or "web dialog" and the second wrapper has a hardcoded label Dialog Contents that is redundant (and may not be appropriate for all cases)
  • when the modal content is short, the modal height will still be 70% on desktop, but there will still be a focusable <div> with an ARIA role and a label... and since in this case there's no scrolling, this <div> isn't really an "interactive" control so it shouldn't be focusable to start with, nor it should have a role and a label

The second issue is actually related to the modal height, which I'd suggest to address separately in #8561. Not arguing, but this PR started with adding an animation and it is now touching several, not strictly related things that would be best to address separately, for better clarity and also for history.

Worth noting that to fix the modal height I was exploring something for #8561. Ideally, the modal height should be determined by its content and have a max-height to account for very long content. It is possible to implement it but the scrolling mechanism should be moved to the modal frame which, coincidentally, already has an ARIA role (it's the dialog) and it's already labelled.
The header would scroll though and the only way I've found to make it fixed is to use position: sticky (doesn't work in IE 11). Will try to push a POC on #8561 soon.

Contributor

afercia commented Sep 17, 2018

Just pushed 4e3c4ec that fixes #9410

Not sure it really "fixes" #9410 as discussion there is still ongoing. Not opposed to exploration though.

The main drawbacks I see in the current state of this PR are:

  • a certain excess of labelling: there are two wrappers, both labelled, they will be announced by screen readers. Screen readers already announce "dialog" or "web dialog" and the second wrapper has a hardcoded label Dialog Contents that is redundant (and may not be appropriate for all cases)
  • when the modal content is short, the modal height will still be 70% on desktop, but there will still be a focusable <div> with an ARIA role and a label... and since in this case there's no scrolling, this <div> isn't really an "interactive" control so it shouldn't be focusable to start with, nor it should have a role and a label

The second issue is actually related to the modal height, which I'd suggest to address separately in #8561. Not arguing, but this PR started with adding an animation and it is now touching several, not strictly related things that would be best to address separately, for better clarity and also for history.

Worth noting that to fix the modal height I was exploring something for #8561. Ideally, the modal height should be determined by its content and have a max-height to account for very long content. It is possible to implement it but the scrolling mechanism should be moved to the modal frame which, coincidentally, already has an ARIA role (it's the dialog) and it's already labelled.
The header would scroll though and the only way I've found to make it fixed is to use position: sticky (doesn't work in IE 11). Will try to push a POC on #8561 soon.

@jasmussen jasmussen requested a review from tofumatt Oct 4, 2018

@tofumatt tofumatt changed the title from Try a modal appear animation to Don't focus close button on modal open/Try a modal appear animation Oct 4, 2018

jasmussen and others added some commits Sep 14, 2018

Try a modal appear animation
This is the first in a series of tiny micro interaction animations I hope to add across the UI. It adds a "fade & appear" animation to any modal you open. It's fast, but it's smooth, and it's non intrusive.

However this animation also brings with us a challenge that needs solving — right now the "Close dialog" tooltip shows every time a modal is opened. This is a problem on its own because it sends mixed signals to the user: I just opened this dialog, should I close it again?

But specifically in this case, the tooltip appears mid-animation which means it's not correctly aligned.

How do we fix this? Here are some thoughts:

- Don't focus the close button. Focus the header, or the modal itself instead, so a single press of the "Tab" button focuses it. I volunteer to make a giant in-your-face focus style for the modal, a dashed line inside the entire thing, if we can do this.
- Don't show the tooltip when the close button is focused on modals.

What other solutions can you think of? It really is a bad experience for sighted users as it is.
Explore a way to not show tooltip
This commit changes the modal component to have the scrollable area itself be focusable, and be the first focusable element described as a region. It also refactors the sizing math of the modal frame, and simplifies the overall content area.

The benefits to this refactor is:

1. The modal contents as immediately focused on open is scrollable using the arrow keys, spacebar, or pageup/down.
2. The close button is not the first element, which means a press of the spacebar to scroll doesn't close the window. It also doesn't show the tooltip.

Additionally this PR fixes a responsive issue with the previous animation.
@tofumatt

I looked into adding an Animate HOC like in Calypso, but in this case we want two animations and it doesn't really end up being an elegant refactor.

I think that kind of HOC might be neat but I see it best handled separately.

Otherwise I find this great. I've removed a redundant label as per @afercia's suggestion so there is no "Dialog Contents" label anymore. I think that makes this good to go.

The only issue is that the modal is focused even when there is no interaction inside the modal other than the ability to tab to the close button. This is unfortunate but improves the design of the modal in all cases and allows users to interact with larger modals (like the keyboard screen one) with the keyboard to scroll them when opened.

I think that's overall better so I'm going to approve this.

@tofumatt

This comment has been minimized.

Show comment
Hide comment
@tofumatt

tofumatt Oct 6, 2018

Member

Fixes #9410.

Member

tofumatt commented Oct 6, 2018

Fixes #9410.

@tofumatt tofumatt modified the milestones: 4.0, 4.1 Oct 6, 2018

@tofumatt tofumatt modified the milestones: 4.1, 4.0 Oct 6, 2018

@tofumatt tofumatt merged commit 86dad5f into master Oct 6, 2018

2 checks passed

codecov/project 49.31% remains the same compared to baff0e6
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@tofumatt tofumatt deleted the try/modal-appear-animation branch Oct 6, 2018

johngodley added a commit that referenced this pull request Oct 9, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment