Closed Bug 200416 Opened 22 years ago Closed 12 years ago

deleting newline causes content to change order

Categories

(Core :: DOM: Editor, defect, P2)

Product:
Core
Component:
DOM: Editor
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla1.5beta

People

(Reporter: barrowma, Assigned: ayg)

References

(Depends on 1 open bug)

Details

(Keywords: topembed+, Whiteboard: EDITORBASE+, edt_b3, edt_c3, edt_x3)

Attachments

(3 files, 10 obsolete files)

sample showing weird deleting behavior
545 bytes, text/html
Details
Patch v13, passes tests
20.37 KB, patch
ehsan.akhgari
: review+
Details | Diff | Splinter Review
Test
1.64 KB, patch
ehsan.akhgari
: review+
Details | Diff | Splinter Review
1. Open the attachment in the editor. 2. Place the caret at the start of the 2nd line of text (to the immediate left of the 'I' in "Intro"). 3. Hit Backspace Expected: I expect the newline to go away and the word "Intro" to be moved to the end of the previous line. Observed: The word "Intro" jumps to the 1st line, pushing the 1st line down. I am using trunk build from 4/2/2003, id: 2003040209.
Whiteboard: EDITORBASE
Possibly related to bug 192320.
I can reproduce on Linux build from 03/29/2003, so apparently NOT related to the fix for bug 192320.
Confirming, this is not caused by that other checkin. I can reproduce this bug even with Mozilla 1.0
-> me
Assignee: jfrancis → kaie
Keywords: topembed → topembed+
Whiteboard: EDITORBASE → EDITORBASE, edt_b3, edt_c3, edt_x3
This is caused by having a block level element (<p>) inside an inline (<font>). This situation leads to strange behavior from the editor, because it tries to satisfy the strict dtd for all editor actions. Allowing the editor to break those rules will lead to undesirable side effects.
Depends on: 202437
EDITORBASE+. Need to investigate common cases of invalid markup causing problems.
Priority: -- → P2
Whiteboard: EDITORBASE, edt_b3, edt_c3, edt_x3 → EDITORBASE+, edt_b3, edt_c3, edt_x3
Kai, this looks related to bug 141019 which I posted a patch for back in 09/2002 but we held off on it because we were hoping to tackle the problem of handling block-in-inline situations during reflow. You might want to see if the patch in that bug fixes this problem too?
Kin, unfortunately the patch from bug 141019 does not fix this bug, and in general doesn't seem to work anymore. See also the comment I added there.
It seems the issue is simpler than we thought, I think I found a very general bug in the JoinBlocks code, if one block contains the other. The code calls a helper function IsDescendantOf. That function returns the offset (position) of the input node into a parent node. However, the caller makes the incorrect assumption this offset will refer to the input parent node, but in this bug the offset actually refers to a node between the input parent node and the input test node. The bug is, the caller makes the incorrect assumption and uses the offset with the original input parent node. This is an incorrect position. The solution is to extend the helper function IsDescendantOf, to make it return the complete pair that is required to indicate the found position, and change the caller to use that correct position instead.
Attached patch Patch v1 (obsolete) — — Splinter Review
Suggested fix.
Attached patch Patch v1 (obsolete) — — Splinter Review
Same patch, larger context in diff. Note that while I changed the parameter list of the function, I removed parameters from two callers that are obviously not using the output data, and by not passing in the out parameter pointer, we save some cycles.
Attachment #122161 - Attachment is obsolete: true
Attachment #122162 - Flags: review?(jfrancis)
Attachment #122162 - Attachment is obsolete: true
Attachment #122162 - Flags: review?(jfrancis)
Attached patch Patch v2 (obsolete) — — Splinter Review
Updated patch
Attachment #122177 - Flags: review?(jfrancis)
are we trying to get this in for 1.4final or 1.5? lets update target milestone..
Attachment #122177 - Flags: review?(jfrancis)
Setting target milestone to 1.4 beta, because I don't see a 1.4 final milestone.
Target Milestone: --- → mozilla1.4beta
Comment on attachment 122177 [details] [diff] [review] Patch v2 This patch is wrong, thanks to Joe for pointing that out. It only worked caused I was lucky. My success during testing stopped me from reviewing my own code too early.
Attachment #122177 - Attachment is obsolete: true
Attached patch Experimental Patch v3 (obsolete) — — Splinter Review
This code is what I had originally intended. It is different from what Joe suggested yesterday (on IRC), but I would like to understand, whether this might still make sense, and if not, why. Let me illustrate what we have and the strategy of this experimental patch: We have <body> <-- left block <inline> text 1 <block> <-- right block text 2 The code tries to move "right block" to a different place. When the code looks for the destination position of the move, it will always use a position that uses an offset in combination with the "left block", which is "<top>" in this case. I believe this strategy is wrong. The current code ignores the fact that there can be more than one nesting level between "left block" and "right block". It appears more logically to me, to simply move the "right block" exactly one level up, instead of moving it directly below the "left block". This patch will use the position of "right block" as the destination position for the new content, the idea is to produce: <body> <inline> text 1 text 2 It seems to work. Does that code make sense?
Comment on attachment 122580 [details] [diff] [review] Experimental Patch v3 Joe, what do you think about this approach?
Attachment #122580 - Flags: review?(jfrancis)
When I said simply move the "right block" exactly one level up I of course meant move the CONTENTS of "right block" exactly one level up
Kai, the strategy you discuss is more elegant from a coding viewpoint than the strategy we use now, but in my experience it does not correspond with user expectations. The reason for this is that users perceive documents "horizontally". When you type backspace, they expect a block to be joined to what came "before", rather than what came "above". Example: <body> <div> <h1> Section One </h1> Here is the text to section one of my document. </div> <div> <h1> Section Two </h1> Here is the text to section two of my document. </div> </body> Consider putting the caret at the front of: <h1> Section Two </h1> and typeing backspace. Under your plan we would get rid of the heading, but not move the "Section Two" text out of it's current div. But users expect the backspace to join that text to the earlier text: the text in the first div. The editor used to work more like the way you propose. Since I changed it, I have received far fewer complaints about how blocks behave when backspacing or forward-deleting over block boundaries, so I think we should stick with the strategy we have now. That strategy is to try to join blocks with what came before, rather than what came above (of course, sometimes those two are the same).
Attachment #122580 - Attachment is obsolete: true
Attachment #122580 - Flags: review?(jfrancis)
Joe, I agree with your comments.
Attached patch Patch v5 (obsolete) — — Splinter Review
This patch looks scary, but it isn't. Most of the patch is a change that doesn't affect the logic at all. I'm moving the "obtain parent block" into the JoinBlocks method, and because the variable is no longer a pointer, a lot of code changes from "*variable" to "variable". This is necessary, because inside the JoinBlocks method, we need access to the original node, not only to its parent. There is only a single place in the patch that contains the real change. Search for @@ -2487,7 +2476,27 @@ It contains a comment and the new logic.
Attachment #122698 - Flags: review?(jfrancis)
Target Milestone: mozilla1.4beta → mozilla1.4final
QA Contact: sairuh → beppe
Comment on attachment 122698 [details] [diff] [review] Patch v5 This looks good. Have you tested it with a case in which there are multiple levels of inlines between the blocks? I don't forsee a problem there but it would be good to make sure it all works as expected.
Attachment #122698 - Flags: review?(jfrancis) → review+
Joe, > Have you tested it with a case in which there are multiple > levels of inlines between the blocks? the original reported case was <font>Text1<p><font>Text2></font><p></font> and now I tested <big><font>Text1<p><font><small><font>Text2 </font></small></font></p></font></big> Both appear to behave correctly.
Attachment #122698 - Flags: superreview?(sfraser)
Attachment #122698 - Flags: superreview?(sfraser) → superreview+
OS: Windows XP → All
Hardware: PC → All
Charles found a crash that is seen with this patch and "testcase 2" in bug 202166. The problem is: It is possible to use edit actions to place the caret at an invalid position. See bug 202166 comment 5. When you are placed at an invalid position, and you hit DEL, the code in the patch in this bug tries to split the style above the point. However, when the caret is at the invalid position, the "point above" might be the top HTML node, and the DOM code will abort when an attempt is made to insert additional nodes to do the split. I wonder which of the following we should do: 1) add an updated patch to this bug to add some self protection 2) add self protection code to SplitStyleAbovePoint, and make it return "success", to make callers continue their execution 3) add self protection code to SplitStyleAbovePoint, and make it return "failure", to make callers stop their execution 4) make sure bug 202166 gets fixed first, and make sure no action can place the caret on the invalid position - although this might be tricky to guarantee
Instead of the call to SplitStyleAbovePoint, I used the following fragment, which makes the crash with the testcase 2 in bug 202166 go away. if (!nsTextEditUtils::NodeIsType(p, NS_LITERAL_STRING("html"))) { res = mHTMLEditor->SplitStyleAbovePoint(address_of(p), &offset, nsnull, nsnull, nsnull, nsnull); if (NS_FAILED(res)) return res; }
The crash is unrelated to 202166. When joining blocks, one of the blocks may be the body. We should never try to split style above the body, so something like the check in your prior comment is needed (there is an IsRootNode() helper you can use to avoid having to declare a string). One thing I don't understand is why the split is being done above aLeftNode in this case (when leftBlock contqains rightBlock). Shouldn't we be splitting above aRightBlock instead? Also, I don't understand why SplitStyleAbovePoint() was ever splitting the body node at all. It should only be splitting inline style nodes. There is something else wrong here...
Although we have a reviewed patch, I'd like to move this patch to 1.5 alpha, because of the risks we are seeing.
Target Milestone: mozilla1.4final → mozilla1.5alpha
Comment on attachment 122698 [details] [diff] [review] Patch v5 Marking as obsolete until we know the patch is really correct. I fear it's not.
Attachment #122698 - Attachment is obsolete: true
After digging into the meaning of SplitStylesAbovePoint more, after more thinking and testing, I think the current approach is not yet correct, and I'm even somewhat confused. Joe, you said, when backspacing between content, we need to split the styles, because we don't want the content below the caret to receive the style of the content above the caret. Are you sure? What about this: <font style="color: red"> left red<br> <p> right, no style </p> </font> The text "right, no style" is displayed in red. If we split the style prior to moving, we'd get (well, not with the current patch): <font style="color: red"> left red<br> </font> right, no style <font style="color: red"> </font> Is that what you suggested? In this case, the second line is no longer displayed in red! I think that's not what I'd expect.
I'm more confused. <font style="color: red; background-color: rgb(0, 255, 255);"> left red<br> <p> right, no style </p> </font> In this example, both strings are displayed with a color of red, but only the first string has the defined background color. Is that a side effect of the invalid HTML (the <p> inside <font>) ? If we can't predict what happens to styles with that invalid HTML (and that is what this bug is about), does it make sense at all to try to care for styles?
I have no problem with the paragraph losing the style here. I think getting the block legally contained in another block is more important. Then future editing on that block (including setting the style, if the user so desires) will be more likely to work correctly.
Joe, if I understand correctly, you say you're ok with a patch that is identical to v5, with the only difference of having the call to SplitStyleAbovePoint removed. I'll attach that patch tomorrow.
Attached patch Patch v7 (obsolete) — — Splinter Review
This patch is nearly identical to v5, the single change is: Removed the call to SplitStyleAbovePoint.
Attachment #124515 - Flags: review?(jfrancis)
Comment on attachment 124515 [details] [diff] [review] Patch v7 I found another case that makes me attach yet another revision of this patch. Load attachment 123082 [details] from bug 202166 in the editor. Hit DEL twice. The caret is now at an invalid position. When you now hit DEL again, the existing code (without the patch) magically fixes the situation. With the patch, we cause more breakage and actually produce text placed after the body close tag. So I looked what happens, when you hit DEL the third time, without having applied the patch. In the call to JoinBlocks, the LeftNode parameter is the root element! I traced more, and we actually end up at the same MoveBlock call that I'm changing. But for some reason, the existing code in MoveBlock seems to be able to deal with this special situation correctly. I have not tried to understand why. Because of that, I'm wrapping my new code with a test against IsRootNode. Only if it's not the root node, we will try to use the new logic.
Attachment #124515 - Attachment is obsolete: true
Attachment #124515 - Flags: review?(jfrancis)
Attached patch Patch v8 (obsolete) — — Splinter Review
Same as before, added: + if (mHTMLEditor->IsRootNode(aLeftNode)) + { + // Continue to use the old style logic, that magically works correctly + // when the caret is placed at an invalid position, + // indicated by aLeftNode being the root node... + res = MoveBlock(aLeftBlock, aRightBlock, leftOffset, rightOffset); + } + else
Attachment #124523 - Flags: review?(jfrancis)
I don't think this is going to work. I will prepare a testcase to illustrate the problem, but first I want to go look at yor other review requests and knock off any easy ones.
A couple of comments: 1) Instead of checking to see if aLeftNode is the root node, check to see if aLeftNode==leftBlock, and if so use leftBlock as the param to MoveBlock() instead of parent of aLeftNode. This will prvent incorrect results if we ever get aLeftNode==leftBlock, but not the root node. 2) We have to split the style. Otherwise if you have this: <p><b>bold para</b></p> <p>unbold para</p> when you backspace at front of second p, it will become bold. Style should not change when blocks are merged unless the style is part of the nature of the block (like font size and weight in headings). For an even worse example you an <a> instead of a <b>. So I still think we should follow my orginal idea of splitting any inline style at the spot we are moving rightBlock to, and then moving rightBlock into LeftBlock at the place where the style splitting terminated in leftBlock.
Attachment #124523 - Flags: review?(jfrancis) → review-
> We have to split the style. Otherwise if you have this: > <p><b>bold para</b></p> > <p>unbold para</p> > when you backspace at front of second p, it will become bold. No, it does not become bold. :-) I just tested the patch with both examples (b and a). The separate styles continue to be separate.
Attached patch Patch v10 (obsolete) — — Splinter Review
> 1) > Instead of checking to see if aLeftNode is the root node, check to see if > aLeftNode==leftBlock, and if so use leftBlock as the param to MoveBlock() > instead of parent of aLeftNode. This will prvent incorrect results if we ever > get aLeftNode==leftBlock, but not the root node. This suggestion works pretty well, thanks. Attaching new patch with this change only.
Attachment #124523 - Attachment is obsolete: true
Comment on attachment 126499 [details] [diff] [review] Patch v10 I am re-requesting review, because I do not see the problem you mention. The second block does not acquire the style of the first block. It seems the call to SplitStyle is not necessary, unless you know a reproducable case that shows unwanted style merging.
Attachment #126499 - Flags: review?(jfrancis)
Comment on attachment 126499 [details] [diff] [review] Patch v10 in non-css mode editor, try this source: <body background=""> <b>line1</b> <p>line2</p> </body> In css mode editor, try this source: <body> <span style="font-weight: bold;">line 1</span> <p>line 2</p> </body> In either case backspacing from front of line2 causes line2 to become bold.
Attachment #126499 - Flags: review?(jfrancis) → review-
I can't put into words how much your comment confuses me. Ironically said, I have the impression we are talking about different Mozilla projects! I have tried: - mozilla 1.4 in css mode - mozilla 1.4 in non-css mode - mozilla latest trunk in css mode - mozilla latest trunk in non-css mode I have tried each of the above with both examples given in comment 42. That means, I have tried 8 combinations. I did not see any style merging in any combination. Text "line 1" always remained bold, and text "line2" always remained non-bold. I have one last idea. Maybe this is a platform dependent problem? I did all my tests on Linux. I can try on Windows or Mac, maybe that will behave differently.
Mozilla 1.4, Mac OS X. Again, "line 2" does not get bold when backspacing to join the lines.
I have now tested the 8 combinations on Windows 2000, 1.4/trunk, css/non-css, testcase/testcase2, and still I don't see what you see.
Target Milestone: mozilla1.5alpha → mozilla1.5beta
Don't ask me why it didn't work earlier, but now I finally see what you report! With the patch applied, the second line turns bold on backspace. Thanks to Joe and Kathy for review comments and testing.
Attached patch Patch v11 (obsolete) — — Splinter Review
Attachment #126499 - Attachment is obsolete: true
Comment on attachment 127407 [details] [diff] [review] Patch v11 As usual, I think the new patch handles all cases described above correctly :-) Please review.
Attachment #127407 - Flags: review?(jfrancis)
It's not likely that I will work on editor/selection bugs in the near future. Mass assining my bugs to nobody.
Assignee: kaie → nobody
snarfing kaie's old bugs
Assignee: nobody → mozeditor
snarfing kaie's old bugs
Assignee: mozeditor → kaie
Assignee: kaie → mozeditor
QA Contact: rubydoo123 → bugzilla
Comment on attachment 127407 [details] [diff] [review] Patch v11 brade: could you review this?
Attachment #127407 - Flags: review?(mozeditor) → review?(brade)
Comment on attachment 127407 [details] [diff] [review] Patch v11 + // We try to work as good as possible with HTML that's already invalid. The above line should change "good" to "well". note: the only difference between patch 11 and patch 5 which had r/sr= (besides offset numbers) is in this block @@ -2626,7 +2653,57 @@. (@@ -2487,7 +2476,27 @@ in patch v5) I haven't done any testing with this patch (at least not in 9+ months). r=brade (although I wish Joe or Kin could look at this too)
Attachment #127407 - Flags: superreview?(kinmoz)
Don't count my r= from patch 5, it had problems. Before getting an r+, my testcase in comment 42 must be passed.
QA Contact: bugzilla → editor
Assignee: mozeditor → nobody
Ehsan, any value in this bug still?
Sigh... Yeah we still have the bug but the patch is obviously rotten throughout the years. Aryeh, can you see if you can revive the patch? I think the basic idea of the patch should still apply.
Attachment #127407 - Flags: superreview?(kinmoz)
Attachment #127407 - Flags: review?(brade)
Patch seems to apply okay after some fixups -- shows you how much attention this code has seen! Also seems to achieve the desired effect. It just remains to be seen whether it causes any test failures.
Assignee: nobody → ayg
Status: NEW → ASSIGNED
I ported to current tip and fixed style issues on touched lines. This basically works, but it causes a whole bunch of failures of exactly the sort it seeks to prevent, e.g., 218 ERROR TEST-UNEXPECTED-FAIL | /tests/dom/imptests/editing/conformancetest/test_runtest.html | [["defaultparagraphseparator","div"],["delete",""]] "foo[<div><p>]bar</p>baz</div>" compare innerHTML; assert_equals: Unexpected innerHTML (after normalizing inline style) expected "foobar<div>baz</div>" but got "foo<div>baz</div>bar" The spec has a (I think!) good algorithm here, namely repeatedly "split the parent" of the nodes to move until their parent is correct. The "split the parent" algorithm depends fairly heavily on other parts of the spec, though, so it's not so practical to import. Let me see if I can figure out what's going on.
Attachment #127407 - Attachment is obsolete: true
There we go -- there was a slight bug in the patch that I fixed. I admit I don't fully understand it, but it seems to have the desired effect, and also passes tests. The minimal test-case to reproduce is: <span>foo<p>bar</p></span> Place the cursor before "bar" and hit delete. I'll add this test to the upstream editing tests and submit a separate patch to sync, so we have test coverage -- this patch by itself has no regression test. Try: https://tbpl.mozilla.org/?tree=Try&rev=c0184a42d035 When reviewing, it helps to realize that the only functional changes should be a) moving some code from two callers of JoinBlocks into the beginning of the function, and b) adding some extra special-casing near the end of JoinBlocks. Everything else is just because of -nsHTMLEditRules::JoinBlocks(nsCOMPtr<nsIDOMNode> *aLeftBlock, - nsCOMPtr<nsIDOMNode> *aRightBlock, +nsHTMLEditRules::JoinBlocks(nsIDOMNode *aLeftNode, + nsIDOMNode *aRightNode, which necessitates the removal of a lot of *'s and the addition of some address_of()s. If there are any functional changes there, it's probably a bug. (If I were writing the patch from scratch, I'd have separated that out into a separate patch, but it's probably easier for you to just review it as-is than for me to separate it out at this point.)
Attachment #684943 - Attachment is obsolete: true
Attachment #684944 - Flags: review?(ehsan)
So it looks like the spec is broken for this specific test-case, too, so I can't add a usable test upstream (since the expected result would be wrong). I'll write a separate mochitest.
Comment on attachment 684944 [details] [diff] [review] Patch v13, passes tests Review of attachment 684944 [details] [diff] [review]: ----------------------------------------------------------------- ::: editor/libeditor/html/nsHTMLEditRules.cpp @@ +2503,5 @@ > bool *aCanceled) > { > + NS_ENSURE_ARG_POINTER(aLeftNode && aRightNode); > + > + nsCOMPtr<nsIDOMNode> aLeftBlock, aRightBlock; I'd prefer that we don't use the 'a' prefix for non-arguments. But I can do that myself when landing your patch.
Attachment #684944 - Flags: review?(ehsan) → review+
(In reply to :Aryeh Gregor from comment #60) > So it looks like the spec is broken for this specific test-case, too, so I > can't add a usable test upstream (since the expected result would be wrong). > I'll write a separate mochitest. OK, so let's wait for the test before landing this then. Thanks!
Attached patch Test — — Splinter Review
The exact innerHTML we produce here is still garbage, although in this case it looks okay: <span>foo</span>bar<span></span> It most likely should be: <span>foobar</span> But the spec is even worse: <span>foo<br></span>bar No idea how the <br> gets there. Should I file a follow-up on fixing the exact HTML, pending a spec change (which I don't expect to make in the foreseeable future)?
Attachment #688213 - Flags: review?(ehsan)
Yes, please.
Attachment #688213 - Flags: review?(ehsan) → review+
https://hg.mozilla.org/mozilla-central/rev/8b86519c08a0 https://hg.mozilla.org/mozilla-central/rev/5d9989bb4d2f
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: