Last Comment Bug 510335 - support right/bottom in stack
: support right/bottom in stack
Status: RESOLVED FIXED
[doc-waiting-1.9.3]
: dev-doc-complete
Product: Core
Classification: Components
Component: XP Toolkit/Widgets: XUL (show other bugs)
: Trunk
: All All
: -- normal with 1 vote (vote)
: ---
Assigned To: Neil Deakin
:
Mentors:
Depends on:
Blocks: 167951 516286
  Show dependency treegraph
 
Reported: 2009-08-13 14:20 PDT by Neil Deakin
Modified: 2010-08-19 05:55 PDT (History)
10 users (show)
enndeakin: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
implement this (22.29 KB, patch)
2009-08-14 12:27 PDT, Neil Deakin
neil: review-
Details | Diff | Splinter Review
address comments (23.65 KB, patch)
2009-08-26 13:43 PDT, Neil Deakin
no flags Details | Diff | Splinter Review
updated patch (32.80 KB, patch)
2009-09-03 11:13 PDT, Neil Deakin
no flags Details | Diff | Splinter Review
let's try this patch (32.68 KB, patch)
2009-09-04 12:06 PDT, Neil Deakin
neil: review+
bzbarsky: superreview+
Details | Diff | Splinter Review

Description Neil Deakin 2009-08-13 14:20:41 PDT
The <stack> only supports setting left and top. It should be able to support right and bottom as well to allow offsets from the right and bottom edges.

Setting left only - offset from left, width is child intrinsic width
Setting right only - offset from right, width is child intrinsic width
Setting both left and right - offset from left and right used, width is changed to the area between these offsets
Setting neither left or right - full width of stack (effectively, both offsets are 0)

Setting top/bottom are similar but for the vertical direction.

A minor incompatibility is that currently setting left only and not top (or vice versa) treats the unspecified value as 0. With this change, according to the above description, horizontal and vertical positioning is independent.
Comment 1 Neil Deakin 2009-08-14 12:27:25 PDT
Created attachment 394552 [details] [diff] [review]
implement this
Comment 2 neil@parkwaycc.co.uk 2009-08-24 09:49:30 PDT
Comment on attachment 394552 [details] [diff] [review]
implement this

There was one noticable flaw with the patch and that is that dynamic changes to the bottom and right attributes do not work until the CSS position or the left or top attributes change. I also tripped over a possibly previously existing bug when I changed the CSS from only specifying a vertical position to only specifying a horizontal position (or vice versa, I forget which); the stack for some reason decided that it wasn't positioned and simply stretched to fit.

>+#define SPECIFIED_LEFT 1
>+#define SPECIFIED_RIGHT 2
>+#define SPECIFIED_TOP 4
>+#define SPECIFIED_BOTTOM 8
Not sure whether style is (1 << 0) ... (1 << 3) here.

> nsStackLayout::AddOffset(nsBoxLayoutState& aState, nsIBox* aChild, nsSize& aSize)
No longer used.

>+PRInt8
[PRUint8? My debugger assumes PRInt8 is an ASCII character.]

>+  PRInt8 offsetSpecified = 0;
>+  aOffset = nsMargin(0, 0, 0, 0);
Nit: could move these back down after the early return.

>-          PRBool offsetSpecified = AddOffset(aState, child, offset);
>+          PRInt8 specified = GetOffset(aState, child, offset);
[No need to rename?]

>+                childRect.width = PR_MAX(clientRect.width - offset.LeftRight() - margin.LeftRight(), 0);
Where does the child's minwidth factor in?

>+              childRect.x = PR_MAX(clientRect.XMost() - offset.right - margin.right - childRect.width, 0);
Is right for children that don't stretch the stack? Surely increasing their right offset should just send them offscreen to the left?
Comment 3 Neil Deakin 2009-08-25 17:23:13 PDT
(In reply to comment #2)
> > nsStackLayout::AddOffset(nsBoxLayoutState& aState, nsIBox* aChild, nsSize& aSize)
> No longer used.

This is used by the grid.

> >-          PRBool offsetSpecified = AddOffset(aState, child, offset);
> >+          PRInt8 specified = GetOffset(aState, child, offset);
> [No need to rename?]

It doesn't 'add' anymore so I changed the name.

> >+                childRect.width = PR_MAX(clientRect.width - offset.LeftRight() - margin.LeftRight(), 0);
> Where does the child's minwidth factor in?

So just factor the minwidth/maxwidth in as well? As in:

  childRect.width = PR_MAX(min.width, PR_MIN(max.width, width));

> >+              childRect.x = PR_MAX(clientRect.XMost() - offset.right - margin.right - childRect.width, 0);
> Is right for children that don't stretch the stack? Surely increasing their
> right offset should just send them offscreen to the left?

I'll remove the PR_MAX part.
Comment 4 Neil Deakin 2009-08-26 13:43:28 PDT
Created attachment 396831 [details] [diff] [review]
address comments

I couldn't reproduce the problem where changing whether left was specified to whether top was specified didn't work, or maybe this patch just fixes that.
Comment 5 Boris Zbarsky [:bz] 2009-09-01 18:02:52 PDT
So it recently came up that it would be awfully nice if stack were not even using top/left (since its doing so means we have to make all changes to abs pos HTML stuff slower).

Can we not start using right/bottom if there's something else we can use, and ideally stop using top/left?
Comment 6 Neil Deakin 2009-09-01 18:44:46 PDT
(In reply to comment #5)
> (since its doing so means we have to make all changes to abs pos HTML stuff slower).

Why would left and top on elements which aren't and cannot be positioned have any impact?
Comment 7 Boris Zbarsky [:bz] 2009-09-01 19:47:22 PDT
Because the style change computation code has no idea whether they're positioned or not: top/left/bottom/right don't live in the same struct as "position".
Comment 8 Boris Zbarsky [:bz] 2009-09-01 20:07:44 PDT
Just to be clear, I have no problem with adding the attributes.  I'd rather we removed the need for the CSS properties altogether if possible; if not I guess adding right/bottom doesn't make things any worse (other than adding more code we want to update when we do remove the CSS properties from here).
Comment 9 neil@parkwaycc.co.uk 2009-09-02 17:19:54 PDT
I still can't get dynamic right/bottom attribute changes to work...

(In reply to comment #3)
> (In reply to comment #2)
> > > nsStackLayout::AddOffset(nsBoxLayoutState& aState, nsIBox* aChild, nsSize& aSize)
> > No longer used.
> This is used by the grid.
Ah, then at some point it should move to nsGridLayout2 or some such?

> > >+                childRect.width = PR_MAX(clientRect.width - offset.LeftRight() - margin.LeftRight(), 0);
> > Where does the child's minwidth factor in?
> So just factor the minwidth/maxwidth in as well?
Well, I thought that perhaps I had overlooked something not shown in context.
Comment 10 Neil Deakin 2009-09-03 11:10:24 PDT
(In reply to comment #9)
> I still can't get dynamic right/bottom attribute changes to work...

Works fine for me:

<stack style="border: 1px solid red;">
  <button label="Right" right="60" bottom="10"
          oncommand="this.setAttribute('right', '20')"/>
  <button label="Bottom" right="10" bottom="10"
          oncommand="this.setAttribute('bottom', '20')"/>
</stack>

> Ah, then at some point it should move to nsGridLayout2 or some such?

I'll make this change now.
Comment 11 Neil Deakin 2009-09-03 11:13:14 PDT
Created attachment 398423 [details] [diff] [review]
updated patch

Removed the right/bottom css properties for now, moved the grid specific calls into nsGridLayout2 and added tests for position changing. I also fixed an array indexing bug in the test which prevented some checks from running.
Comment 12 neil@parkwaycc.co.uk 2009-09-03 12:55:31 PDT
I did some poking around and to get the bottom and right attribute changes to work in all cases you need to update nsBoxFrame::AttributeChanged to clear the NS_STATE_STACK_NOT_POSITIONED bit and you also need to update nsXULAttribute::GetAttributeChangeHint to request a reflow.

I'm not sure it makes sense to have a mismatch between the attributes and the CSS - I'd be happy with removing the CSS top and left too.
Comment 13 Neil Deakin 2009-09-04 12:06:05 PDT
Created attachment 398728 [details] [diff] [review]
let's try this patch
Comment 14 neil@parkwaycc.co.uk 2009-09-04 15:32:54 PDT
Comment on attachment 398728 [details] [diff] [review]
let's try this patch

>+      is(stack.getBoundingClientRect().width, ignoreStackSizing ? 12 : 26, "right changed no left stack width" + add);
>+      
Nit: whitespace
Comment 15 Boris Zbarsky [:bz] 2009-09-08 06:04:58 PDT
Comment on attachment 398728 [details] [diff] [review]
let's try this patch

>+++ b/layout/xul/base/src/nsStackLayout.cpp
>+#define SPECIFIED_LEFT (1 << 0)
>+#define SPECIFIED_RIGHT (1 << 1)
>+#define SPECIFIED_TOP (1 << 2)
>+#define SPECIFIED_BOTTOM (1 << 3)

I'd prefer we bitshift by NS_SIDE_RIGHT and the like.  It'll have the same effect (though a different bit order).

>+++ b/layout/xul/base/src/nsStackLayout.h
>+  static PRUint8 GetOffset(nsBoxLayoutState& aState, nsIBox* aChild, nsMargin& aMargin);

The return value, at least, needs to be documented.

This looks great otherwise. Are we sure we're not going to break people with the top/left style removal?  If so, can you file a followup bug on me to adjust the reflow hints accordingly?
Comment 17 Eric Shepherd [:sheppy] 2010-08-19 05:55:13 PDT
Added to documentation:

https://developer.mozilla.org/en/XUL/Attribute/bottom
https://developer.mozilla.org/en/XUL/Attribute/right

And confirmed to be linked from:

https://developer.mozilla.org/en/xul/stack

Note You need to log in before you can comment on or make changes to this bug.