Closed
Bug 507936
Opened 15 years ago
Closed 15 years ago
Should join node when deleting at end of paragraph
Categories
(Core :: DOM: Editor, defect)
Tracking
()
RESOLVED
FIXED
mozilla1.9.3a4
People
(Reporter: cpearce, Assigned: liucougar)
References
(Blocks 1 open bug)
Details
Attachments
(2 files, 7 obsolete files)
(deleted),
text/html
|
Details | |
(deleted),
patch
|
Details | Diff | Splinter Review |
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).
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 2•15 years ago
|
||
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-
Comment 3•15 years ago
|
||
(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.
revised patch according to review, added two new unit tests
Attachment #409810 -
Attachment is obsolete: true
Attachment #413400 -
Flags: review?(peterv)
Comment 6•15 years ago
|
||
(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")
Comment 8•15 years ago
|
||
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
Assignee | ||
Comment 10•15 years ago
|
||
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.
Comment 12•15 years ago
|
||
I've started looking at the latest patch.
Comment 13•15 years ago
|
||
Is the patch supposed to fix the testcase in this bug (attachment 392180 [details])? Because it doesn't for me.
Comment 14•15 years ago
|
||
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-
Assignee | ||
Comment 15•15 years ago
|
||
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)
Assignee | ||
Comment 16•15 years ago
|
||
re-upload the patch
Attachment #426790 -
Attachment is obsolete: true
Attachment #426790 -
Flags: review?(peterv)
Attachment #426791 -
Flags: review?(peterv)
Comment 17•15 years ago
|
||
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.
Assignee | ||
Comment 18•15 years ago
|
||
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 19•15 years ago
|
||
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+
Assignee | ||
Comment 20•15 years ago
|
||
Attachment #430482 -
Attachment is obsolete: true
Attachment #431987 -
Flags: review+
Assignee | ||
Comment 21•15 years ago
|
||
(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]
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
Keywords: checkin-needed
Whiteboard: [needs landing]
Assignee | ||
Comment 23•15 years ago
|
||
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]
Comment 24•15 years ago
|
||
(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
Thanks.
Updated•15 years ago
|
Attachment #431987 -
Flags: review+
Updated•15 years ago
|
Attachment #432489 -
Flags: review+
Comment 26•15 years ago
|
||
Status: NEW → RESOLVED
Closed: 15 years ago
Flags: in-testsuite+
Keywords: checkin-needed
Resolution: --- → FIXED
Whiteboard: [needs landing]
Target Milestone: --- → mozilla1.9.3a4
Assignee | ||
Comment 27•15 years ago
|
||
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
Comment 28•15 years ago
|
||
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.
Description
•