Closed
Bug 1223561
Opened 9 years ago
Closed 8 years ago
"Assertion failure: !aBuilder->IsForPainting() || !ShouldInTopLayerForFullscreen(elem)" with full-screened HTML <table>
Categories
(Core :: Web Painting, defect)
Core
Web Painting
Tracking
()
RESOLVED
FIXED
mozilla53
People
(Reporter: jruderman, Assigned: xidorn)
References
(Blocks 1 open bug)
Details
(Keywords: assertion, testcase)
Attachments
(3 files)
Assertion failure: !aBuilder->IsForPainting() || !ShouldInTopLayerForFullscreen(elem), at layout/generic/nsViewportFrame.cpp:130
|
Reporter | |
Comment 1•9 years ago
|
||
|
||
Updated•8 years ago
|
Component: Layout → Layout: View Rendering
|
Assignee | |
Comment 2•8 years ago
|
||
It would be great if someone brought this to me earilier :/
Blocks: fullscreen-api
|
|
Assignee | |
Updated•8 years ago
|
Flags: needinfo?(xidorn+moz)
|
|
Assignee | |
Comment 3•8 years ago
|
||
So the issue here is, when we want to put <table> into fullscreen, we are actually putting nsTableWrapperFrame into fullscreen, but the wrapper frame doesn't inherit the top layer property, so it wouldn't be positioned correctly.
Assignee: nobody → xidorn+moz
Flags: needinfo?(xidorn+moz)
| Comment hidden (mozreview-request) |
|
||
Comment 5•8 years ago
|
||
| mozreview-review | ||
Comment on attachment 8813024 [details]
Bug 1223561 - Support putting <table> into fullscreen.
https://reviewboard.mozilla.org/r/94546/#review95048
::: dom/html/test/file_fullscreen-table.html:1
(Diff revision 1)
> +<!DOCTYPE html>
I presume you've tested that this fails without the patch?
::: dom/html/test/mochitest.ini:74
(Diff revision 1)
> file_fullscreen-plugins.html
> file_fullscreen-rollback.html
> file_fullscreen-scrollbar.html
> file_fullscreen-selector.html
> file_fullscreen-svg-element.html
> + file_fullscreen-table.html
Please associate new support files with the test that they're a support file for, not the directory as a whole. (Feel free to fix the old ones for this test while you're there, too.)
(Listing them as support files for all tests in the directory was just a hack to migrate to mochitest.ini from the old Makefile.in solution.)
::: layout/generic/nsPlaceholderFrame.cpp:209
(Diff revision 1)
> + nsIFrame* parentFrame = GetParent();
> + if ((GetStateBits() & PLACEHOLDER_FOR_TOPLAYER) &&
> + parentFrame->GetType() == nsGkAtoms::tableWrapperFrame) {
> + MOZ_ASSERT(mOutOfFlowFrame->GetType() == nsGkAtoms::backdropFrame,
> + "Only placeholder of backdrop frame can be put inside "
> + "a table wrapper frame");
> + return parentFrame->StyleContext();
> + }
I don't understand why you need to do make the placeholder inherit from the table wrapper frame. Please add a comment explaining why.
::: layout/style/res/ua.css:26
(Diff revision 1)
> box-sizing: border-box; /* XXX do we really want this? */
> }
>
> *|*::-moz-table-wrapper {
> display: inherit !important; /* table or inline-table */
> + -moz-top-layer: inherit !important;
I think this is correct since the -moz-top-layer looks like (from reading nsCSSFrameConstructor) it will probably be ignored for the table frame. But I'd like to confirm that agrees with your understanding.
(I also suspect there should perhaps be a comment here that says the inherited properties need to be safe to have on both the table and the table wrapper, generally because code ignores them for the table.)
Attachment #8813024 -
Flags: review?(dbaron) → review+
|
|
Assignee | |
Comment 6•8 years ago
|
||
| mozreview-review-reply | ||
Comment on attachment 8813024 [details]
Bug 1223561 - Support putting <table> into fullscreen.
https://reviewboard.mozilla.org/r/94546/#review95048
> I presume you've tested that this fails without the patch?
I didn't actually run the test itself without the patch... but I manually tested the prototype of this test, which shows in debug build, it would crash because of the assertion, and in release build, the table would not be put into fullscreen.
> I don't understand why you need to do make the placeholder inherit from the table wrapper frame. Please add a comment explaining why.
It is not necessary to make the backdrop placeholder inherit from the table wrapper frame. This is just a consequence of the backdrop construction code (see nsFrameConstructorState::ConstructBackdropFrameFor).
I can make it inherit from the table frame (while still be a child of the table wrapper frame), but that wouldn't make anything simpler: it would still need a special handling in this method, because CorrectStyleParentFrame below only goes upwards along the frame tree, thus it wouldn't reach the table frame from the table wrapper frame either.
Another option is to make backdrop placeholder a child of the table frame instead. However, what really is in the top layer is the table wrapper frame, which means making backdrop placeholder under the table frame would require extra special handling in both frame construction code and painting code.
So it seems to me having it inherit from table wrapper frame is the simplest approach, and I don't see any reason we shouldn't be doing so.
|
|
Assignee | |
Comment 7•8 years ago
|
||
| mozreview-review-reply | ||
Comment on attachment 8813024 [details]
Bug 1223561 - Support putting <table> into fullscreen.
https://reviewboard.mozilla.org/r/94546/#review95048
> I think this is correct since the -moz-top-layer looks like (from reading nsCSSFrameConstructor) it will probably be ignored for the table frame. But I'd like to confirm that agrees with your understanding.
>
> (I also suspect there should perhaps be a comment here that says the inherited properties need to be safe to have on both the table and the table wrapper, generally because code ignores them for the table.)
Yes, that matches my understanding. -moz-top-layer is handled in nsFrameConstructorState::AddChild, but table frame is appended to its parent (the table wrapper frame) via SetInitialChildList directly.
| Comment hidden (mozreview-request) |
Pushed by xquan@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/535f8682016b
Support putting <table> into fullscreen. r=dbaron
|
||
Comment 10•8 years ago
|
||
Sorry had to back out for test_fullscreen-api.html timeout, https://treeherder.mozilla.org/logviewer.html#?job_id=7078671&repo=autoland#L6339
Flags: needinfo?(xidorn+moz)
|
||
Comment 11•8 years ago
|
||
Backout by ihsiao@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/4044c905be7b
Backed out changeset 535f8682016b for test_fullscreen-api.html timeout
|
|
Assignee | |
Comment 12•8 years ago
|
||
Hmmm, it isn't really timeout. It crashes on some Linux opt builds inside ElementRestyler::RestyleSelf...
| Comment hidden (mozreview-request) |
|
|
Assignee | |
Updated•8 years ago
|
Flags: needinfo?(xidorn+moz)
|
|
||
Comment 14•8 years ago
|
||
Pushed by xquan@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/a214a23b603e
Support putting <table> into fullscreen. r=dbaron
|
||
Comment 15•8 years ago
|
||
| bugherder | ||
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox53:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla53
|
||
Updated•6 years ago
|
Component: Layout: View Rendering → Layout: Web Painting
You need to log in
before you can comment on or make changes to this bug.
Description
•