Open Bug 205350 Opened 21 years ago Updated 2 years ago

DEL key does nothing at <br>|<br> in front of bulleted list

Categories

(Core :: DOM: Editor, defect)

Product:
Core
Component:
DOM: Editor
Type:
defect

Tracking

()

Milestone:
mozilla1.5beta

People

(Reporter: KaiE, Unassigned)

Details

(Keywords: topembed+, Whiteboard: editorbase+, edt_x3)

Attachments

(2 files, 7 obsolete files)

testcase
284 bytes, text/html
Details
Patch v18
3.83 KB, patch
mozeditor
: review+
kinmoz
: superreview-
Details | Diff | Splinter Review 
<body>
a<br>
<br>
<ul>
  <li>b</li>
</ul>
</body>

Place caret on the empty line between a and b.
Hit the DEL key.

Actual result:
  Tothing happens

Expected result:
  To see the expected behaviour, place caret in front of "b" 
  and hit backspace:

  - The empty line should get deleted.
  - the list item should no longer be a list item
  - "b" should move up
Keywords: topembed
Whiteboard: editorbase, edt_x3
Hmm. I'm unsure about the "expected behaviour".

Maybe the expected behaviour should not be identical with the backspace
behaviour while positioned on the next element.


Maybe it's ok to delete the line end and position the caret in front of "b".

I'm not 100% sure when we say a <br> is visible or not.

But we seem to say (as seen in bug 200417), a <br> at the end of a block is NOT
visible.


We have nsHTMLEditor::IsVisBreak. This routine currently says, if there is a BR
in front of a BR, the second BR is not visible. The code does not check for the
end-of-block condition.


I suggest we remove the code that makes this simple incorrect decision, and let
the nsWSRunObject code handle the situation.
Attached patch Patch v1 (obsolete) — — Splinter Review 

    
  

        Attachment #123051 -
        Flags: review?(jfrancis)

    
    

    
  
sigh :-)
I accidentially wrote the word "not" at the wrong place in the previous comment :-)
Let me try again:


We seem to say (as seen in bug 200417), a <br> at the end of a block is NOT
visible.

But nsHTMLEditor::IsVisBreak() says, if there is another BR
in front of a BR, the second BR *is visible*. The existing code does not check
whether the second BR is at the end of a block before making the conclusion.


The patch removes the incorrect short circuit conclusion.


    
    

    
  

      
      
      
      

        Attached file
          
        testcase
        — 
      

    


  
Open attachment and edit document (ctrl+e)
Place caret on second line and press delete key.

Expected:  list item deletes and character appears on second line.

    
    

    
  
Comment on attachment 123051 [details] [diff] [review]
Patch v1

Do we need "priorNode" anymore?
Joe or Kin needs to review this one.

    
    

    
  
I don't think this is the right thing to do.  The way to think about whether a
br is visible, is to ask: if I removed this br, would the document look different?

In the case of: <block>text<br></block>, the answer is no.
In the case of: <block>text<br><br></block>, the answer is yes (remove either br
and a blank line disappears).

The deletion code, upon finding out that a visible <br> is ahead of the
selection, should remove that <br> when forward deleting.  So I think the
IsVisBreak() code is right and the deletion code that calls it is doing the
wrong thing.  

I say this without having stepped through the deletion code in question.


    
  

        Attachment #123051 -
        Flags: review?(jfrancis) → review-

    
    

    
  

      
      
      
      

        Attached patch
          
          
        Patch v2 (obsolete)
        — 
          — Splinter Review
      

    


  
Deleting any <BR> that is ahead, whether it's visible or not, sounds reasonable
to me, too.

        Attachment #123051 -
        Attachment is obsolete: true

    
    

    
  
Comment on attachment 123544 [details] [diff] [review]
Patch v2

I just see that you said, only when forward deleting you want to delete any
<br>.

        Attachment #123544 -
        Attachment is obsolete: true

    
    

    
  
The reason why my code worked is: Not only do we delete the <br>, but deleting
an invisible <br> also causes the delete code to continue (recurse) the deletion
attempt.

If we are not recursing, the fragments do not get joined.

We obviously must not recurse when we are deleting a visible <br>.

If the role of IsVisBreak is NOT to give us information, whether the passed in
node is visible or not, then that helper function looks inappropriate for the
deletion code to make its decision.


    
    

    
  
> The deletion code, upon finding out that a visible <br> is ahead of the
> selection, should remove that <br> when forward deleting.  So I think the
> IsVisBreak() code is right and the deletion code that calls it is doing the
> wrong thing.  

