677752 - [contentEditable] indent and justify* fail on editable nodes that have only one child

Status: RESOLVED FIXED

(Core :: DOM: Editor, defect)

Description

[Fabien Cazenave [:kaze]]

Attachment: testcase

With the attached example, indent, outdent and justify* don’t work as expected:

<section contenteditable> foo bar </section>
<div contenteditable> foo bar </div>
<p contenteditable> foo bar </p>

----

Steps to reproduce (justify*):

1. put the caret in the <section> element
2. click on the “right” button
3. put the caret in the <div> element
4. click on the “right” button
5. put the caret in the <p> element
6. click on the “right” button

Actual results:
(note: an exception is raised on the first element)

<div _moz_dirty style="text-align: right;"> </div>
<section contenteditable> foo bar </section>
<div contenteditable style="text-align: right;">
  foo bar
</div>
<p contenteditable style="text-align: right;">
  foo bar
</p>

Expected results:

<section contenteditable>
  <div style="text-align: right;"> foo bar </div>
</section>
<div contenteditable>
  <div style="text-align: right;"> foo bar </div>
</div>
<p contenteditable> foo bar </p>

----

Now reload the page and repeat the same steps with the “indent” button.
Actual results:

<section contenteditable style="margin-left: 40px;">
  foo bar
</section>
<div contenteditable style="margin-left: 40px;">
  foo bar
</div>
<p contenteditable style="margin-left: 40px;">
  foo bar
</p>

Expected results:

<section contenteditable>
  <div style="margin-left: 40px;"> foo bar </div>
</section>
<div contenteditable>
  <div style="margin-left: 40px;"> foo bar </div>
</div>
<p contenteditable> foo bar </p>

----

Now reload the page and repeat the same steps with the “outdent” button.
Actual results:

<section contenteditable> foo bar </section>
foo bar
<div contenteditable></div>
<p contenteditable> foo bar </p>

Expected results:

<section contenteditable> foo bar </section>
<div contenteditable> foo bar </div>
<p contenteditable> foo bar </p>

----

To sum it up: Gecko should create a <div> around the text node where possible (section, div) and ignore these execCommands otherwise (paragraph). Adding an attribute to the active editing host is wrong, imho (*content* editable).
(Note to self: add a <span> for the unit tests.)

FWIW, Chromium gives the expected results except that it inserts a <div> container in the <p contenteditable> node, which is not valid.

Comment 1

[Fabien Cazenave [:kaze]]

FTR, fixing this bug would get us 15 additional points in the A and AC sections of the browserscope/richtext2 tests:

Section A -- Apply Formatting Tests: +10 points
        without patch : 21/31 (selection:  9/31)
        with patch    : 28/31 (selection: 12/31)

  FB:BQ_TEXT-1_SI    EXECUTION EXCEPTION: (mouseover)
  FB:BQ_TEXT-1_SI    EXECUTION EXCEPTION: (mouseover)
  FB:BQ_BR.BR-1_SM   EXECUTION EXCEPTION: (mouseover)
  FB:BQ_BR.BR-1_SM   EXECUTION EXCEPTION: (mouseover)
  IND_TEXT-1_SI      EXECUTION EXCEPTION: (mouseover)
  IND_TEXT-1_SI      EXECUTION EXCEPTION: (mouseover)
  JC_TEXT-1_SC       editing host is modified
  JF_TEXT-1_SC       editing host is modified
  JL_TEXT-1_SC       editing host is modified
  JR_TEXT-1_SC       editing host is modified

Section AC -- Apply Formatting Tests, using styleWithCSS: +5 points
        without patch :  7/18 (selection:  5/18)
        with patch    : 12/18 (selection:  5/18)

  IND_TEXT-1_SI      editing host is modified
  JC_TEXT-1_SC       editing host is modified
  JF_TEXT-1_SC       editing host is modified
  JL_TEXT-1_SC       editing host is modified
  JR_TEXT-1_SC       editing host is modified

Comment 2

[Fabien Cazenave [:kaze]]

Attachment: this is not a patch

The problem would be “solved” with this patch but it modifies `IsNodeInActiveEditor', which raises (at least) two regressions -- see attachment for more details. I’ll try to find a more incremental approach later.

This patch also checks that the active editor can contain block-level elements before applying indent/outdent/justify*. There are probably other execCommand actions that should do similar checks.

Comment 3

[Fabien Cazenave [:kaze]]

Attachment: testcase: list outdent

On the two regressions that I’ve mentioned above, one is related to the use of Shift+Tab to outdent a list item. The `test_htmleditor_keyevent_handling.html' unit test reports 4 regressions:

  • 7372 ERROR TEST-UNEXPECTED-FAIL
      | non-tabbable HTML editor: Shift+Tab after Tab on UL
      - got "<ul><li id=\"target\">ul list item</li></ul>",
      expected "<ul><ul><li id=\"target\">ul list item</li></ul></ul>"

  • 7379 ERROR TEST-UNEXPECTED-FAIL
      | non-tabbable HTML editor: Shift+Tab on UL
      - got "ul list item",
      expected "<ul><li id=\"target\">ul list item</li></ul>"

  • 7415 ERROR TEST-UNEXPECTED-FAIL
      | non-tabbable HTML editor: Shift+Tab after Tab on OL -
      got "<ol><li id=\"target\">ol list item</li></ol>",
      expected "<ol><ol><li id=\"target\">ol list item</li></ol></ol>"

  • 7422 ERROR TEST-UNEXPECTED-FAIL
      | non-tabbable HTML editor: Shfit+Tab on OL
      - got "ol list item",
      expected "<ol><li id=\"target\">ol list item</li></ol>"

