i/6444: Fix balloon toolbar items grouping on reappearance #7721
Conversation
panr
changed the title
| @@ -772,6 +772,26 @@ describe( 'ToolbarView', () => { | |||
| sinon.assert.calledOnce( view._behavior._updateGrouping ); | |||
| } ); | |||
|
|
|||
| it( 'does not update the state of grouped items if toolbar is not in the DOM', () => { | |||
There was a problem hiding this comment.
This test is unrelated to the change (it is valid, though, it should stay). It passes with or without the fix.
I think we're missing a twin to "should queue the update of the grouped items state when invisible (and execute it when visible again)" except that it should check element.remove() instead of element.style.display = 'none'.
| @@ -630,6 +630,8 @@ class DynamicGrouping { | |||
| // nothing else. This happens, for instance, when the toolbar is detached from DOM and | |||
| // some logic adds or removes its #items. | |||
| if ( !this.viewElement.ownerDocument.body.contains( this.viewElement ) ) { | |||
| this.shouldUpdateGroupingOnNextResize = true; | |||
There was a problem hiding this comment.
This is a hack and it only hides the problem.
I did some logging in _enableGroupingOnResize() only to find out that the entry.contentRect.width has a wrong (outdated) value when the toolbar re-appears (4. from the original issue #6444). The value of entry.contentRect.width represents the width of the #viewElement when the toolbar was last resized, not when it re-appeared. This suggests the ResizeObserver implementation is buggy so maybe this is where the fix should land.
diff --git a/packages/ckeditor5-ui/src/toolbar/toolbarview.js b/packages/ckeditor5-ui/src/toolbar/toolbarview.js
index ea12e500ff..d5cc4d7160 100644
--- a/packages/ckeditor5-ui/src/toolbar/toolbarview.js
+++ b/packages/ckeditor5-ui/src/toolbar/toolbarview.js
@@ -728,6 +728,8 @@ class DynamicGrouping {
// TODO: Consider debounce.
this.resizeObserver = new ResizeObserver( this.viewElement, entry => {
+ console.log( !previousWidth, previousWidth !== entry.contentRect.width, previousWidth, entry.contentRect.width );
+
if ( !previousWidth || previousWidth !== entry.contentRect.width || this.shouldUpdateGroupingOnNextResize ) {
this.shouldUpdateGroupingOnNextResize = false;Also, with your fix enabled the toolbar re-positions in a wrong place (Chrome). I wonder if this could be related to #7705. And maybe fb6b0dc was wrong because we're losing the non-zero->0 and 0->non-zero state changes. This all has to be checked.
There was a problem hiding this comment.
Oh damn it... I thought it would be easier to fix... Also, just tested it with fb6b0dc being reverted and... I can't reproduce the issue 🤔 Could you check it too and confirm? You always can find some edge cases ;-D
There was a problem hiding this comment.
I guess we should close this PR, because reverting fb6b0dc gives us the same results...
There was a problem hiding this comment.
Regarding the #6570 issue, when I reverted the fb6b0dc on master block toolbar works fine. There's no sign of the bug I encountered before 🤔
@oleq Any chance you could confirm that when you have time?
btw. I know that sometimes you may have the positioning issue with the balloon toolbar, but it looks like it's quite rare, so maybe we should agree that having this issue is better than the balloon toolbar that won't expand? 🤔
|
I'm closing it due to #7795 fix. |
Suggested merge commit message (convention)
Fix (ui): Balloon toolbar will regroup items on reappearance after the editor resizing. Closes #6444.