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

Replace jQuery show animation with CSS animation in sidebar #11044

Merged
merged 1 commit into from
Jan 31, 2025

Conversation

Cwiiis
Copy link
Contributor

@Cwiiis Cwiiis commented Jan 29, 2025

The sidebar-showing animation was relying on jQuery, which just animates the width/height properties (absolutely killer for performance). Replace this with a CSS animation. Note, the animation isn't identical; previously the document size would animate, but this now just snaps to the destination size. Animating document size changes is unlikely to be viable any time soon, but we may want to consider some kind of 2-part move + resize animation.

Change-Id: I68eb16e9eff9a3a9601f3f903f9edb25b97a56cd

Checklist

  • I have run make prettier-write and formatted the code.
  • All commits have Change-Id
  • I have run tests with make check
  • I have issued make run and manually verified that everything looks okay
  • Documentation (manuals or wiki) has been updated or is not required

@Cwiiis Cwiiis requested a review from gokaysatir January 29, 2025 10:54
@Cwiiis Cwiiis force-pushed the private/cwiiis/sidebar-css-animation branch 2 times, most recently from a9ce24a to f669d30 Compare January 30, 2025 09:59
@Cwiiis
Copy link
Contributor Author

Cwiiis commented Jan 30, 2025

Looking into each failure - I observe correct behaviour, I expect some tests are breaking because they ran and completed while the sidebar was still expanding, which would change horizontal scroll positions. The sidebar now doesn't cause the document to go through a series of resizes, so some adjustments need to be made - making them now.

@Cwiiis
Copy link
Contributor Author

Cwiiis commented Jan 30, 2025

I've fixed the related failures, there's one more failure here that I believe is an intermittent ("Moving between paragraphs" in desktop/writer/editable_area_spec.js), probably caused by the same issue we had with an intermittent search highlighting failure - If you move up and down quickly, the cursor is in the wrong place, if you leave time before moving down, it's in the correct place. I expect this is due to not waiting for a server acknowledge of the first cursor move and I expect this may have been made worse by this patch as the jank caused by the expanding sidebar may have slowed this test down.

The sidebar-showing animation was relying on jQuery, which just animates
the width/height properties (absolutely killer for performance). Replace
this with a CSS animation. Note, the animation isn't identical;
previously the document size would animate, but this now just snaps to
the destination size. Animating document size changes is unlikely to be
viable any time soon, but we may want to consider some kind of 2-part
move + resize animation.

Signed-off-by: Chris Lord <[email protected]>
Change-Id: I68eb16e9eff9a3a9601f3f903f9edb25b97a56cd
@Cwiiis Cwiiis force-pushed the private/cwiiis/sidebar-css-animation branch from f669d30 to f4f248b Compare January 30, 2025 11:53
ceHelper.checkPlainContent('green red');
ceHelper.moveCaret('left', '', 4);
ceHelper.checkCaretPosition(5);
helper.getBlinkingCursorPosition('P2');
cy.wait(800);
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not happy about the size of these waits, but these seemed to be the minimum values to get this test to pass consistently locally. Both of these are masking a real bug, I can easily replicate the failure with manual inputs and it doesn't mirror the behaviour in desktop Writer. This seems very similar to #10585 and I wouldn't be surprised if the underlying reason is basically the same.

@gokaysatir gokaysatir requested a review from eszkadev January 30, 2025 16:18
Copy link
Contributor

@gokaysatir gokaysatir left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This works and i guess increases the rendering performance.

For the visual changes, i would also inform @pedropintosilva :) It'd be good to have his opinion.

Thank you for the improvement.

@pedropintosilva pedropintosilva merged commit 2a318fc into master Jan 31, 2025
14 checks passed
@pedropintosilva pedropintosilva deleted the private/cwiiis/sidebar-css-animation branch January 31, 2025 08:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

perf: Expanding the sidebar causes several resizes of the document
4 participants