Closed Bug 507936 Opened 15 years ago Closed 14 years ago

Should join node when deleting at end of paragraph

Categories

(Core :: DOM: Editor, defect)

x86
All
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla1.9.3a4

People

(Reporter: cpearce, Assigned: liucougar)

References

(Blocks 1 open bug)

Details

Attachments

(2 files, 7 obsolete files)

Attached file Testcase
This is a spin off of bug 502259 and bug 419217. I think we need to always call JoinBlock at the end of nsHTMLEditRules::WillDeleteSelection(), as per the original patch in bug 419217.

Note: peterv was wary of this (Bug 419217 comment 5) but liucougar reckons this is safe (bug 502259 comment 35).
Blocks: 442758
Blocks: 519751
Attached patch proposed fix and unit test (obsolete) — Splinter Review
call JoinBlocks if nothing visible to the user is deleted. I define "visible" here as: IsVisTextNode or IsVisBreak or anything else, so "invisible" only refers empty textnode (may contain \n \r etc) or invisible breaks (such as end of paragraph break)

this patch also fixes bug 519751 (including unit test)
Assignee: nobody → liucougar
Attachment #409810 - Flags: review?(roc)
Attachment #409810 - Flags: review?(roc) → review?(peterv)
Comment on attachment 409810 [details] [diff] [review]
proposed fix and unit test

>diff -r a954f52e3149 editor/libeditor/html/nsHTMLEditRules.cpp

>+            //if something visible is deleted, no need to join
>+            //visible means: visible textnode or visible break or anything else

Spaces after // and capitalize sentences. I'd say "Visible means all nodes except non-visible textnodes and breaks."

>+            if (join && 
>+              (mHTMLEditor->IsTextNode(somenode) ? 
>+                (NS_OK==mHTMLEditor->IsVisTextNode(somenode, &join, PR_TRUE) && !join) : 
>+                (!nsTextEditUtils::IsBreak(somenode) || mHTMLEditor->IsVisBreak(somenode))))
>+            {
>+              join = PR_FALSE;
>+            }

I think this would be clearer as:

            if (join) {
              if (mHTMLEditor->IsTextNode(somenode)) {
                mHTMLEditor->IsVisTextNode(somenode, &join, PR_TRUE);
              }
              else if (nsTextEditUtils::IsBreak(somenode)) {
                join = mHTMLEditor->IsVisBreak(somenode);
              }
            }

>@@ -2525,14 +2536,6 @@

>-        PRBool join = leftBlockParent == rightBlockParent;
>-        if (!join) {
>-          nsCOMPtr<nsINode> parent1 = do_QueryInterface(leftParent);
>-          nsCOMPtr<nsINode> parent2 = do_QueryInterface(rightParent);
>-          PRUint16 pos = nsContentUtils::ComparePosition(parent1, parent2);
>-          join = (pos & (nsIDOM3Node::DOCUMENT_POSITION_CONTAINS |
>-                         nsIDOM3Node::DOCUMENT_POSITION_CONTAINED_BY)) != 0;
>-        }

I haven't seen any explanation why bug 419217 comment 5 isn't a valid concern here. Minussing until I get an answer why it isn't.

>diff -r a954f52e3149 layout/generic/test/test_backspace_delete.xul

>+    if(node){

Space after if and before {

>+  todo_is(false, true, 'The above testDelete should use the same nodecallback' +
>+    'as in the proceeding setupTest: the cursor should stay at the end of "two", while currently it is at the beginning of "three" after delete');

Please wrap at 80 chars if possible (in this and other lines you add in this files).
Attachment #409810 - Flags: review?(peterv) → review-
(In reply to comment #2)
>                 join = mHTMLEditor->IsVisBreak(somenode);

Make that !mHTMLEditor->IsVisBreak(somenode).
(In reply to comment #2)
> I haven't seen any explanation why bug 419217 comment 5 isn't a valid concern
> here. Minussing until I get an answer why it isn't.
here are some quotes of comments in nsHTMLEditRules::JoinBlocks:

// do not try to merge table elements
...
// bail if both blocks the same
...
// Joining a list item to its parent is a NOP.
...
// special rule here: if we are trying to join list items, and they are in different lists,
// join the lists instead.
...
// normal case.  blocks are siblings, or at least close enough to siblings.  An example
// of the latter is a <p>paragraph</p><ul><li>one<li>two<li>three</ul>.  The first
// li and the p are not true siblings, but we still want to join them if you backspace
// from li into p.

so JoinBlocks actually works for cases which is close enough to sliblings. and it will do nothing in cases where it does not make sense. Plus, with this patch, all existing tests related to delete/backspace still pass.
Attached patch revised patch with test case (obsolete) — Splinter Review
revised patch according to review, added two new unit tests
Attachment #409810 - Attachment is obsolete: true
Attachment #413400 - Flags: review?(peterv)
(In reply to comment #4)
> so JoinBlocks actually works for cases which is close enough to sliblings. and
> it will do nothing in cases where it does not make sense.

Which says nothing about the case I pointed out, nodes which are not siblings at all, but in completely different subtrees? And it will definitely try to move the second block into the first in that case.
(In reply to comment #6)
> Which says nothing about the case I pointed out, nodes which are not siblings
> at all, but in completely different subtrees? And it will definitely try to
> move the second block into the first in that case.

could you give me an example of "nodes in different subtrees" which can be considered "adjacent" (when displayed on screen)? (I think JoinBlocks won't be called on two nodes if they are not "adjacent")
Ok, I understand now, it's the check for visible nodes that's supposed to save us. Note that my replacement code is wrong, it needs to be:


            if (join) {
              if (mHTMLEditor->IsTextNode(somenode)) {
                mHTMLEditor->IsVisTextNode(somenode, &join, PR_TRUE);
              }
              else {
                join = nsTextEditUtils::IsBreak(somenode) &&
                       !mHTMLEditor->IsVisBreak(somenode);
              }
            }

But this will change the behaviour again from the one added in bug 419217, identical blocks with the same parent (or contained in one another) won't be joined if there are any visible nodes in the selection. If you delete a selection starting before f and ending after g in

1) <p><div>abcdef</div><div>bar</div><div>ghi</div></p>
2) <p><div>abcdef</div><div>ghi</div></p>
3) <div>abcdef<div><div>bar</div>ghi</div></div>
4) <div>abcdef<div>ghi</div></div>

we'll join in 2 of the 4 cases. Why should there be any difference in these 4 cases?
(In reply to comment #8)
> we'll join in 2 of the 4 cases. Why should there be any difference in these 4
> cases?

I agree, all the cases should generate the same result, will attach another patch to fix this
fixes the issues mentioned in comment 8: if the original selection is not collapsed, always call JoinBlocks (because the selection touches two block nodes, so JoinBlocks should be called)

also added unit test coverage for all the cases in comment 8
Attachment #413400 - Attachment is obsolete: true
Attachment #413400 - Flags: review?(peterv)
Attachment #413691 - Flags: review?(peterv)
If Peter can't review this by the end of the week, I'll rubber-stamp it r+ myself so we can make progress.
I've started looking at the latest patch.
Is the patch supposed to fix the testcase in this bug (attachment 392180 [details])? Because it doesn't for me.
Comment on attachment 413691 [details] [diff] [review]
revised patch to cover more cases, with more test case

Unless I'm missing something this doesn't fix the testcase in this bug, and that testcase isn't added as a mochitest.
The logic is becoming very complex, I'm not sure I understand it. AFAICT you want to join if either the selection is not collapsed or nothing "visible" is deleted. If that's the case then maybe it would be clearer to make that last if check (!origCollapsed || join) and drop the setting of join to false where we delete text in the textnodes? I'm not sure it's equivalent, like I said the logic is hard to follow.
Why do you only care about the original value of bCollapsed, not the final value?
There are tabs in the patch.

Note that I'll be away next week, I'll try to review a new patch as soon as I get back.
Attachment #413691 - Flags: review?(peterv) → review-
this patch fixes the first test case attached to this ticket, and it added a unit test for it as well to mochitest

basically, the logic it implements to call JoinBlocks is this: if the original selection is collapsed, call JoinBlocks if the no visible elements are deleted. if the original selection is not collapsed, always call JoinBlocks

peter, let me know what do you think
Attachment #413691 - Attachment is obsolete: true
Attachment #426790 - Flags: review?(peterv)
re-upload the patch
Attachment #426790 - Attachment is obsolete: true
Attachment #426790 - Flags: review?(peterv)
Attachment #426791 - Flags: review?(peterv)
Comment on attachment 426791 [details] [diff] [review]
fixes the first test case, added a unit test for it 

I'm still trying to figure out the collapsing change. But I think it would make sense to set join to PR_TRUE at the start, and then check if (join && origCollapsed) before doing the check for visible nodes, instead of setting it to false first and then to true if !origCollapsed.
Attached patch revised patch with test case (obsolete) — Splinter Review
set join to true at the beginning, and check origCollapsed before doing check for visible nodes
Attachment #426791 - Attachment is obsolete: true
Attachment #430482 - Flags: review?(peterv)
Attachment #426791 - Flags: review?(peterv)
Comment on attachment 430482 [details] [diff] [review]
revised patch with test case

>diff --git a/editor/libeditor/html/nsHTMLEditRules.cpp b/editor/libeditor/html/nsHTMLEditRules.cpp

>+  PRBool bCollapsed, origCollapsed, join = PR_FALSE;

Declare origCollapsed where it's first initialized.


>+  // this is used later to determine whether we should join blocks
>+  // we don't really care about bCollapsed because it will be modified
>+  // by ExtendSelectionForDelete later. JoinBlocks should happen
>+  // if the original selection is collapsed and the cursor is at the
>+  // end of a block element, in which case ExtendSelectionForDelete 
>+  // would always make the selection not collapsed.
>+  origCollapsed = bCollapsed;

Start sentences with a capital.
Probably should end the first sentence with a period after "join blocks".
s/this/origCollapsed/

>+              else {
>+                join = nsTextEditUtils::IsBreak(somenode) && 
>+                  !mHTMLEditor->IsVisBreak(somenode);

Line up the ! to nsTextEditUtils.

>+  // if JoinBlocks is (not) called, eNext (ePrevious) action should 
>+  // collapse to end of endNode, while ePrevious (eNext) should 
>+  // collapse to start of startNode. This fixes first test case in 
>+  // Bug 507936

This needs a much better comment, it doesn't explain why we do this and I had to run through it in a debugger to try to understand what it does. Here's what I think  is happening, it would be good if you could confirm that, and write a comment that reflects it. If we're joining: if deleting forward the selection should be collapsed to the end of the selection, if deleting backward the selection should be collapsed to the beginning of the selection. But if we're not joining then the selection should collapse to the beginning of the selection if we're deleting forward, because the end of the selection will still be in the next block. And same thing for deleting backwards (selection should collapse to the end, because the beginning will still be in the first block).
Attachment #430482 - Flags: review?(peterv) → review+
Attachment #430482 - Attachment is obsolete: true
Attachment #431987 - Flags: review+
(In reply to comment #19)
> This needs a much better comment, it doesn't explain why we do this and I had
> to run through it in a debugger to try to understand what it does. Here's what
> I think  is happening, it would be good if you could confirm that, and write a
> comment that reflects it. If we're joining: if deleting forward the selection
> should be collapsed to the end of the selection, if deleting backward the
> selection should be collapsed to the beginning of the selection. But if we're
> not joining then the selection should collapse to the beginning of the
> selection if we're deleting forward, because the end of the selection will
> still be in the next block. And same thing for deleting backwards (selection
> should collapse to the end, because the beginning will still be in the first
> block).

what you described is exactly the logic. the comment is basically a copy of the above.
Keywords: checkin-needed
Whiteboard: [needs landing]
backed out changes to testDeleteNextWord in test cases, because it does not work in windows
Attachment #431987 - Attachment is obsolete: true
Attachment #432489 - Flags: review+
Keywords: checkin-needed
Whiteboard: [needs landing]
(In reply to comment #22)
> I checked this in, but I had to back it out due to test failures on Windows:
> 
> http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1268621590.1268623163.1374.gz

Looks like you never actually pushed the back out. I did so though:

http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=b88494638342&tochange=22073cb47ea9
Attachment #431987 - Flags: review+
Attachment #432489 - Flags: review+
http://hg.mozilla.org/mozilla-central/rev/7b0b38d70b2b
Status: NEW → RESOLVED
Closed: 14 years ago
Flags: in-testsuite+
Keywords: checkin-needed
Resolution: --- → FIXED
Whiteboard: [needs landing]
Target Milestone: --- → mozilla1.9.3a4
this was also commited to branch 1.9.3 (which will become Firefox 3.7) before backout due to test failures in windows

could someone recommit this patch to that branch? thanks
There's no 1.9.3 branch. 1.9.3 has yet to branch off from mozilla-central.
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: