Unable to move cursor to a bullet item

VERIFIED FIXED in mozilla1.0

Status

()

P1
normal
VERIFIED FIXED
17 years ago
17 years ago

People

(Reporter: lchiang, Assigned: mozeditor)

Tracking

Trunk
mozilla1.0
x86
Windows NT
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

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

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

17 years ago
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.
(Reporter)

Comment 1

17 years ago
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.
(Reporter)

Comment 2

17 years ago
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
(Reporter)

Updated

17 years ago
Whiteboard: EDITORBASE

Comment 3

17 years ago
--> joe
Assignee: kin → jfrancis
(Assignee)

Comment 4

17 years ago
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

Updated

17 years ago
Keywords: nsbeta1 → nsbeta1+
Whiteboard: EDITORBASE → EDITORBASE+
Priority: -- → P1
Target Milestone: --- → mozilla1.0
(Assignee)

Comment 5

17 years ago
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.
(Assignee)

Comment 6

17 years ago
Created attachment 78519 [details] [diff] [review]
patch to libeditor
(Assignee)

Updated

17 years ago
Whiteboard: EDITORBASE+ → EDITORBASE+; fixinhand; need r=,sr=,a=,adt=,e=mc**2,f=ma,...
(Assignee)

Comment 7

17 years ago
KIN: having mail woes here.  this is the patch I would like you to look at
wednesday morning if you get the chance.  

Comment 8

17 years ago
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.

(Assignee)

Comment 9

17 years ago
Created attachment 78625 [details] [diff] [review]
upgraded patch.  this one goes to 11

Kin, this fixes the probs we discussed.
Attachment #78519 - Attachment is obsolete: true

Updated

17 years ago
Attachment #78625 - Flags: superreview+

Comment 10

17 years ago
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.

Updated

17 years ago
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+

Comment 13

17 years ago
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.
(Assignee)

Comment 14

17 years ago
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]

Updated

17 years ago
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

Updated

17 years ago
Blocks: 132837
(Assignee)

Comment 16

17 years ago
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
Last Resolved: 17 years ago
Resolution: --- → FIXED
Sujay: Can you verify the fix on the trunk? Thanks.

Comment 19

17 years ago
adding adt1.0.0 nomination.  Pluease update the bug with testing results when
you have them.
Keywords: adt1.0.0

Comment 20

17 years ago
verified in 4/16 trunk build.
Status: RESOLVED → VERIFIED

Comment 21

17 years ago
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!
(Assignee)

Comment 23

17 years ago
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.

(Assignee)

Comment 24

17 years ago
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+
(Assignee)

Updated

17 years ago
Keywords: adt1.0.0+, approval → fixed1.0.0
Whiteboard: EDITORBASE+; fixed on trunk; need a=,[adt2] → EDITORBASE+; fixed on trunk; fixed on branch; [adt2]

Comment 27

17 years ago
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.