68767 - extra blank space added when block level element (hr, table) is inserted at the end of a line, between two lines

Status: VERIFIED FIXED

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

Description

[shrirang khanzode]

Steps :

1) Open a blank document 
2) Type "abcd" on first line and hit return 
3) Type "efgh" on second line and hit return 
4) More caret back up to the first line and hit horizontal line icon 
5) Notice that caret moves to the beginning of the next line
6) Hitting any key adds space between the current line and the "efgh" line

Expected: No blank space should be seen when text is typed after <hr> is added

Comment 1

[rubydoo123]

what is happening is that when you type in the text you get:

abcd< br>
efgh< br>

when you insert the HR, you end up with this:
abcd
< hr align="Left" width="100%" size="2">< br>
efgh< br>

when you then begin to type after the HR, you end up with this:
abcd
< hr align="Left" width="100%" size="2">ijkl< br>
efgh< br>

so, the br is producing the spacing. If you place the caret to the left of the 
'e' it works as expected because the br remains behind the abcd. Even if I go to 
the first line and hit end, I still end up entering the HR to the left of the br

Comment 2

[rubydoo123]

moving to 1.0

Comment 3

[Asa Dotzler [:asa]]

Bugs targeted at mozilla1.0 without the mozilla1.0 keyword moved to mozilla1.0.1 
(you can query for this string to delete spam or retrieve the list of bugs I've 
moved)

Comment 4

[TucsonTester1]

verified on mac os x build 2001121805, changing platform/os to all.

Comment 5

[Syd Logan]

plussing

Comment 6

[Kevin McCluskey (gone)]

Bulk moving all nsbeta1+ bugs which were targetted after Mozilla1.0 to Mozilla1.0

Comment 7

[Joe Francis]

pri = 2 for original 1.0 EB+ bugs

Comment 8

[Joe Francis]

The trunk is the wave of the future!

Comment 9

[Joe Francis]

The days of having a half dozen milestones out in front of us to divide bugs 
between seem to be gone, though I dont know why.  Lumping everything together as 
far out as I can.  I'll pull back things that I am working on as I go.

Comment 10

[Charles Manske]

I cannot reproduce this problem on Windows 2000.
mjudge recently fixed lots of problems concerning HRs in bug 159207, so maybe this
was fixed by that?
I'm not possitive if this is a dup or just WFM, so let's test on all platforms 
first.

Comment 11

[Kathleen :Brade]

I see a blank line after inserting the hrule; I assume that is what this bug is
about.

Comment 12

[shrirang khanzode]

Attachment: screenshot

kathy is right,yep. I saw that the HR still added a blank line.attaching
screenshot.

Comment 13

[Joe Francis]

This bug is a lot less bad now due to layout changes for HR's.  Now you get the
extra space mentioned in the testcase right away, rather than have it appear
after you type.  This makes the behavior much less odd.

Fixing this completely would involve checking for a following br when inserting
an HR, and removing the br if present.

Comment 14

[Kevin McCluskey (gone)]

nsbeta1-

Comment 15

[saari (gone)]

EDITORBASE+ topembed+ normalization

Comment 16

[Kai Engert [:KaiE:]]

Updating summary to jfrancis' latest comment, which I can confirm.

Let me summarize:
  abcd<br>
  efgh<br>
  <br>
is the raw content we have.

The bug is only seen when inserting the <hr> if the caret is positioned in the
same line as "abcd", only if the caret is positioned at the end of the line.


jfrancis suggests to delete a following <br> break when inserting a <hr> line.

Another possible fix were to detect that we are positioned at the last character
in front of a <br>, and not insert the <hr> at the cursor position, but insert
it after the <br>?

I see an advantage in using this alternate approach to fix it. The advantage
will be seen if the user decides to delete the horizontal line. If we have
removed the <br>, the lines will no longer be separated and will be joined into
a single line showing abcdefgh.

Comment 17

[Joe Francis]

Kai, your approach sounds reasonable.  Note that implementing it is trickier
than you might think at first.  You have to use the nsWSRunObject code,
specifically the NextVisibleNode() call, to determine if you are adjacent to a
br.  This code will properly deal with the case of invisble whitespace in the dom.  

Comment 18

[Kai Engert [:KaiE:]]

I think we should extend this bug to handle inserting a table as well, I see the
same extra spacing gets added when inserting a table.

