The default bug view has changed. See this FAQ.

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

RESOLVED FIXED in mozilla9

Status

()

Core
Editor
RESOLVED FIXED
6 years ago
3 years ago

People

(Reporter: kaze, Assigned: kaze)

Tracking

(Depends on: 1 bug)

Trunk
mozilla9
x86
All
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(4 attachments, 3 obsolete attachments)

(Assignee)

Description

6 years ago
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.
(Assignee)

Comment 1

6 years ago
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
(Assignee)

Comment 2

6 years ago
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.
Assignee: nobody → kaze
Status: NEW → ASSIGNED
(Assignee)

Comment 3

6 years ago
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.
(Assignee)

Comment 4

6 years ago
Created attachment 554868 [details] [diff] [review]
almost a patch

Includes the fixes on `test_htmleditor_keyevent_handling.html' as explained above.
Attachment #552336 - Attachment is obsolete: true
(Assignee)

Comment 5

6 years ago
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?
(Assignee)

Comment 6

6 years ago
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).
(Assignee)

Comment 7

6 years ago
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
(Assignee)

Comment 8

6 years ago
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.
Attachment #554868 - Attachment is obsolete: true
(Assignee)

Comment 9

6 years ago
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
Attachment #555065 - Attachment is obsolete: true
(Assignee)

Updated

6 years ago
Attachment #555087 - Flags: review?(ehsan)
Comment on attachment 555087 [details] [diff] [review]
patch proposal

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

Looks good!
Attachment #555087 - Flags: review?(ehsan) → review+
http://hg.mozilla.org/mozilla-central/rev/d303dca1216d
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla9

Updated

3 years ago
Depends on: 1006793
You need to log in before you can comment on or make changes to this bug.