Bug 89159 – Selecting from a table and deleting causes an assertion failure

• Status: NEW
• Component: HTML Editing

[Shinya Kawanaka]

Created attachment 147714 [details]
Repro

Select from "hoge 2" to its some right area. Then you can see just below of "hoge 2" is selected.

Then push JustifyCenter button, then TD 1 and TD 2 will be justified to center, though they are not content editable.
Or press 'Delete' key, then assert will be triggered.

ASSERTION FAILED: isEditablePosition(*this)
Position.cpp::trailingWhitespacePosition

[Shinya Kawanaka]

In DeleteSelectionCommand::initializePositionData(), there are a few call of Position::upstream() and Position::downstream(). They break shadow boundaries.

Fixing Position::upstream() and Position::downstream() will fix this issue, I believe.

I tried fixing them before, but it's harder than I expected...

[Shinya Kawanaka]

The sentence "breaking Shadow boundaries" is not correct.
I should have said: element having Shadow DOM should be treated as atomic...

[Hayato Ito]

I've filed a bug for the spec: https://www.w3.org/Bugs/Public/show_bug.cgi?id=17608

I think hoge1 and hoge2 should be contenteditable in this case. But in current WebKit implementation, we use '-user-modify' style property to judge it is contenteditable or not. So the implementation might be tricky.

'hoge1 and hoge2' should not inherit 'user-modify' in this case because there is a shadow boundary. We use 'composed shadow DOM tree' in resolving style inheritance.

So these elements must not be 'contenteditable' in current WebKit's style resolution. But it seems the 'user-modify' property of those elements became READ-WRITE. It's like 'an unexpected pass'. We should investigate further...

[Hayato Ito]

It seems that the root cause is ComparePositions() in the repro case.

Let me explain using the following simplified example.

<div id=top contenteditable>  // There is no contenteditable ancestor
   <div>hello</div>
   <div>world</div>
</div>

After mouse dragging from [wo|rld] to outside of top, suppose VisibleSelection::validate() is called with the status:

  start: {anchorNode: text node of [world], anchorType: OffsetInAnchor}
  end:  {anchorNode: #top, anchorType: AfterAnchor}
  
After validate(), a visibleSelection should be:

  start: {anchorNode: text node of [world], anchorType: OffsetInAnchor}
  end:  {anchorNode: #top, anchorType: AfterChildren}

But the actual result is:

  start: {anchorNode: text node of [world], anchorType: OffsetInAnchor}
  end:  {anchorNode: #top, anchorType: AfterAnchor}

Yeah, nothing changes. That should be changed after adjustSelectionToAvoidCrossingEditingBoundaries() is called in validate().
The following code in adjustSelectionToAvoidCrossingEditingBoundaries():

  VisiblePosition last = lastEditablePositionBeforePositionInRoot(m_end, baseRoot);   

returns Position: {anchorNode: #top, anchorType: AfterAnchor} as last.

In lastEditablePositionBeforePositionInRoot(m_end, baseRoot), comparePositions is called as follows:

  if (comparePositions(position, lastPositionInNode(highestRoot)) == 1) 

  with position: anchorNode: {anchorNode: #top, anchorType: AfterAnchor}
         lastPositionInNode(highestRoot): {anchorNode: #top, anchorType: AfterChildren}

Ideally, comparePositions() should return 1 in this case. But that seemed to return non-1 value. If it returns 1, lastPositionInNode(highestRoot) is used as end() and things will work well.

[Shinya Kawanaka]

I found that this happend even if Shadow DOM is not attached.
It's a real editing problem.

[Shinya Kawanaka]

So I remove the dependency from

[Shinya Kawanaka]

Oops...
So I remove the dependency to 82697.

[Shinya Kawanaka]

Created attachment 149776 [details]
Repro

Removed Shadow DOM from tests.

[Hayato Ito]

Quick update: Now ComparePositions()  were fixed. But it seemed there are other code places we needed to fix after ComparePositions returns correct value.

(In reply to comment #8)
> Created an attachment (id=149776) [details]
> Repro
> 
> Removed Shadow DOM from tests.

(In reply to comment #4)
> It seems that the root cause is ComparePositions() in the repro case.
> 
> Let me explain using the following simplified example.
> 
> <div id=top contenteditable>  // There is no contenteditable ancestor
>    <div>hello</div>
>    <div>world</div>
> </div>
> 
> After mouse dragging from [wo|rld] to outside of top, suppose VisibleSelection::validate() is called with the status:
> 
>   start: {anchorNode: text node of [world], anchorType: OffsetInAnchor}
>   end:  {anchorNode: #top, anchorType: AfterAnchor}
> 
> After validate(), a visibleSelection should be:
> 
>   start: {anchorNode: text node of [world], anchorType: OffsetInAnchor}
>   end:  {anchorNode: #top, anchorType: AfterChildren}
> 
> But the actual result is:
> 
>   start: {anchorNode: text node of [world], anchorType: OffsetInAnchor}
>   end:  {anchorNode: #top, anchorType: AfterAnchor}
> 
> Yeah, nothing changes. That should be changed after adjustSelectionToAvoidCrossingEditingBoundaries() is called in validate().
> The following code in adjustSelectionToAvoidCrossingEditingBoundaries():
> 
>   VisiblePosition last = lastEditablePositionBeforePositionInRoot(m_end, baseRoot);   
> 
> returns Position: {anchorNode: #top, anchorType: AfterAnchor} as last.
> 
> In lastEditablePositionBeforePositionInRoot(m_end, baseRoot), comparePositions is called as follows:
> 
>   if (comparePositions(position, lastPositionInNode(highestRoot)) == 1) 
> 
>   with position: anchorNode: {anchorNode: #top, anchorType: AfterAnchor}
>          lastPositionInNode(highestRoot): {anchorNode: #top, anchorType: AfterChildren}
> 
> Ideally, comparePositions() should return 1 in this case. But that seemed to return non-1 value. If it returns 1, lastPositionInNode(highestRoot) is used as end() and things will work well.

[Ahmad Saleem]

Blink fix (for similar assertion): https://chromium.googlesource.com/chromium/src/+/308ca1b8a40d58ec8e8c1bf6a0f62a7681c62fea