346189 - Overlayed children affect computed size of flexible stack

Status: RESOLVED FIXED

(Core :: XUL, enhancement)

Description

[Andrew Miller]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8.0.5) Gecko/20060719 Firefox/1.5.0.5
Build Identifier: Latest Seamonkey trunk CVS

A common use case for the stack element is to have a flexible main control, which is then overlayed at certain co-ordinates by additional controls (for example, the editable tree originally by Neil Deakin).

Unfortunately, doing this means that the preferred size of the stack is controlled  by the position and size of the overlayed controls (so for example, in the edittree example, the appearance of the textbox will trigger a reflow, and potentially cause the resizing of the stack and the tree control).

To avoid this, the stack needs to be told which children should be used to control the size, and which are merely overlays.

A new -moz CSS attribute for overlayed children seems like the best way to do this.


Reproducible: Always

Steps to Reproduce:
Try putting an edittree, from the bindings http://lxr.mozilla.org/seamonkey/source/mail/extensions/newsblog/content/edittree.xml
in a flexible layout (with another flexible element there as well).
Actual Results:  
The appearance of textbox on the edittree resizes the entire tree, and the textbox co-ordinates end up being out of date.

Expected Results:  
No resize should occur as a result of the textbox being added (although that it would be acceptable if edittree had to be modified to add a new style to prevent the reflow).

Comment 1

[Andrew Miller]

Attachment: Patch, adds new -moz-parent-size-interaction style

Comment 2

[Andrew Miller]

Attachment: Testcase for -moz-parent-size-interaction

A testcase for this bug. To use:
Click the "I am the main control button". Ensure that the size of the "I am the main control button" does not changes as a result of the "I am the overlay control" appearing. Now click the "I am the overlay control" button and make sure the size still doesn't change.

Comment 3

[[e-mail]]

One approach, assuming everyone agreed, would be for stack sizing to simply ignore all positioned children, which would also fix bug 345937.

Comment 4

[Andrew Miller]

