Closed Bug 71883 Opened 23 years ago Closed 23 years ago

caret jumps to front of text after adding tags in head save

Categories

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

Product:
Core
Component:
DOM: Editor
Type:
defect

Tracking

()

Status:
VERIFIED FIXED
Milestone:
mozilla0.9

People

(Reporter: sujay, Assigned: cmanske)

References

Details

(Whiteboard: [QAHP])

Attachments

(5 files)

Fix.
549 bytes, patch
Details | Diff | Splinter Review
Updated fix to include resetting focus after save doc commands
1.58 KB, patch
Details | Diff | Splinter Review
Prevent "InsertElementTxn" from reseting selection when not in the body
899 bytes, patch
Details | Diff | Splinter Review
New editor interface: InsertElementInHead, so we can suppress changing the selection
9.46 KB, patch
Details | Diff | Splinter Review
Another strategy: Expose method to set "don't change selection flag" and avoid new interface method
7.42 KB, patch
Details | Diff | Splinter Review 
using 3/12 build of netscape

1) launch netscape
2) launch composer
3) enter text "helloworld"
4) click Save button
5) give it title + file name
6) click OK in Save panel

after doing this notice the caret jumps to front of text in document.
It should stay at end of the text where you left caret.
You don't have to do step 4; you can instead go to the Format menu and set the 
title.  In fact, setting adding any tags in the head seems to cause this problem 
(I see this when adding a description as well).

Reassign to cmanske since he maybe knows what's going on.
Assignee: beppe → cmanske
setting out to mozilla1.0
Priority: -- → P4
Target Milestone: --- → mozilla1.0
Simple and a "must fix" problem: We need to add:
    window._content.focus();
at the end of the Page Properties command.
This bug exists in NS6.0 release, but there the caret is missing when focus
is lost. Does this reveal a problem in Composer content window? I.e., when
we don't have focus, the caret is restored, but you can't type. Since we
obviously must catch all cases where we should force focus back to the content
window, which is dumb IMHO, the apearance of the caret masks the fact that
focus wasn't really reset. Of course the fact that you can't type tells you that
focus has been lost.
Severity: normal → critical
Status: NEW → ASSIGNED
Target Milestone: mozilla1.0 → mozilla0.9
Attached patch Fix. — — Splinter Review 

    
    

    
  
Note that the last command in the patch is the "nsPagePropertiesCommand"
which launches the dialog. Kathy's observation revealed the same bug for this
as well as the save commands, so both cases should be tested.


Summary: caret jumps to front of text after initial save → Focus is lost (caret jumps to front of text) after saving or using Page Properties dialog
Whiteboard: FIX IN HAND need r=, sr=

    
    

    
  
I tested the 2nd patch (3/15/01 10:44) but that doesn't fix the problem at all.
Remove status whiteboard.

Whiteboard: FIX IN HAND need r=, sr=

    
    

    
  
change summary to be more correct; reducing severity since there is no data loss.

Note: none of the other dialogs have a problem with the caret moving to another 
part of the file.

Severity: critical → normal
Summary: Focus is lost (caret jumps to front of text) after saving or using Page Properties dialog → caret jumps to front of text after adding tags in head
                                       save

    
    

    
  
The fix is necessary, but not sufficient.
These changes are required to shift focus back to the content window, but
apparently there's a change in the DOM manipulation code such that the selection
is changed when we alter nodes in the <head>.
I'll have to preserve the selection in the SetDocumentTitleTxn::Do() method.

Summary: caret jumps to front of text after adding tags in head
                                       save → caret jumps to front of text after adding tags in head                                       save

    
  
Whiteboard: [QAHP]

    
    

    
  
Note that the changes to ComposerCommands.js were checked in previously -- that
fixed the "loosing focus" problem.
Patch on 4/4 fixes the "change selection" problem by not doing that unless the
element we are appending a child element on is in the <body>

Keywords: patch, 
          
            review
Whiteboard: [QAHP] → FIX IN HAND need r=, sr=

    
    

    
  
Regarding the 04/04/01 17:31 patch ...

InsertElementTxn is supposed to be generic so 
we're not supposed to introduce any HTML dependencies 
like nsHTMLEditUtils::InBody() into it.

I thought that when you jfrancis and I talked we decided that the right thing to 
do was to use:

    nsAutoTxnsConserveSelection dontSpazMySelection(this);

in the nsHTMLEditor call that initiates the creation of the transactions that 
modify the HEAD region of the document? Did that not work for some reason? Note 
that using nsAutoTxnsConserveSelection should force bAdjustSelection to be false 
so there should be no need for your InBody() call.

    
  
Whiteboard: FIX IN HAND need r=, sr= → [QAHP] FIX IN HAND need r=, sr=

    
    

    
  
But that will only work for the SetDocumentTitleTxn. When we edit other elements
in the <head> (mostly <meta> elements), JS is simply calling
nsEditorShell.InsertElement, which calls nsIEditor::InsertNode().
So you would favor adding a new interface such as InsertHeadElement() so we can
wrap the InsertNode transaction?


    
    

    
  
unfortunately, kin is right.  we cant add html dependencies into 
InsertElementTxn.  :-C

I think making a more specific htmlEditor call, like you suggest, is the way to 
go.


    
    

    
  
Ok. The comment above was cmanske, not beppe, bty. I forgot to
logoff beppe first since I'm on her computer today.


    
    

    
  
Joe: can you explain *why* we collapse selection at inserted element? We have 
a method InsertElementAtSelection, which implies a connection to selection,
but InsertNode transaction takes a parent element, so it doesn't seem correct to
me that it should touch the selection.


    
    

    
  
It is historical.  Steve Clark niitially wrote it this way.  There is old code in 
editor (most of it not in use now due to overrides in the rules code) that 
depends on this behavior.  We would have to do a difficult sweep throughout 
editor to remove this dependency.  


    
    

    
  
Removing status, keywords since 4/4/01 patch is not the correct fix.

Keywords: patch, 
          
            review
Whiteboard: [QAHP] FIX IN HAND need r=, sr= → [QAHP]

    
    

    
  
Seems like mjudge is best choices for review.
Kin: can you look at this for sr=?

Keywords: patch, 
          
            review
Whiteboard: [QAHP] → [QAHP] FIX IN HAND need r=, sr=

    
    

    
  
What is this change:

     // We can encounter "__moz_text" nodes here -- must find a row
     while (nextRow && !IsRowNode(nextRow))
     {
-      nsCOMPtr<nsIDOMNode> nextNode;
       res = nextRow->GetNextSibling(getter_AddRefs(nextNode));

?

sr=sfraser on the rest.


    
    

    
  
Good catch! Meant to comment on that. It removes my only C++ compile warning,
because nextNode was defined above the "while" loop. I checked carefully that
it was ok to reuse the object.


    
  
Whiteboard: [QAHP] FIX IN HAND need r=, sr= → [QAHP] FIX IN HAND need r=

    
    

    
  
r=jfrancis
don't need the changes to nsIEditor.h, which is going away.  But it won't hurt 
either.


    
    

    
  
Joe is right: nsIEditor.h change is not needed -- not checked in.
Everything else checked in.
Status: ASSIGNED → RESOLVED
Closed: 23 years ago
Keywords: patch, 
          
            review
Resolution: --- → FIXED
Whiteboard: [QAHP] FIX IN HAND need r= → [QAHP]

    
    

    
  
verified on all platofrms(0419 builds). caret now behaves..
Status: RESOLVED → VERIFIED

    
  
Blocks: 77421

          You need to log in
          before you can comment on or make changes to this bug.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: