RichText: List: Fix getParentIndex

[ellatrix]

Description

Fixes #13559 (comment). The bug is caused by a bug in getParentIndex. It should not consider the line index of the given index as the parent index. It is better to only accept a line index as an argument for getParentIndex, so it can be skipped reliably.

How has this been tested?

See #13559 (comment).

Screenshots

Types of changes

Bug fix

Checklist:

• My code is tested.
• My code follows the WordPress code style.
• My code follows the accessibility standards.
• My code has proper inline documentation.
• I've included developer documentation if appropriate.

[mcsf]

Thanks for the fix, it seems to do the trick.

I have some comments on the new indentation implementation, though:

1. In getLineIndex and getParentLineIndex:

while ( index-- ) {

Since index is provided by the function caller, I would guard this more safely with index-- <= 0.

2. In getParentLineIndex:

if ( formatsAtIndex.length === startFormats.length - 1 ) {

Just from reading, this seems to encompass any kind of rich-text format, not just ul / ol. Is there a risk that this will fail (i.e. that we have a false-positive) for certain rich-text values if there happens to be some other format (like strong) whose edge sits right at LINE_SEPARATOR?

[ellatrix]

Since index is provided by the function caller, I would guard this more safely with index-- <= 0.

Could you elaborate on why this is safer?

Just from reading, this seems to encompass any kind of rich-text format, not just ul / ol. Is there a risk that this will fail (i.e. that we have a false-positive) for certain rich-text values if there happens to be some other format (like strong) whose edge sits right at LINE_SEPARATOR?

I'm not sure I'm following. Could you illustrate with an example?

[ellatrix]

Just from reading, this seems to encompass any kind of rich-text format, not just ul / ol. Is there a risk that this will fail (i.e. that we have a false-positive) for certain rich-text values if there happens to be some other format (like strong) whose edge sits right at LINE_SEPARATOR?

Ah I think I get what you mean. Formats at line separators should only be line formats like ul / ol. It's a good point to raise though. While the normal formats are ignored when building the DOM or HTML tree, they do get applied through applyFormat. We should prevent application there.

[mcsf]

I'm not sure I'm following. Could you illustrate with an example?

Simply put, if something somewhere (plain human error, or some other unexpected evaluation) ends up calling getLineIndex( value, -1 ), the VM will break.

[mcsf]

While the normal formats are ignored when building the DOM or HTML tree, they do get applied through applyFormat. We should prevent application there.

Great, thanks!

[mcsf]

LGTM!, assuming the next PR (#13563) handles my last items of feedback.

[ellatrix]

Simply put, if something somewhere (plain human error, or some other unexpected evaluation) ends up calling getLineIndex( value, -1 ), the VM will break.

Ah, the selection can't be negative, so I didn't add logic for it. Should I account for it?