Closed Bug 135337 Opened 22 years ago Closed 22 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: 22 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: