Closed Bug 135337 Opened 24 years ago Closed 24 years ago

Unable to move cursor to a bullet item

Categories

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

Product:
Core
Component:
DOM: Editor
Platform:
x86
Windows NT
Type:
defect

Tracking

()

Status:
VERIFIED FIXED
Milestone:
mozilla1.0

People

(Reporter: lchiang, Assigned: mozeditor)

References

Details

(Whiteboard: EDITORBASE+; fixed on trunk; fixed on branch; [adt2])

Attachments

(1 file, 1 obsolete file)

upgraded patch. this one goes to 11
11.61 KB, patch
glazou
: review+
kinmoz
: superreview+
shaver
: approval+
Details | Diff | Splinter Review
Unable to move cursor to a bullet item In editing a document I have, I inserted a new bullet item at the beginning of a list. I am unable to use the cursor or mouse to move my caret into that bullet item so that I can begin typing. Here is the HTML of the text: <html> <head> <title></title> <meta http-equiv="content-type" content="text/html; charset=ISO-8859-1"> <meta name="author" content="Lisa Chiang"> </head> <body> <h4> <font face="Arial,Helvetica"><font size="-1">Lowlights (from All Projects):</font></font></h4> <ul> <li> </li> <li><font face="Arial,Helvetica"><font size="-1">ProjectA and ProjectB Smoketest Blocker Bugs (and other blocker bugs):</font></font></li> </ul> </body> </html> Here is what I did: 1. I am editing my status report. I ran into this problem. (I can't paste my status report to open source bug system so I just copied the problem section into a new compose window.) 2. In that new compose window, I paste the text. 3. Confirm that I cannot move the caret into that bullet item so I can start typing there. 4. Save the document 5. Open it again, and I am able to move my caret there. It is strange that I cannot move my cursor into that bullet until I save.
I also want to note: 1. My document with the bullets was created in Communicator 4.7 and I'm editing it in the trunk builds now. 2. I insert a new bullet item into the top of the list. 3. This is when I run into the problem described above.
I'd like to nominate for nsbeta1. Editing bullets is a common task and the user should not have to spend time "cleaning up" empty bullet items. I have to go back to Communicator 4.7 to do this.
Keywords: nsbeta1
Whiteboard: EDITORBASE
--> joe
Assignee: kin → jfrancis
I bet this won't be too hard to fix. If the answer is that we are not checking paste contents for empty blocks, then fixing that may slow down paste. But probably not enough to matter. I can probably recycle some rules code to touch up the paste contents *before* we actually paste them; that will minimize the performance hit, since adding br's to a doc frag is less expensive then adding them into the real document. cc'ing kin since I try to keep him in the loop on html paste issues.
Status: NEW → ASSIGNED
Keywords: nsbeta1 → nsbeta1+
Whiteboard: EDITORBASE → EDITORBASE+
Priority: -- → P1
Target Milestone: --- → mozilla1.0
This wasn't what I thought it was at first. I finally had to bite the bullet and make nsHTMLEditor::IsEmptyNode() call my whitespace handling code to determine the visibility of whitespace. While in here I also fixed some otherprobs with IsEmptynode(). This also fixes 136504.
Attached patch patch to libeditor (obsolete) — — Splinter Review
Whiteboard: EDITORBASE+ → EDITORBASE+; fixinhand; need r=,sr=,a=,adt=,e=mc**2,f=ma,...
KIN: having mail woes here. this is the patch I would like you to look at wednesday morning if you get the chance.
also when this bug is fixed verify these steps...this is from bug 84485: 1. open a new composer window 2. insert a list -- 1st list item enter text -- 2nd list item enter text 3. drag a link to the end of the 2nd list item, hit enter 4. while the caret is in the 3rd list item, press the left arrow so the caret moves to the end of the 2nd list item 5. hit return, a new blank list item will be inserted, continue to hit return, additional list items are inserted and the list does not terminate. Additional information: -- doing this same test without the link at the end of the 2nd bullet does not produce the same effect, the list is terminated.
Kin, this fixes the probs we discussed.
Attachment #78519 - Attachment is obsolete: true
Attachment #78625 - Flags: superreview+
Comment on attachment 78625 [details] [diff] [review] upgraded patch. this one goes to 11 sr=kin@netscape.com - In IsVisTextNode(), does the code in the |if (aSafeToAskFrames)| block handle empty textnodes? - Fix indentation of comment in the |if (aSafeToAskFrames)| block, it seems outdented by 2 spaces in the diff.
Whiteboard: EDITORBASE+; fixinhand; need r=,sr=,a=,adt=,e=mc**2,f=ma,... → EDITORBASE+; fixinhand; need r=,a=,adt=,e=mc**2,f=ma,...
Comment on attachment 78625 [details] [diff] [review] upgraded patch. this one goes to 11 >+/////////////////////////////////////////////////////////////////////////// >+// IsVisTextNode: figure out if textnode aTextNode has any visible content. >+// >+nsresult >+nsHTMLEditor::IsVisTextNode( nsIDOMNode *aNode, >+ PRBool *outIsEmptyNode, >+ PRBool aSafeToAskFrames) Comment deals with |aTextNode| instead of |aNode|. >- if (nsTextEditUtils::NodeIsType(child, NS_LITERAL_STRING("a")) || >- nsTextEditUtils::NodeIsType(child, NS_LITERAL_STRING("b")) || >+ if (nsTextEditUtils::NodeIsType(child, NS_LITERAL_STRING("b")) || > nsTextEditUtils::NodeIsType(child, NS_LITERAL_STRING("i")) || I don't understand this change. Why did you remove anchors from this list ?
Comment on attachment 78625 [details] [diff] [review] upgraded patch. this one goes to 11 r=glazman, after explanations provided by jfrancis and kin in #composer
Attachment #78625 - Flags: review+
Just so it's documented in the bug ... Joe removed anchors because the method in question only gets called when adding a new empty list item. He thought, and I agree, that if a list item ended in an anchor, and the caret was at the end of that list item, and the user hit return, that most of the time they don't want the anchor to continue in the next list item. If the user hit return in the middle of the anchor, splitting the list item will still preserve the anchor in both list items.
requesting adt approval. This also fixes 132837, 136504, and a side issue from 84485. Big win for list editing with styles and/or links.
Keywords: adt1.0.0
Whiteboard: EDITORBASE+; fixinhand; need r=,a=,adt=,e=mc**2,f=ma,... → EDITORBASE+; fixinhand; need r=,a=,[adt2]
Whiteboard: EDITORBASE+; fixinhand; need r=,a=,[adt2] → EDITORBASE+; fixinhand; need a=,[adt2]
Remove adt1.0.0. Let's get this one on the trunk first, let it bake, let Sujay take a good look at it, then renominate for inclusion in the 1.0 branch.
Keywords: adt1.0.0 → approval
Blocks: 132837
fixed on trunk
Whiteboard: EDITORBASE+; fixinhand; need a=,[adt2] → EDITORBASE+; fixed on trunk; need a=,[adt2]
Resolving as FIXED. Please add fixed1.0.0 keyword after the fix has been checked into the 1.0.0 branch.
Status: ASSIGNED → RESOLVED
Closed: 24 years ago
Resolution: --- → FIXED
Sujay: Can you verify the fix on the trunk? Thanks.
adding adt1.0.0 nomination. Pluease update the bug with testing results when you have them.
Keywords: adt1.0.0
verified in 4/16 trunk build.
Status: RESOLVED → VERIFIED
What areas are affected and what else needs to be tested besides this actual bug?
Sujay/Joe - Can you pls respond to Michael's last question? We'd liek to take this before Friday, but we want to make sure we have minimized all risks. thanks!
Risk evaluation: This bug fix has two pieces. One is a change to terminate links when you begin a new list item. This change was trivial (removing "a" tag from list of tags we want to preserve across new list items) and zero risk. The other piece is not trivial and involves a change to an important piece of editor infrastructure: IsEmptyNode(). This routine's task is to determine if a node is "practically" empty. Nodes in a document may appear empty to a user when in fact they still have content. The editor needs to figure out when a node appears empty in order to respond to user actions in a way the user expects. {Example: if the user is in a list item that appears empty, and hits backspace, they expect the list item to be deleted. They do NOT expect some invisible whitespace inside that list item to be deleted instead.} As it happens it is difficult to determine if whitespace is visible. Last year I wrote a class for tackling this issue: nsWSRunObject. It is used ni a variety of laces in editor. But it was not used in IsEmptyNode(), which really was an oversight. This bug exposed the need for nsWSRunObject to be used from IsEmptyNode(). While doing this I also noted some inconsistencies in IsEmptyNode() which I fixed via some cleanup and refactoring. Now IsEmptyNode uses nsWSRunObject to determine the visibility of whitespace, and it does this consistently (before it had two slightly different code paths for dealing with text). The risk is medium. I would characterise it as a fairly straightforward change to a critical piece of editor infrastructure. Hope this helps.
Oh, another data point on the risk/reward curve: This fix also fixes bug 132837, which is adt1.0.0+.
adt1.0.0+ (on ADT's behalf) approval for checkin into 1.0. Pls check this into the trunk, once you have drivers approval, and add fixed1.0.0 keyword.
Keywords: adt1.0.0 → adt1.0.0+
Comment on attachment 78625 [details] [diff] [review] upgraded patch. this one goes to 11 a=shaver for 1.0 branch.
Attachment #78625 - Flags: approval+
Keywords: adt1.0.0+, approval → fixed1.0.0
Whiteboard: EDITORBASE+; fixed on trunk; need a=,[adt2] → EDITORBASE+; fixed on trunk; fixed on branch; [adt2]
verified in 4/25 branch build.
Keywords: verified1.0.0
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: