Fix insertTextAtRange

[gggdomi]

Is this adding or improving a feature or fixing a bug?

Fixing a bug

What's the new behavior?

No expected change in behaviour

How does this change work?

1. Fixed a typo that overwrote normalize option when specified, instead of providing a default value when not specified
2. As a result, the normalization code in the subsequent lines is called (as it should be), and slate changes at-current-range insertText hanging-selection-single-block & slate changes at-current-range insertText hanging-selection-multiple-blocks test failed because normalizeAncestor is undefined when it should be the whole document -> fix suggested by @zhujinxuan included

[ghost]

the undefined check here is probably incorrect, because line 845 is missing a call to getOptions to get the correct normalize option value based on the flags and the option object passed in (see any of the other above functions). With the addition of that function, it'll no longer be possible to have an undefined normalize value, so the conditional will possibly need to change to only overriding if normalization is true.

[zhujinxuan]

@DamareYoh Agree. We need to use

let normalize = change.getFlag('normalize', options)

in line 845

[gggdomi]

Got it, I added the getFlag part.
Do we really want to ignore an explicitly set { normalize: true } in the function call though?
It sounds counter-intuitive to me.
An additional call to change.normalizeNodeByKey(change.value.startBlock.key) would be required to do the trick, but would users know?

I'll change the condition if you confirm.

[zhujinxuan]

Indeed, we are trying to normalize the change.deleteAtRange in the insertTextAtRange function... That is why we need such a complex normalization.

[gggdomi]

@zhujinxuan what's the reason for doing so?
Prevent calling normalization twice for performance reasons?

[zhujinxuan]

Because both insertTextAtRange (though bugy right now) and insertTextByKey are public interface, we want the same performance optimization exist in both functions for someone calling it from outside.

(About the other bug of insertTextAtRange, I will start to fix it after this PR merged)

[gggdomi]

But why couldn't we move the optimisation code to deleteAtRange then?
Plus I don't see a lot of common normalization code between insertTextAtRange and insertTextByKey

[gggdomi]

Now that normalize uses getFlag and can't be undefined anymore, what should this condition be?

1. if (normalize) would ignore {normalize: true} for one-line changes, which sounds counter-intuitive to me (the user expects its options to be taken into account)
2. if (!normalize) would not trigger the optimization unless {normalize: false} is specified
3. if (options.normalize === undefined) sounds like the correct behaviour to me (we optimize the normalization unless the user wants a specific behaviour), but I'm not sure it respects the code-base guidelines?

Hopefully we could merge this PR once this is settled?

[cameracker]

(Hi, DamareYoh is my non-work alt so I made the original suggestion).

What if we reverted the normalize = change.getFlag('normalize', options) to what it was before,

and then change the range.isExpanded usage to:

if (normalize === undefined) {
   normalize = change.getFlag('normalize', options) && range.isExpanded;
} 

Or something like that. This approach would

1. Maintain the original behavior where the default is to not normalize the insertion of a text character when the range is collapsed
2. Maintain the original behavior where the user could override the default behavior specifically by passing in an explicit { normalize: true } option
3. Utilize the getFlag('normalize', options) behavior so that any logic that turns off normalization external to the function is appropriately utilized

[gggdomi]

Sounds like a very good alternative solution!
I'll fix the PR with your suggestion asap.

[cameracker]

Cool! One thing id suggest though is making some tests to verify. The logic here is complicated enough to be a source of mistakes and has been in the past, as we can tell :)

[zhujinxuan]

I strongly agree we shall have some tests about that. We fix this this time, but we may carelessly disturb that in future PRs. We shall have tests to ensure normalize of insertTextAtRange is and will be always working correctly.

[gggdomi]

There is a problem with testing normalization. All tests pass because input.toJSON() and output.toJSON() return equals values (empty nodes are deleted during .toJSON()), despite looking different, such as in:

const input = (
  <value>
    <document>
      <paragraph>
        <link>
          <hashtag key="a">
            <cursor />
          </hashtag>
        </link>
      </paragraph>
    </document>
  </value>
)

const output = (
  <value>
    <document>
      <paragraph>
        <cursor />
      </paragraph>
    </document>
  </value>
)

I'm not sure how to test if normalization is happening or not.

[zhujinxuan]

@gggdomi Can you leave each one character in the inlines? I remember delete at range will remove the inlines if the inline is all covered. (I am pretty unsure about the current logic of deleteAtRange and insertAtRange)

[ianstormtaylor]

Fixed by #3093.