But these four tests should be marked as UNEXPECTED_PASS. The code in the test itself includes a `XXX' mark, see:
    // XXX why do we fail to outdent on non-tabbable HTML editor?
https://mxr.mozilla.org/mozilla-central/source/editor/libeditor/html/tests/test_htmleditor_keyevent_handling.html?force=1#328

In this test, oudent fails because the list is the only child of the <div contenteditable> element. Try the attached testcase, you’ll see that the behavior is incorrect with Nightly but correct with the patch proposal. Here’s what the innerHTML of the <div contenteditable> should be:

 • initial state:           <ol><li>Mozilla</li><ol> 
 • press [Tab]:         <ol><ol><li>Mozilla</li></ol><ol>
 • press [Shift+Tab]:       <ol><li>Mozilla</li><ol> 
 • press [Shift+Tab] again:         Mozilla

Without the patch, the list is moved out of the <div contenteditable> when [Shift+Tab] is pressed. The second [Shift+Tab] is ignored because the editable is empty.

Comment 4

[Fabien Cazenave [:kaze]]

Attachment: almost a patch

Includes the fixes on `test_htmleditor_keyevent_handling.html' as explained above.

Comment 5

[Fabien Cazenave [:kaze]]

Attachment: testcase: backspace / delete at the edges of an editable block

The last unit test that fails with this patch is `test_bug414526.html'. The `IsNodeInActiveEditor' method has been implemented in bug 414526 to make sure that:
 • pressing [BackSpace] at the beginning of an editable element doesn’t modify the previous sibling;
 • pressing [Delete] at the end of an editable element doesn’t modify the next sibling.

The test that fails does the contrary, see attachment.
With the following editable <div>:
  <div contenteditable> <p>editor3</p> </div>

this test ensures that the content is not modified when:
 • pressing [BackSpace] as the caret is put at the *end* of the <div>
 • pressing [Delete] as the caret is put at the *beginning* of the <div>

Does this test really make sense? I’ve tested with Chromium and Opera, none of them would pass such a test.
My guess would be that there’s a mistake in this test, especially when I read the FAIL report that does not conform to the real test being performed:
  "Pressing delete key at *end* of editor3 changes the content"…

…but that might be a bit optimistic. Ehsan, what do you think?

Comment 6

[Fabien Cazenave [:kaze]]

Here’s a real bug (though not a regression):
 • open the attachment 554888 [details]
 • click on “move caret to end”
 • press [Delete]

Expected results:
  nothing happens. We should get:
  <div contenteditable> <p>editor3</p> </div>

Actual results:
  the editable content is modified. We get:
  <div contenteditable> <p>e</p> <br> </div>

Bug confirmed with Firefox 6, Aurora and Nightly. I’d say it’s related to the way the selection is collapsed (or the way it’s extended for deletion, if we consider that putting the caret outside of a text node is valid).

Comment 7

[Fabien Cazenave [:kaze]]

Ooops, just saw that this bug is already mentioned in the test:
https://mxr.mozilla.org/mozilla-central/source/editor/libeditor/html/tests/test_bug414526.html?force=1#114

Comment 8

[Fabien Cazenave [:kaze]]

Attachment: patch proposal

Turns out that the tests mentioned in comment #5 are perfectly valid and meaningful, but I’ve been confused by the report strings. I’ve added a few comments in `test_bug414526.html', made the report strings sharper for the two concerned tests, and added a TODO test.

This patch leaves `IsNodeInActiveEditor' alone to avoid a failure on this test, and limits the promotion range for block-level operations instead. Please have a look at the patch header for a (too) long explanation.

The patch has been pushed to TryServer, waiting for test results:
http://tbpl.allizom.org/?tree=Try&usebuildbot=1&rev=ef7f93e77da1

I still have to write specific tests for the current bug.

Comment 9

[Fabien Cazenave [:kaze]]

Attachment: patch proposal

Same patch, now including a specific unit test. Should be ready for review if TryServer remains green:
http://tbpl.allizom.org/?tree=Try&usebuildbot=1&rev=465a6a0cc512

Comment 10

[(no longer active)]

Comment on attachment 555087 [details] [diff] [review]
patch proposal

Review of attachment 555087 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good!

Comment 11

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

http://hg.mozilla.org/mozilla-central/rev/d303dca1216d