The deletion code does delete the visible <br>, but somehow we seem to have code
elsewhere that adds the <br> back, and no code that attempts to join what comes
after the deleted <br>.

    
    

    
  
Comment on attachment 123051 [details] [diff] [review]
Patch v1

Joe, please re-review the patch after reading comment 10 and comment 11.

The deletion code must know whether the deleted node is visible, in order to
make the recursion decision. If you think we should not change IsVisBreak, only
because the deletion code needs that additional information, we will have to
use another (new) function to provide that information. I will also attach
another patch so you can choose which patch you think makes more sense.

        Attachment #123051 -
        Attachment is obsolete: false

        Attachment #123051 -
        Flags: review- → review?(jfrancis)

    
    

    
  

      
      
      
      

        Attached patch
          
          
        Alternate Patch (v4) (obsolete)
        — 
          — Splinter Review
      

    


  

    
  

        Attachment #123549 -
        Flags: review?(jfrancis)

    
  

        Attachment #123051 -
        Flags: review?(jfrancis)

    
  

        Attachment #123549 -
        Flags: review?(jfrancis)

    
    

    
  
Had a chat with Joe. I had not explained clearly enough, that we already do
delete the BR, but that it doesn't seem to have an effect.

Joe says, we don't want to recurse the deletion, as I'm proposing it here.
He suspects the real bug is that the post processing code is adding a BR back,
possibly having to do with MozBR. He gave me additional information, I'm
investigating.


    
    

    
  
Discussed in edt bug triage.  Plussing.
Keywords: topembed → topembed+

    
    

    
  
Like Joe suspects, the post processing code is adding a mozBR back, in
AdjustSelection.

I have a patch that removes that code, without yet knowing whether removing that
code is always correct.

While that change causes the correct delete behaviour, the caret moves to an
unexpected position. It gets moved to the beginning of the document.

I'm trying to fix that, but no luck yet, I'll need some more time.


    
    

    
  

      
      
      
      

        Attached patch
          
          
        Patch v7 (obsolete)
        — 
          — Splinter Review
      

    


  

        Attachment #123051 -
        Attachment is obsolete: true

        Attachment #123549 -
        Attachment is obsolete: true

    
    

    
  
Based on Joe's comment, I'm removing the code that does add the MozBR. I still
don't know whether removing that code is correct for all cases. It works correct
to fix this bug.

When I noticed the caret ends up on an invalid position, I tried to add code to
the deletion code, that puts the caret on the next node after the selection.
That didn't work, the post processing code disturbed my attempts, and the caret
continued to stay at some invalid position. It really was an invalid position,
because the caret shows up at the beginning of the document, but using cursor
right or left moves to an expected position.

This patch fixes this bug for me, but the selection adjustion correction code is
more or less a wild guess. I saw, when removing the "nearNode" special cases
detection (checking for break, textnode, image, hr), this will cause the use of
FindNearSelectableNode function. This worked good. I'm now guessing that
positioning the caret on a non-visible BR is NOT a good idea (against what the
existing code says), and I'm changing to code to check for visibility of the BR.


    
  

        Attachment #123740 -
        Flags: review?(jfrancis)

    
    

    
  
In theory these changes look good, but they may not be complete.  Next we must
confirm that typing return always generates 2 br's in those cases where 2 are
required.  One simple first test would be to go to source mode for a blank
document, remove the <br>, add some body text, and then go back to normal view
and type a return at the end of the text.  Do we get 2 br's?  If we do, we need
to find the code that added in the 2nd br and make sure that it is fully general.

    
    

    
  
Comment on attachment 123740 [details] [diff] [review]
Patch v7

Thanks for the additional information.
Unfortunately, in your reported case, we don't get two <BR>s.

        Attachment #123740 -
        Attachment is obsolete: true

        Attachment #123740 -
        Flags: review?(jfrancis) → review-

    
    

    
  

      
      
      
      

        Attached patch
          
          
        Patch v8 - ignores whitespace (obsolete)
        — 
          — Splinter Review
      

    


  
I no longer feel brave enough to make this larger change.

This patch still has the change to allow visible breaks as "a good place for
the caret to be"

However, I'm no longer removing all that additional code. Instead, I'm checking
whether we are NOT deleting. If we are deleting, we don't add a BR back. Any
other case will be unchanged.


Note: I'm renaming a paramter in AdjustSelection. The second parameter is a
"direction", but was named "aAction". But now I want to pass in the action! So,
I have renamed aAction to aDirection to reflect reality, and have added the new
parameter aAction.

    
    

    
  
