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

RESOLVED FIXED in Firefox 47

Status

()

Core
Editor
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: Jorg K (GMT+2), Assigned: Jorg K (GMT+2))

Tracking

(Blocks: 1 bug)

Trunk
mozilla47
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox47 fixed)

Details

(Whiteboard: btpp-active)

Attachments

(2 attachments, 4 obsolete attachments)

(Assignee)

Description

a year ago
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.
(Assignee)

Comment 1

a year ago
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.
(Assignee)

Comment 2

a year ago
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)
(Assignee)

Comment 5

a year ago
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
(Assignee)

Comment 7

a year ago
(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
(Assignee)

Comment 9

a year ago
(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 :)
(Assignee)

Comment 11

a year ago
Created attachment 8723110 [details] [diff] [review]
WIP: This patch fixes the problem (v1).

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
(Assignee)

Comment 12

a year ago
Created attachment 8723128 [details] [diff] [review]
WIP: This patch fixes the problem (v2).

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
(Assignee)

Comment 13

a year ago
Created attachment 8723151 [details] [diff] [review]
WIP: This patch fixes the problem (v3).

Slightly more elegant option. Let's try it, too:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=a98654d7ad5c
(Assignee)

Comment 14

a year ago
Created attachment 8723171 [details] [diff] [review]
WIP: This patch fixes the problem (v3a).

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
(Assignee)

Comment 15

a year ago
Created attachment 8723225 [details] [diff] [review]
Proposed solution (v1) with test

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)
(Assignee)

Updated

a year ago
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)
(Assignee)

Comment 17

a year ago
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?(roc) → review+
(Assignee)

Updated

a year ago
Attachment #8723225 - Flags: review?(enndeakin)
(Assignee)

Updated

a year ago
Keywords: checkin-needed

Comment 18

a year ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/f859932803e8
Keywords: checkin-needed
(Assignee)

Comment 19

a year ago
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)
(Assignee)

Comment 21

a year ago
(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.

Comment 22

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/f859932803e8
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
status-firefox47: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
(Assignee)

Comment 23

a year ago
Please note: The reviewer was Robert O'Callahan (:roc) and not Ehsan as stated in the commit message.
(Assignee)

Updated

a year ago
Blocks: 1253288
Pushed in THUNDERBIRD_45_VERBRANCH
https://hg.mozilla.org/releases/mozilla-esr45/rev/bab23fd1d89b
(Assignee)

Updated

a year ago
Blocks: 1248971
(Assignee)

Updated

a year ago
Depends on: 1263883

Updated

a year ago
Blocks: 1265034
You need to log in before you can comment on or make changes to this bug.