Closed Bug 756045 Opened 12 years ago Closed 12 years ago

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

Categories

(Core :: CSS Parsing and Computation, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla15

People

(Reporter: ayg, Assigned: ayg)

References

Details

Attachments

(1 file, 1 obsolete file)

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-
Attached patch Patch v1 (obsolete) — Splinter Review
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
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]
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/mozilla-central/rev/8be8a188bd60
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: