Last Comment Bug 677752 - [contentEditable] indent and justify* fail on editable nodes that have only one child
: [contentEditable] indent and justify* fail on editable nodes that have only o...
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Editor (show other bugs)
: Trunk
: x86 All
: -- normal (vote)
: mozilla9
Assigned To: Fabien Cazenave [:kaze]
:
Mentors:
Depends on: 1006793
Blocks:
  Show dependency treegraph
 
Reported: 2011-08-09 16:38 PDT by Fabien Cazenave [:kaze]
Modified: 2014-05-13 10:19 PDT (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
testcase (2.09 KB, text/html)
2011-08-09 16:38 PDT, Fabien Cazenave [:kaze]
no flags Details
this is not a patch (9.44 KB, patch)
2011-08-11 05:12 PDT, Fabien Cazenave [:kaze]
no flags Details | Diff | Splinter Review
testcase: list outdent (1.08 KB, text/html)
2011-08-22 07:31 PDT, Fabien Cazenave [:kaze]
no flags Details
almost a patch (14.66 KB, patch)
2011-08-22 08:20 PDT, Fabien Cazenave [:kaze]
no flags Details | Diff | Splinter Review
testcase: backspace / delete at the edges of an editable block (1.14 KB, text/html)
2011-08-22 09:53 PDT, Fabien Cazenave [:kaze]
no flags Details
patch proposal (48.64 KB, patch)
2011-08-23 03:41 PDT, Fabien Cazenave [:kaze]
no flags Details | Diff | Splinter Review
patch proposal (54.02 KB, patch)
2011-08-23 06:23 PDT, Fabien Cazenave [:kaze]
ehsan: review+
Details | Diff | Splinter Review

Description Fabien Cazenave [:kaze] 2011-08-09 16:38:34 PDT
Created attachment 551932 [details]
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] 2011-08-11 02:41:38 PDT
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] 2011-08-11 05:12:05 PDT
Created attachment 552336 [details] [diff] [review]
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] 2011-08-22 07:31:42 PDT
Created attachment 554858 [details]
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] 2011-08-22 08:20:05 PDT
Created attachment 554868 [details] [diff] [review]
almost a patch

Includes the fixes on `test_htmleditor_keyevent_handling.html' as explained above.
Comment 5 Fabien Cazenave [:kaze] 2011-08-22 09:53:09 PDT
Created attachment 554888 [details]
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] 2011-08-22 10:25:59 PDT
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] 2011-08-22 10:31:12 PDT
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] 2011-08-23 03:41:32 PDT
Created attachment 555065 [details] [diff] [review]
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] 2011-08-23 06:23:32 PDT
Created attachment 555087 [details] [diff] [review]
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 :Ehsan Akhgari 2011-08-23 12:09:42 PDT
Comment on attachment 555087 [details] [diff] [review]
patch proposal

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

Looks good!
Comment 11 Marco Bonardo [::mak] 2011-08-24 01:39:33 PDT
http://hg.mozilla.org/mozilla-central/rev/d303dca1216d

Note You need to log in before you can comment on or make changes to this bug.