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)

Product:
Core
Component:
DOM: Core & HTML
Version:
55 Branch
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla58
Tracking Flags:
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)

trigger.html
(deleted), text/html
Details
log_minidump.txt
(deleted), text/plain
Details
log_stderr.txt
(deleted), text/plain
Details
Be consistent (and follow the spec) in terms of how we determine the element that propagates styles to the viewport
(deleted), patch
dholbert
: review+
Details | Diff | Splinter Review
Attached file trigger.html (deleted) —
Testcase found while fuzzing mozilla-central rev 0bd9b61304e2.
Flags: in-testsuite?
Attached file log_minidump.txt (deleted) —
Attached file log_stderr.txt (deleted) —
Version: unspecified → 58 Branch
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
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...
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: nobody → bzbarsky
Status: NEW → ASSIGNED
Flags: needinfo?(bzbarsky)
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 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+
> 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>
Thanks! Yeah, that's what I had in mind.
Fwiw, my try run where I purposefully broke scroll propagation from <body> caught that problem too, by not failing the new reftests... ;)
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
https://hg.mozilla.org/mozilla-central/rev/c1fb8f188a56
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox58: fix-optional → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla58
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: