Wrong height calculation with multi-line description, "display: -moz-box", and "overflow: hidden"

RESOLVED WONTFIX

Status

()

P1
normal
RESOLVED WONTFIX
2 years ago
8 months ago

People

(Reporter: Paolo, Unassigned)

Tracking

(Depends on: 1 bug, {regression, regressionwindow-wanted})

unspecified
regression, regressionwindow-wanted
Points:
---
Dependency tree / graph
Bug Flags:
qe-verify +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [reserve-photon-performance])

Attachments

(4 attachments, 9 obsolete attachments)

601.26 KB, video/quicktime
Details
475.53 KB, video/quicktime
Details
59 bytes, text/x-review-board-request
Details
59 bytes, text/x-review-board-request
Details
(Reporter)

Description

2 years ago
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
Neil, you wrote the patch for revision 6eb0ddec35a0, does this seem like something you might be able to help with?
Flags: needinfo?(neil)
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.
(Reporter)

Comment 5

2 years ago
(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.

Comment 6

2 years ago
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.

Comment 7

2 years ago
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.

Comment 8

2 years ago
Created attachment 8783021 [details] [diff] [review]
Patch in progress

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
(Reporter)

Comment 10

2 years ago
(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?
Flags: needinfo?(paolo.mozmail)
(Reporter)

Comment 12

2 years ago
Replied in the bug.
Flags: needinfo?(paolo.mozmail)
Hey Neil, are you planning on fixing the failing tests so we can get this landed?
Flags: needinfo?(enndeakin)

Comment 14

2 years ago
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
Flags: needinfo?(enndeakin)
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.

Comment 16

2 years ago
OK, I can see these now. I'll take another look.
(Reporter)

Updated

2 years ago
Flags: needinfo?(neil)
(Reporter)

Comment 17

2 years ago
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.
Flags: needinfo?(enndeakin)
(Reporter)

Comment 18

2 years ago
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.
(Reporter)

Comment 19

2 years ago
(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.
(Reporter)

Comment 20

2 years ago
David, maybe you can help here, seems you have also worked on this code?
Flags: needinfo?(dbaron)

Comment 21

2 years ago
Created attachment 8799896 [details] [diff] [review]
Another patch

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.
Flags: needinfo?(enndeakin)
I'd prefer that Neil look at it, if possible.
Flags: needinfo?(dbaron)
(Reporter)

Comment 23

2 years ago
(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.
Blocks: 1009116
Flags: needinfo?(enndeakin)
(Reporter)

Comment 24

2 years ago
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.

Updated

2 years ago
Blocks: 1224412

Comment 25

2 years ago
> 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.
Flags: needinfo?(enndeakin) → needinfo?(paolo.mozmail)
(Reporter)

Comment 26

2 years ago
(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.
Flags: needinfo?(paolo.mozmail)
(Reporter)

Updated

2 years ago
Flags: needinfo?(enndeakin)
(Reporter)

Comment 27

2 years ago
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?

Comment 28

2 years ago
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.
Flags: needinfo?(enndeakin)
(Reporter)

Comment 29

2 years ago
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.
Keywords: regression
(Reporter)

Comment 30

2 years ago
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.
Flags: needinfo?(enndeakin)

Comment 31

2 years ago
Created attachment 8824066 [details] [diff] [review]
Debugging patch

This is debugging patch, which applies on top of the other one. I haven't updated it for a month though.
Flags: needinfo?(enndeakin)
(Reporter)

Updated

2 years ago
Blocks: 1332436
(Reporter)

Updated

2 years ago
Blocks: 1331496

Comment 32

2 years ago
Created attachment 8848180 [details] [diff] [review]
Part 1 - expand scrollframe size when scrolled frame size is increased during layout
Assignee: nobody → enndeakin
Attachment #8783021 - Attachment is obsolete: true
Attachment #8799896 - Attachment is obsolete: true
Attachment #8824066 - Attachment is obsolete: true
Status: NEW → ASSIGNED

Comment 33

2 years ago
Created attachment 8848182 [details] [diff] [review]
Part 3 - use desired size for preferred height for blocks in boxes

Comment 34

2 years ago
Created attachment 8848183 [details] [diff] [review]
Part 3 - disable overflow while the popup notification is transitioning

Comment 35

2 years ago
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.
Flags: needinfo?(paolo.mozmail)
(Reporter)

Comment 36

2 years ago
(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?
Flags: needinfo?(paolo.mozmail) → needinfo?(enndeakin)

Comment 37

2 years ago
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?
Flags: needinfo?(enndeakin)
(Reporter)

Comment 38

2 years ago
Created attachment 8849935 [details]
Transition: Correct

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

Comment 39

2 years ago
Created attachment 8849936 [details]
Transition: Incorrect

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

Updated

2 years ago
Flags: needinfo?(enndeakin)

Comment 40

2 years ago
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/
Flags: needinfo?(enndeakin)

Updated

2 years ago
Whiteboard: [photon]
(Reporter)

Updated

2 years ago
Flags: needinfo?(paolo.mozmail)

Updated

2 years ago
Flags: qe-verify+
Priority: -- → P1
QA Contact: adrian.florinescu
Whiteboard: [photon] → [photon-performance]

Updated

2 years ago
Iteration: --- → 55.4 - May 1
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.
(Reporter)

Comment 42

a year ago
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.
Flags: needinfo?(paolo.mozmail) → needinfo?(enndeakin)
(Reporter)

Comment 43

a year ago
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.
(Reporter)

Comment 44

a year ago
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.

Comment 45

a year ago
Created attachment 8862828 [details] [diff] [review]
Part 1 - expand scrollframe size when scrolled frame size is increased during layout, include test
Attachment #8848180 - Attachment is obsolete: true
Attachment #8848182 - Attachment is obsolete: true
Attachment #8848183 - Attachment is obsolete: true
Flags: needinfo?(enndeakin)

Comment 46

a year ago
Created attachment 8862887 [details] [diff] [review]
Part 2 - allow -moz-stack-sizing to work in a single direction only

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.
Attachment #8862887 - Flags: feedback?(paolo.mozmail)

Comment 47

a year ago
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.
Attachment #8848182 - Attachment description: Part 2 - use desired size for preferred height for blocks in boxes → Part 3 - use desired size for preferred height for blocks in boxes

Updated

a year ago
Attachment #8848182 - Attachment is obsolete: false
(Reporter)

Comment 48

a year ago
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.
Flags: needinfo?(enndeakin)
(Reporter)

Comment 50

a year ago
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.
Flags: needinfo?(enndeakin)
Attachment #8862887 - Flags: feedback?(paolo.mozmail)
Iteration: 55.4 - May 1 → 55.5 - May 15
(Reporter)

Comment 51

a year ago
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.
Comment hidden (mozreview-request)
(Reporter)

Updated

a year ago
Flags: needinfo?(enndeakin)
(Reporter)

Comment 53

a year ago
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.
Flags: needinfo?(enndeakin)
(Reporter)

Comment 54

a year ago
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.
Attachment #8862828 - Flags: feedback+
(Reporter)

Comment 55

a year ago
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.
Attachment #8862887 - Flags: feedback-
(Reporter)

Comment 56

a year ago
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.
Attachment #8848182 - Flags: feedback+
(Reporter)

Comment 57

a year ago
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.
Attachment #8862887 - Flags: feedback-

Comment 58

a year ago
(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.
Flags: needinfo?(enndeakin)
(Reporter)

Comment 59

a year ago
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;
}
(Reporter)

Comment 60

a year ago
Sorry:

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

Updated

a year ago
Blocks: 1363753
(Reporter)

Updated

a year ago
Depends on: 1364115
(Reporter)

Updated

a year ago
No longer depends on: 1364115
(Reporter)

Comment 61

a year ago
Comment on attachment 8862887 [details] [diff] [review]
Part 2 - allow -moz-stack-sizing to work in a single direction only

Moved to bug 1364115.
Attachment #8862887 - Attachment is obsolete: true
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Reporter)

Updated

a year ago
Attachment #8865149 - Attachment is obsolete: true
(Reporter)

Updated

a year ago
Attachment #8862828 - Attachment is obsolete: true
(Reporter)

Updated

a year ago
Attachment #8848182 - Attachment is obsolete: true
(Reporter)

Comment 64

a year ago
mozreview-review
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.
Assignee: enndeakin → paolo.mozmail
Iteration: 55.5 - May 15 → ---
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.
Flags: needinfo?(paolo.mozmail)
(Reporter)

Comment 66

a year ago
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.
Flags: needinfo?(paolo.mozmail) → needinfo?(dbaron)
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.
Flags: needinfo?(dbaron)
(Reporter)

Comment 68

a year ago
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.
Flags: needinfo?(dbaron)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Reporter)

Updated

a year ago
Attachment #8866858 - Flags: review?(dbaron)
(Reporter)

Comment 71

a year ago
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.)
Attachment #8866859 - Flags: review?(dbaron)
(Reporter)

Updated

a year ago
No longer blocks: 1009116
Whiteboard: [photon-performance] → [reserve-photon-performance]
(Reporter)

Updated

a year ago
Blocks: 1364738
(Reporter)

Updated

a year ago
Blocks: 1369339
(Reporter)

Updated

a year ago
Duplicate of this bug: 1332436
(Reporter)

Updated

a year ago
Blocks: 1369729
(Reporter)

Updated

a year ago
Blocks: 1390040
(Reporter)

Updated

11 months ago
Depends on: 1033225
(Reporter)

Updated

11 months ago
Depends on: 1398963
(Reporter)

Comment 73

8 months ago
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.
Assignee: paolo.mozmail → nobody
Status: ASSIGNED → RESOLVED
Last Resolved: 8 months ago
Flags: needinfo?(dbaron)
Resolution: --- → WONTFIX
(Reporter)

Updated

8 months ago
No longer blocks: 1390040
You need to log in before you can comment on or make changes to this bug.