101041 - If you select text and then toggle list on/off/on, the next line on the page will be added to the list if selected text includes end of line

Status: VERIFIED FIXED

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

Description

[TucsonTester2]

From Bugzilla Helper:
User-Agent: Mozilla/5.0 (Windows; U; Win 9x 4.90; en-US; rv:0.9.4)
Gecko/20010913 Netscape6/6.2
BuildID:    2001091303

If you select a line of text either by double-clicking on it with mouse or
dragging the mouse through the text to the end of the line, and then click on
the button for list (either bullet or number), it puts that line of text into a
list.  If you then click the list button to unlist, and then click the list
button to list it again, it will add the next line on the page to the list. 
Repeatedly clicking the list button will repeatedly add the next line on the
page.  This does not happen if you select text on the line and do not include
the end of the line.

Reproducible: Always
Steps to Reproduce:
1.Open a new page in composer
2.Enter three lines of text (or more)
3.Double click on the first line to highlight the entire line
4.Click on the button to make a bullet list
5.Click on the bullet list button to unlist the line of text
6.Click on the bullet list button to relist the line of text 

Actual Results:  When you unlist in step five, it still shows just the first
line of text highlighted, there is nothing to indicate that it is focused on
anything beyond the first line of text.  When you hit the list button to relist
the line, the list will add the second line of text.  If you then unlist again,
both lines of text are highiighted as if you had selected them, even though you
had not.  And if you keep toggling list off/on, it will keep adding an
additional line each time you do it.

Expected Results:  I would expect that if all you are doing is toggling back and
forth between list and unlist, that the same lines would be used each time, I
would not expect the software to decide that I wanted to add an additional line
to the list.

If you select the text by using shift and the right arrow key instead of the
mouse, and you click the arrow key just enough to include the last character on
the line, then toggling list on/off/on only effects text on the selected line. 
If you use shift and the right arrow key and click the arrow key just enough to
include the last character on the line, and then you click the right arrow key
one more time, then turning list on/off/on will pick up the next line of text,
but repeatedly toggling list off and on won't pick up any additional lines
beyo9nd that.  The problem only occurs when selecting the entire line by
double-clicking on it, or selecting the end of the line with the mouse.
When faced with an on/off switch, some of us do tend to play with the switch
when when undecided about whether or not to use that.  Someone who does that
without paying close attention may suddenly find their list is much longer than
they had wished.

Comment 1

[michael wendell]

Confirmed on Mac OSX using build 20011003, and Win 2k using build 20011002.

One thing to note, in order to reproduce this, your 3 or more lines of text have
to all end in a hard return, and contain atleast one space.  Otherwise, this
works fine.  

Also, I think I have seen something like this before, and this might be a dupe,
but I cannot seem to find anything else related.

Comment 2

[Kathleen :Brade]

-->jfrancis

Comment 3

[Syd Logan]

P1 because lists need to be right, darn it.

Comment 4

[Daniel Glazman (:glazou) (not active in Mozilla any more)]

This bug is caused by this code in nsHTMLEditRules::GetPromotedPoint :

<excerpt>
    // if this is an inline node, 
    // look ahead through any further inline nodes that
    // aren't across a <br> from us, and that are enclosed in the same block.
    
    if (!IsBlockNode(node))
    {
      nsCOMPtr<nsIDOMNode> block = nsHTMLEditor::GetBlockNodeParent(node);
      nsCOMPtr<nsIDOMNode> nextNode, nextNodeBlock;
      res = mHTMLEditor->GetNextHTMLNode(node, address_of(nextNode));
      
      while (nextNode && NS_SUCCEEDED(res))
      {
        nextNodeBlock = nsHTMLEditor::GetBlockNodeParent(nextNode);
        if (nextNodeBlock != block)
          break;
        if (nsTextEditUtils::IsBreak(nextNode))
        {
          node = nextNode;
          break;
        }
        if (IsBlockNode(nextNode))
          break;
        node = nextNode;
        res = mHTMLEditor->GetNextHTMLNode(node, address_of(nextNode));
      }
    }