Comment on attachment 123763 [details] [diff] [review]
Patch v8 - ignores whitespace

I'm simply too optimistic, I found additional cases where this doesn't work.

        Attachment #123763 -
        Attachment is obsolete: true

    
    

    
  
While playing with the test case and my patches, I found bug 206500.


    
    

    
  
The current code works like this: The delete code deletes the BR. Later, some
"Adjust" code tries to look at the "state", and decide whether a BR needs to be
added.

I think, it should not only consider the current state, it should also consider
what action has just taken place. If the code has just deleted a BR, it does not
make sense to add it back.

I'm attaching a somewhat larger patch that allows us to pass a flag around, that
will allow AdjustSelection to know whether we have just deleted a BR.


While testing the patch, I ran into another situation that I think is weird.
Currently, when you are at the end of a document, at a separate new line, you
press DEL, and it looks like nothing happens. But actually, our code deletes the
BR and adds it back! Since this patch causes our code to no longer add the BR
back, we must make sure we don't delete the BR if we are positioned at the end
of the document. The patch tries to detect that, I hope the detection logic is
correct.


    
    

    
  

      
      
      
      

        Attached patch
          
          
        Patch v11 (obsolete)
        — 
          — Splinter Review
      

    


  

    
    

    
  
The other case to worry about is ranged deletion (instead of collapsed
selection).  For instance, make sure that if you have <body>foo</body>, that you
can select "foo" and delete and still get the br added.  

I'm not sure that we are taking the right approach here.  Maybe instead what we
should do is to adjust the selection in the case where we are deleting a br via
backspace or delete.  For instance, in the backspace case, if selection is then
adjusted to be before the first (undeleted) br, then the later code to insert a
br back won't be trigerred.

    
    

    
  
Comment on attachment 123823 [details] [diff] [review]
Patch v11

with your example, having <body>foo</body> selecting and deleting, we don't get
a <br> with this patch :-(

        Attachment #123823 -
        Flags: review-

    
    

    
  
In fact we rely on this code that inserts a <br> in the deletion case.  Another
example:
<body>some text<br>
<ul><li><br></li></ul>
</body>

Click in the empty list item and hit backspace.  We have to put inthe extra <br>
here.  Otherwise the caret would be on the line "some text", but the selection
is actually after that <br>.  Hence typing would casue characters to appear on
the next line, rather than where the cursor was.  

So I am becoming more convinced that adjusting our selection right in the
deletion code in the case where we are deleting a <br> (in the collapsed
selection case) is the right thing to do.  Care must be taken to identify those
cases where we need to adjust selection, and where it should be adjusted to.  

    
  
Target Milestone: --- → mozilla1.5alpha

    
    

    
  
> Otherwise the caret would be on the line "some text", but the selection
> is actually after that <br>.

I'm confused. I thought, when the selection is collapsed, caret is directly
defined by the selection?


    
    

    
  
> > Otherwise the caret would be on the line "some text", but the selection
> > is actually after that <br>.

> I'm confused. I thought, when the selection is collapsed, caret is directly
> defined by the selection?

Nevermind, I think I got it now. You say, in your example from comment 28, if we
were not inserting a <br> on deleting, we would end up with a document consisting of
<body>some text<br>
</body>
with the selection being after the <br>, and the caret displayed on the first line.

I confuses me that the caret would be displayed on the same line as "some text"
if the selection is after the <br>.


    
    

    
  

      
      
      
      

        Attached patch
          
          
        Patch v16 (obsolete)
        — 
          — Splinter Review
      

    


  
Needs some more testing, but fixes the testcase.

        Attachment #123823 -
        Attachment is obsolete: true

    
    

    
  
The patch v16 fixes the original testcase.
It also fixes the the testcase from comment 28.

However, it does work in the situation described in bug 26.
But wait a minute!

I just tested, neither Mozilla 1.0, Netscape 7, Mozilla 1.2, Mozilla 1.3,
Mozilla behave correctly with the example from comment 26! No version adds the
<br>, and all show the caret on the invalid position!

I would like to suggest that we fix the issue described in comment 26 in a
separate bug.


It seems that both patches v11 and v16 seem to fix the bug.
Joe, do you prefer v16?


    
  

        Attachment #124915 -
        Flags: review?(jfrancis)

    
    

    
  
The behaviour described in comment 26 has been added as another testcase for bug
202166.


    
    

    
  
I will have to do some testing with this to make sure it does the right thing.

    
    

    
  
I don't understand this patch, becasue i don't know why we are moving the
selection by "characters".  But at any rate, the patch does not work.  Try this:

steps:
1) new composer doc
2) type return
3) type a letter
4) hit up arrow
5) hit DEL

