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

#6444: Balloon toolbar should reposition and ungroup items correctly when the window resizes #7795

Merged
merged 3 commits into from
Aug 7, 2020

Conversation

oleq
Copy link
Member

@oleq oleq commented Aug 6, 2020

Suggested merge commit message (convention)

Fix: Balloon toolbar should reposition and ungroup items correctly when the window resizes. Closes #6444.


Additional information

This PR is another attempt at https://github.com/ckeditor/ckeditor5/pull/7721/files.

There are 2 things in this PR (this is hard but here we go):

  1. A reverted ResizeObserver optimization (mentioned here https://github.com/ckeditor/ckeditor5/pull/7721#discussion_r461502549). 

    Without it (or... actually with it), the previousWidth !== entry.contentRect.width in this ToolbarView line https://github.com/ckeditor/ckeditor5/blob/i/6444-toolbar-reposition-on-regroup/packages/ckeditor5-ui/src/toolbar/toolbarview.js#L757 does not work.

    The ToolbarView optimizes the number of _updateGrouping() calls so they only happen when:

    - it's the first resize in its lifetime (resize observer always hits when the toolbar first appears),
    - the element width changes (but not, for instance, its height) (:point_left::point_left::point_left:  this one matters),
    - the update has been queued due to toolbar maxWidth change when the toolbar was invisible (editable resized when the toolbar was invisible)

    When the mentioned ResizeObserver optimization is in place, the "toolbar visible"->"toolbar invisible" state change is lost (the observer callback is not called then). So previousWidth is actually a width before the toolbar disappeared. And when it re-appears, well... it's still the same, so the grouping is not updated.

    So this one actually fixes [Safari] Balloon editor toolbar should always expand when there's enough space #6444.
     

  2. A new event fired by ToolbarView when it groups or ungroups its items (and BalloonToolbar making use of it).

    When 1. is applied, the balloon toolbar does group/ungroup when it re-appears (GIF from [Safari] Balloon editor toolbar should always expand when there's enough space #6444). That's fine.

    But a side-effect of this grouping/ungrouping is that it changes its width and being positioned absolutely (to its upper left corner) the balloon triangle starts pointing to nothing (or nonsense) 😕 (see the GIF below).

    In other words: fixing 1. revealed 2.

    So the BalloonToolbar plugin must listen to the ToolbarView event and re-position itself once the geometry of the toolbar has changed (due to grouping/ungrouping).

    Side note: At first I wanted to avoid the new event and make the BalloonToolbar use another resize observer (observing the toolbar element) and re-position based on this. But then I figured this may end in a race condition since 

    - a toolbar internally uses a resize observer on itself to group/ungroup items,
    - there's this resize observer observing the editable that sets toolbar's maxWidth

    If I added another resize observer, understanding which executed first and which last and whether the balloon repositioning happens before toolbar items grouping or after it would be a total mess. So an event is safer because it always happens after the internal toolbar's resize observer.

oleq added 3 commits August 6, 2020 14:58
…again because it is needed for ToolbarViews that live in balloons and group/ungroup their items at the same time.
… know that the geometry of the toolbar has changed.
…-position itself if the geometry of the toolbar has changed.
@oleq oleq requested a review from panr August 6, 2020 13:32
@oleq
Copy link
Member Author

oleq commented Aug 6, 2020

P.S. when reviewing please make sure #6575 didn't re-appear.

@panr
Copy link
Contributor

panr commented Aug 7, 2020

P.S. when reviewing please make sure #6575 didn't re-appear.

Couldn't reproduce it (even with BlockToolbar and BalloonToolbar mixed together) 👍

Copy link
Contributor

@panr panr left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@panr panr merged commit 3252378 into master Aug 7, 2020
@panr panr deleted the i/6444-toolbar-reposition-on-regroup branch August 7, 2020 08:15
@panr
Copy link
Contributor

panr commented Aug 7, 2020

I closed #7721 PR, since it's no longer needed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Safari] Balloon editor toolbar should always expand when there's enough space
2 participants