Turn "ASSERTION: anonymous nodes should not be in child lists" into an NS_WARNING

RESOLVED FIXED in mozilla15

Status

()

Core
CSS Parsing and Computation
--
enhancement
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: ayg, Assigned: ayg)

Tracking

Trunk
mozilla15
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

See bug 751842 comment 32.  We know about the assertion, and we aren't going to fix it anytime soon.  It gets hit a million time during editor tests, which both slows them down and clogs the logs.
Flags: in-testsuite-
Created attachment 624687 [details] [diff] [review]
Patch v1

This might need more test updates.
Attachment #624687 - Flags: review?(bzbarsky)
Whiteboard: [autoland-try:-u all]
Assignee: ayg → nobody
Component: Editor → Style System (CSS)
QA Contact: editor → style-system

Updated

5 years ago
Whiteboard: [autoland-try:-u all] → [autoland-in-queue]
Bah, autoland is flaky.  Manual try push: https://tbpl.mozilla.org/?tree=Try&rev=eb9a4fff4e2f
Whiteboard: [autoland-in-queue]
Created attachment 624724 [details] [diff] [review]
Patch v2, try failure fixed
Assignee: nobody → ayg
Attachment #624687 - Attachment is obsolete: true
Attachment #624687 - Flags: review?(bzbarsky)
Attachment #624724 - Flags: review?(bzbarsky)
Why are editor tests hitting this?  The manual insanity it does with the resize handles or something?
I'll leave Ehsan to answer that . . .
(In reply to Boris Zbarsky (:bz) from comment #4)
> Why are editor tests hitting this?  The manual insanity it does with the
> resize handles or something?

There's a ton of cases where you can hit this from content script.  Calling .focus() on an abs-pos editable element is one example.
> Calling .focus() on an abs-pos editable element is one example.

Er.... why does that hit this assert?

Basically, if hitting this case is nominally a bug, we should document here why this is a warning, not an assert, and document at the buggy callsites that once they're fixed this should become an assert.

If this case is not nominally a bug, we shouldn't have a warning.
(In reply to Boris Zbarsky (:bz) from comment #7)
> > Calling .focus() on an abs-pos editable element is one example.
> 
> Er.... why does that hit this assert?

Because that triggers the resizing and positioning controls to be displayed on focus, and we hit this assertion once for every one of those controls.

> Basically, if hitting this case is nominally a bug, we should document here
> why this is a warning, not an assert, and document at the buggy callsites
> that once they're fixed this should become an assert.
> 
> If this case is not nominally a bug, we shouldn't have a warning.

The fact that we hit this assert is because we inject those controls in a very bad way, but that bad way works as expected, so this is technically not a bug.  It's just ugly things that we should not be doing.  The proper way of doing this is to add those as anonymous abs-pos children of the element they're supposed to control, but doing that requires us to enable all sorts of frames to be abs-pos containers, and also make sure that elements such as images can deal with anonymous children, so it is a longer term project.
Oh, I'd somehow misread "editable element" as "text input".  I have _no_ idea why, and I'm sorry about the confusion.

So yes, this is all about the resize handles.

Aryeh, please add comments to the code where the warning is pointing that out, and please add code comments to the caller of ContentRemoved caller in editor about reinstating the assert once that caller is fixed.
Comment on attachment 624724 [details] [diff] [review]
Patch v2, try failure fixed

r=me with my requested comment fixes.
Attachment #624724 - Flags: review?(bzbarsky) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/8be8a188bd60
Target Milestone: --- → mozilla15
https://hg.mozilla.org/mozilla-central/rev/8be8a188bd60
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.