Bug 1355924 adds a new animation that is implemented by a wide SVG image 630px wide by 20px high. The markup looks like this: <toolbarbutton> <image id=a/> <hbox id=b> <image id=c/> </hbox> </toolbarbutton> #a is an image that has an intrinsic size of 16x16 #b has position:fixed and overflow:hidden, and a size of 18w x 20h #c has a background-image set, and has a size of 630w x 20h When the size of #b is set using `width: 18px; height: 20px;` we get an assertion for: "inline-size less than zero: 'aContainingBlockISize >= 0'". When the size of #b is set using `min-width: 18px; max-width: 18px; min-height: 20px; max-height: 20px;` the assertion goes away. Both ways of setting width achieve the same visual output.

STR (using mochitest mentioned in bug 1382946): 1. Apply this patch. 2. ./mach mochitest ./layout/style/test/chrome/test_display_mode_reflow.html ACTUAL RESULTS: Pretty soon, this goes by in your output: 4 INFO TEST-PASS | layout/style/test/chrome/test_display_mode_reflow.html | offset top returns to original value GECKO(3132) | [3132, Main Thread] ###!!! ASSERTION: inline-size less than zero: 'aContainingBlockISize >= 0', file /scratch/work/builds/mozilla-inbound/mozilla/layout/generic/nsFrame.cpp, line 5830

The negative size comes from this line of CalculateHypotheticalPosition: aCBSize = aFrame->GetLogicalSize(wm) - borderPadding.Size(wm); https://dxr.mozilla.org/mozilla-central/rev/33b7b8e81b4befcba503c0e48cd5370aeb715085/layout/generic/ReflowInput.cpp#1133 At this point, we have the following variable values: * aFrame->mRect is 0,0,0,0. So aFrame->GetLogicalSize returns a 0-sized rect. * borderPadding is top=0, right=120, bottom=0, left=120. So, aCBSize gets a width (i.e. iSize) of 0 - 120 - 120 = -240. That aCBSize gets returned (by reference) up to the caller, GetHypotheticalBoxContainer (where it's called "blockContentSize"), and its negative ISize gets passed down into ComputeISizeValue, which calls mFrame->ComputeISizeValue, which has this precondition.

Hmm -- I tried to circle back to this today, but the tests noted in bug 1382946 [test_display_mode*] are now failing with an unrelated error on my local linux desktop & laptop (including if I update to a cset from ~1 month ago, the day when I posted comment 2). And they don't trigger the assertion (because they don't get to it). The errors look like this: 15 INFO TEST-UNEXPECTED-FAIL | layout/style/test/chrome/test_display_mode.html | TypeError: style is null - Should not throw any errors queryApplies@chrome://mochitests/content/chrome/layout/style/test/chrome/test_display_mode.html:38:5 17 INFO TEST-UNEXPECTED-FAIL | layout/style/test/chrome/test_display_mode_reflow.html | TypeError: secondDiv is null - Should not throw any errors @chrome://mochitests/content/chrome/layout/style/test/chrome/test_display_mode_reflow.html:38:7 Not sure what changed about my environment to make that problem start happening. However, it's OK, because I have simpler STR now -- simply activate fullscreen mode via the fullscreen API, e.g. by clicking "Launch Fullscreen" here: http://davidwalsh.name/demo/fullscreen.php That reliably triggers the assertion for me, in a build with attachment 8912447 [details] [diff] [review] applied.

OK -- so the question from comment 3 here is, why does our containing block have a 0-sized frame rect (i.e. border-box size), despite having nonzero border + padding? The answer is: it's "XUL collapsed", which triggers this code: > 173 // See if we are collapsed. If we are, then simply iterate over all our > 174 // children and give them a rect of 0 width and height. > 175 if (aBox->IsXULCollapsed()) { > 176 nsIFrame* child = nsBox::GetChildXULBox(aBox); > 177 while(child) > 178 { > 179 nsBoxFrame::LayoutChildAt(aState, child, nsRect(0,0,0,0)); https://dxr.mozilla.org/mozilla-central/rev/20d57b9c4183973af4af5e078dff2aec0b74f928/layout/xul/nsSprocketLayout.cpp#179 The LayoutChildAt() call stomps on the frame's mRect with the nsRect(0,0,0,0) arg there. We should probably be setting the frame's "LogicalUsedBorderAndPadding" to 0,0,0,0 there as well, to reflect reality. That's the source of the (currently nonzero) "borderPadding" that we're working with in the code discussed in comment 3.

Specifically, I suspect we should probably add an additional special-case to nsFrame::GetUsedBorder/GetUsedPadding to make it return 0 if we're XUL and XUL-Collapsed. We already do this for some other special cases, e.g. SVG Text frames, and this seems to be the correct behavior for XUL Collapse, based on this MDN documentation about "visibility:collapse" (which is what's used to determine XUL-collapsedness for XUL frames): # For XUL elements, the computed size of the element is always zero, # regardless of other styles that would normally affect the size, # although margins still take effect. https://developer.mozilla.org/en-US/docs/Web/CSS/visibility That's the best documentation I've been able to find on what "XUL Collapsed" means, BTW -- the only other documentation I could find on this feature is a brief hand-wavy statement here ("the element is collapsed and does not appear"): https://developer.mozilla.org/en-US/docs/Mozilla/Tech/XUL/Attribute/collapsed

(In reply to Daniel Holbert [:dholbert] from comment #6) > Specifically, I suspect we should probably add an additional special-case to > nsFrame::GetUsedBorder/GetUsedPadding to make it return 0 if we're XUL and > XUL-Collapsed. Er, that's not correct, actually. In the code I quoted in comment 5, the *parent* is XUL collapsed, and its *children* are the things with a 0-sized rect (whose border/padding we want to ignore, and whose IsXULCollapsed() methods won't return true even though this frame is 0-sized). Let's just modify the GetHypotheticalBoxContainer code to detect this case (parent being IsXULCollapsed) and avoid subtracting out the borderpadding in that case.

Fwiw, I think we should just WONTFIX minor XUL layout issues like this bug. To quote roc from three years ago: "XUL is a dead-end technology and investment in XUL provides minimal returns --- this includes effort spent reviewing and maintaining XUL." https://groups.google.com/forum/#!msg/mozilla.dev.platform/HDJl1iBifB8/1TG876POw8cJ

Comment on attachment 8920738 [details] Bug 1379332: When computing abspos CB content-box size, don't bother subtracting borderpadding if CB is a 0-sized child of a XUL-collapsed frame. https://reviewboard.mozilla.org/r/191752/#review197374 LGTM, but please don't ask me to review XUL changes in the future, unless they have the explicit goal of helping us transition from XUL to HTML/CSS in our UI; or security issues. Other stuff will get automatic r-.

(In reply to Mats Palmgren (:mats) from comment #10) > LGTM, but please don't ask me to review XUL changes in the future, > unless they have the explicit goal of helping us transition from XUL > to HTML/CSS in our UI; or security issues. Yeah -- I wouldn't have even bothered to fix this, except that it's caused a super-gross pattern to start accumulating in our frontend CSS (to work around this bug) -- for example: https://dxr.mozilla.org/mozilla-central/rev/ce1a86d3b4db161c95d1147676bbed839d7a4732/browser/themes/shared/toolbarbutton-icons.inc.css#108-115 I don't want that pattern to keep spreading, hence trying to get this closed out. (And I'm cleaining up that CSS hackery in bug 1410561)

Pushed by dholbert@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/e91ca49ccf6c When computing abspos CB content-box size, don't bother subtracting borderpadding if CB is a 0-sized child of a XUL-collapsed frame. r=mats

Apparently the pref(...) line in crashtest.list is making Android upset, because we don't have a default value for the stylo pref there (we just let it be disabled by virtue of being unspecified), so it's "unrecognized" and triggers a failure: > REFTEST TEST-UNEXPECTED-FAIL | http://10.0.2.2:8854/tests/layout/xul/crashtests/1379332-1.xul | boolean preference 'layout.css.servo.enabled' not known or wrong type > REFTEST TEST-UNEXPECTED-FAIL | http://10.0.2.2:8854/tests/layout/xul/crashtests/1379332-2.xul | boolean preference 'layout.css.servo.enabled' not known or wrong type This general issue is already filed as bug 1350948. It looks like I can work around this by adding "skip-if(Android)" to the front of these lines. At least, we do that for a few other tests that set prefs, e.g. here: https://dxr.mozilla.org/mozilla-central/rev/ce1a86d3b4db161c95d1147676bbed839d7a4732/layout/reftests/svg/reftest.list#34

RyanVM, could you land this followup on Autoland? It should fix the crashtest orange mentioned above.

Pushed by ryanvm@gmail.com: https://hg.mozilla.org/integration/autoland/rev/9e434f5e600d followup: Skip this bug's crashtests on Android to avoid triggering "boolean preference...not known or wrong type" error there. (test-only, no review)

(Sorry, I used the wrong bug number in that followup commit message -- I pasted in "bug 1350948" instead of this bug number. Oh well, not a big deal - not a substantial patch, and it is tangentially related to bug 1350948, and I added a comment over there to point back to this bug.)

(Ah, and RyanVM backed out and re-landed the followup, with the corrected number, which is how comment 15 ended up on this correct bug despite my typo. Thanks, Ryan!)

https://hg.mozilla.org/mozilla-central/rev/e91ca49ccf6c https://hg.mozilla.org/mozilla-central/rev/9e434f5e600d