(In reply to comment #3)
> One approach, assuming everyone agreed, would be for stack sizing to simply
> ignore all positioned children, which would also fix bug 345937.
> 
Roc agreed to the principle of this approach on IRC.

I have a patch for this (not tested yet, will post when ready, because this needs extensive testing to ensure it doesn't break Firefox / Thunderbird at least, and there are probably some more things it would be good to test).

Comment 5

[Andrew Miller]

Attachment: Patch implementing suggestion by roc & (independently) neil

Comment 6

[Robert O'Callahan (:roc) (email my personal email if necessary)]

+  // As an optimization, we cache the fact that we are not positioned to avoid
+  // wasting time fetching attributes and checking style data.
+  if (aChild->GetStateBits() & NS_STATE_STACK_NOT_POSITIONED)
+    return PR_FALSE;

You would need to remove these bits if the attributes or styles are changed dynamically. Actually I think you should simply not do this optimization.

+  if (content) {

You can assume 'content' is not null.

+    nsAutoString value;
+
+    content->GetAttr(kNameSpaceID_None, nsHTMLAtoms::left, value);
+    if (!value.IsEmpty()) {

Don't do this. Call HasAttr instead.

Comment 7

[[e-mail]]

(In reply to comment #6)
Actually he was just copying code from AddOffset so for consistency the same changes would need to be applied there too.

Comment 8

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to comment #7)
> (In reply to comment #6)
> Actually he was just copying code from AddOffset so for consistency the same
> changes would need to be applied there too.

Okay, then he should be sharing code with AddOffset. I suggest creating a GetOffset method that returns a PRBool and takes an nsSize out parameter.

I'm not really excited about this optimization, but if you want to keep it I think it would be enough to clear NS_STATE_STACK_NOT_POSITIONED in nsBoxFrame::DidSetStyleContext.

Comment 9

[Andrew Miller]

Attachment: Last patch but addressing comments from neil and roc.

Comment 10

[[e-mail]]

Comment on attachment 232069 [details] [diff] [review]
Last patch but addressing comments from neil and roc.

>+  return (eStyleUnit_Coord == pos->mOffset.GetLeftUnit()) ||
>+    eStyleUnit_Coord == pos->mOffset.GetTopUnit();
>+
>+  nsIContent* content = aChild->GetContent();
>+
>+  return content->HasAttr(kNameSpaceID_None, nsHTMLAtoms::left) ||
>+    content->HasAttr(kNameSpaceID_None, nsHTMLAtoms::top);
Unreachable code! Did you mean
if (eStyleUnit_Coord == pos->mOffset.GetLeftUnit()) ||
    eStyleUnit_Coord == pos->mOffset.GetTopUnit())
  return PR_TRUE;
or perhaps
return eStyleUnit_Coord == pos->mOffset.GetLeftUnit() ||
       eStyleUnit_Coord == pos->mOffset.GetTopUnit() ||
       aChild->GetContent()->HasAttr(kNameSpaceID_None, nsHTMLAtoms::left) ||
       aChild->GetContent()->HasAttr(kNameSpaceID_None, nsHTMLAtoms::top);

Also there's an issue here that although positioned content no longer changes the computed preferred size the layout still stretches the stack to fit.

Comment 11

[Andrew Miller]

Attachment: Last patch but addressing comments from neil

Comment 12

[Andrew Miller]

Attachment: Last patch but with the right flags to diff

Comment 13

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 232482 [details] [diff] [review]
Last patch but with the right flags to diff

+  if ((eStyleUnit_Coord == pos->mOffset.GetLeftUnit()) ||
+      eStyleUnit_Coord == pos->mOffset.GetTopUnit())

Be consistent with your parens. Probably just remove the unnecessary first set.

Comment 14

[Andrew Miller]

Attachment: Previous patch + address roc's comment 13

Comment 15

[[e-mail]]

This breaks card games http://cardgames.mozdev.org/ which positions cards in stacks (!) and expects them to stretch to fit the cards. (It also uses a stack to allow cards to be dragged but in that case it styles it with hidden overflow to stop it from stretching.)

Comment 16

[Andrew Miller]

(In reply to comment #15)
> This breaks card games http://cardgames.mozdev.org/ which positions cards in
> stacks (!) and expects them to stretch to fit the cards. (It also uses a stack
> to allow cards to be dragged but in that case it styles it with hidden overflow
> to stop it from stretching.)
> 
Couldn't they fix it by changing the size of the stack when cards are moved? I think that their use case is probably rarer than the use case we are supporting here.

If we want to preserve support for cardgames.mozdev.org, we could go with my -moz-parent-size-interaction patch, which will only affect children which explicitly ask for the new behaviour.

Comment 17

[Robert O'Callahan (:roc) (email my personal email if necessary)]

How about an attribute on stack elements, say ignorepositionedchildren="true"

Comment 18

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I discussed this with Andrew on IRC. I think perhaps we should go with the style property of his original patch, but extended to be more generally useful because I don't think it's justifiable just for stacks. We could use it in non-stack box layout, e.g. in an hbox to control whether a child element's height affects the hbox height. This would probably be a small change.

I also advocated changing the property name to "-moz-parent-stretch" with values "none" and "normal".

Comment 19

[[e-mail]]

Comment on attachment 232493 [details] [diff] [review]
Previous patch + address roc's comment 13

OK, I've decided that the cardgames extension can be trivially fixed and improved by hit-testing the bottom card of the pile rather than the whole pile.

Comment 20

[Robert O'Callahan (:roc) (email my personal email if necessary)]

But what about all the other XUL apps out there that we might be breaking?

Comment 21

[Andrew Miller]

Attachment: Implement -moz-parent-stretch

This implements the -moz-parent-stretch CSS property, with values none and normal, as suggested by roc. The property currently will only take effect when the parent is a stack, but we could later allow this to work with other parent nodes (e.g. hbox / vbox).

Comment 22

[Andrew Miller]

Attachment: Testcase for -moz-parent-no-stretch

A testcase for -moz-parent-no-stretch. To use, click the main button, and check that the appearance of the new button doesn't cause the stack and main button to resize.

Comment 23

[Andrew Miller]

Sorry, that should actually have said -moz-parent-stretch: none, not -moz-parent-no-stretch.

Comment 24

[Jeff Walden [:Waldo]]

If you have time and patience to convert your test to a reftest <http://lxr.mozilla.org/mozilla/source/layout/tools/reftest/README.txt> (basically, you write a XUL file that uses the new feature, possibly with onload scripting, and you write a file that doesn't use the feature but creates the same pixel-identical output so that when automated tests are run the files can be compared for visual equality), that would be even better than just posting the testcase here.

Comment 25

[Andrew Miller]

(In reply to comment #24)
> If you have time and patience to convert your test to a reftest
> <http://lxr.mozilla.org/mozilla/source/layout/tools/reftest/README.txt>
If someone does write one, it would probably be easier (and more robust) to check:
-moz-parent-stretch: normal behaves the same as when the attribute isn't there.
-moz-parent-stretch: none renders differently from when the attribute isn't there.

Another test would be to let a child with an offset and -moz-parent-stretch: none be obscured by another non-offset small-size flexible child, and check that this produces the same results as when the obscured child is not present at all.

Comment 26

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 251107 [details] [diff] [review]
Implement -moz-parent-stretch

+  PRBool        mParentStretch;

PRPackedBool

The stack code looks OK. We need style system peer review here.

Comment 27

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 251107 [details] [diff] [review]
Implement -moz-parent-stretch

You need to make the mochitests in layout/style/test/ pass.  They'll fail with this patch for a bunch of reasons:

>+    return ParseVariant(aErrorCode, aValue, VARIANT_NORMAL | VARIANT_NONE,
>+                        nsnull);

You need to parse inherit and initial (H).

>+  if (eCSSUnit_None == xulData.mParentStretch.GetUnit()) {
>+    xul->mParentStretch = PR_FALSE;
>+  }

You need to handle Normal, Inherit, and Initial.

> nsStyleXUL::nsStyleXUL(const nsStyleXUL& aSource)

You need to fix the copy-constructor.

>+  PRBool        mParentStretch;

Use PRPackedBool, and add the [reset] comment

>--- dom/public/idl/css/nsIDOMCSS2Properties.idl	(/mozilla/trunk)	(revision 239)
>+++ dom/public/idl/css/nsIDOMCSS2Properties.idl	

You need to rev the IID (of nsIDOMNSCSS2Properties, NOT nsIDOMCSS2Properties).



I'm also a bit skeptical that doing it this way fits with the XUL box model.  I wonder if you could handle this by just overloading -moz-box-flex for children of stacks (does it currently do anything for children of stacks?).  I'd be interested in opinions from Hixie or Hyatt.

Comment 28

[Andrew Miller]

(In reply to comment #27)
> I'm also a bit skeptical that doing it this way fits with the XUL box model.  
> I wonder if you could handle this by just overloading -moz-box-flex for 
> children of stacks (does it currently do anything for children of stacks?).

I'm not sure that flexibility and stretching are quite the same concept, so it would be quite an unnatural overload.

Stretching = parent size changes to accommodate child.
Flex = child size changes to allocate space available in the parent. 

Stretching is currently always on (c.f. flexibility) for stacks in the box model, but occasionally this is inconvenient. I think that providing a way to turn off stretching is a different concept to flexibility, and as roc suggested previously on IRC, could be generalised to other types of boxes in the future.

>  I'd be
> interested in opinions from Hixie or Hyatt.
> 

Comment 29

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

But flexibility does influence whether things can flex to smaller sizes, I think.

Comment 30

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

To be clear, I'm ok with this patch going in if:
 * hixie and hyatt don't have any better ideas (or don't respond)
 * you fix the things I pointed out, and the other things necessary to pass the style system mochitests (which probably requires some changes to property_database.js and nsComputedDOMStyle.cpp)

That said, I'm still not even really convinced of the use case being worth adding a property (how often do you really want things to overlap if they don't fit?), and I'm somewhat skeptical of both boolean properties and mode-switching properties in general.

Comment 31

[Andrew Miller]

(In reply to comment #27)
> > nsStyleXUL::nsStyleXUL(const nsStyleXUL& aSource)
> 
> You need to fix the copy-constructor.

I am probably missing something obvious here, but what are you suggesting that I need to do in addition to the current copy constructor (which calls memcpy to copy everything)?

Comment 32

[[e-mail]]

(In reply to comment #31)
>(In reply to comment #27)
>>>nsStyleXUL::nsStyleXUL(const nsStyleXUL& aSource)
>>You need to fix the copy-constructor.
>I am probably missing something obvious here, but what are you suggesting that
>I need to do in addition to the current copy constructor (which calls memcpy to
>copy everything)?
I'd suggest increasing the -u on your diff to make it obvious ;-)

Comment 33

[Andrew Miller]

Attachment: Attachment addressing review comments by dbaron, roc, and neil

Comment 34

[Hixie (not reading bugmail)]

Seems fine to me. I'd probably change the names a bit, I think:

   -moz-stack-sizing: ignore
   -moz-stack-sizing: stretch-to-fit

...would be better names, but that's not critical.

Comment 35

[Andrew Miller]

Attachment: The test-case updated to use -moz-stack-stretch

Comment 36

[Andrew Miller]

Attachment: Use the -moz-stack-sizing naming scheme suggested by Hixie

Comment 37

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 276898 [details] [diff] [review]
Use the -moz-stack-sizing naming scheme suggested by Hixie

It might have been better to make the member variable of nsStyleXUL an PRUint8 taking the constants rather than a PRPackedBool, but it's ok this way.

Could you add the new nsIDOMNSCSS2Properties attribute at the end instead of the middle?

r=dbaron with that

Comment 38

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

And this should get documented in the release notes of the appropriate release (maybe) and in the CSS properties documentation on http://developer.mozilla.org/ .

Comment 39

[Andrew Miller]

Attachment: As before but move the attribute to the end of the interface

Comment 40

[Robert O'Callahan (:roc) (email my personal email if necessary)]

This probably needs to go through the Gecko 1.9 meeting to get approval, as it's really a new feature. Can you write down some uses cases for this and put them in a page in wiki.mozilla.org?

Comment 41

[Andrew Miller]

(In reply to comment #40)
> This probably needs to go through the Gecko 1.9 meeting to get approval, as
> it's really a new feature. Can you write down some uses cases for this and put
> them in a page in wiki.mozilla.org?
> 
An initial page has been created at http://wiki.mozilla.org/XUL:Stack_Sizing

Comment 42

[Robert O'Callahan (:roc) (email my personal email if necessary)]

If you call in to the Gecko 1.9 meeting on Thursday morning (6am, instructions posted to mozilla.dev.planning), that would help and you might find the experience interesting :-)

Comment 43

[Robert O'Callahan (:roc) (email my personal email if necessary)]

And add a link to that page to the Gecko 1.9 meeting agenda wiki page.

Comment 44

[Robert O'Callahan (:roc) (email my personal email if necessary)]

This didn't get on the meeting agenda :-( sorry. Try next week.

Comment 45

[Reed Loden [:reed]]

Removing checkin-needed until approval is granted.

Comment 46

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 278345 [details] [diff] [review]
As before but move the attribute to the end of the interface

At yesterday's Gecko-1.9 meeting, we decided not to take this for 1.9. Sorry.

Comment 47

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(We need to make sure this doesn't get lost and gets checked in when we open for post-1.9 development ... not sure the best way to do that)

Comment 48

[Martijn Wargers (dead)]

(In reply to comment #47)
> (We need to make sure this doesn't get lost and gets checked in when we open
> for post-1.9 development ... not sure the best way to do that)

A new 'checkin-needed-2.0' keyword or something like that?

Comment 49

[Nickolay_Ponomarev]

(In reply to comment #48)
> A new 'checkin-needed-2.0' keyword or something like that?
> 
bsmedberg thinks it's not necessary:

http://groups.google.com/group/mozilla.dev.planning/browse_thread/thread/768870e0cd278ad1/a2d62459ffd20c98#a2d62459ffd20c98

("Adding a checkin-after-1.9 keyword?" thread in m.d.planning)

Comment 50

[Martijn Wargers (dead)]

Thanks Nickolay, I'll respond there if I feel the need to (instead of spamming this bug).

Comment 51

[Andrew Miller]

Attachment: Fixes bitrot of the previous patch

Comment 52

[Andrew Miller]

Actually, it looks like I forward ported the wrong branch in my git and my latest actually based off an earlier version. I will try to get the correct version fixed up at some point.

Comment 53

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Attachment: updated patch

Here's an updated patch based on the previous one that roc reviewed.  I'm writing some tests now, will attach shortly.

Comment 54

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Attachment: reftest

A reftest

Comment 55

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Attachment: reftest reference

Comment 56

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

(The reftest needs a max-width: 400px on the vbox to function properly in the reftest framework)

Comment 57

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Just noticed -- the patch needs an if (child->GetStyleXUL()->mStretchStack) in ::Layout around the two blocks that set grow to TRUE?  Otherwise we'll end up resizing the stack during layout even for children that aren't supposed to size us.  I'll add a testcase to cover this.

Comment 58

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Attachment: updated patch, again

Patch updated with stretch check in Layout, and reftests in patch.

Comment 59

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Comment on attachment 320760 [details] [diff] [review]
updated patch, again

dbaron already reviewed the style system bits.

Comment 60

[Vladimir Vukicevic [:vlad] [:vladv] (needinfo me, slow to respond)]

Checked this in; forgot to mark fixed.

Comment 61

[Biju]

if somebody gets time please provide a better documentation at
http://developer.mozilla.org/en/docs/CSS:-moz-stack-sizing
I have provided a stub article there.

Comment 62

[Eric Shepherd [:sheppy]]

Can someone explain to me the use case for this so I can finish the documentation?  I vaguely see what it does, but I don't entirely get the point. :)

Comment 63

[Mark Finkle (:mfinkle) (use needinfo?)]

(In reply to comment #62)
> Can someone explain to me the use case for this so I can finish the
> documentation?  I vaguely see what it does, but I don't entirely get the point.
> :)

Normally, a <stack> will change it's size so it's child elements are always visible. If you move a child element far to the right, the <stack would change it's width so the child is still visible.

Using the "-moz-stack-sizing: ignore" on the child elements tells the stack to _not_ change it's size to accommodate that child. The style is applied to the children of the stack, not the stack itself. This allows the developer to ignore certain children, but not others.

Comment 64

[Eric Shepherd [:sheppy]]

The doc is updated; thanks for the added info, mfinkle.