Comment 19

[Kai Engert [:KaiE:]]

Attachment: Patch v1

Comment 20

[Kai Engert [:KaiE:]]

-> me

The patch I just attached seems to fix it.
I hope the patch is correct, too.

Comment 21

[Kai Engert [:KaiE:]]

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

Joe, can you please review?

Comment 22

[Kai Engert [:KaiE:]]

Note that in the patch I do not try to adjust the caret/selection.
Should I do something?

Comment 23

[rubydoo123]

Kai, can you test this scenario with your patch:
1. open a blank composer window
2. enter a table (2x2 is fine) - do not hit enter after the table insertion
3. view source, since you have removed the trailing br, you should see
</table>
</body> 
4. save the file, and close it
5. open file and view source to ensure teh trailing br is indeed gone, then
select to edit
6. the insertion caret should be the first cell. Click below the table, do you
see the caret? Try to insert text after the table
 -- are you able to do that?

Comment 24

[Kai Engert [:KaiE:]]

beppe: In step 3 of your explanation you assume the patch removes the <br>. But
the patch does never remove any <br>. The patch uses a trick to make sure the
<br> does not result in extra blank space displayed.

Suppose the source is
  xyz<br>

The bug occurs when the cursor is positioned after the "z", at the end of the line.

Now insert a horizontal line or a table.
Without the patch, the new source is:
  xyz
  <hr>
  <br>

or, when inserting a table
  xyz
  <table>...</table>
  <br>

My point is, we should not remove the <br>. I think the bug is that we insert
the <hr>, the <table> at the wrong position.
I vote for (and this is what the patch does) producing the following code instead:
  xyz<br>
  <hr>

or
  xyz<br>
  <table>...</table>

If the user later decides to remove the <hr> or the <table>, the advantage is,
the layout will look as it was before inserting the new element, because we did
not remove the <br>.


Because of that I can not test whether the <br> is gone, because the patch
doesn't remove it :-)

However, I did the other part of your proposed test.
I created a new composer document and inserted a 2x2 table.
I saved and closed the file.
When I open the file again, the caret is in the first cell.
I can click below the table and see the caret.
I can enter text below the table.


The test you suggested is a nice edge case that I had not been thinking about
before.
When the document is completely empty, and inserting a table like you suggested,
the document contains of
  ... <body><br></body>...
and I believe the caret is placed before the <br>.
After reading my above explanation, one might think that the patch does the
wrong think, because it would produce
  ...<body><br><table>...</table></body>...
But my testing shows, it does not, which is good!

Indeed, the patch produces the expected code
  ...<body><table>...</table><br></body>...

The patch does not skip after the <br> in this case, and here is the explanation
why:

When I said further above, that we always skip a next <br>, this was not correct.
We only skip visible <br> that results in a real line break.
In the special situation when having an empty document and inserting a table,
the <br> is not found as being visible, and therefore the patch does not skip to
the position after it.

So it appears the patch is behaving correctly in the situations I have tested yet.

Comment 25

[rubydoo123]

Kai: excellent, and I apologize for assuming the br was removed (my bad). My
concern is, is that the user can still get a selection after the table or image.
That has been an issue in the past and I just wanted to ensure that function did
not go away.

Comment 26

[Joe Francis]

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

r=jfrancis,
but I would consider testing for block nodes in general rather than just Tables
and HR's.

Comment 27

[Kai Engert [:KaiE:]]

> but I would consider testing for block nodes in general rather than just Tables
> and HR's. 

Good point. I agree that it makes sense to extend that special handling to any
insertion of block level elements.

In Mozilla's composer UI, the only ways I found to insert a block level element
was by using the "insert table" or "insert horizontal line" command.

However, I found the more general command "insert html" and used that for my
testing.

When I used "insert html" to insert a block like "<p>bla</p>" I see the same bug
again. So it indeed makes sense to extend the handling to all block level blocks.

In a first step I extended the patch to check for other block level tags for the
InsertElementAtSelection method only. But this was not sufficient, since "insert
html" uses method InsertHTMLWithCharsetAndContext.


I am now attaching a new patch that moves the new logic to a new method
NormalizeEOLInsertPosition, and makes both insert methods call this new method.

