Editor: Animate opening and closing editor right sidebar #60561
Conversation
youknowriad
added
[Type] Enhancement
| @@ -313,25 +307,8 @@ function Layout( { initialPost } ) { | |||
| editorNotices={ <EditorNotices /> } | |||
| secondarySidebar={ secondarySidebar() } | |||
| sidebar={ | |||
| ( ( isMobileViewport && sidebarIsOpened ) || | |||
| ( ! isMobileViewport && ! isDistractionFree ) ) && ( | |||
There was a problem hiding this comment.
Sidebar animations can't work if the sidebar is conditionally rendered, it means that we have to "always" render the sidebar and let the "complementary areas" handle the showing/hiding of the panels.
This means that I removed the "toggle sidebar" button that is present in the post editor (this also aligns behavior with the site editor).
|
The following accounts have interacted with this PR and/or linked issues. I will continue to update these lists as activity occurs. You can also manually ask me to refresh this list by adding the If you're merging code through a pull request on GitHub, copy and paste the following into the bottom of the merge commit message. To understand the WordPress project's expectations around crediting contributors, please review the Contributor Attribution page in the Core Handbook. |
| @@ -204,59 +249,56 @@ function ComplementaryArea( { | |||
| { title } | |||
| </ComplementaryAreaMoreMenuItem> | |||
| ) } | |||
| { isActive && ( | |||
There was a problem hiding this comment.
This condition moved within the "fill" to ensure the AnimatePresence animation work. This is probably the most impactful change here that we need to assess in terms of performance...
youknowriad
requested review from
draganescu,
talldan,
adamziel and
kevin940726
as code owners
|
Size Change: +735 B (0%) Total Size: 1.75 MB
ℹ️ View Unchanged
|
|
This is quite nice:
There's still a jump if you go from site view to edit view with the inspector appearing. I suppose that's mostly separate since that's already an issue in trunk. One thing to consider is the potential performance problem of reflowing the content dynamically, which is likely to be especially a problem on pages that are very very long. I've seen a few apps, I don't recall the name, that have similar animations for the sidebars appearing, and the way they performantly handle content reflowing is to immediately jump the container width to its final width, but while keeping the background color you really don't notice this jump at all. That might be something to consider here. |
Indeed, I think it's something we need to consider. One level of added complexity for us is that the background is applied within the iframe (and mostly provided by themes) while the divs animating are editor UI (in a very different place). It makes this very challenging to achieve. |
|
I feel like I'm probably retreading a past conversation about frame background colors, but I'll ask regardless; if we can know the background color, can we apply it to a div wrapping the iframe, animate that, but shift the iframe to its final width immediately? |
We can try at least :) |
|
Pushed a small tweak to update the animation profile so that it matches the figma prototype. That's not set in stone, but I figured it was worth trying out here. One observation; when you close the inspector, the active tab is instantly deslected resulting in a flash where the majority of the panel contents vanishes before the panel itself has slid out of view. Easily observed when the animation is slowed down: flash.mp4Is there any way to delay/avoid that?
Agree it would be good to avoid this. Not just because the animation is jarring, but also because there's some visual glitches that appear during the switch. |
Yes I know, unfortunately, there's no way to avoid that, it's because of how the block controls API (fills) work. |
|
Actually there might be a solution for the empty tabs, let me check. |
|
@jameskoster I managed to solve it for 90% of the cases. There are some cases where the sidebar content switches from "document" to "block" or vice versa on close but I think it's fine.
I think both of these are fine because they're actually the state of the sidebar if you open it again. |
This one's a bit of a shame, but otherwise it's much better now 👍 |
What do you mean with immediately? Does that mean change the width before or after the animation? Based on the previous sentence, I assume you mean after the animation, right? What if we change the with before the animation? It sounds horrendous, but in my head it might actually be nice, and it's technically way less complex. |
I mean with. In this branch when you toggle the sidebar, the viewport and the sidebar both animate at the same time. What I'm suggesting is that as soon as you click to toggle the sidebar, the content snaps to the final width, and the sidebar animates in. Combined with the background color of the canvas remaining in place, I don't think you'd notice that the content didn't also animate, and it would avoid the constant reflowing. I was testing this exact thing in a writing app recently, that did the same thing to avoid reflowing, and it felt entirely non-disruptive. |
|
Yeah it will depend on whether the sidebar hides or shows:
That'd be to prevent content and sidebar overlapping |
|
Sounds good to me. If the background color stays part of the designated editor area, then I don't think anyone would notice. |
|
So what we need is some kind of background detection like they do for iOS web pages to fill in the notch area :) |
youknowriad
force-pushed
the
add/animate-complementary-area
branch
from
93c569b to
93f8085
Compare
|
I've been thinking about the background thing (avoid text reflow). It's not that simple. Copying the background from the iframe and applying it outside won't work. It breaks when backgrounds are not simple colors. Because you'll have two backgrounds applied one on the upper div and one the inner one, creating weird things (for gradients...) So the right solution would be to animate the "body" of the iframe (where the background is applied) but the inner div within the body shouldn't animate. But this is very hard to do because within the "Iframe" component we have no awareness of sidebars/animations... I'm out of ideas here for now. |
youknowriad
force-pushed
the
add/animate-complementary-area
branch
from
97509d0 to
73fd919
Compare
Sounds good to me, happy to try it. |
|
Oh, one thing that's actually a little jarring, if you go directly to the post editor, it also animates in there. Or even if you just open a post or page or site editor in the edit view, then reload the page, it animates in. The animation really makes the most sense when going from Site View to Edit View, and not always. Can we limit it to that? |
Fixed the initial animation but the edit mode thing requires more changes that I prefer to do separately. |
| @@ -372,7 +372,7 @@ test.describe( 'Multi-block selection (@firefox, @webkit)', () => { | |||
| const { height } = await paragraphBlock.boundingBox(); | |||
| await paragraphBlock.click( { position: { x: 0, y: height / 2 } } ); | |||
| await paragraphBlock.click( { | |||
| position: { x: 20, y: height / 2 }, | |||
| position: { x: 25, y: height / 2 }, | |||
There was a problem hiding this comment.
Changing this solved the e2e test failure. I'm not exactly sure why the mouse need to move a little further to select the right characters (only in webkit) in this PR. but this unblocks this PR.
|
There’s wrinkle here when scrollbars aren’t overlaid. The horizontal scrollbar shows up during animation. editor-right-sidebar-horizontal-scrollbar.mp4That’s macOS 13.6.6 with scrollbars set to show. |
|
Good catch @stokesman fix here #60889 |
This breaks the ARIA landmarks principle, as there is now an empty ARIA landmark. @youknowriad there is a reason why we put a button there. When the panel is closed it needs to have some content anyways as ARIA landmarks can't be empty. Also, they can't be dynamic. An ARIA landmark must always be rendered and it must contains something, otherwise it is pointless to use ARIA landmarks in the first place. I think we discussed this point a few times previously and I'm a little surprised this button has been removed so lightly, touching an important accessibility feature in a PR that doesn't have the accessibility label. Noting there are other parts in the editor where the ARIA landmarks principle is broken, as there are some landmarks that are rendered conditionally or do not contain any content, depending on some conditions. It seems to me best practices on how to use ARIA landmarks are a little obscure and often missed, I will try to document it better in a dedicated issue and in the documentation first. Then I will creeate specific issues for each case that needs to be addressed. For the future, I'd appreciate changes like this to be discussed more broadly to prevent introducing regressions in the accessibility level of the editor. Thanks. |
|
@afercia The button was not present in the site editor, and the implementation was not correct in many ways. We can bring it back in both editor though. I can take a look. |
|
Yes I know the button was only in the Post Editor. |
|
Hello @youknowriad, In past we where changing it from CSS to Do you think you can introduce a filter for changing the sidebar width? This will help us a lot. |
I think we can consider a filter or a preference. Would you mind creating an issue for it (if non existent)? |
|
Hey @youknowriad, sure, we can even create a PR in case you are able to include this filter in 6.6 release. |
|
Customizing the sidebar width was brought up in #62599. I think it may have been fine to reopen that one with edits (leave out the bit about the class name).
It’s too late in the release cycle for such additions. |
Related to #60423
What?
This PR animates the right sidebar of the post and site editors (opening and closing the sidebar). It takes a different approach than #60423 (animates the width of the complementary area which automatically pushes the content so we don't need to animate the content).
Notes
Testing Instructions