384147 - tabbing out list item doesn't merge with next list item at same depth

Status: RESOLVED FIXED

(Core :: DOM: Editor, defect)

Description

[Mathieu Fenniak]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8.1.4) Gecko/20061201 Firefox/2.0.0.4 (Ubuntu-feisty)
Build Identifier: 

When "tabbing out" a list item, if it is followed by a list item that has the same "depth", the two lists should be merged to maintain expected ordering of ordered lists.

Reproducible: Always

Steps to Reproduce:
1. Using the Mozilla Rich Text Editing Demo, create an ordered list with the text "Hello".
2. At the end of "Hello", press enter, then press tab, and type "foo".
3. Return to the end of the line "Hello", press enter, and then press tab, and type "bar".
Actual Results:  
The list appears as:
    1. Hello
        1. bar
        1. foo

Expected Results:  
The list should appear as:
    1. Hello
        1. bar
        2. foo

Comment 1

[Mathieu Fenniak]

Attachment: patch v1

Attaching proposed patch.  Modifies nsHTMLEditRules::WillHTMLIndent to check for a case where the next sibling of the being-indented list item is a list, and join that list as the top-most item.

Comment 2

[Mathieu Fenniak]

Attachment: patch v2

Attaching updated patch.

This patch merges with lists at the same depth, both before and after the current list item.  v1 just merged with lists before the current list item.

Patch v2 also includes detailed unit tests using mochitest that confirm list indent and outdent behavior works as expected in a variety of cases, including with ol & ul items, items at different depths, and multiple items being indented and outdented at once.

Comment 3

[Mathieu Fenniak]

The tests for this bug don't work currently due to bug #389199 (tab key doesn't work in contenteditable areas and designmode documents).

Comment 4

[Mathieu Fenniak]

Attachment: patch v3

New patch rewrites unit tests to work again -- they were broken by changes to tab behavior in editors.

Daniel, can you review this patch?  I believe it greatly improves list indent/outdent handling by searching for sibling lists to merge items into.  The tests seem pretty comprehensive to me, as well.

Comment 5

[Daniel Glazman (:glazou) (not active in Mozilla any more)]

Comment on attachment 280902 [details] [diff] [review]
patch v3

r=[e-mail]
seems ok to me... I would like this to be tested in thunderbird please.

Comment 6

[Mathieu Fenniak]

Comment on attachment 280902 [details] [diff] [review]
patch v3

Daniel: Thanks for taking the time to review.  I just tested this patch with Thunderbird trunk, it works great.

Requesting superreview.

Comment 7

[Simon Fraser [no longer active]]

Comment on attachment 280902 [details] [diff] [review]
patch v3

Sorry, I'm not qualified to sr this.

Comment 8

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 280902 [details] [diff] [review]
patch v3

+      sibling = nsnull;

I think it would be a little cleaner if you declared a new nsCOMPtr temporary just for the use of the new code you're adding, instead of overloading this one. But sr+ either way. If you change it, no need to re-request sr.

I like the tests!

Comment 9

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 280902 [details] [diff] [review]
patch v3

I'll take the liberty of approving this for 1.9 too.

Comment 10

[Reed Loden [:reed]]

Checking in editor/composer/test/Makefile.in;/cvsroot/mozilla/editor/composer/test/Makefile.in,v  <--  Makefile.in
new revision: 1.3; previous revision: 1.2
done
RCS file: /cvsroot/mozilla/editor/composer/test/test_bug384147.html,v
done
Checking in editor/composer/test/test_bug384147.html;
/cvsroot/mozilla/editor/composer/test/test_bug384147.html,v  <--  test_bug384147.html
initial revision: 1.1
done
Checking in editor/libeditor/html/nsHTMLEditRules.cpp;
/cvsroot/mozilla/editor/libeditor/html/nsHTMLEditRules.cpp,v  <--  nsHTMLEditRules.cpp
new revision: 1.336; previous revision: 1.335
done

Comment 11

[Reed Loden [:reed]]

Had to back this out because it turned the Windows and Linux unit test tinderboxen orange. However, the Mac unit test tinderbox was quite happy with the new test and processed it normally, which is weird...

Here's the error message from one of the Windows machines:
*** 12872 INFO Running /tests/editor/composer/test/test_bug384147.html...
*** 12873 ERROR FAIL | Error thrown during test: uncaught exception: [Exception... "Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsIDOMNSHTMLDocument.execCommand]"  nsresult: "0x80004005 (NS_ERROR_FAILURE)"  location: "JS frame :: http://localhost:8888/tests/editor/composer/test/test_bug384147.html :: test :: line 62"  data: no] | got 0, expected 1 | /tests/editor/composer/test/test_bug384147.html

philor pointed out bug 396848, so maybe that's related?

Comment 12

[Reed Loden [:reed]]

Weird... it only turned one of the Windows unit test machines orange. The other one took the patch fine!

Comment 13

[Mathieu Fenniak]

Attachment: patch v4

Reed, thanks for taking the time to check this patch in.  I was able to reproduce the test failure, occasionally.  Clicking "Run Tests" a dozen times would result in one or two failures with the same error.

Attached patch fixes the occasional failure (where the editor is not setup by the time execCommand starts being called) by replacing "setTimeout(test, 0);" with "window.onload = test;".  It's a very small change only affecting the test, not the rest of the patch, so I think it should be OK for commit.

Comment 14

[Robert Sayre]

how about 

addLoadEvent(function(){});

that will keep things more orderly and predictable.

Comment 15

[Mathieu Fenniak]

Attachment: patch v5

New patch uses addLoadEvent to run tests per Robert's suggestion.  Minor change.  Tests run cleanly without intermittent failures.

Comment 16

[Reed Loden [:reed]]

Comment on attachment 281824 [details] [diff] [review]
patch v5

>+// If executed directly, a race condition exists that will cause execCommand
>+// to fail occasionally (but often).  Defering to onload works.
>+addLoadEvent(function() {

Mind correcting the comment, please? :)

Comment 17

[Reed Loden [:reed]]

(In reply to comment #16)
> Mind correcting the comment, please? :)

Mathieu, any update on this?

Comment 18

[Mathieu Fenniak]

Attachment: patch v6

Fixed the comment.  Sorry about the delay, Reed.  This should be good to check in.

Comment 19

[Reed Loden [:reed]]

Checking in editor/composer/test/Makefile.in;
/cvsroot/mozilla/editor/composer/test/Makefile.in,v  <--  Makefile.in
new revision: 1.5; previous revision: 1.4
done
Checking in editor/composer/test/test_bug384147.html;
/cvsroot/mozilla/editor/composer/test/test_bug384147.html,v  <--  test_bug384147.html
new revision: 1.3; previous revision: 1.2
done
Checking in editor/libeditor/html/nsHTMLEditRules.cpp;
/cvsroot/mozilla/editor/libeditor/html/nsHTMLEditRules.cpp,v  <--  nsHTMLEditRules.cpp
new revision: 1.338; previous revision: 1.337
done