In my testing it appears to work well. When the cursor is positioned at the end
of a line with a trailing <br>, I can insert various blocks of html code, like
"<p>bla</p>" or "<div>bla</div>" and it seems to be correct, there is no longer
additional spacing shown, and the inserted html correctly shows up after the
trailing <br>.

Comment 28

[Kai Engert [:KaiE:]]

Attachment: Patch v2

Comment 29

[Kai Engert [:KaiE:]]

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

Can you please review?

If you think more tags should be added to IsBlockLevelNode please let me know.

Comment 30

[Kai Engert [:KaiE:]]

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

I have to take my review request back. This patch behaves incorrect when the
caret is positioned on the beginning of a empty line.

Comment 31

[Kai Engert [:KaiE:]]

I think the conclusion of the new case I found is:
Do not try to skip over the next visible break, if the current cursor position
is at the beginning of a block.

Because if we are at the beginning of a block level element, the new block level
element is inserted before the current position's element, and by skipping over
the break, we no longer insert before.

I think the solution is to query the insert position. If the parent node of the
insert position is a block level node, and our offset is zero, then do not try
to skip.

Comment 32

[Kai Engert [:KaiE:]]

Attachment: Patch v3

Well, I realize that I was simplifying too much.

Finding out, whether the caret is at a position, where skipping over the next
break is not desired, is somewhat tricky.

I believe I have it working now. Please look at the comments in the attached
patch for my explanation.

Comment 33

[Joe Francis]

I talked with Kai about this.  I recommend these chages:
use nsHTMLEditor::IsBlockNode() instead of writing new routine.
use the values returned by PriorVisibleNode to detect block boundaries rather
than writing new code.

Comment 34

[Kai Engert [:KaiE:]]

Attachment: Patch v4

New patch that addresses Joe's comments.

Comment 35

[Kai Engert [:KaiE:]]

Attachment: Patch v5

Updated patch, only difference is: No longer calling Utils::IsBreak, but
checking for flag nsWSRunObject::eBreak.

Comment 36

[Kai Engert [:KaiE:]]

Attachment: Patch v6

No longer checking the offset returned from PriorNode

Comment 37

[Joe Francis]

Comment on attachment 119278 [details] [diff] [review]
Patch v6

r=jfrancis

Comment 38

[kinmoz]

Comment on attachment 119278 [details] [diff] [review]
Patch v6

sr=[e-mail] ... Looks good, I'd just rename |brNode| to |brParentNode|
since that's what this code really gets:


+  nsCOMPtr<nsIDOMNode> brNode;
+  PRInt32 brOffset=0;
+
+  GetNodeLocation(nextVisNode, address_of(brNode), &brOffset);
+
+  *insertParentNode = brNode;
+  *insertOffset = brOffset + 1;

Comment 39

[Kai Engert [:KaiE:]]

Attachment: Patch v7

Same patch as before, only the variable is renamed.

Comment 40

[Kai Engert [:KaiE:]]

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

carrying forward reviews

Comment 41

[Joe Francis]

Comment on attachment 119273 [details] [diff] [review]
Patch v5

getting obsolete patch off my review radar.

Comment 42

[Kai Engert [:KaiE:]]

fixed on trunk

Comment 43

[rubydoo123]

using the trunk build from today [2003051308] on winXP, I did the following:

1. opened new compose window
2. entered:
abcd[enter]
efgh[enter]
ijkl[enter]
mnop

3. placed the caret in the line 'abcd' and hit the [END] key and selected
Insert|HRule
and then hit the spacebar.

The caret stayed in the correct location and there was no br added

4. placed the caret in the line 'efgh' and hit the [END] key and selected
Table|Insert Table
5. selected the '<table>' symbol in the status bar and hit the [->] key to get
focus out of the table and hit [ENTER].

A blank line was added between the table and 'ijkl' and the caret was at the
beginning of 'ijkl' and not in the blank line.

I'm not sure if you want me to open a bug for that or do you want me to reopen
this one?

Comment 44

[Kai Engert [:KaiE:]]

If I understand correctly, you complain about the incorrect caret placement
after hitting enter.

I think it is a separate problem, and I just filed bug 205665 based on your
description.

Comment 45

[rubydoo123]

great, that is what I needed to know. Since that is a separate issue, then this
is now verified.

Comment 46

[Kai Engert [:KaiE:]]

As a final test, I tried backing out this patch and repeated your test. But even
without the patch, the caret still gets placed on the second line after the
table. So we can be sure.