Closed Bug 1727008 Opened 3 years ago Closed 3 years ago

Failing message composition tests after bug 1726064

Categories

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

Product:
Core
Component:
DOM: Editor
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
93 Branch
Tracking Flags:
Tracking Status
firefox-esr78 --- unaffected
firefox-esr91 --- unaffected
firefox91 --- unaffected
firefox92 --- unaffected
firefox93 --- fixed

People

(Reporter: darktrojan, Assigned: masayuki)

References

(Regression)

Details

(Keywords: regression)

Attachments

(1 file)

Bug 1727008 - `HTMLEditor` shouldn't strip `<html>` element nor `<body>` elements r=m_kato!
48 bytes, text/x-phabricator-request
Details | Review

The failing tests are:

  • comm/mail/test/browser/composition/browser_attachmentDragDrop.js
  • comm/mail/test/browser/composition/browser_imageInsertionDialog.js
  • comm/mail/test/browser/content-policy/browser_composeMailto.js

All have nsIHTMLEditor.insertElementAtSelection returning NS_ERROR_FAILURE. I backed out 30478b345a7e1a0bfab8c20dcc9a8784efb36a79 and the tests pass.

Flags: needinfo?(masayuki)

Do you see warnings of debug build at the XPCOM API returning error? Currently, editor's warnings show the failing stack and lines.

Flags: needinfo?(masayuki) → needinfo?(geoff)

Is this what you're after?

Edit: here's another one. The third test doesn't run in debug.

Flags: needinfo?(geoff)

Thanks, perhaps, this is what I want to know:

WARNING: '!editingHost', file /builds/worker/checkouts/gecko/editor/libeditor/HTMLEditor.cpp:752
WARNING: '!editingHost', file /builds/worker/checkouts/gecko/editor/libeditor/HTMLEditor.cpp:1856
WARNING: '!targetElement', file /builds/worker/checkouts/gecko/editor/libeditor/EditorBase.cpp:2353
WARNING: HTMLEditor::InsertElementAtSelectionAsAction() failed: 'NS_SUCCEEDED(rv)', file /builds/worker/checkouts/gecko/editor/libeditor/HTMLEditor.cpp:1730
[Exception... "Component returned failure code: 0x80004005 (NS_ERROR_FAILURE) [nsIHTMLEditor.insertElementAtSelection]" nsresult: "0x80004005 (NS_ERROR_FAILURE)" location: "JS frame :: chrome://messenger/content/messengercompose/EdImageProps.js :: onAccept :: line 235" data: no]

So, it does not provide the stack at failing, but it's also a big hint.

Really odd. All error cases put warnings:
https://searchfox.org/mozilla-central/source/editor/libeditor/HTMLEditor.cpp#1736-1737,1743-1746,1759-1762,1772-1773,1780-1781,1789-1790,1797-1798,1813-1816,1821-1823,1835-1836,1843-1844,1856-1857,1866-1867,1873-1876,1881-1882,1885-1886,1889-1890,1894-1895,1913-1915

Anyway, if ScanEmptyBlockInclusiveAncestor returns nullptr and it causes error of InsertHTMLAtSelectionAsAction, it seems that the HTMLEditor works with nested editing hosts unless my patch does wrong. However, there are a lot of warning of non-editable situations, e.g., not found an editing host from selection, absolutely positioned element handling reached <html> element at scanning such element. So, it seems that the mail code depends on buggy behavior before landing my patch.

If you wrap those lines with try {} catch (ex) {}, what happends?

Ignoring the exception causes unexpected content staying and expected content want's inserted? (And I don't think that Thunderbird uses contenteditable="false" in its UI code. So, I'm really confused why the patch affects to Tb since the patches try to fix nested contenteditable elements issues. So, for solving it, editable state is checked stricter than before in some places.)

I wonder, how can I use Developer Tools in the email composing window?

It's not perfect, but you can select messengercompose.xhtml from the menu top-right corner (next to the three-dots menu). The debugger may or many not work from there, it's not very reliable.

Thanks! It's enough to check the DOM tree.

Ah, I read the warnings wrong. Perhaps, it deletes <body> and/or <html> unexpectedly due to they can be empty. Then, the editor loose edting host and returns error.

Assignee: nobody → masayuki
Severity: -- → S2
Status: NEW → ASSIGNED
Component: Composition → DOM: Editor
Keywords: regression
Priority: -- → P1
Product: MailNews Core → Core

That's great. Sorry to take up your entire Monday!

I've checked the tests that failed, and started a Try-CC run just in case something else turns up: https://treeherder.mozilla.org/jobs?repo=try-comm-central&revision=5332bd9de3da91957fa7437f8ad11d4550a3ce1c

The new editor utility method does not stop scanning editable elements even if
it reaches the document root nor the (primary) <body> element. Of course,
they should stop there if scanning editable block. And
ScanEmptyBlockInclusiveAncestor() shouldn't store the removable empty block
element to them.

Depends on D123312

(In reply to Geoff Lankow (:darktrojan) from comment #11)

That's great. Sorry to take up your entire Monday!

I've checked the tests that failed, and started a Try-CC run just in case something else turns up: https://treeherder.mozilla.org/jobs?repo=try-comm-central&revision=5332bd9de3da91957fa7437f8ad11d4550a3ce1c

No worry. It's 100% of my mistake. I need to check similar code.

Pushed by masayuki@d-toybox.com:
https://hg.mozilla.org/integration/autoland/rev/2139b3543587
`HTMLEditor` shouldn't strip `<html>` element nor `<body>` elements r=m_kato
Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/30122 for changes under testing/web-platform/tests

Now there's a new problem: https://treeherder.mozilla.org/logviewer?job_id=349228271&repo=try-comm-central&lineNumber=26446

Still, 1 test failure is better than 3.

(In reply to Geoff Lankow (:darktrojan) from comment #16)

Now there's a new problem: https://treeherder.mozilla.org/logviewer?job_id=349228271&repo=try-comm-central&lineNumber=26446

Still, 1 test failure is better than 3.

The first try had a bug. Could you retry it with the landed one?

(Sorry, I'm now in my bed.)

Flags: needinfo?(geoff)

Set release status flags based on info from the regressing bug 1726064

status-firefox91: --- → unaffected
status-firefox92: --- → unaffected
status-firefox93: --- → affected
status-firefox-esr78: --- → unaffected
status-firefox-esr91: --- → unaffected

https://hg.mozilla.org/mozilla-central/rev/2139b3543587

Status: ASSIGNED → RESOLVED
Closed: 3 years ago
status-firefox93: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → 93 Branch
Upstream PR merged by moz-wptsync-bot

It's still there, I'll make a new bug for it since this one closed.

Flags: needinfo?(geoff)
Regressions: 1727201
No longer regressions: 1727201
Has Regression Range: --- → yes
No longer regressions: 1728433
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: