Closed
Bug 1250010
Opened 9 years ago
Closed 9 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)
Core
DOM: Editor
Tracking
()
RESOLVED
FIXED
mozilla47
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)
(deleted),
text/html
|
Details | |
(deleted),
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.
Assignee | ||
Comment 1•9 years ago
|
||
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•9 years ago
|
||
I was going to mention: This works in Chrome ;-)
Comment 3•9 years ago
|
||
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>
Comment 4•9 years ago
|
||
(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•9 years 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.
Comment 6•9 years ago
|
||
Is this related to bug 1249374?
Flags: needinfo?(mozilla)
Whiteboard: btpp-followup-2016-03-01
Assignee | ||
Comment 7•9 years 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)
Comment 8•9 years ago
|
||
(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•9 years 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.
Comment 10•9 years ago
|
||
(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•9 years ago
|
||
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•9 years ago
|
||
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•9 years ago
|
||
Slightly more elegant option. Let's try it, too:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=a98654d7ad5c
Assignee | ||
Comment 14•9 years ago
|
||
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•9 years ago
|
||
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•9 years 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 16•9 years ago
|
||
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•9 years 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)
Updated•9 years ago
|
Whiteboard: btpp-followup-2016-03-01 → btpp-active
Attachment #8723225 -
Flags: review?(roc) → review+
Assignee | ||
Updated•9 years ago
|
Attachment #8723225 -
Flags: review?(enndeakin)
Assignee | ||
Updated•9 years ago
|
Keywords: checkin-needed
Comment 18•9 years ago
|
||
Keywords: checkin-needed
Assignee | ||
Comment 19•9 years 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•9 years 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•9 years ago
|
||
bugherder |
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
Assignee | ||
Comment 23•9 years ago
|
||
Please note: The reviewer was Robert O'Callahan (:roc) and not Ehsan as stated in the commit message.
Comment 24•9 years ago
|
||
Pushed in THUNDERBIRD_45_VERBRANCH
https://hg.mozilla.org/releases/mozilla-esr45/rev/bab23fd1d89b
You need to log in
before you can comment on or make changes to this bug.
Description
•