200417 - backspace, enter keys have no visible impact

Status: RESOLVED FIXED

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

Description

[Dave Barrowman]

The attached sample seems to have content which confuses the backspace and enter
keys.

To reproduce:

1. open the attachment in the editor
2. place caret at the start of the last line ("AFTER")
3. Hit Backspace: preceding newline will be deleted and "AFTER" will move up
4. Hit Backspace again: "AFTER" stays put **This is the bug** (in some cases,
caret will move up, but "AFTER" never moves)

5. With caret still on the same line as "AFTER", hit Backspace again: preceding
newline will be deleted and "AFTER" will move up to the "FOOBAR More stuff." line
6. Hit Enter: "AFTER" moves down, as expected

7. Hit Enter again: nothing happens, "AFTER" stays put **This is the bug too**

Comment 1

[Dave Barrowman]

Attachment: sample showing backspace / enter bug

Comment 2

[Dave Barrowman]

Possibly related to bug 200418.  Also possibly related to bug 192320.

Comment 3

[Dave Barrowman]

I can reproduce on Linux build from 3/29/2003, so looks like it is NOT related
to the fix for bug 192320 (i.e., this bug existed before and after that fix landed).

Comment 4

[Kai Engert (:KaiE:)]

Confirming, this is not caused by that other checkin.
I can reproduce this bug even with Mozilla 1.0

Comment 5

[Kai Engert (:KaiE:)]

-> me

Comment 6

[Joe Francis]

This bug is because the code that merges the paragraphs on delete is not
detecting the invisible <br> at the end of the upper paragraph.  A trailing br
in a block has no effect, but of course when the blocks are merged it is no
longer trailing.  I have to detect this and delete it.  

Comment 7

[Kevin McCluskey (gone)]

editorbase+

Comment 8

[Kai Engert (:KaiE:)]

I retested this bug. I see a small improvement.

The initial comment reports two bugs, step 4 and step 7.
I still see the bug in step 4, but step 7 works with the latest trunk.

Comment 9

[Kai Engert (:KaiE:)]

Attachment: Patch v1

Joe, your assessment was correct. This patch fixes the problem.

Comment 10

[Kai Engert (:KaiE:)]

While working on this bug, I found bug 204017, which uses a similar testcase,
but without the disturbing whitespace, which this bug cares for. However, bug
204017 is seen with both testcases.

Comment 11

[chris hofmann]

is the patch looking good for this?  are we trying to land for 1.4 final or 1.5

Comment 12

[Kai Engert (:KaiE:)]

Setting target milestone to 1.4 beta, because I don't see a 1.4 final milestone.

Comment 13

[Kai Engert (:KaiE:)]

Just for completeness: In comment 10 I talked about the wrong bug number, what I
meant was bug 204016 - however that bug is invalid.

Comment 14

[Kai Engert (:KaiE:)]

Yesterday I talked with Joe about this bug and the patch, who gave me very
helpful review comments.

To summarize: He thinks the approach is right, but we should improve the patch
to cover more scenarios. Patch v1 would not help if there are several
consecutive text nodes. Joe suggests to use nsWSRunObject and the
Start/EndReasonNodes it provides.

Joe was concerned the routine might now be slower because of using
nsWSRunObject, but it would be ok if it doesn't get called too often for any
given editor action. I think that is the case, since only JoinBlocks calls it.
It doesn't look like JoinBlocks is called more than once for an action.

I'm attaching a new patch that implements the suggestion. It fixes the bug for
me, too.

The idea is: Construct an nsWSRunObject using the after-the-last-node position
in the given block. Internally that object will search for the nodes that
separate the whitespace runs. A break is a whitespace run. mStartNode terminates
the run looking backwards. If the start reason is eBreak, it means, there is are
no visible nodes between the input position and the found start node, meaning
the break node at the end is invisble, because it is at the end of the block.

The patch also changes the logic for searching backwards from a given position,
using the same strategy.

Comment 15

[Kai Engert (:KaiE:)]

Attachment: Patch v2

Comment 16

[Joe Francis]

Comment on attachment 122572 [details] [diff] [review]
Patch v2

looks good.

Comment 17

[Charles Rosendahl]

Looking at a test build, I see minor 'jumping' in the edited text as the two
paragraphs in the test case are merged into a single paragraph during the
backspace.  I don't see any issues using the enter key.

The jumping is minor compared to the existing behaviour.

Comment 18

[Kai Engert (:KaiE:)]

The "jumping" you mention is not introduced by this patch.

You can see the same jumping with a trunk build, without the patch.

Comment 19

[Kai Engert (:KaiE:)]

Comment on attachment 122572 [details] [diff] [review]
Patch v2

Requesting approval for 1.4 final checkin, for this editor behaviour
correctness fix.

Comment 20

[(not reading, please use [e-mail] instead)]

adding some QA, so they know to keep an eye out for 1.4 regressions.

Comment 21

[(not reading, please use [e-mail] instead)]

Comment on attachment 122572 [details] [diff] [review]
Patch v2

a=sspitzer

one comment:

+  nsCOMPtr<nsIDOMNode> aTestNode;
+  PRInt32 aTestOffset = 0;

the aFoo notation is for arguments, not locals.  

see http://www.mozilla.org/hacking/mozilla-style-guide.html#Visual

Comment 22

[Kai Engert (:KaiE:)]

Attachment: Patch v2 - variables renamed - this one gets checked in

According to Seth's request, I have renamed the variables.
No other changes in this patch.

Comment 23

[Kai Engert (:KaiE:)]

fixed on trunk