Closed Bug 384147 Opened 15 years ago Closed 15 years ago

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

Categories

(Core :: DOM: Editor, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla1.9beta1

People

(Reporter: mfenniak-moz, Assigned: mfenniak-moz)

Details

Attachments

(1 file, 5 obsolete files)

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
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
Attached patch patch v1 (obsolete) — Splinter Review
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.
Assignee: nobody → mfenniak-moz
Attached patch patch v2 (obsolete) — Splinter Review
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.
Attachment #273347 - Attachment is obsolete: true
The tests for this bug don't work currently due to bug #389199 (tab key doesn't work in contenteditable areas and designmode documents).
Attached patch patch v3 (obsolete) — Splinter Review
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.
Attachment #273475 - Attachment is obsolete: true
Attachment #280902 - Flags: review?(daniel)
Comment on attachment 280902 [details] [diff] [review]
patch v3

r=daniel@glazman.org
seems ok to me... I would like this to be tested in thunderbird please.
Attachment #280902 - Flags: review?(daniel) → review+
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.
Attachment #280902 - Flags: superreview?(sfraser_bugs)
Comment on attachment 280902 [details] [diff] [review]
patch v3

Sorry, I'm not qualified to sr this.
Attachment #280902 - Flags: superreview?(sfraser_bugs) → superreview?
Attachment #280902 - Flags: superreview? → superreview?(roc)
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!
Attachment #280902 - Flags: superreview?(roc) → superreview+
Comment on attachment 280902 [details] [diff] [review]
patch v3

I'll take the liberty of approving this for 1.9 too.
Attachment #280902 - Flags: approval1.9+
Keywords: checkin-needed
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
Status: ASSIGNED → RESOLVED
Closed: 15 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.9 M9
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?
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Weird... it only turned one of the Windows unit test machines orange. The other one took the patch fine!
Attached patch patch v4 (obsolete) — Splinter Review
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.
Attachment #280902 - Attachment is obsolete: true
how about 

addLoadEvent(function(){});

that will keep things more orderly and predictable.
Attached patch patch v5 (obsolete) — Splinter Review
New patch uses addLoadEvent to run tests per Robert's suggestion.  Minor change.  Tests run cleanly without intermittent failures.
Attachment #281655 - Attachment is obsolete: true
Keywords: checkin-needed
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? :)
(In reply to comment #16)
> Mind correcting the comment, please? :)

Mathieu, any update on this?
Attached patch patch v6Splinter Review
Fixed the comment.  Sorry about the delay, Reed.  This should be good to check in.
Attachment #281824 - Attachment is obsolete: true
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
Status: REOPENED → RESOLVED
Closed: 15 years ago15 years ago
Keywords: checkin-needed
OS: Linux → All
Hardware: PC → All
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.