Open Bug 98778 Opened 23 years ago Updated 2 years ago

After inserting HR, new text gets inserted at incorrect position (summary in comment #18)

Categories

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

defect

Tracking

()

mozilla1.5beta

People

(Reporter: TucsonTester1, Unassigned)

Details

(Keywords: topembed+, Whiteboard: EDITORBASE+; 2 days <HR>,edt_a3,edt_b3,edt_c3,edt_x3)

Attachments

(1 file)

From Bugzilla Helper:
User-Agent: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:0.9.3+)
Gecko/20010903 Netscape6/6.1b1
BuildID:    20010903

When you enter in multiple lines with color fonts then add a HRule. You will
notice extra font tags. not needed. 

Reproducible: Always
Steps to Reproduce:
1.Open New Composer Document
2.type 1 line of text, click enter
3.change font color using color pallet from tool bar. 
4.type 1 line of text, click enter
5.select Hrule from toolbar
6.click on source and look at the sorce code 

Actual Results:  You get something that looks like this 
<body>
<font color="#ff0000">Red Text <br>
<font color="#3333ff">Changed to blue Text <br>
</font></font>
<hr width="100%" size="2">  ( <font color="#ff0000"><font color="#3333ff">) <br>
</font><br>
</font>
</body>
The stuff in () is not needed . 

Expected Results:  The font tags should not even be there after the HR tag
Confirming bug.
reporter : right ; the FONT element should not be here if you add no more text

jfrancis : (thinking at loud) could a way of solving this be the following one :
when you insert a HR as last child, you add a BR after it (so the selection works
fine) and you cache html inline styles. In that case, no FONT element is added
until a new char is entered, right ?

brade : I think that it is a WONTFIX only if jfrancis says that what I am
proposing is not possible.
Status: UNCONFIRMED → NEW
Ever confirmed: true
spam composer change
Component: Editor: Core → Editor: Composer
-->core
Joe--please read/address Daniel's comment above.
Component: Editor: Composer → Editor: Core
Hardware: PC → All
I can't respond until I investigate this bug.  It may be that inserting the HR is 
causing the font tag to be split, due to the DTD.
Target Milestone: --- → mozilla1.0
Assignee: brade → jfrancis
Summary: Placing a H.Rule after multiple lines of colored text, results in access font tags → Placing a H.Rule after multiple lines of colored text, results in excess font tags
-->jfrancis for investigation
Status: NEW → ASSIGNED
Whiteboard: EDITORBASE; 2 days
Whiteboard: EDITORBASE; 2 days → EDITORBASE-; 2 days
The extra tags are because the intent is for the text following the HR to
continue having the same color as the text ahead of the HR. Notice if you follow
the steps but instead insert an image, you get the same color as in the text
above the image, and the "extra" tags that supported this are there. Bug is the
it doesn't work for HR. EDITORBASE.
Summary: Placing a H.Rule after multiple lines of colored text, results in excess font tags → Colors and attributes of text should persist after you enter an HR
Whiteboard: EDITORBASE-; 2 days → EDITORBASE; 2 days
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)
Target Milestone: mozilla1.0 → mozilla1.0.1
Priority: -- → P2
Target Milestone: mozilla1.0.1 → mozilla1.0
minusing
Whiteboard: EDITORBASE; 2 days → EDITORBASE-; 2 days <HR>
Moving bugs to Mozilla1.1 that are not EDITORBASE+.
Target Milestone: mozilla1.0 → mozilla1.1
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.
Target Milestone: mozilla1.1alpha → mozilla1.2beta
[ushing these out as far as bugzilla will let me.  I'll pull them back as I work
on them.
Target Milestone: mozilla1.2beta → mozilla1.4beta
editorbase+
QA Contact: sujay → beppe
Whiteboard: EDITORBASE-; 2 days <HR> → EDITORBASE+; 2 days <HR>
EDITORBASE+ topembed+ normalization
Keywords: topembed+
using the build 2002031804 on win32, this is still a problem. The resulting HTML
code is this:
<body>
this is one line of text<br>
<span style="color: rgb(51, 51, 255);">this is a line of blue text<br>
</span>
<hr style="width: 100%; height: 2px;">no this is not blue<span
 style="color: rgb(51, 51, 255);"><br>
</span>
</body>

The span gets carried over, however, the focus point is to the left of the span.
-> me
Assignee: jfrancis → kaie
Status: ASSIGNED → NEW
Whiteboard: EDITORBASE+; 2 days <HR> → EDITORBASE+; 2 days <HR>,edt_a3,edt_b3,edt_c3,edt_x3
Discussed in bug triage.  Can we get a status update and will this make 1.4 final?
I have not yet worked on this bug, but it's on my list of important bugs.
I would like to try to get this into 1.4 final.
Target Milestone: mozilla1.4beta → mozilla1.4final
Summary:

This bug requests, after inserting a HR, the text style for newly entered text
below the HR, should be the same as above the HR.

The newest trunk code splits the SPAN. This is correct, because a SPAN must not
contain a HR.


For the future talk, let's say the text above the HR is blue, and we assume the
text below to HR to be blue. The bug is: the new text is black.


The interesting thing is: The entered text is only black (incorrect), outside
the SPAN, if you enter the text directly after having inserted the HR.

If you do anything else in between, either move the cursor around, or save and
reopen, the bug does not appear again.


Conclusion:
- the editor produces correct HTML
- the "edit HR" action places the caret at an invalid position


Suggested strategy for fixing the bug:
Either correct the incorrect caret placement code, or, after having inserted a
HR, execute code that ensures the caret is at the correct position.
Summary: Colors and attributes of text should persist after you enter an HR → After inserting HR, new text gets inserted at incorrect position (summary in comment #18)
Attached patch Patch v1Splinter Review
This fixes the problem for me in the reported.
It seems logical to me, the idea is, after inserting an element, find the next
visible thing after the newly inserted element, and place caret in front of
that next element.

The existing code makes the assumption that it will work to simply go to the
next offset+1 position. But that fails in our case, because we insert
additional nodes (split SPAN) into the DOM tree.
Attachment #122787 - Flags: review?(jfrancis)
I'm going to have to think about this one for a bit, there may be some fallout
from this approach...
20030512 Test Build

Intended behaviour?
1.  Inserting the hrule into a span breaks the span into two spans and moves the
cursor within the span.  
2.  The inserted hrule does not have the same color styling as the text it is
being inserted into. (?)
2.  If cursor is then placed immediately before or after the hrule, text is
entered outside of the spans in the same area as the hrule without any styling. (?)
Some comments below, but I think each of the following suggestions should be
discussed in separate new bugs, since all behaviour you describe can be seen
with trunk builds, too.


> 1.  Inserting the hrule into a span breaks the span into two spans and moves the
> cursor within the span.  

I think that is the intended behaviour, because the sequence <span><hr></span>
seems to be invalid.
Maybe the the <hr> element should obtain the current style.


> 2.  The inserted hrule does not have the same color styling as the text it is
> being inserted into. (?)
> 2.  If cursor is then placed immediately before or after the hrule, text is
> entered outside of the spans in the same area as the hrule without any 
> styling. (?)

I'm not sure how we are supposed to behave in the two cases.
I have some ideas about this.  I did some work on the ANGELON branch earlier
that remembers inline styles when you delete, so that further typing is still in
a style after deleletion.  

This work could be extended to the case where we are inserting a block level
element.  It would keep the insert element code cleaner (it would simply make a
single call to cache the current styles) and it would leave the source cleaner
(we could eliminate empty inline tags that ended up on one side of the inserted
element), and yet it would stll fix the problem.

I think leveraging that other work (which is unfortunately not on the tip) is
the way to go.  The first task is to port the earlier work to the tip.
Joe, the alternate approach you are proposing sounds like a lot of more work.

Do you actually reject the patch I have attached in this bug?

If you do reject it, could you please provide a pointer to the code you are
mentioning, that should get ported, enabling me to do this work?
Thanks.

Moving target milestone, it sounds unlikely this will make it for 1.4 final,
unless Joe accepts the existing patch.

Target Milestone: mozilla1.4final → mozilla1.5beta
Comment on attachment 122787 [details] [diff] [review]
Patch v1

minusing for now because I think &visType needs to be checked to determine
proper way to adjust selection.

But it may be that this whole approach is not the best.  I will talk to Kai
more about this after some investigation.
Attachment #122787 - Flags: review?(jfrancis) → review-
Kai, the code I mentioned earlier has landed.  Try using
nsHTMLEditRules::CacheInlineStyle() to remember the style. Then in
nsHTMLeditRules::AfterEditInner() you will want to find the if clause that
contains ReapplyCachedStyles() and add the appropriate action types to the if
clause.

This should make for a simple fix, and one that keeps the source clean. 
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: rubydoo123 → editor
Assignee: mozeditor → nobody
Moving to p3 because no activity for at least 1 year(s).
See https://github.com/mozilla/bug-handling/blob/master/policy/triage-bugzilla.md#how-do-you-triage for more information
Priority: P2 → P3

Mass-removing myself from cc; search for 12b9dfe4-ece3-40dc-8d23-60e179f64ac1 or any reasonable part thereof, to mass-delete these notifications (and sorry!)

Severity: normal → S3
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: