Closed Bug 202166 Opened 22 years ago Closed 22 years ago

Edit actions place caret on invalid position

Categories

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

Product:
Core
Component:
DOM: Editor
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla1.5alpha

People

(Reporter: KaiE, Assigned: KaiE)

References

Details

(Keywords: fixed1.4.1, Whiteboard: editorbase+)

Attachments

(3 files, 1 obsolete file)

testcase
191 bytes, text/html
Details
testcase 2
319 bytes, text/html
Details
Patch v4
6.32 KB, patch
mozeditor
: review+
kinmoz
: superreview+
mkaply
: approval1.4.1+
Details | Diff | Splinter Review
Open a document in composer that only has a single <hr> in its body. (see attached testcase) Do the following keyboard input: - enter (nothing happens) - cursor up (nothing happens) - hit DEL Actual behaviour: - <hr> is removed (fine) - cursor is placed in the off somewhere above the first line, caret now only partially visible
Attached file testcase —
Whiteboard: editorbase
Depends on: 201974
Depends on: 169213
If my memory serves me correctly, this has to do with the obligatory break tag disappearing from the body when the body is empty. When you open a new doc in composer, there is a break tag present in the body - removing it causes this behavior as well.
editorbase+
Priority: -- → P2
Whiteboard: editorbase → editorbase+
editorbase-. Hopefully will be fixed with general caret placement fixes.
Whiteboard: editorbase+ → editorbase-
I see additional actions that place the caret on such an invalid position. With the attached "testcase 2", we even crash with the new code in bug 200416. As a workaround, I will have to add self protectional code to bug 200416. I think it should not be possible to put the caret to such an invalid position. To see the problem yourself: - open "testcase 2" - hit DEL twice, cursor goes to invalid position and with patch v5 from bug 200416 applied - hit DEL another time, crash
Whiteboard: editorbase- → editorbase
Attached file testcase 2 —
Another way to reach to produce the invalid placement is a document that consists of: <body> foo </body> Select the complete word "foo" and hit backspace => caret on invalid position. This can be reproduced with all versions of Mozilla from 1.0 to 1.4 branch and latest trunk.
Attached patch Patch v2 (obsolete) — — Splinter Review
I think I have a working patch! (/me crosses fingers) I think the bug is in nsTextEditRules::CreateBogusNodeIfNeeded(). The function skips insertion of a bogus node as soon as (mEditor->IsMozEditorBogusNode(bodyChild) || mEditor->IsEditable(bodyChild)) is true. But I think it should ignore whitespace only text nodes! I'm attaching a patch that seems to fix it for the "only <hr> node" and "only foo text" cases, where one deletes the complete selection in one step.
The patch fixes the second testcase in this bug, too.
Attachment #124992 - Flags: review?(jfrancis)
It seems that IsEditable() returns true because the frame for the empty text nodes are marked as dirty because reflow has not been run yet. Perhaps we should make IsEditable() virtual so that nsHTMLEditor can override it so that it can use the ws code to determine if the textnode really is visible or not. I'm suggesting that we fix IsEditable() because i noticed that it gets called several times before the CreateBogusNodeIfNeeded() call, which means other parts of the code are getting the wrong answer.
editorbase+
Whiteboard: editorbase → editorbase+
Attachment #124992 - Attachment is obsolete: true
Attachment #124992 - Flags: review?(jfrancis)
Attached patch Patch v4 — — Splinter Review
New patch, based on Kin's comment in the bug and from a chat on IRC. I think it is reasonable to not make all of IsEditable virtual, but only the portion where it currently "guesses". This patch continues to guess in the simple editor world, but calls into the HTML editor when we are in HTML mode.
Attachment #125002 - Flags: superreview?(kin)
Comment on attachment 125002 [details] [diff] [review] Patch v4 The patch is along the lines of what I had in mind. Before sr'ing I'd like jfrancis to chime in on it, we were talking about this approach a couple of days ago and he was concerned about the possible perf impact of calling the WSRunObject code from IsEditable() since it's called so many times from so many different places ... though the fact that it only calls that code if the frame is marked as dirty makes me feel a little better.
Attachment #125002 - Flags: review?(jfrancis)
Comment on attachment 125002 [details] [diff] [review] Patch v4 looks ok to me. it would be nice to profile editor init and see if we spend tons of time in here when loading large documents.
Attachment #125002 - Flags: review?(jfrancis) → review+
Comment on attachment 125002 [details] [diff] [review] Patch v4 sr=kin@netscape.com assuming there's no significant negative perf impact.
Attachment #125002 - Flags: superreview?(kin) → superreview+
I'm not sure how you want me to test and what you consider significant. I have used a small stopwatch class (see also bug 208935) and printf statements to dump the amount of time spent in IsVisTextNode() when called from IsTextInDirtyFrameVisible(). In documents like the mozilla.org/start page, when I select a word in the middle and hit enter, it gets called about 1-5 times. All times were lower than measurable, that is smaller than 1 millisecond (on a 1 Mhz machine). When I edited the complex homepage from www.spiegel.de and used "insert table" somewhere in the middle of a paragraph, I saw up to 150 calls! Most were slower than measurable, with about 5 being reported as "1 millisecond". I'm unsure whether you think this is significant.
> I saw up to 150 calls! Most were slower than measurable Make that: ^FASTER Most calls were reported as 0 ms.
It seems editor is even faster with this patch. I added stopwatch code to nsEditor::Start/EndOperation. I made sure that I don't get hit by recursiveness by adding printf statements to both methods, and I saw the trace output on the console was always alternating. Editing www.spiegel.de I timed to insert a table and to mark everything, then time how long it takes to make everything bold. There were only a very small difference, where it looked like its faster with the patch applied. Next I loaded lxr.mozilla.org for file nsHTMLEditRules.cpp, selected all and made it bold. Everything took incredibly long. The "make bold" operation took 90393 with existing code, and 84822 ms with the patch applied. Unless you want more testing, I'll check it in.
fixed on trunk
Status: NEW → RESOLVED
Closed: 22 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.5alpha
Comment on attachment 125002 [details] [diff] [review] Patch v4 Requesting 1.4 branch approval for this editor correctness fix.
Attachment #125002 - Flags: approval1.0.x?
Attachment #125002 - Flags: approval1.0.x? → approval1.4?
It's fine with me if this goes in 1.4
Comment on attachment 125002 [details] [diff] [review] Patch v4 moving approval request forward.
Attachment #125002 - Flags: approval1.4? → approval1.4.x?
Comment on attachment 125002 [details] [diff] [review] Patch v4 a=mkaply for 1.4.1
Attachment #125002 - Flags: approval1.4.x? → approval1.4.x+
Please add the fixed1.4.1 keyword when this is checked in.
Flags: blocking1.4.x+
Landed on 1.4 branch for 1.4.1
Keywords: fixed1.4.1
Blocks: 224532
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: