Closed Bug 201974 Opened 22 years ago Closed 22 years ago

Caret partially/completely visible with initial <p> </p>

Categories

(Core :: DOM: Editor, defect)

Product:
Core
Component:
DOM: Editor
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED

People

(Reporter: KaiE, Assigned: KaiE)

References

(Blocks 1 open bug)

Details

(Keywords: topembed+, Whiteboard: editorbase, edt_x3)

Attachments

(2 files, 2 obsolete files)

testcase, open in composer
(deleted), text/html
Details
Patch v2
(deleted), patch
KaiE
: review+
sfraser_bugs
: superreview+
Details | Diff | Splinter Review
Prepare a HTML document with a plain text editor. Make the HTML document contain this: <body> <p> </p> x </body> Open the file in composer. Expected behaviour ================== I think the editor should display two visible lines. A blank line at the top. A second line showing the "x". The caret should be placed on top of the file, on the blank line. Actual behaviour ================ There is no initial blank line shown. The first fully visible line contains the "x". The caret is only halfways visible, placed "in the off" above the "x"! This misplacement is the bug. It can be shown the caret is really placed in the off: Type cursor down. The cursor goes down and is now in front of the x. Type cursor up and nothing happens. The cursor does not go back to "the off". Slightly modified testcase with even worse behaviour ==================================================== Modify the initial HTML document, remove the space between <p> and </p>, i.e.: <body> <p></p> x </body> When you open this document editor, the caret is not visible at all!
Attached file testcase, open in composer (deleted) —
Nominating
Keywords: topembed
Whiteboard: editorbase
After asking on irc, it seems the rendered display is correct, only the caret placement is incorrect.
I'll try to change nsHTMLEditor::BeginningOfDocument() to skip any empty initial <p> elements.
Discussed in edt. Plussing. Seen in embedding customer product. Please make sure fix gets rolled back to their branches.
Keywords: topembed → topembed+
Whiteboard: editorbase → editorbase,edt_x3
Attached patch Patch v 0.1 (obsolete) (deleted) — — Splinter Review
First attempt of a patch. This patch fixes the testcase. I have not yet done much additional testing. The idea of the bug is: nsWSRunObject terminates its search for more whitespace as soon as it finds a block node. However, as in our example, a block can be completely invisble. This patch extends the handling for block elements. When a block element is found, it does another check whether the block contains anything visible. If not, it skips the block and continues searching for visible things. While this patch seems to work at first sight, there are three things I would like to check more, and I'd be happy to get advice from you. First, I'm making use of the special return value "eThisBlock", I'm using it as the indicator whether the search for more whitespace failed. Is this really correct? Second, I need to understand better whether end.SetPoint(nextNode,1); is indeed the correct way to skip the block element. Third, I think we should do the same handling when searching for previous whitespace? If you agree, additional code for the backwards direction must be written.
Assigning to myself, since I seem to be close to a fix.
Assignee: jfrancis → kaie
Attachment #120488 - Flags: review-
This patch is not the right approach. Although the empty block may not be visible, it still does end any whitespace run before it (and start any run after it, and contain any run in it, etc). So we don't want to change the definition of the whitespace run in this case. Also, note that some blocks are visible even if they are empty (for instance: td, li). I think the correct appraoch here is to write a new routine which makes use of the existing tools (NextVisibleNode, IsEmptyBlock) to traverse from a point and find the next piece of visible content. The nsWSRUnObject::NextVisibleNode method is perhaps misnamed since it really will find the next visible content, or the end of the whitespace run, whichever it encounters first.
Even better would be if we could simply have a method on selection that let us get back the translated node/offset that corresponds to the first visible frame. In general we cant take the approach of asking layout about frmaes data in the editor, since the editor often needs to figure out things on the fly when the dom has been touched by editor but not yet reflowed. However the case of placing selection at the front of the document is a special case: when we want that we are never in the middle of an operation (I think).
Attached patch Patch v1 (obsolete) (deleted) — — Splinter Review
Thanks a lot for your comments, very helpful! With your explanation, I agree we must not change the current general behaviour of the WSRunObject. I like your suggestion to make use of IsEmptyBlock, I had not seen that method before. I also like your suggestion to use special handling for the BeginningOfDocument case (you are suggesting that indirectly, by expressing your hope, that selection/layout frames will be "in synch" in the special case when calling BeginningOfDocument). But your second proposal to query selection still sounds more risky, since we might not be able to predict all situations when BeginningOfDocument gets called? Therefore I'd like to try a simpler approach first. I'm attaching a new patch that also seems to work in the simple cases I have tested so far. My simple test included a document that starts with a table, having whitespace only in its first cell. This works with and without the patch. The idea of the new patch is, and hopefully my assumptions are correct: - only modify the BeginningOfDocument function, since that is the only area we currently see the problem - when nsWSRunObject terminated its search for whitespace, because it found a block boundary, it returns eOtherBlock (I could not find other situations where it would return eOtherBlock) - if BeginningOfDocument of NextVisibleNode detects the return value eOtherBlock, it will test whether the returned node is empty - if the node returned with eOtherBlock is empty, BeginningOfDocument will skip the block and continue the search
Attachment #120488 - Attachment is obsolete: true
Attachment #120594 - Flags: review?(jfrancis)
I found another way to make the editor place the caret on the invalid position. That other bug 202166 is not fixed by this patch (as expected). However, the logic of the fix we choose here could get reused for bug 202166.
Blocks: 202166
Comment on attachment 120594 [details] [diff] [review] Patch v1 This looks like a good solution. One thing to try would be to make a document that has an hr as the first element inside the body, and see how this code works then. I'm worried that we will try to put the selection "inside" the <hr>. This would be a bug with the existing code rather than introduced by your patch, but we shoudl still fix it while we are here. You may want to make your else clause something like "else if (IsContainer(visNode))", and then have a final else that skips over the node if it isn't a container.
Attachment #120594 - Flags: review?(jfrancis) → review+
You are right. With a <hr> only document, the selection is indeed logically inside the <hr>, and typing doesn't work. The patch doesn't fix it, as you suspect. I tried your suggestion (if I have understood correctly), but now the position is placed after the <hr>! I extended the patch to handle this case specially, and to not continue looping, and not skip the object, but instead place the caret in front of that object. However, this exposes a new bug, most likely in GetNodeLocation? If we try to obtain the location of the initial only <hr> node, and place the caret at that position, the caret again appears only half visible! But as we discussed on IRC, this seems to be a separate bug. We'll do the modification anyways, since it doesn't make the situation worse and only affects the special <hr> only document, and by making the change we have a testcase for the other problem.
Attached patch Patch v2 (deleted) — — Splinter Review
Addressed comment as discussed on IRC.
Attachment #120594 - Attachment is obsolete: true
Comment on attachment 120647 [details] [diff] [review] Patch v2 carrying forward review
Attachment #120647 - Flags: review+
Comment on attachment 120647 [details] [diff] [review] Patch v2 Peter, can you please review?
Attachment #120647 - Flags: superreview?(peterv)
Blocks: 202191
Attachment #120647 - Flags: superreview?(peterv) → superreview+
fixed on trunk
Status: NEW → RESOLVED
Closed: 22 years ago
Resolution: --- → FIXED
using today's build (2003.04.23) with the attached test case, this is what i see after i load it in composer: a. there's no blank line at the top, only the "x" --expected per comment 3. b. the caret is to the left of the "x" (again, expected). c. hitting the right or down arrow will move the caret to the right of the "x" (expected). d. hitting the left arrow (when the caret is to the right of the "x") will move the caret to the left of the "x" (expected). e. hitting the up arrow (when the caret is to the right of the "x") does NOT move the caret to the left of the "x", as i would expect. is there another bug covering (e)? if not, should i reopen this one or spin off another bug?
Blocks: PhtN6
Whiteboard: editorbase,edt_x3 → editorbase, edt_x3
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: