Closed Bug 207183 Opened 21 years ago Closed 21 years ago

Smiley gets deleted, editor tries to manipulate whitespace in the -moz-user-select:all block

Categories

(Core :: DOM: Editor, defect)

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

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla1.5alpha

People

(Reporter: KaiE, Assigned: KaiE)

References

Details

(Keywords: fixed1.4.1, Whiteboard: [adt2] editorbase)

Attachments

(3 files, 5 obsolete files)

test case 1 - type a letter after smiley
85 bytes, text/html
Details
test case 2 - hit backspace at end of document
86 bytes, text/html
Details
Patch v3b
13.16 KB, patch
KaiE
: review+
KaiE
: superreview+
mkaply
: approval1.4.1+
Details | Diff | Splinter Review 
This bug is seen *only* on the initial attempt in a Mozilla session.
If you need to test again, you need to restart Mozilla!

Seen while testing bug 118912. The patch in bug 118912 seems to have caused this
new behaviour, but that change seems necessary.


To reproduce:
- start Mozilla
- open a new mail message compose window
- type some chars, e.g. "wsfijf"
- use smiley toolbar icon to insert the topmost smiley
- type any letter, e.g. "x"

Actual behaviour:
  The smiley gets deleted

Expected behaviour:
  The smiley should not get deleted
This bug is most likely caused by the selection being inside the span when the
smiley is inserted. 

Probably the code that inserts the smiley places the selection beyond the span,
but then later one (in the processing handled by
nsHTMLEditRules:AfterEditInner()) the selection is getting pulled back into the
span (in the case where the smiley is at the end of it's block, as is often the
case when you insert it as you type).

The selection adjusting code needs to make sure that it does not cross
-moz-user-select: all boundaries when adjusting the selection.

I don't know why you only see this the first try.  It's probably something silly
like the additioan of a trailing br once you do the delete step.  This would
then give the selection somewhere happy to be other than in the span the next
time you try.
> I don't know why you only see this the first try.  It's probably something silly
> like the additioan of a trailing br once you do the delete step.  This would
> then give the selection somewhere happy to be other than in the span the next
> time you try.

I think you misunderstood. It's not happening each time I open a mail compose
window, it's only happening the first time I open a mail compose window!

If it were something like an additional <br>, I think I should be able to
reproduce each time I open a new mail composer window, but I don't :-)

At times I see the bug, we are constructing a nsWSRunObject which decides
  mEndReason == eThisBlock (128)
and ww get a WS run of
  eTrailingWS
which causes nsWSRunObject::InsertText
to call DeleteChars to delete the trailing space after the smiley.
However, all of the smiley gets deleted.
(GetNextWSNode in GetWSNodes returns NULL)

At times it works correctly, we get
  mEndReason == eBreak (32)
and we get a WS run of
  eNormalWS
which only causes nsWSRunObject::InsertText
to do some NBSP checking.


When I do
  "open new mail message, type x, insert smiley, send mail"
the first time after having started Mozilla, I get
  <body text="#000000" bgcolor="#ffffff">
  x<span class="moz-smiley-s1"><span> :-) </span></span>
  </body>

and at any later time I get
  <body text="#000000" bgcolor="#ffffff">
  x<span class="moz-smiley-s1"><span> :-) </span></span><br>
  </body>


One could argue, it is a bug that we don't get the trailing <br> on the initial
attempt in the application session.

But I think it's not good we rely on the trailing <br> so much.


What would the real fix be?
  "Do not try to delete the trailing space at the end of a block, 
   when the space is part of a moz-select-all block"
?
Whiteboard: editorbase
adt: nsbeta1+/adt2
Keywords: nsbeta1+
Whiteboard: editorbase → [adt2] editorbase
Confirming bug on Mac OS X with 1.4 branch, setting platform to ALL.
OS: Windows 2000 → All
There is another variation of this regression, probably also because the editor
code is trying to clean up whitespace.

I'm not yet sure whether I'll file a separate bug for this other regression.,
for now I'll not.

To reproduce the other regression:
- use any document, new or existing
- place caret at location of choice
- enter char a
- enter a smiley
- enter char b
- hit backspace

Expected behaviour:
  only "b" should get deleted

Actual behaviour:
  both "b" and the smiley left to it get deleted

Testcase 1.

Open in editor.
Place caret after smiley.
Type any letter.
Smiley gets deleted.
test case 2

- open in editor
- place caret at end of document
- hit backspace
- both "b" and smiley get deleted
By now it's clear this bug caused by the editor's logic attempt to clean up the
whitespace surrounding the smiley, when that whitespace appears to be insignificat.
However, that surrounding whitespace is part of the smiley atom by use of the
-moz-user-select property, and deleting it deletes all of the smiley.

For testcase 1, the code is deleting a space.
For testcase 2, the code is trying to convert a space into a NBSP, but again, it
does so by trying to delete a space, which results in deleting all of the smiley.


I'm not yet sure what the ideal fix for this bug should be.

My only idea so far is: When deleting, never clean up insignificant whitespace
that is contained in select-all blocks.

I'm attaching a patch that fixes both testcases.
Attached patch Patch v1 - diff -w (obsolete) — — Splinter Review 

    
    

    
  

      
      
      
      

        Attached patch
          
          
        Patch v1 - full patch (obsolete)
        — 
          — Splinter Review
      

    


  

    
  

        Attachment #124738 -
        Flags: review?(jfrancis)

    
  

        Attachment #124738 -
        Attachment is obsolete: true

        Attachment #124738 -
        Flags: review?(jfrancis)

    
    

    
  

      
      
      
      

        Attached patch
          
          
        Patch v2 - diff -w (obsolete)
        — 
          — Splinter Review
      

    


  
Fixed copy&paste error in patch.

        Attachment #124737 -
        Attachment is obsolete: true

    
    

    
  

      
      
      
      

        Attached patch
          
          
        Patch v2 - full patch (obsolete)
        — 
          — Splinter Review
      

    


  

    
  

        Attachment #124741 -
        Flags: review?(jfrancis)

    
    

    
  
I mentioned this to kaie on IRC, but thought I'd mention it here ... the fact
that it only happens on the initial open of a mail window, is probably due to
the fact that mail caches the MailCompose window ... and when you initially
bring it up, it is blank and the edit rules probably have the mBogusNode pointer
set ... when you close it and bring it up again, I believe the mail app just
does a select all and delete to clear the window out ... which does *not*
restore the mBogusNode.

I suggested to kaie that you may be able to reproduce the problem in new mail
compose windows by leaving the old one up, forcing mail to create a new window
instead of using the cached version.

    
    

    
  
I took a peek at the patch, and it seems to work ok. What I'm wondering is if we
should only skip the various deletes when we cross a boundary from editable to
non-editable ... for example when we are inserting text into an editable region
of the doc but the ws code crosses into a non-editable region when looking for
whitespace to delete, as is  the case in this bug ... but do we want to allow
someone to edit the contents underneath a -moz-user-select:all subtree
programatically with the editor? If so, this patch will prevent the ws run
object from doing it's magic.

    
    

    
  
Kin:

> but do we want to allow
> someone to edit the contents underneath a -moz-user-select:all subtree
> programatically with the editor? 

Daniel says: Yes, we do, in templates.


> If so, this patch will prevent the ws run
> object from doing it's magic.

I'm not sure I understand correctly, so I must state what I assume you are saying.

I assume you are saying: "When we manipulate text inside a contents underneath a
-moz-user-select:all subtree, we still want the nsWSRunObject to do it's magic".

I assume you mean, when manipulating text, you want whitespace cleanup to happen.

But, whenever the nsWSRunObject tries to do "it's magic" and cleanup whitespace,
won't it automatically always delete the whole object?

From my current knowledge I would have assumed: Any attempt to manipulate the
whitespace inside such an protected block is futile. One must remove the
grouping (done with moz-select:all) before the intended action can succeeded.

Please help me to undestand better :-)


    
    

    
  
Changing to better summary.

Summary: Initial attempt in session only: Typing any char deletes smiley → Smiley gets deleted, editor tries to manipulate whitespace in the -moz-user-select:all block

    
    

    
  
> I assume you are saying: "When we manipulate text inside a contents
> underneath a -moz-user-select:all subtree, we still want the nsWSRunObject
> to do it's magic".
>
> I assume you mean, when manipulating text, you want whitespace cleanup to
> happen.


Right, I was wondering if we should allow editing operations that happen
*entirely* underneath a -moz-user-select:all subtree to proceed as normal,
whitespace cleanup and all. How that could be accomplished is still up for
dicsucssion.


> But, whenever the nsWSRunObject tries to do "it's magic" and cleanup
> whitespace, won't it automatically always delete the whole object?


It may with the current implementation.


> From my current knowledge I would have assumed: Any attempt to manipulate the
> whitespace inside such an protected block is futile. One must remove the
> grouping (done with moz-select:all) before the intended action can succeeded.


A while back we had a meeting where we discussed -moz-user-select:all and it's
current use ... if I remember correctly we came up with the conclusion that
-moz-user-select:all wasn't the same as not-editable ... that is
-moz-user-select:all basically meant that user gestures (clicks, user initiated
edits, etc) would treat the subtree as a unit and it if it wasn't marked as
not-editable, you should be able to edit the subtree.

I'm curious what jfrancis has to say about all this. I was just flagging the
issue for discussion because it seems like we'll run into bugs like this more
and more, and if we keep putting checks, like we do in the patch, at the lowest
levels, we may never be able to edit these subtrees without actually removing
the -moz-user-select:all style as you mentioned above.

    
    

    
  
I think this patch is exactly the right idea.  One thing I would like to see is
for a bool to be added to DeleteChars() that indicates if we need to check for
select-all style.  That way we have the code that checks in only one place,
which is easier to maintain and less cluttered.

    
    

    
  

      
      
      
      

        Attached patch
          
          
        Patch v3 (obsolete)
        — 
          — Splinter Review
      

    


  
implemented jfrancis' suggestion

        Attachment #124740 -
        Attachment is obsolete: true

        Attachment #124741 -
        Attachment is obsolete: true

    
  

        Attachment #124741 -
        Flags: review?(jfrancis)

    
  

        Attachment #124885 -
        Flags: review?(jfrancis)

    
    

    
  
Comment on attachment 124885 [details] [diff] [review]
Patch v3

looks good!

        Attachment #124885 -
        Flags: review?(jfrancis) → review+

    
    

    
  
Comment on attachment 124885 [details] [diff] [review]
Patch v3

sr=kin@netscape.com

==== I'd rename |ar_outsideSelectAll| to |ar_outsideUserSelectAll| or
|ar_outsideUserSelectAllSubtree|.

        Attachment #124885 -
        Flags: superreview+

    
    

    
  
Comment on attachment 124885 [details] [diff] [review]
Patch v3

Also as a sidenote, I think most of the enums under the  editor directory try
to stick to  using 'k' or 'e' prefixes.

    
    

    
  

      
      
      
      

        Attached patch
          
          
        Patch v3b
        — 
          — Splinter Review
      

    


  
New patch, with the only change:
- Renamed ar_outsideSelectAll to eOutsideUserSelectAll
- Renamed ar_anywhere to eAnywhere

        Attachment #124885 -
        Attachment is obsolete: true

    
    

    
  
Comment on attachment 125319 [details] [diff] [review]
Patch v3b

carrying forward r=jfrancis and sr=kin

        Attachment #125319 -
        Flags: superreview+

        Attachment #125319 -
        Flags: review+

    
    

    
  
fixed on trunk
Status: NEW → RESOLVED
Closed: 21 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla1.5alpha

    
    

    
  
Comment on attachment 125319 [details] [diff] [review]
Patch v3b

requesting 1.4 branch approval for this editor correctness fix

        Attachment #125319 -
        Flags: approval1.4?

    
    

    
  
This should go on 1.4 if possible.  There has been interest from embedders over
the -moz-user-select:all style, and honoring it correctly in editor.  

    
    

    
  
This problem was also reported in bug 211082.

    
  
Blocks: stable1.4

    
    

    
  
Comment on attachment 125319 [details] [diff] [review]
Patch v3b

moving approval request forward.

        Attachment #125319 -
        Flags: approval1.4? → approval1.4.x?

    
    

    
  
Comment on attachment 125319 [details] [diff] [review]
Patch v3b

a=mkaply

        Attachment #125319 -
        Flags: approval1.4.x? → approval1.4.x+

    
    

    
  
Please add the fixed1.4.1 keyword when this is checked in.
Flags: blocking1.4.x+

    
    

    
  
Checked in to 1.4 branch for 1.4.1

Keywords: fixed1.4.1

    
  
Blocks: 224532

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






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: