Closed
Bug 1411138
Opened 7 years ago
Closed 7 years ago
Assertion failure: !OwnerDoc()->IsScrollingElement(this) (How can we have a scrollframe if we're the scrollingElement for our document?), at /builds/worker/workspace/build/src/dom/base/Element.cpp:700
Categories
(Core :: DOM: Core & HTML, defect)
Tracking
()
RESOLVED
FIXED
mozilla58
Tracking | Status | |
---|---|---|
firefox-esr52 | --- | unaffected |
firefox56 | --- | wontfix |
firefox57 | --- | wontfix |
firefox58 | --- | fixed |
People
(Reporter: jkratzer, Assigned: bzbarsky)
References
(Blocks 1 open bug)
Details
(Keywords: assertion, testcase)
Attachments
(4 files)
Testcase found while fuzzing mozilla-central rev 0bd9b61304e2.
Flags: in-testsuite?
Reporter | ||
Comment 1•7 years ago
|
||
Reporter | ||
Comment 2•7 years ago
|
||
Reporter | ||
Updated•7 years ago
|
Version: unspecified → 58 Branch
Comment 3•7 years ago
|
||
INFO: Last good revision: 9883fe74207112ee8451d127a834835f2ac28604
INFO: First bad revision: dd1c5aecc373a8bbba824c445a4a8c4e127adda0
INFO: Pushlog:
https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=9883fe74207112ee8451d127a834835f2ac28604&tochange=dd1c5aecc373a8bbba824c445a4a8c4e127adda0
So all the way back to when the assert got added.
Blocks: 1364360
Has Regression Range: --- → yes
status-firefox56:
--- → wontfix
status-firefox57:
--- → wontfix
status-firefox58:
--- → fix-optional
status-firefox-esr52:
--- → unaffected
Flags: needinfo?(bzbarsky)
Version: 58 Branch → 55 Branch
Assignee | ||
Comment 4•7 years ago
|
||
So https://drafts.csswg.org/cssom-view/#dom-document-scrollingelement is defined in terms of https://drafts.csswg.org/cssom-view/#the-html-body-element which is the first <body> child of <html>.
Our scroll propagation is implemented in terms of nsHTMLDocument::GetBody which implements https://html.spec.whatwg.org/multipage/dom.html#dom-document-body which is defined in terms of https://html.spec.whatwg.org/multipage/dom.html#the-body-element-2
This mismatch is what's causing the assertion: We don't propagate scroll, but think we're the scrollingElement.
https://www.w3.org/TR/CSS21/visufx.html#overflow-clipping says:
UAs must apply the 'overflow' property set on the root element to the viewport.
When the root element is an HTML "HTML" element or an XHTML "html" element, and
that element has an HTML "BODY" element or an XHTML "body" element as a child,
user agents must instead apply the 'overflow' property from the first such child
element to the viewport, if the value on the root element is 'visible'.
which suggests to me that scroll propagation should also use nsIDocument::GetBodyElement() instead of nsHTMLDocument::GetBody...
Assignee | ||
Comment 5•7 years ago
|
||
Looks like Safari does the scroll propagation sort of right and no one else does. I also filed https://github.com/w3c/csswg-drafts/issues/1905 on the css-overflow draft having a worse definition than CSS2.1...
Assignee | ||
Comment 6•7 years ago
|
||
MozReview-Commit-ID: 8tH3nCDQJID
Attachment #8921971 -
Flags: review?(dholbert)
Assignee | ||
Updated•7 years ago
|
Assignee: nobody → bzbarsky
Status: NEW → ASSIGNED
Assignee | ||
Updated•7 years ago
|
Flags: needinfo?(bzbarsky)
Comment 7•7 years ago
|
||
Comment on attachment 8921971 [details] [diff] [review]
Be consistent (and follow the spec) in terms of how we determine the element that propagates styles to the viewport
Review of attachment 8921971 [details] [diff] [review]:
-----------------------------------------------------------------
::: testing/web-platform/meta/MANIFEST.json
@@ +417141,5 @@
> "d2c5d8d1696112b771a332011c4f33065817ed9a",
> "testharness"
> ],
> "XMLHttpRequest/open-url-encoding.htm": [
> + "a36c7b0e5919af7842883582ef9fc415d8f7ef25",
FWIW: all these unrelated hash changes in the manifest are triggering patch-rejection for me, when I try to apply the patch -- it seems they've already been made on current mozilla-inbound, over in https://hg.mozilla.org/integration/mozilla-inbound/rev/f95a678186b4 (bug 1410245)
Comment 8•7 years ago
|
||
Comment on attachment 8921971 [details] [diff] [review]
Be consistent (and follow the spec) in terms of how we determine the element that propagates styles to the viewport
Review of attachment 8921971 [details] [diff] [review]:
-----------------------------------------------------------------
r=me, but I've got one observation/suggestion for the reference case:
::: testing/web-platform/tests/css/CSS2/visufx/support/overflow-propagation-001-ref.html
@@ +5,5 @@
> + document.body.remove();
> + var b = document.createElement("body");
> + b.style = "overflow: hidden; margin: 100px; width: 100px; height: 100px; border: 1px solid green; position: absolute; top: 0; left: 0";
> + b.textContent = "The body should have visible overflow of the text that totally doesn't fit in the little box.";
> + document.documentElement.appendChild(b);
This seems a bit complex/scripted, for a reference case. (and unnecessarily so, IIUC)
Perhaps this should just be static HTML, without 'overflow' set on the <body> at all? (Something close to overflow-propagation-001a.html, but with 'overflow' set directly on <html>)
<!DOCTYPE html>
<meta charset=utf-8>
<html style="overflow:hidden">
<body style="margin: 100px; width: 100px; height: 100px;
border: 1px solid green; position: absolute;
top: 0; left: 0">
The body should have visible overflow of the
text that totally doesn't fit in the little box.
</body>
</html>
Attachment #8921971 -
Flags: review?(dholbert) → review+
Assignee | ||
Comment 9•7 years ago
|
||
> This seems a bit complex/scripted, for a reference case.
Er.. that's just wrong. the actual reference should be:
<!doctype html>
<html style="overflow: hidden">
<meta charset=utf-8>
<body style="margin: 100px; width: 100px; height: 100px; border: 1px solid green; position: absolute; top: 0; left: 0">
The body should have visible overflow of the text that totally doesn't fit
in the little box.
</body>
Comment 10•7 years ago
|
||
Thanks! Yeah, that's what I had in mind.
Assignee | ||
Comment 11•7 years ago
|
||
Fwiw, my try run where I purposefully broke scroll propagation from <body> caught that problem too, by not failing the new reftests... ;)
Comment 12•7 years ago
|
||
Pushed by bzbarsky@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/c1fb8f188a56
Be consistent (and follow the spec) in terms of how we determine the element that propagates styles to the viewport. r=dholbert
Comment 13•7 years ago
|
||
bugherder |
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
Updated•6 years ago
|
Component: DOM → DOM: Core & HTML
You need to log in
before you can comment on or make changes to this bug.
Description
•