Open Bug 779067 Opened 12 years ago Updated 2 years ago

Cannot join <li> elements with typing Backspace at start of a list item

Categories

(Core :: DOM: Editor, defect)

Product:
Core
Component:
DOM: Editor
Version:
15 Branch
Platform:
x86
Windows XP
Type:
defect

Tracking

()

People

(Reporter: adrian, Unassigned)

References

(Blocks 1 open bug,
URL
)

Details

(Keywords: dupeme, testcase, Whiteboard: [h2review-noted]) 

User Agent: Mozilla/5.0 (Windows NT 5.1) AppleWebKit/535.1 (KHTML, like Gecko) Chrome/13.0.782.107 Safari/535.1

Steps to reproduce:

I have the following html loaded into Firefox:

<html>
    <body>
         <ul contenteditable="true">
              <li></li>
              <li>B</li>
         </ul>
    </body>
</html>

I put the caret into the first bullet and press [BACKSPACE] or [DELETE].


Actual results:

For each [BACKSPACE] or [DELETE] a BR is created outside the contenteditable element:

After a few strokes I have this html:

<html>
    <body>
         <br _moz_dirty="">
         <br _moz_dirty="">
         <br _moz_dirty="">
         <br _moz_dirty="">
         <br _moz_dirty="">
         <ul contenteditable="true">
              <li></li>
              <li>B</li>
         </ul>
    </body>
</html>

The first bullet must be empty to produce this error. If there are any characters in the first bullet it behaves as expected.


Expected results:

The html outside a contenteditable should not be modified.
Reproducible back to first contenteditable supporting Version (Mozilla/5.0 (Windows; U; Windows NT 6.1; en-US; rv:1.9.0.19) Gecko/2010031422 Firefox/3.0.19 ID:2010031422).
URL: data:text/html,<html><body><ul conten...
Component: Untriaged → Editor
Keywords: testcase
Product: Firefox → Core
Whiteboard: [dupeme]
This might be slightly off-topic, but here's a way to show that backspace does not perform as expected within contentEditable=true list items.

= Replicate =

1. Visit http://jsfiddle.net/sFSb8/
2. Position cursor to start of second list item.
3. Press backspace.

= Expected Results =

The second list item is joined with the first list item. The third list item becomes the second list item, and so on down the line.

= Actual Results =

Backspace has no effect.

= Work Around =

1. Position the cursor to the end of the first line.
2. Press delete (this will join the first line with second line, as expected).
3. Press enter (this will split the first line and second line, resulting in what *looks* like the starting state).
4. Press backspace (now it works, but only for joining the first and second lines, which have been modified).

Firefox 26 for Ubuntu.
Keywords: dupeme
Whiteboard: [dupeme]

I don't see the <br> elements anymore, but I cannot join the list items.

Summary: When typing a [BACKSPACE] or [DELETE] into a contenteditable UL or OL, a BR tag is created outside the contenteditable element → Cannot join <li> elements with typing Backspace at start of a list item

Items can be joined with Delete but not with Backspace. It can join with Backspace if it has no previous text sibling.

I'm seeing that joining by Delete calls HandleDeleteNonCollapsedSelection while joining by Backspace calls TryToJoinBlocksWithTransaction, where the latter tries to join <li> with its parent which is no-op.

xul.dll!mozilla::HTMLEditor::HandleDeleteNonCollapsedSelection(short aDirectionAndAmount, short aStripWrappers, mozilla::HTMLEditor::SelectionWasCollapsed aSelectionWasCollapsed) Line 2984 (c:\Users\Kagami\Documents\GitHub\gecko-dev\editor\libeditor\HTMLEditSubActionHandler.cpp:2984)
xul.dll!mozilla::HTMLEditor::HandleDeleteSelectionInternal(short aDirectionAndAmount, short aStripWrappers) Line 2378 (c:\Users\Kagami\Documents\GitHub\gecko-dev\editor\libeditor\HTMLEditSubActionHandler.cpp:2378)

xul.dll!mozilla::HTMLEditor::TryToJoinBlocksWithTransaction(nsIContent & aLeftContentInBlock, nsIContent & aRightContentInBlock) Line 3454 (c:\Users\Kagami\Documents\GitHub\gecko-dev\editor\libeditor\HTMLEditSubActionHandler.cpp:3454)
xul.dll!mozilla::HTMLEditor::HandleDeleteCollapsedSelectionAtCurrentBlockBoundary(short aDirectionAndAmount, mozilla::dom::Element & aCurrentBlockElement, const mozilla::EditorDOMPointBase<nsCOMPtr<nsINode>,nsCOMPtr<nsIContent> > & aCaretPoint) Line 2960 (c:\Users\Kagami\Documents\GitHub\gecko-dev\editor\libeditor\HTMLEditSubActionHandler.cpp:2960)
xul.dll!mozilla::HTMLEditor::HandleDeleteAroundCollapsedSelection(short aDirectionAndAmount, short aStripWrappers) Line 2461 (c:\Users\Kagami\Documents\GitHub\gecko-dev\editor\libeditor\HTMLEditSubActionHandler.cpp:2461)
xul.dll!mozilla::HTMLEditor::HandleDeleteSelectionInternal(short aDirectionAndAmount, short aStripWrappers) Line 2370 (c:\Users\Kagami\Documents\GitHub\gecko-dev\editor\libeditor\HTMLEditSubActionHandler.cpp:2370)

Edit: That code path difference is because TextEditor::ExtendSelectionForDelete extends selection for Delete but not for Backspace.

I tried modifying TextEditor::ExtendSelectionForDelete to always call CharacterExtendForBackspace for ePrevious just as it does for eNext. It fixes this bug, but with several unexpected failures on WPT (and also several new passes). Seems we should insert some new condition to call it, what do you think, Masayuki?

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=5ec201f0cbd4bbd862b3b6d73a2fc9f42bf762aa

Flags: needinfo?(masayuki)

(In reply to Kagami :saschanaz from comment #5)

Edit: That code path difference is because TextEditor::ExtendSelectionForDelete extends selection for Delete but not for Backspace.

(In reply to Kagami :saschanaz from comment #6)

I tried modifying TextEditor::ExtendSelectionForDelete to always call CharacterExtendForBackspace for ePrevious just as it does for eNext. It fixes this bug, but with several unexpected failures on WPT (and also several new passes). Seems we should insert some new condition to call it, what do you think, Masayuki?

Try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=5ec201f0cbd4bbd862b3b6d73a2fc9f42bf762aa

The removing code must be required for various cases. This bug is Backspace key handling when selection is collapsed and the caret is start of a node. So, I guess that you need to call CharacterExtendForBackspace() only when insertionPoint.IsStartOfContainer() is true.

Flags: needinfo?(masayuki)

So, I guess that you need to call CharacterExtendForBackspace() only when insertionPoint.IsStartOfContainer() is true.

It works and I guess it's the right fix, but it still causes the same test failures, reviving Bug 200416 for example. Given that a similar bug still exists (Bug 1621592), maybe a fix here should include another fix for that bug.

Hmm, CharacterExtendForBackspace() goes into nsIFrame::PeekOffset(). Its current behavior does not match with editor's requirement (according to the unexpected failures), and changing it may break Selection API too (or may fix its bugs too, though). As far as I've checked, it's useful for editor only when insertionPoint is middle of a text node.

Perhaps, in eNext case, editor should stop using CharacterExtendForDelete() unless selection is collapsed in start or middle of a text node. Then HTMLEditor::HandleDeleteCollapsedSelectionAtCurrentBlockBoundary() should keep looking for nearest editing content which should be joined with current block.

AFAICT HandleDeleteCollapsedSelectionAtCurrentBlockBoundary alone tries joining <li> with the parent <ul> instead of its sibling, and CharacterExtendForDelete prevents that by also selecting whitespaces between them. So I think we need CharacterExtendForDelete here.

(In reply to Kagami :saschanaz from comment #10)

AFAICT HandleDeleteCollapsedSelectionAtCurrentBlockBoundary alone tries joining <li> with the parent <ul> instead of its sibling, and CharacterExtendForDelete prevents that by also selecting whitespaces between them. So I think we need CharacterExtendForDelete here.

Do you mean that GetPreviousEditableHTMLNode() and GetNextEditableHTMLNode() may return block parent here? If so, I don't understand what's going on. They must return next/previous <li> element or its first/last leaf node.
https://searchfox.org/mozilla-central/rev/278046367dab878316f60f0bd7f740cf73f3c447/editor/libeditor/EditorBase.cpp#3915,3919-3921,3926-3931,3933,3968

And also, TryToJoinBlocksWithTransaction() must get 2 <li> elements here:
https://searchfox.org/mozilla-central/rev/278046367dab878316f60f0bd7f740cf73f3c447/editor/libeditor/HTMLEditSubActionHandler.cpp#3463-3464

But we hit here?:
https://searchfox.org/mozilla-central/rev/278046367dab878316f60f0bd7f740cf73f3c447/editor/libeditor/HTMLEditSubActionHandler.cpp#3501-3505

Flags: needinfo?(saschanaz)
Assignee: nobody → saschanaz
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Flags: needinfo?(saschanaz)

Do you mean that GetPreviousEditableHTMLNode() and GetNextEditableHTMLNode() may return block parent here?

Both returns a text node, one from between two <li>s and and another from the second <li>. I'm not sure the former one is really "editable", as it's invisible, but nsINode::IsEditable() thinks it is as it's in contenteditable.

And also, TryToJoinBlocksWithTransaction() must get 2 <li> elements here:

As the former text node is from the parent <ul>, the result here is <ul> and <li>.

But we hit here?:

Thus, yes.

(In reply to Kagami :saschanaz from comment #12)

Do you mean that GetPreviousEditableHTMLNode() and GetNextEditableHTMLNode() may return block parent here?

Both returns a text node, one from between two <li>s and and another from the second <li>. I'm not sure the former one is really "editable", as it's invisible, but nsINode::IsEditable() thinks it is as it's in contenteditable.

And also, TryToJoinBlocksWithTransaction() must get 2 <li> elements here:

As the former text node is from the parent <ul>, the result here is <ul> and <li>.

Ah, that's what the unexpected behavior of the utility methods!

Probably, EditorBase::FindNode() should keep scanning next leaf node here when it meets a text node which is non-visible text node under <ul>, <ol>, <dl>, <table>, <thead>, <tbody>, <thead>, <tr>, etc (or all invisible text nodes? I'm not sure...). I guess that changing IsEditable() in any class shouldn't be changed because if it returns false, editor tries to keep it even when its conteainer is being removed.

See Also: → 1191875
Whiteboard: [h2review-noted]

Hasn't been active on this one. Still intend to take a look later, but please feel free to take this.

Assignee: krosylight → nobody
Status: ASSIGNED → NEW
Severity: normal → S3
You need to log in before you can comment on or make changes to this bug.