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

getLeaf() should ignore non-leaf blots #3489

Merged
merged 1 commit into from
Nov 29, 2021
Merged

getLeaf() should ignore non-leaf blots #3489

merged 1 commit into from
Nov 29, 2021

Conversation

luin
Copy link
Member

@luin luin commented Nov 25, 2021

Previously, Quill#getLeaf() can return non-leaf blots, which is unexpected. For example, it breaks Selection#getBounds() as it calls leaf.position(offset, true), which is only provided by leaf blots.

The reason Quill#getLeaf() may return a non-leaf blot is, in ParentBlot#path(), if this.children.find(index, inclusive) return [null, 0] (aka the parent blot is empty), the last element of the path would be the parent blot itself, causing Scroll#leaf() gets the parent blot.

The reason that a parent blot can be empty is in Container#optimize(), we don't call container's children's optimize() function, so the children who are parent blots don't have a chance to create a default child. I haven't reproduced this in practice so not 100% sure, but programmatically a reproducible example can be found in the added test case in this PR.


This PR ensures Scroll#leaf() always returns a leaf or null. In fact, this won't happen if we get Container#optimize() fixed, so it may be debatable whether we should do the check. However, IMO people may override Container#optmize() and forget optimizing children even we fix it on our side, so to make Scroll#leaf() complete, a check would be helpful.

@jhchen jhchen merged commit 7406491 into develop Nov 29, 2021
@jhchen jhchen deleted the zh-get-leaf branch November 29, 2021 00:26
Selection,
'<table><tr><td data-row="a">a</td></tr></table>',
);
this.quill.updateContents([{ retain: 1 }, { insert: '\n' }]);
Copy link

Choose a reason for hiding this comment

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

this.quill is undefined, so this test fails for our fork here.

Copy link

Choose a reason for hiding this comment

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

The following patch fixes the test here:

diff --git a/test/unit/core/selection.js b/test/unit/core/selection.js
index 164f5f8b..ef1d1832 100644
--- a/test/unit/core/selection.js
+++ b/test/unit/core/selection.js
@@ -1,6 +1,9 @@
+import Delta from 'quill-delta';
+
 import Selection, { Range } from '../../../core/selection';
 import Cursor from '../../../blots/cursor';
 import Emitter from '../../../core/emitter';
+import Editor from '../../../core/editor';
 
 describe('Selection', function() {
   beforeEach(function() {
@@ -632,12 +635,13 @@ describe('Selection', function() {
     });
 
     it('empty container', function() {
-      const selection = this.initialize(
-        Selection,
+      const [editor, selection] = this.initialize(
+        [Editor, Selection],
         '<table><tr><td data-row="a">a</td></tr></table>',
       );
-      this.quill.updateContents([{ retain: 1 }, { insert: '\n' }]);
+      editor.applyDelta(new Delta([{ retain: 1 }, { insert: '\n' }]));
       expect(selection.getBounds(2, 0)).toEqual(null);
+      this.bounds = selection.getBounds(0, 1);
     });
   });
 });

rockwotj pushed a commit to shortwave/quill that referenced this pull request Dec 6, 2021
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.

3 participants