</excerpt>

Comment 5

[Joe Francis]

pushing off 098 to 099

Comment 6

[Syd Logan]

selection bug, should this go to mjudge? plussing.

Comment 7

[Joe Francis]

Attachment: patch for nsHTMLEditRules.cpp

I tweaked GetPromotedPoint to do the right thing when the selection ends just
after a BRnode.  If the change looks a little complex it's because I made it
work in the case where the selection is inside a text node; either at the
beginning of it or inside it but with only non-rendering ws before it.

Comment 8

[Joe Francis]

Attachment: patch fornsHTMLEditRules.cpp, rev 2

first patch had the unfortunate tendency to suck

Comment 9

[kinmoz]

Comment on attachment 69037 [details] [diff] [review]
patch fornsHTMLEditRules.cpp, rev 2

So I was wondering what would happen if the selection ended at the beginning of
a text node (as you described above) *but* with it being wrapped in an inline
style. Something like:

   Line 1<br>
   <b>Line</b> 2<br>

To get the selection you described I had to click in front of the 'L' on "Line
1" and then drag just before the "L" on "Line 2" ... this resulted in the
selection ending in the text node on line 2 at zero offset.

With your fix, listing with this selection caused it to look like this:

    * Line 1
    * Line
  2

Not quite what I'd expect. :-) I think we may need to promote the end point out
to the block offset right after the br to handle this case too?

Comment 10

[Joe Francis]

Damn.  I guess I will have to bite the bullet and fix this the right way.  The
problem with that is it's pretty ahrd and will take a while, and then we will
need to test pretty heavily.

The right thing to do is to take the iterating code in nsWSRunObject and
generalize it, and then have both the ws code and the range promotion code use
it.  At least, I *think* this is the right thing.  :-)

good catch Kin.

Comment 11

[Joe Francis]

Another thing I'll have to examine is what the differences are in the existing
code for collapsed vs non-collapsed selections.  In you example above, you
expect "Line 1" to be a list, but if the selection were collapsed at the same
(righthand) endpoint, you would expect a list made from "Line 2". 
GetPromotedPoint() doesn't know or care about the selection, so I'll have to
make sure we are doing right thing before we get that far.

Comment 12

[Joe Francis]

this bug is a bit worse than first expected.  

First off, the existing code makes no distinction between how it promotes
collapsed vs non-collapsed selections, so I'll have to fix that.  

Next, we have the same problem as the original bug report, but also in the other
direction.  Ie, if you have several lines of body text, and you select all of
one of them, ending your selection at the end of the *previous* line (ie, you
selected right to left and then up to previous line end), then making a list or
any other block operation will grab that prior line too, even though it looks
unselected.

Finally, this same problem, in both directions, also occurs with block
boundaries, not just breaks.  

Having thought about it some more though I think there really isn't a problem
(here at least) with how GetPromotedPoint works.  The problem is that I have to
normalize non-collapsed selections to not be trivially beyond block or br
boundaries.  Working on a way to do that...

Comment 13

[Joe Francis]

Attachment: mega patch to libeditor

This patch includes a bunch of unrelated bug fixes.  sorry.  i wanted to give
you one that would apply.  The key change for this bug is about 150 lines of
new routine:  nsHTMLEditRules::NormalizeSelection().  I managed to get the
nsWSRunObject code to do most of the work for me, after tweaking it a bit to
expose more of it's calculations to nsHTMLEditRules.

Share and Enjoy and for gosh sakes, let's be Careful Out There.

This fixes the original bug. It fixes kin's test case.	It also fixes a related
but unreported bug of getting similar weirdness when you select from end of
block A through block B to beginning of block C, etc.

Comment 14

[Joe Francis]

fixed on tip

Comment 15

[michael wendell]

Verified on Win XP using build 02-20.