observe selection goes to wrong place. 

I don't think we can be moving selection by characters as part of this bugfix.

    
  

        Attachment #124915 -
        Flags: review?(jfrancis) → review-

    
    

    
  
editorbase+
Whiteboard: editorbase, edt_x3 → editorbase+, edt_x3

    
    

    
  
> I don't understand this patch, becasue i don't know why we are moving the
> selection by "characters".

Joe, I'm trying to adjust the selection, because you suggested to do that in
comment 28.

Moving by characters was the only idea I had, since you didn't give more details
how exactly you suggest to adjust the selection.


> I don't think we can be moving selection by characters as part of this bugfix.

I'm confused. Could you please explain what exactly you were suggesting in your
comment 28?


    
    

    
  
Comment on attachment 124915 [details] [diff] [review]
Patch v16

Obsoleting because this patch does not work in the case mentioned in comment 35

        Attachment #124915 -
        Attachment is obsolete: true

    
    

    
  
Comment on attachment 123823 [details] [diff] [review]
Patch v11

un-obsoleting this patch, because it's the best patch we have so far.

        Attachment #123823 -
        Attachment is obsolete: false

        Attachment #123823 -
        Flags: review- → review?(jfrancis)

    
    

    
  
I had a chat with Joe on IRC. I now understand what he means with adjusting the
selection, patch v16 was using an unnecessarily complex (and incorrect) strategy.

I have a new patch that seems to fix this bug, and I will attach it as patch v18.

However, I'm unsure whether we really want patch v18. I still tend to prefer v11.

I prefer v11 because of the caret location after the user has deleted the caret
in the original testcase, in front of the bulleted list.

With patch v11, the caret is placed at the first position in the list.
With patch v18, the caret is placed at an earlier position.

I think what patch v11 provides is closer to what the user expects.
While patch v11 is larger, I think it is more logically to read in the
sourcecode, instead of using the trick of adjusting the selection.

Joe, do you prefer v11 or v18 ?


    
    

    
  

      
      
      
      

        Attached patch
          
          
        Patch v18
        — 
          — Splinter Review
      

    


  

    
  

        Attachment #126570 -
        Flags: review?(jfrancis)

    
    

    
  
It is true that we need to be smarter about adjusting selection.  But patch 11
is not the way to do it.  We should just file seperate bugs on that issue if
patch 18 (or some descendant) lands.

    
    

    
  
Comment on attachment 126570 [details] [diff] [review]
Patch v18

This looks good to me.	Note to other reviewers: I have not tested this.

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

    
  

        Attachment #123823 -
        Attachment is obsolete: true

        Attachment #123823 -
        Flags: review?(jfrancis)

    
  

        Attachment #126570 -
        Flags: superreview?(kin)

    
    

    
  
I think this fix needs to be modified so that it looks at the content after the
BR being deleted ... right now it always moves the caret when you have a
<br>|<br> case, but that moves the caret strangely when you have the case:

  Line1<br><br>Line3<br>

which renders like this:

  Line1
  |
  Line3

In that case above, I would expect forward delete to keep my caret on the 2nd
line, and the text on Line3 to move just after the caret:

  Line1
  |Line3

But with the current patch it will move the caret like this:

  Line1|
  Line3


    
  

        Attachment #126570 -
        Flags: superreview?(kin) → superreview-

    
    

    
  
Comment on attachment 126570 [details] [diff] [review]
Patch v18

I should also mention that I don't really understand what this check is about:

-      if (startNode == stepbrother)
+	 if (startNode == afterDeleteNode)

The original code just seems broken ... it will still leave 2 text nodes next
to each other without merging them ... imagine this case:

  Line1|<br>Line2

If I hit forward delete, the br is deleted, and since |startNode| is the body
node and |afterDeleteNode| is the text node containing "Line2", nothing gets
joined.

    
  
Target Milestone: mozilla1.5alpha → mozilla1.5beta

    
    

    
  
It's not likely that I will work on editor/selection bugs in the near future.
Mass assining my bugs to nobody.


Assignee: kaie → nobody

    
    

    
  
snarfing kaie's old bugs
Assignee: nobody → mozeditor

    
  
QA Contact: bugzilla → editor

    
  
Assignee: mozeditor → nobody

    
    

    
  
Any chance someone will look at this again?  The original bug is pretty annoying for users, and repros with many different combinations of html, as simple as "foo<br><br>bar".

    
  
Severity: normal → S3

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






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: