Note: There are a few cases of duplicates in user autocompletion which are being worked on.

Overlayed children affect computed size of flexible stack

RESOLVED FIXED

Status

()

Core
XUL
--
enhancement
RESOLVED FIXED
11 years ago
29 days ago

People

(Reporter: Andrew Miller, Assigned: vlad)

Tracking

(Blocks: 1 bug, {dev-doc-complete})

Trunk
dev-doc-complete
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(4 attachments, 14 obsolete attachments)

740 bytes, application/vnd.mozilla.xul+xml
Details
2.09 KB, application/vnd.mozilla.xul+xml
Details
1.38 KB, application/vnd.mozilla.xul+xml
Details
17.95 KB, patch
roc
: review+
Details | Diff | Splinter Review
(Reporter)

Description

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

Comment 1

11 years ago
Created attachment 231713 [details] [diff] [review]
Patch, adds new -moz-parent-size-interaction style
(Reporter)

Updated

11 years ago
Attachment #231713 - Flags: superreview?(roc)
Attachment #231713 - Flags: review?(roc)
(Reporter)

Comment 2

11 years ago
Created attachment 231715 [details]
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

11 years ago
One approach, assuming everyone agreed, would be for stack sizing to simply ignore all positioned children, which would also fix bug 345937.
(Reporter)

Comment 4

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

Comment 5

11 years ago
Created attachment 231888 [details] [diff] [review]
Patch implementing suggestion by roc & (independently) neil
Attachment #231713 - Attachment is obsolete: true
Attachment #231888 - Flags: superreview?(roc)
Attachment #231888 - Flags: review?(neil)
Attachment #231713 - Flags: superreview?(roc)
Attachment #231713 - Flags: review?(roc)
(Reporter)

Updated

11 years ago
Attachment #231888 - Attachment description: Patch implement suggestion by roc & (independently) neil → Patch implementing suggestion by roc & (independently) neil
+  // 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

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

Comment 9

11 years ago
Created attachment 232069 [details] [diff] [review]
Last patch but addressing comments from neil and roc.
Attachment #231888 - Attachment is obsolete: true
Attachment #232069 - Flags: superreview?(neil)
Attachment #232069 - Flags: review?(roc)
Attachment #231888 - Flags: superreview?(roc)
Attachment #231888 - Flags: review?(neil)
(Reporter)

Updated

11 years ago
Blocks: 345937

Comment 10

11 years ago
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.
Attachment #232069 - Flags: superreview?(neil) → superreview-
(Reporter)

Comment 11

11 years ago
Created attachment 232480 [details] [diff] [review]
Last patch but addressing comments from neil
Attachment #232069 - Attachment is obsolete: true
Attachment #232480 - Flags: superreview?(roc)
Attachment #232480 - Flags: review?(neil)
Attachment #232069 - Flags: review?(roc)
(Reporter)

Comment 12

11 years ago
Created attachment 232482 [details] [diff] [review]
Last patch but with the right flags to diff
Attachment #232480 - Attachment is obsolete: true
Attachment #232482 - Flags: superreview?(roc)
Attachment #232482 - Flags: review?(neil)
Attachment #232480 - Flags: superreview?(roc)
Attachment #232480 - Flags: review?(neil)
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.
Attachment #232482 - Flags: superreview?(roc) → superreview+
(Reporter)

Comment 14

11 years ago
Created attachment 232493 [details] [diff] [review]
Previous patch + address roc's comment 13
Attachment #232482 - Attachment is obsolete: true
Attachment #232493 - Flags: review?(neil)
Attachment #232482 - Flags: review?(neil)

Comment 15

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

Comment 16

11 years ago
(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.
How about an attribute on stack elements, say ignorepositionedchildren="true"
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

11 years ago
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.
Attachment #232493 - Flags: review?(neil) → review+
But what about all the other XUL apps out there that we might be breaking?
(Reporter)

Comment 21

11 years ago
Created attachment 251107 [details] [diff] [review]
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).
Assignee: nobody → ak.miller
Attachment #232493 - Attachment is obsolete: true
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
(Reporter)

Comment 22

11 years ago
Created attachment 251109 [details]
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.
Attachment #231715 - Attachment is obsolete: true
(Reporter)

Comment 23

11 years ago
Sorry, that should actually have said -moz-parent-stretch: none, not -moz-parent-no-stretch.
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.
(Reporter)

Comment 25

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

Updated

11 years ago
Attachment #251107 - Flags: review?(roc)
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.
Attachment #251107 - Flags: review?(roc) → review?(dbaron)
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.
Attachment #251107 - Flags: review?(dbaron) → review-
(Reporter)

Comment 28

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

But flexibility does influence whether things can flex to smaller sizes, I think.
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.
(Reporter)

Comment 31

10 years ago
(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

10 years ago
(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 ;-)
(Reporter)

Comment 33

10 years ago
Created attachment 276861 [details] [diff] [review]
Attachment addressing review comments by dbaron, roc, and neil
Attachment #251107 - Attachment is obsolete: true
Attachment #276861 - Flags: review?(dbaron)
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.
(Reporter)

Comment 35

10 years ago
Created attachment 276897 [details]
The test-case updated to use -moz-stack-stretch
Attachment #251109 - Attachment is obsolete: true
(Reporter)

Comment 36

10 years ago
Created attachment 276898 [details] [diff] [review]
Use the -moz-stack-sizing naming scheme suggested by Hixie
Attachment #276861 - Attachment is obsolete: true
Attachment #276898 - Flags: superreview?(roc)
Attachment #276898 - Flags: review?(dbaron)
Attachment #276861 - Flags: review?(dbaron)
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
Attachment #276898 - Flags: review?(dbaron) → review+
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/ .
(Reporter)

Comment 39

10 years ago
Created attachment 278345 [details] [diff] [review]
As before but move the attribute to the end of the interface
Attachment #276898 - Attachment is obsolete: true
Attachment #278345 - Flags: superreview?(roc)
Attachment #276898 - Flags: superreview?(roc)
Attachment #278345 - Flags: superreview?(roc) → superreview+
(Reporter)

Updated

10 years ago
Attachment #278345 - Flags: approval1.9?
(Reporter)

Updated

10 years ago
Keywords: checkin-needed
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?
(Reporter)

Comment 41

10 years ago
(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
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 :-)
And add a link to that page to the Gecko 1.9 meeting agenda wiki page.
This didn't get on the meeting agenda :-( sorry. Try next week.
Removing checkin-needed until approval is granted.
Keywords: checkin-needed
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.
Attachment #278345 - Flags: approval1.9? → approval1.9-
(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)
(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

10 years ago
(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)
Thanks Nickolay, I'll respond there if I feel the need to (instead of spamming this bug).
(Reporter)

Comment 51

10 years ago
Created attachment 305922 [details] [diff] [review]
Fixes bitrot of the previous patch
Attachment #278345 - Attachment is obsolete: true
(Reporter)

Comment 52

10 years ago
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.
Created attachment 320551 [details] [diff] [review]
updated patch

Here's an updated patch based on the previous one that roc reviewed.  I'm writing some tests now, will attach shortly.
Attachment #305922 - Attachment is obsolete: true
Attachment #320551 - Flags: review?(roc)
Created attachment 320553 [details]
reftest

A reftest
Created attachment 320554 [details]
reftest reference
(The reftest needs a max-width: 400px on the vbox to function properly in the reftest framework)
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.
Created attachment 320760 [details] [diff] [review]
updated patch, again

Patch updated with stretch check in Layout, and reftests in patch.
Assignee: ak.miller → vladimir
Attachment #320551 - Attachment is obsolete: true
Attachment #320760 - Flags: review?(roc)
Attachment #320551 - Flags: review?(roc)
Comment on attachment 320760 [details] [diff] [review]
updated patch, again

dbaron already reviewed the style system bits.
Attachment #320760 - Flags: superreview+
Attachment #320760 - Flags: review?(roc)
Attachment #320760 - Flags: review+
Blocks: 435077

Updated

9 years ago
Blocks: 436061
Checked this in; forgot to mark fixed.
Status: ASSIGNED → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED

Comment 61

9 years ago
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.
Keywords: dev-doc-needed

Updated

9 years ago
Component: XP Toolkit/Widgets: XUL → XUL
QA Contact: xptoolkit.xul → xptoolkit.widgets
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. :)
(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.
The doc is updated; thanks for the added info, mfinkle.
Keywords: dev-doc-needed → dev-doc-complete
You need to log in before you can comment on or make changes to this bug.