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

RESOLVED FIXED in mozilla1.9beta1

Status

()

Core
Editor
RESOLVED FIXED
11 years ago
11 years ago

People

(Reporter: Mathieu Fenniak, Assigned: Mathieu Fenniak)

Tracking

unspecified
mozilla1.9beta1
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 5 obsolete attachments)

(Assignee)

Description

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

Updated

11 years ago
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
(Assignee)

Comment 1

11 years ago
Created attachment 273347 [details] [diff] [review]
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.
Assignee: nobody → mfenniak-moz
(Assignee)

Comment 2

11 years ago
Created attachment 273475 [details] [diff] [review]
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.
Attachment #273347 - Attachment is obsolete: true
(Assignee)

Comment 3

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

Comment 4

11 years ago
Created attachment 280902 [details] [diff] [review]
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.
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+
(Assignee)

Comment 6

11 years ago
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 7

11 years ago
Comment on attachment 280902 [details] [diff] [review]
patch v3

Sorry, I'm not qualified to sr this.
Attachment #280902 - Flags: superreview?(sfraser_bugs) → superreview?
(Assignee)

Updated

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

Updated

11 years ago
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
Last Resolved: 11 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!
(Assignee)

Comment 13

11 years ago
Created attachment 281655 [details] [diff] [review]
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.
Attachment #280902 - Attachment is obsolete: true

Comment 14

11 years ago
how about 

addLoadEvent(function(){});

that will keep things more orderly and predictable.
(Assignee)

Comment 15

11 years ago
Created attachment 281824 [details] [diff] [review]
patch v5

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
(Assignee)

Updated

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

Comment 18

11 years ago
Created attachment 283013 [details] [diff] [review]
patch v6

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
Last Resolved: 11 years ago11 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.