Execute this test case in the Browser Console. Due to this bug, not all the text in the opened window is visible: window.openDialog("data:application/vnd.mozilla.xul+xml,<?xml-stylesheet href='chrome://global/skin/global.css'?><window xmlns='http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul'><vbox style='overflow:hidden;'><description style='width:150px;'>Test description wrap, test description wrap.</description></vbox></window>"); Add "display: block;" near "overflow: hidden;" and the issue is solved. There may be some old related bugs or duplicates, but none with a recent test case. https://bugzilla.mozilla.org/buglist.cgi?quicksearch=XUL+overflow+hidden

http://hg.mozilla.org/mozilla-central/rev/6eb0ddec35a0 seems related, also maybe http://hg.mozilla.org/mozilla-central/rev/2d053341a876

Neil, you wrote the patch for revision 6eb0ddec35a0, does this seem like something you might be able to help with?

I'm fairly sure this is also the cause of issues like bug 1281616 and bug 1277895.

(In reply to :Gijs Kruitbosch (PTO recovery mode) from comment #3) > I'm fairly sure this is also the cause of issues like bug 1281616 and bug > 1277895. Well, hmm, maybe not - but they're related in that all of these issues are about the size calculation of display: -moz-box elements that have wrapping text.

(In reply to :Paolo Amadini from comment #0) > Add "display: block;" near "overflow: hidden;" and the issue is solved. Note that the issue is _not_ solved by setting "display: block;" on the <vbox> if the test case is altered to set "width: 150px;" on the <window> element or the <vbox> element instead of the <description> element. But fixing the test case above may be a starting point.

This is probably a variation of the blocks-in-box issue (such as 228673) where the initial height and later computed height differ. You can fix the testcase by adding flex="1" to the vbox, but I assume you may have had some more complex code you were using.

I looked at this some more. It looks like an issue where the scrollframe is clipping using the single-line height rather than the desired height. Using overflow: -moz-hidden-unscrollable instead also fixes this issue and should handle your usecase as you don't want it to scroll anyway.

This patch fixed the issue by ensuring the scrollframe size is set accordingly. I don't see any issues from manual testing, but three tests fail due to this change. https://treeherder.mozilla.org/#/jobs?repo=try&revision=3e9c9f22521f8f3848180f8b264b8ed7c4ae9e30

Does anyone know if there is a trick to fix issue in bug 1277895? Like adding some property to vbox? The dialog is defined in: https://dxr.mozilla.org/mozilla-central/source/toolkit/components/prompts/content

(In reply to Dragana Damjanovic [:dragana] from comment #9) > Does anyone know if there is a trick to fix issue in bug 1277895? Like > adding some property to vbox? Does the suggestion from comment 7 apply to your case?

(In reply to :Paolo Amadini from comment #10) > (In reply to Dragana Damjanovic [:dragana] from comment #9) > > Does anyone know if there is a trick to fix issue in bug 1277895? Like > > adding some property to vbox? > > Does the suggestion from comment 7 apply to your case? I made a patch bug 1277895 comment 27, but it is still cut. I cannot reproduce it so I am always asking Honza to try it. Can you take a look at the patch whether my change was correct? Any other suggestions?

Replied in the bug.

Hey Neil, are you planning on fixing the failing tests so we can get this landed?

I don't plan to any time soon no. There are three failing tests which I cannot reproduce locally: layout/xul/test/test_bug393970.xul devtools/client/storage/test/browser_storage_delete_tree.js toolkit/mozapps/extensions/test/browser/test-window/browser_bug587970.js

I am able to reproduce the failure in layout/xul/test/test_bug393970.xul. This test fails anytime that there are multiple tests in the test run. It doesn't matter if the other test runs before or after this test. I believe this is because of the scroll frame that the test runner uses when multiple tests are run. The code in this patch is guarded by a condition to only run if inside of a scroll frame, so this makes sense. `mach mochitest -f chrome layout/xul/test/` is all I need to reproduce the failure locally on Windows 10 for me.

OK, I can see these now. I'll take another look.

The current patch causes a different issue, in that if the height of the containing area is constrained, explicitly on the element or maybe because it is in a stack and it has "-moz-stack-sizing: ignore;", then the constraint is ignored and the container does not scroll. This test case works without the patch and fails with the patch: window.openDialog("data:application/vnd.mozilla.xul+xml,<?xml-stylesheet href='chrome://global/skin/global.css'?><window xmlns='http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul'><vbox style='overflow:hidden;height:10px;'><description style='width:150px;'>Test description wrap, test description wrap.</description></vbox></window>"); This should display only a fraction of the first line of the description.

Also, the problem seems to be with height of description elements that require a reflow in order for their height to be known. If I set the "height" property on the description explicitly, then the scroll area is calculated correctly in both the test cases from comment 0 and comment 17.

(In reply to :Paolo Amadini from comment #18) > Also, the problem seems to be with height of description elements that > require a reflow in order for their height to be known. If I set the > "height" property on the description explicitly, then the scroll area is > calculated correctly in both the test cases from comment 0 and comment 17. ...assuming the "height" set on the element is greater than the actual height after the reflow occurs. If the "height" property is smaller, then the difference between them is not allocated. If the "height" is higher, then everything works as expected with empty space after the text.

David, maybe you can help here, seems you have also worked on this code?

I looked at this more a while ago and had this patch which improves things further. This isn't really right though; ideally we should have something in nsXULScrollFrame::XULLayout that handles this somehow.

I'd prefer that Neil look at it, if possible.

(In reply to Neil Deakin from comment #21) > I looked at this more a while ago and had this patch which improves things > further. This isn't really right though; ideally we should have something in > nsXULScrollFrame::XULLayout that handles this somehow. In fact, this patch too doesn't help with the case I'm working with. To see what I'm seeing, apply the "work in progress" patch from bug 1009116, then download a test file blocked by Application Reputation: http://testsafebrowsing.appspot.com/s/pua.exe Without the patch on this bug, when I open the subview I see it transitioning smoothly from the height of the download item to the height that the subview would have if all the description elements only had one line of text. Since that's less than the actual height of the description elements, the body of the subview becomes scrollable. The expected behavior is for the subview to transition smoothly from the height of the download item to the full height of the subview. I do obtain that smooth transition if I manually set the "height" property of each individual description element to the height of all its lines of text. What I see with this patch instead is that the subview jumps suddenly to the final height when opening, so even with the changes made here it still forces the outer element to be bigger.

By the way, checking for unconstrained size was the first thing that came to my mind when I looked at the first version of the patch, I'd have tried it if I knew how to do it. At the same time it felt strange that the added code would be the only one that would modify the parent element, so you're probably right that the issue is somewhere else in the scroll frame code, or maybe other computations of the minimum size. Unrelated, why do we call XULLayout again on the child in ChildResized? The code is very difficult to follow, but at least in some cases (if not all) the result seems to be the same as the XULLayout call we just did outside the function.

> The expected behavior is for the subview to transition smoothly from the > height of the download item to the full height of the subview. I do obtain > that smooth transition if I manually set the "height" property of each > individual description element to the height of all its lines of text. OK, so the issue here is that the pre-transition size of the stack relies on -moz-stack-sizing: ignore being set on the child, yet the post-transition size of of the stack relies on that not being set. In the _transitionHeight method you correctly use the right states to calculate the right heights, but then when reseting back to the old height to really start the transition, -moz-stack-sizing hasn't been reset as well, so it causes the old size to grow to accommodate the child. It does work if -moz-stack-sizing is set when the old height is assigned, and then reset after the transition is complete. Does that seem reasonable? I will make an updated patch tomorrow that also fixes the cases where border and padding are not taken into account.

(In reply to Neil Deakin from comment #25) > It does work if -moz-stack-sizing is set when the old height is assigned, > and then reset after the transition is complete. > > Does that seem reasonable? I get inconsistent results between the case where a child resized and the case where a child didn't resize. One of them must be incorrect. With the patch on this bug and the one on bug 1009116 both applied, if I go to the Browser Toolbox and manually set the "height" property of the three description elements to be higher than their text, then independently from any "-moz-stack-sizing" settings the stack keeps the height that I explicitly set in code, and the transition works fine. In other words, when there is an explicit height set on the stack, it forces its children to be smaller, and that's independent from whether the children could force the stack to be larger when the height is not set. This is the behavior that I also observe without the patch, so it's actually what I would expect for the stack element.

While it's definitely wrong, what I have observed is that if modify the call to BoxLayout in nsFrame::RefreshSizeCache to use the width from GetRect(), if available, instead of the one from GetPrefISize(), then the height of the description elements is computed correctly in a few more cases. Does this ring a bell?

Here's what some debugging is showing: 1. Just before the transition starts, the viewStack is the width of both the main view and the subview added together. It's height however has been explicitly assigned to just the main view height (the value of oldHeight in _transitionHeight) 2. Because the subview is actually a taller than that, the 'viewContainer' adds a vertical scrollbar and tries to lay out again with that scrollbar visible. This causes the second <description> element (downloadsPanel-blockedSubview-details1) to no longer fit horizontally and wrap onto a second line. 3. This causes the code in the patch in the bug to apply. The transition handling is working fine but the original height has been increased so the height jumps to the final size at the transition start. When the <description> is given a explicit height that is larger than the text, the same occurs, except that the addition of the vertical scrollbar doesn't cause any height change as the now wrapped description is still smaller than that assigned height. So the If I disable the code that shows/hides scrollbars, the issue goes away and the transition works fine. I am going to continue investigating to see what change can be made to make scrollbar visibility changes work.

Ah, thanks for the debugging, this is definitely tricky! If this helps, I'm observing the issue on Mac OS X 10.9 and I don't think I have space reserved for vertical scrollbars.

Neil, can you attach or e-mail me the experiments you tried and we talked about? The versions that include the debugging output statements will also be helpful to me.

This is debugging patch, which applies on top of the other one. I haven't updated it for a month though.

The patches above fix the issue with the testcase, as well as the incorrect layout of test_bug393970.xul Part 2 fixes the developer tools storage tree which has a padding on an inline inside a vbox. Part 3 fixes the popup notification issue described in comment 23 by disabling the overflow while the transition is occurring which prevents the scrollbar from appearing and disappearing during the transition and affecting the stack height. I don't see any UI issues nor any test failures so far: https://treeherder.mozilla.org/#/jobs?repo=try&revision=b216be48ea3d959e9228c69fab99cf5d5940ad73 I am a bit unclear of the priority here though. The dependent bug 1009116 is marked P1, is 3 years old and doesn't describe which feature is depending on this. Paolo? Also, feel free to test.

(In reply to Neil Deakin from comment #35) > I am a bit unclear of the priority here though. The dependent bug 1009116 is > marked P1, is 3 years old and doesn't describe which feature is depending on > this. The dependent bug in turns blocks Downloads Panel and Control Center issues like bug 1311671, and is relevant in general for getting performance improvements in subview opening and simplifying the code. The P1 priority on the bug is outdated in the sense that it's been triaged in the Privacy and Security team and I'm currently working on Search, but it is something I'll still help with when this platform dependency is fixed. > Also, feel free to test. With all the patches in this bug and my work in progress from bug 1009116 applied, in the Downloads Panel I only get the right transition if I add this rule: #downloadsPanel-blockedSubview-title, #downloadsPanel-blockedSubview-details1, #downloadsPanel-blockedSubview-details2 { height: 50px; } This is the main remaining issue to be fixed in the platform. Do you have any idea on how to do that?

I don't see anything notably out of place with the transition with or without the 50px height set. What issue are you seeing? The same as earlier?

This is the expected transition, with the 50px height set.

This is the transition I see without the 50px height set.

I see the expected version when I use all of these patches along with the patch in 1009116. That said, it would helpful to have an updated version of the patch in bug 1009116, although my local version doesn't appear to haven't notable that would cause an issue. There are builds at https://archive.mozilla.org/pub/firefox/try-builds/neil@mozilla.com-b216be48ea3d959e9228c69fab99cf5d5940ad73/try-macosx64-debug/

Is there an update on this? 1009116 is waiting on tests of the builds here, and per #c40, Neil can't recreate with the version of the patch in 1009116.

There are definitely some differences between the version I tried above and the tryserver build from changeset b216be48ea3d. The tryserver build does not include the patch from bug 1009116, so I imported it over b216be48ea3d. In this state, an important missing part is that the width of the subview in the panel is not constrained. This means that I see the correct transition, but also that none of the description elements has wrapping text. Neil, is this what you're seeing? I added the following rule to "downloads.inc.css": #downloadsPanel-multiView > .panel-viewcontainer > .panel-viewstack { max-width: 30em; } With this change, the text in the description elements now wraps, and I see the incorrect transition. The tryserver build also does not include the "Part 3" patch from attachment 8848183 [details] [diff] [review] on this bug, so I imported it as well. Even with this patch applied, I still see the incorrect transition. Note that the tryserver build included changeset 2e78703533dd (rounddocsizeup) and 0dcdd3016f27 (robustarrowpaneltest) that aren't attached to this bug. The latter is test-only, but I'm not sure if the first can make a difference. The test above had all of them applied, and the transition was incorrect when the text was wrapping.

Neil, since the tryserver build from comment 42 and earlier is based on an older changeset, I started a new one for you to test: https://treeherder.mozilla.org/#/jobs?repo=try&revision=7dbda817618cac663e165109961909a00c8e597a It is based on the latest mozilla-central on which I applied part 1, part 2, the patch for bug 1009116, and part 3. The patch for bug 1009116 has another small difference compared to the earlier one I used with the tryserver build from comment 42, which is a <vbox> element instead of a <box> with "-moz-box-orient: vertical", but it should have no practical effect as far as I can tell. Both versions had the incorrect transition for me.

Sorry for the delay in providing the feedback about the tryserver builds. I knew this was related to Photon work, but I didn't realize that things like like bug 1354141 were going on right now, so I considered this mostly a background task for me. I'll try to give a faster turnaround. Neil, let me know if there are other tests with which I can help.

After much analysis, I realized that what the popup actually wants to do is use a different value horizontally versus vertically for -moz-stack-sizing when transitioning. The horizontal size should expand to accommodate the size of both panes. However, vertically, it wants to use the transition height rather than to grow to be the full height of the second pane with the wrapping text. Part 1 corrects the scrolling wrapping. Part 2 fixes the jumping transition for me.

Comment on attachment 8848182 [details] [diff] [review] Part 3 - use desired size for preferred height for blocks in boxes This patch is actually needed to fix the browser_storage_delete_tree.js test.

Do you have a link to the actual final changeset, or to a tryserver build? I've tried to apply the patches on the latest mozilla-central, but then when I open the Application Menu or the Downloads Panel I only saw the arrow and not the panel, so there might be something wrong with the way I applied the patches.

There is a build at https://treeherder.mozilla.org/#/jobs?repo=try&revision=bb30c3f7c7f68b57aaeef362bf749674ebeb127a

Comment on attachment 8862887 [details] [diff] [review] Part 2 - allow -moz-stack-sizing to work in a single direction only I don't know whether this patch is part of the solution or not, but I tried the build and the transition I observed is not the one I expect. In particular, the two buttons at the bottom of the subview should always be aligned with the bottom of the panel while the subview expands, and the scrollframe should be reduced in size. Here, instead, the buttons are hidden until the panel has expanded fully, and the scrollframe with the description element is never shrinked. Again, this is fixed by setting "height: 80px;" on all the description elements. I suspect the problem may be in the other sizing patch. When that code is invoked, apparently it *forces* the outer part of the scrollframe to be taller, even though the elements around it should push back and take priority, forcing the outer scrollframe to be smaller, and scrolling to occur inside it.

Comment on attachment 8862828 [details] [diff] [review] Part 1 - expand scrollframe size when scrolled frame size is increased during layout, include test Review of attachment 8862828 [details] [diff] [review]: ----------------------------------------------------------------- ::: layout/generic/nsGfxScrollFrame.cpp @@ +5138,5 @@ > + // If an explicit width or height was assigned then just use that size > + // instead of expanding in that direction. > + bool widthSet, heightSet; > + nsSize prefSizeCSS(NS_INTRINSICSIZE, NS_INTRINSICSIZE); > + nsIFrame::AddXULPrefSize(this, prefSizeCSS, widthSet, heightSet); I think this code tries to avoid the expansion when there is a width or height set, but it keeps into account only those explicitly set on the frame, and not those determined by outer constraints.

These reftests demonstrate the issue from comment 51: https://reviewboard.mozilla.org/r/136814/diff/1#index_header Number 1 is the same as the one in the current patch - fails on trunk and succeeds with the patch. Number 2 I believe is the case handled by the code in comment 51, succeeds on trunk and succeeds with the patch. Number 3 is the case of the external height constraint, succeeds on trunk and fails with the patch. For the latter two cases, both test and reference have "overflow: hidden;". I used a height that shows basically half of the first line of text, and while the test has a second line, the reference has only one line. This should be the two cases of not expanding and expanding, triggered by the "height: 80px" change in the Downloads Panel.

Comment on attachment 8862828 [details] [diff] [review] Part 1 - expand scrollframe size when scrolled frame size is increased during layout, include test This actually helps with the Application Menu for the layout of the block-in-box containing the customizable UI elements, so overall it's better than what we have now, and we could consider landing this without waiting for the fix for the third reftest, if doing the latter will take much more time.

Comment on attachment 8862887 [details] [diff] [review] Part 2 - allow -moz-stack-sizing to work in a single direction only The "transitioning" attribute here actually breaks the cases where we start a new transition in the middle of an existing one. I don't think this is the right path, for now at least. The platform patch however might be helpful to simplify how we style the width of the subviews, we might consider landing that in a separate bug.

Comment on attachment 8848182 [details] [diff] [review] Part 3 - use desired size for preferred height for blocks in boxes This works fine. It does not seems to affect any of my tests.

Comment on attachment 8862887 [details] [diff] [review] Part 2 - allow -moz-stack-sizing to work in a single direction only Review of attachment 8862887 [details] [diff] [review]: ----------------------------------------------------------------- Actually, this helps with keeping style changes to a minimum for our current use cases, so we may want to land this soon as well. ::: layout/xul/nsStackLayout.cpp @@ +76,5 @@ > + > + if (pref.width > prefSize.width && > + child->StyleXUL()->mStackSizing != StyleStackSizing::IgnoreVertical) { > + prefSize.width = pref.width; > + } It looks like IgnoreHorizontal and IgnoreVertical are swapped.

(In reply to :Paolo Amadini from comment #57) > Comment on attachment 8862887 [details] [diff] [review] > Part 2 - allow -moz-stack-sizing to work in a single direction only > > Review of attachment 8862887 [details] [diff] [review]: > ----------------------------------------------------------------- > > Actually, this helps with keeping style changes to a minimum for our current > use cases, so we may want to land this soon as well. > > ::: layout/xul/nsStackLayout.cpp > @@ +76,5 @@ > > + > > + if (pref.width > prefSize.width && > > + child->StyleXUL()->mStackSizing != StyleStackSizing::IgnoreVertical) { > > + prefSize.width = pref.width; > > + } > > It looks like IgnoreHorizontal and IgnoreVertical are swapped. No, it's correct. IgnoreVertical means stretch horizontal only. We might want to tweak the naming of the constants to be clearer.

Uhm, this is what I read in the code: if (sizing is "stretch horizontal only") { do nothing; } else { modify width based on child; } Shouldn't this be: if (sizing is "stretch horizontal only") { modify width based on child; }

Sorry: if (sizing is "stretch vertical only") { do nothing; } else { modify width based on child; }

Comment on attachment 8862887 [details] [diff] [review] Part 2 - allow -moz-stack-sizing to work in a single direction only Moved to bug 1364115.

Comment on attachment 8866859 [details] Bug 1293242 - Part 2 - Propagate resized child size up to parent scrollable frame. https://reviewboard.mozilla.org/r/138442/#review141680 Carrying over Neil's patches, including the new reftests. ::: layout/reftests/xul/description-inside-overflowhidden-3.xul:1 (Diff revision 1) > +<?xml version="1.0"?> > +<?xml-stylesheet href="chrome://global/skin/" type="text/css"?> > +<window xmlns="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul"> > + > +<!-- Height constrained indirectly, the scrollframe should not expand. --> > +<vbox style="height: 7px;"> > + <vbox flex="1" style="max-width: 150px; background: green; overflow: hidden;"> > + <!-- This wraps, so it triggers scrollframe expansion code. --> > + <description style="width: 150px;"><span style="width: 149px;">Line 1</span> Line 2</description> > + </vbox> > +</vbox> > + > +</window> This test works on central but doesn't work after the patch is applied. David, any idea on how to fix this case? Note that if this is difficult, we should likely fix this case in a follow-up bug, and we can land this bug without this case handled in order to support the panelmultiview layout.

Part 1 is pretty scary -- looks like the last time we tried to make that sort of change was bug 413027 (although then for both min and pref). It does make a bit of sense, though. The approach that you're changing dates back at least to bug 110328, if not earlier. I'd note that this code doesn't handle vertical writing-mode at all, and it really should... I'm curious how you have determined that a change like that is safe to make.

I'm carrying over Neil's patches because apparently he is away for three months, and we have to fix several bugs in front-end layout that currently require expensive workarounds. I don't really have a lot of domain expertise here, even though I understand what the patches are trying to do, so any information you can add will be useful. See also comment 64 for a case that is regressed by this patch. The behavior of the patch is much better than what we have now though, and we can live with this regression in the front-end much more easily, if fixing things properly is too difficult. If you have any idea on how to fix all three of the test cases I provided, I can try to put some code together.

It's not clear to me how to proceed here. Rewriting the rules that choose the vertical size of CSS layout inside of XUL layout is a significant change that needs strong ownership to make successfully. You seem to be suggesting that you can't do that, and I think suggesting that I do it instead. But that's not a review request -- it's a request that I put this bug on my list of engineering work to do.

If I understand you correctly, Neil's patches for the block-in-box issues have more implications than just the test cases in this bug, and they aren't in a state that is advanced enough to be easily landable, differently from his patches for the stack layout that I recently landed. In fact it seems to me that, apart from the scroll height, we have a more fundamental issue with the computation of the preferred height. This may have originally been changed to be more efficient and not require a reflow, except this made the behavior buggy and shifted the burden of dealing with these bugs to the front-end team, eventually hurting performance because of the workarounds we introduced. We've lived with this situation basically since forever, it just became more apparent with the Photon efforts. We may be in a situation where we still don't have enough layout developers available to fix this properly. I don't have a clear idea of how much effort this would be - likely my impression was underestimated based on the presence of working patches. David, I think it's up to you to propose a possible way forward, that may also be to do nothing for now. I implemented a workaround in bug 1009116 for which we force a height on description elements synchronously, which seems to handle most cases and be robust enough for now, even though problematic for performance. So, this bug is not a blocker for the other one anymore.

Comment on attachment 8866859 [details] Bug 1293242 - Part 2 - Propagate resized child size up to parent scrollable frame. (Just secured these patches to the MozReview repository for now.)

Given that there is now a practical plan and progress in bug 1033225 to get rid of this layout mode, and we already spent quite a lot of time implementing the needed workarounds in the front-end code, we don't have to fix this bug anymore.