Closed Bug 1250010 Opened 8 years ago Closed 8 years ago

Bug in nsHTMLEditRules::ReturnInParagraph() leads to lost style (font, bold, etc.) in empty paragraph (detailed debug in comment #9)

Categories

(Core :: DOM: Editor, defect)

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

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla47
Tracking Flags:
Tracking Status
firefox47 --- fixed

People

(Reporter: jorgk-bmo, Assigned: jorgk-bmo)

References

(Blocks 1 open bug)

Details

(Whiteboard: btpp-active)

Attachments

(2 files, 4 obsolete files)

Test page showing the problem.
267 bytes, text/html
Details
Proposed solution (v1) with test
5.92 KB, patch
roc
: review+
Details | Diff | Splinter Review 
When in ReturnInParagraphCreatesNewParagraph mode, creating an empty paragraph causes <p><br></p> to be inserted. This leads to a loss of font if the previous paragraph carried a font. Note that non-empty paragraphs do have a font element inserted, so the type-in state is used for non-empty paragraphs.
As the text says: Hit <enter> a few times creating a few more paragraphs, leave one empty. Then return to it and notice that bold and font colour are lost.
I was going to mention: This works in Chrome ;-)
IN my test (EB 46.0a2) I have used setting default font to large + Cambria, and generated the following markup:

  <body bgcolor="#FFFFFF" text="#000000">
    <p><font size="+1"><font face="Cambria">Hello Axel,</font></font></p>
    <p><font size="+1"><font face="Cambria">This is a test. (Last
          Paragraph)</font></font><br>
    </p>
  </body>

I am not sure about the <br> this must be inserted automatically as I did not hit Enter at the end of the second paragraph. Note that if you use the Stationery addon (without selecting a particular template) this will actually remove the font tags.

====
I know it is out of scope for this bug but It would be nicer if the the font tags could be replaced at some stage with inline styles within the <p> tag

<p style="font-size: +1; font-family: Cambria"> </p>
(In reply to Jorg K (GMT+1) from comment #1)
> Created attachment 8721897 [details]
> Test page showing the problem.
> 
> As the text says: Hit <enter> a few times creating a few more paragraphs,
> leave one empty. Then return to it and notice that bold and font colour are
> lost.

Trying that now - correct: empty paragraphs loose the <font> tags used for styling. Again, would it be hard to use an inline style attribute? (probably yes)
We notice that CreateStyleForInsertText() is called in nsHTMLEditRules::WillInsertText() here:
https://dxr.mozilla.org/mozilla-central/source/editor/libeditor/nsHTMLEditRules.cpp#1318

It is NOT called in nsHTMLEditRules::WillInsertBreak():
https://dxr.mozilla.org/mozilla-central/source/editor/libeditor/nsHTMLEditRules.cpp#1523

Looks like CreateStyleForInsertText() is geared around text nodes, not <br> elements since it inserts text here:
https://dxr.mozilla.org/comm-central/source/mozilla/editor/libeditor/nsHTMLEditRules.cpp#4550

So most likely the fix is more difficult than sticking a call to CreateStyleForInsertText() into WillInsertBreak(). Most likely a new function CreateStyleForInsertBreak() is necessary.
Is this related to bug 1249374?
Flags: needinfo?(mozilla)
Whiteboard: btpp-followup-2016-03-01
(In reply to Andrew Overholt [:overholt] from comment #6)
> Is this related to bug 1249374?
I don't think so. I haven't looked much into the bug here. Take a look at attachment 8721897 [details]. If you enter more paragraphs, empty ones with have a <br> in them. I don't see how this relates to clearing content out of a contenteditable. Well, only in so far, as the M-C editor loves to put extra <br>s everywhere ;-)
Flags: needinfo?(mozilla)
(In reply to Jorg K (GMT+1) from comment #7)
> (In reply to Andrew Overholt [:overholt] from comment #6)
> > Is this related to bug 1249374?
> I don't think so. I haven't looked much into the bug here. Take a look at
> attachment 8721897 [details]. If you enter more paragraphs, empty ones with
> have a <br> in them. I don't see how this relates to clearing content out of
> a contenteditable. Well, only in so far, as the M-C editor loves to put
> extra <br>s everywhere ;-)

:) I assumed the underlying reason was the same.

As you know from [1], the Editor is currently unowned so I don't see a fix forthcoming any time soon.

[1]
https://groups.google.com/forum/#!searchin/mozilla.dev.platform/ehsan$20editor/mozilla.dev.platform/GKZX7CrUbnY/b1sxG9ZpBgAJ
(In reply to Andrew Overholt [:overholt] from comment #8)
> As you know from [1], the Editor is currently unowned so I don't see a fix
> forthcoming any time soon.
That doesn't stop a willing contributor to propose a fix and have it reviewed. I've fixed a few M-C editor bugs in the past 12 months.

I've done a little debugging in nsHTMLEditRules::ReturnInParagraph() where a <br> is inserted:
https://dxr.mozilla.org/comm-central/source/mozilla/editor/libeditor/nsHTMLEditRules.cpp#6552

and in nsHTMLEditRules::SplitParagraph():
https://dxr.mozilla.org/comm-central/source/mozilla/editor/libeditor/nsHTMLEditRules.cpp#6564

In the attachment 8721897 [details] I have a bold red text in a paragraph and I press <enter> twice after the text. This is what happens:

==== Inserted br
br@1FAFD150 <-- This br is inserted and the paragraph is split

===== SplitParagraph - Entry, dumping para to split
p@1D5AF3A0
  b@1D5B05B0
    font@1D5B0650 color="red"
      Text@1D5B06A0
      br@1FAFD150
===== SplitParagraph - After Split
p@1FAFFAC0
  b@1FAFD970
    font@1FAFD3D0
      Text@1D5B06A0
p@1D5AF3A0
  b@1D5B05B0
    font@1D5B0650
      br@1FAFD150
===== SplitParagraph - Exit
p@1FAFFAC0
  b@1FAFD970
    font@1FAFD3D0
      Text@1D5B06A0
p@1D5AF3A0
  b@1D5B05B0
    font@1D5B0650
      br@1FAFD150

So hitting <enter> once give a correct result. The result are two paragraphs, one with the original text, the other with just the br that was inserted before splitting. Both text and br sit inside a <b><font>.

Now to what happens after the second enter, which splits the second paragraph:

==== Inserted br
br@1FAFD2E0 <-- This br is inserted and the paragraph is split
===== SplitParagraph - Entry, dumping para to split
p@1D5AF3A0
  b@1D5B05B0
    br@1FAFD2E0 <-- As we see, br is inserted wrong, inside <b> but outside <font>
    font@1D5B0650
      br@1FAFD150
===== SplitParagraph - After Split
p@1D762220
  b@1FAFD8D0
    br@1FAFD2E0 <-- The split worked, but the inserted br is still at the wrong level.
    font@1FAFD5B0
p@1D5AF3A0
  b@1D5B05B0
    font@1D5B0650
      br@1FAFD150
==== Deleted br
br@1FAFD2E0 <-- It gets deleted and ...
===== SplitParagraph - Exit
p@1D762220
  br@1D769830 type="_moz" <-- ... replaced with another one, a MozBR, which is placed
                              before the <b> and the <font>
  b@1FAFD8D0
    font@1FAFD5B0 color="red"
p@1D5AF3A0
  b@1D5B05B0
    font@1D5B0650 color="red"
      br@1FAFD150

So the first problem to solve here is why the upon hitting <enter> for the second time, br@1FAFD2E0 gets inserted at the wrong level.
(In reply to Jorg K (GMT+1) from comment #9)
> (In reply to Andrew Overholt [:overholt] from comment #8)
> > As you know from [1], the Editor is currently unowned so I don't see a fix
> > forthcoming any time soon.
> That doesn't stop a willing contributor to propose a fix and have it
> reviewed. I've fixed a few M-C editor bugs in the past 12 months.

You're completely correct and I didn't mean to imply otherwise :)
This patch fixes the problem. The inserted br now goes to the right spot and the subsequent SplitParagraph works as desired.

Let's see how much stuff this change breaks. Try run here:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=ca61613c68c7
Assignee: nobody → mozilla
Status: NEW → ASSIGNED
Oops, that broke hitting <enter> inside a text. Fixed it now, so new try run:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=1508aff833ee
Attachment #8723110 - Attachment is obsolete: true
Slightly more elegant option. Let's try it, too:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=a98654d7ad5c
This time without the debug output since that doesn't compile on opt. New try:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=ff66b794d7e3
Attachment #8723151 - Attachment is obsolete: true
This should do it. A test is added. Previous try
https://treeherder.mozilla.org/#/jobs?repo=try&revision=ff66b794d7e3
is green.

Here's another try with the test included:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=14cbebba8f53

Who said "I don't see a fix forthcoming any time soon" ;-)
Attachment #8723128 - Attachment is obsolete: true
Attachment #8723171 - Attachment is obsolete: true
Attachment #8723225 - Flags: review?(ehsan)
Summary: Type-in state is not applied to empty paragraphs in ReturnInParagraphCreatesNewParagraph mode → Bug in nsHTMLEditRules::ReturnInParagraph() leads to lost style (font, bold, etc.) in empty paragraph (detailed debug in comment #9)
Comment on attachment 8723225 [details] [diff] [review]
Proposed solution (v1) with test

Sorry but I gave up the ownership of this code specifically so that I don't have to be involved with it any more.  roc may be open to reviewing this.  If not, please try Masayuki.

Thanks!
Attachment #8723225 - Flags: review?(ehsan)
Comment on attachment 8723225 [details] [diff] [review]
Proposed solution (v1) with test

Hello Gentlemen, can one of you please review this. I only need one review, not two ;-) Sorry about the r?-SPAM.
Attachment #8723225 - Flags: review?(roc)
Attachment #8723225 - Flags: review?(enndeakin)
Whiteboard: btpp-followup-2016-03-01 → btpp-active
Attachment #8723225 - Flags: review?(enndeakin)
Keywords: checkin-needed
Dear Sheriff,

when you merge this to M-C, could you please correct the reviewer. Is that possible?
It's not r=ehsan but r=roc.

Sorry about that, reviewer changed in the middle of the process.
Flags: needinfo?(wkocher)
Flags: needinfo?(cbook)
We can't change that during the merge. If it's really important to fix, we can back out the original commit and reland it with the corrected reviewer, but usually it's fine to just note in the bug that the reviewer in the commit message was incorrect.
Flags: needinfo?(wkocher)
Flags: needinfo?(cbook)
(In reply to Wes Kocher (:KWierso) from comment #20)
> If it's really important to fix, we can back out the original commit
> and reland it with the corrected reviewer,
"Important" is in the eye of the beholder. Ehsan cancelled the review in comment #16 and told me to find another reviewer, which I did. It's unfortunate that he is now featured as the reviewer in the commit comment.

It's quite annoying that you can't change commit comments (well, you can before the push with --amend) and that your merge doesn't allow it either.

I think backing out and relanding is OK, as long as you don't automatically run the entire test suite again.

I leave it to your judgement.
https://hg.mozilla.org/mozilla-central/rev/f859932803e8
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
status-firefox47: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
Please note: The reviewer was Robert O'Callahan (:roc) and not Ehsan as stated in the commit message.
Blocks: 1253288
Blocks: 1248971
Depends on: 1263883
Blocks: 1265034
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: