Closed Bug 1625218 Opened 5 years ago Closed 4 years ago

Body Text instead of Paragraph shows in the Formatting bar with Use Paragraph format enabled

Categories

(Thunderbird :: Message Compose Window, defect, P1)

Product:
Thunderbird
Component:
Message Compose Window
Platform:
x86_64
All
Type:
defect

Tracking

(thunderbird_esr78 fixed)

Status:
RESOLVED FIXED
Milestone:
Thunderbird 79.0
Tracking Flags:
Tracking Status
thunderbird_esr78 --- fixed

People

(Reporter: walts48, Unassigned)

References

(Regression)

Details

(Keywords: regression, Whiteboard: [fixed by bug 1582410 (which caused five regressions)])

Attachments

(1 file, 1 obsolete file)

bug1625218_toolbar_p.patch
(deleted), patch
Details | Diff | Splinter Review

In preferences set Composition to "Use Paragraph format instead of Body Text by default".

Click "Write" to open a composition window.

Enter an address and subject, or just enter the body section.

Notice that "Body Text" is the default selection in the Formatting bar.

It should be "Paragraph".

Hmm, if the compose window doesn't show the right indicators, that will make for a lot of unhappy users.

Masayuki-san, could you please take a look, this seems to be caused by one of these:

bcaada0fa740bf6d07b95df1e1da820e12f990a9 Masayuki Nakano — Bug 1581337 - Make HTMLEditor::DeleteMostAncestorMailCiteElementIfEmpty() do nothing if found mail-cite element is not empty r=m_kato
71cfe2c9fc79e46e8fe0c360839ca2a5b595c696 Masayuki Nakano — Bug 1581034 - part 2: Get rid of TextEditUtils::IsBreak() and TextEditUtils itself r=m_kato
4e5e43eefae5f5762a3651b44ea08450fe478d83 Masayuki Nakano — Bug 1581034 - part 1: Get rid of TextEditUtils::IsBody() r=m_kato
620fa59408e852fe7cec0e1584ccde64d69e41c9 Masayuki Nakano — Bug 1540029 - part 11: Get rid of AutoEditInitRulesTrigger r=m_kato
72e2b71294b4b30411516c671382b6fba25cb610 Masayuki Nakano — Bug 1540029 - part 10: Get rid of TextEditRules and HTMLEditRules r=m_kato

Severity: normal → S2
Flags: needinfo?(masayuki)
Keywords: regressionwindow-wanted
Priority: -- → P1

Jorg K:

I still don't have idea what changed the initialization behavior. However, according to "Insert HTML" dialog, HTMLEditor puts <p> element with <br> element (and perhaps, it's followed by empty text node), and <pre class="moz-signature"> follows it with preceding empty text node. If so, the DOM tree does not match what NotifyComposeBodyReadyNew expected. Could you fix this on there?

(you don't accept ni?...)

Flags: needinfo?(masayuki)

Thanks for the comment.
https://searchfox.org/comm-central/rev/7979a5b153574449076ca28611f2e6afd5677511/mail/components/compose/content/MsgComposeCommands.js#537-558
has if (insertParagraph && gBodyFromArgs) { just above, so that's code that runs when you start TB with -compose.

The case we're looking at is:
https://searchfox.org/comm-central/rev/7979a5b153574449076ca28611f2e6afd5677511/mail/components/compose/content/MsgComposeCommands.js#530-533
and
https://searchfox.org/comm-central/rev/7979a5b153574449076ca28611f2e6afd5677511/mail/components/compose/content/MsgComposeCommands.js#562-577
where we insert a <p><br></p> and set the state on cmd_paragraphState.

The "mode" indicator "Body Text" or "Paragraph" in Thunderbird is driven off the cmd_paragraphState, I believe. I've just tried

  <body>
    <p>xxx</p>
    <p>yyy</p>
    zzz<br>
  </body>

and clicking into the xxx, yyy or zzz does set the "mode" indicator correctly. So could it be that this line
document.getElementById("cmd_paragraphState").setAttribute("state", "p");
doesn't work any more?

I still don't understand the regression reason, though, nsIHTMLEditor.insertElementAtSelection() behaves unexpectedly for you.
It collapse selection after the <p> element in this case:
https://searchfox.org/mozilla-central/rev/dc4560dcaafd79375b9411fdbbaaebb0a59a93ac/editor/libeditor/HTMLEditor.cpp#1712,1716

Therefore, when you call document.getElementById("cmd_paragraphState").setAttribute("state", "p");, the selection is collapsed outside the <p> element.

And oddly, after that nsIEditor.beginningOfDocument() is called, then, selection will be collapsed into the <p> element (probably). I don't understand why this was previously working, but I guess that you can fix this with swapping order of document.getElementById("cmd_paragraphState").setAttribute("state", "p"); and editor.beginningOfDocument();.

(I really don't understand why the patches caused this regression...)

Attached patch suggestion.patch - Not working (obsolete) (deleted) — — Splinter Review

Like so? That doesn't change anything.

As the bug says, when you start a new composition, you the "mode" indicator shows "Body Text" although paragraphs are inserted. Also, if you create multiple paragraphs, then clicking onto the first one, the indicator is still doesn't display "Paragraph". Also, there is an assortment of disconcerting warnings in the console:

[7636, Main Thread] WARNING: '!aTarget', file c:/mozilla-source/comm-central/editor/libeditor/HTMLEditorObjectResizer.cpp, line 734
[7636, Main Thread] WARNING: HTMLEditor::OnMouseDown() failed, but ignored: 'NS_SUCCEEDED(rvIgnored)', file c:/mozilla-source/comm-central/editor/libeditor/HTMLEditorEventListener.cpp, line 408
[7636, Main Thread] WARNING: HTMLEditor::GetAbsolutelyPositionedSelectionContainer() reached <html> element: file c:/mozilla-source/comm-central/editor/libeditor/HTMLAbsPositionEditor.cpp, line 93

So to me it looks like there's something going quite wrong inside the editor. The last warning looks like it tried to find a selection container, like a paragraph, but for some reason, ascended right to the <html> element without finding it.

Attachment #9146730 - Attachment is patch: true

I think this is triggered by Bug 1581305.

Hey, Alice, yes, that is a good candidate, let's go with that assumption for now. Sorry, when I see editor issues, I immediately jump to M-C, my apologies!

Magnus, a broken "mode" indicator makes decent editing in TB pretty hard. Can you please get this fixed, maybe it's really related to the observer changes you made. cmd_paragraphState was also part of those changes.

Flags: needinfo?(mkmelin+mozilla)
Regressed by: 1581305
Attached patch bug1625218_toolbar_p.patch (deleted) — — Splinter Review

Working... But, I don't understand exactly why both the .doCommand() and window.updateCommands("style") are needed.

Also calling window.updateCommands("style") before editor.beginningOfDocument() etc. won't help.

If I move the editor.beginningOfDocument(); call to up earlier, thing also doesn't work.

Assignee: nobody → mkmelin+mozilla
Status: NEW → ASSIGNED
Flags: needinfo?(mkmelin+mozilla)
Attachment #9146730 - Attachment is obsolete: true

Thanks, I'm glad the bug moved onto the radar and the solution is within reach :-)

Comment on attachment 9146765 [details] [diff] [review] bug1625218_toolbar_p.patch So what's the way forward here? It's all a bit of a mystery to me. The added `doCommand()` calls are from XULElement.webidl, right? No documentation there, so what do these calls do? I can't see any being added in https://hg.mozilla.org/comm-central/rev/f5158588080a. As for the `window.updateCommands("style")` calls? Who can we ask? Masayuki-san, or someone else from the DOM team? Perhaps Emilio?

Bug 1582410 fixes this.

Assignee: mkmelin+mozilla → nobody
Status: ASSIGNED → NEW
Depends on: 1582410
Whiteboard: smoketestbeta → smoketestbeta, [the fix is bug 1582410]
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Whiteboard: smoketestbeta, [the fix is bug 1582410] → [fixed by bug 1582410]
Target Milestone: --- → Thunderbird 79.0
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Target Milestone: Thunderbird 79.0 → ---
Whiteboard: [fixed by bug 1582410] → [fixed by bug 1582410 (which caused five regressions)]

The try build
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=e03a557a6bfe67461dba44a851bf0c5a8278f975
"mostly" works and restores TB 68 functionality. The only thing that doesn't work is that when starting up the compose window with "Fixed width" as composing preference, it's not shown until the first text is typed. Maybe we can improve on that here.

Summary: Body Text instead of Paragraph shows in the Formatting bar with Use Paragraph format enabled → Body Text instead of Paragraph shows in the Formatting bar with Use Paragraph format enabled (TB 78 ESR only)

Comment on attachment 9146765 [details] [diff] [review]
bug1625218_toolbar_p.patch

Sigh, I tested this some more. It works well enough in paragraph mode, but it doesn't work at all in "body text" mode. Can this be made to work, or do we have to abandon the idea of using this instead of bug 1582410 and all the regressions it causes?

Attachment #9146765 - Flags: review+
Attachment #9146765 - Flags: approval-comm-esr78?

OK, let's return the bug to the state at comment #12.
Summary: This was fixed by bug 1582410 and the regressions of the latter were fixed bug bug 1658999.

Status: REOPENED → RESOLVED
Closed: 4 years ago → 4 years ago
status-thunderbird_esr78: affected → fixed
tracking-thunderbird_esr78: ? → ---
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 79.0
Summary: Body Text instead of Paragraph shows in the Formatting bar with Use Paragraph format enabled (TB 78 ESR only) → Body Text instead of Paragraph shows in the Formatting bar with Use Paragraph format enabled
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: