1364115 - Allow -moz-stack-sizing to work in a single direction only

Status: RESOLVED FIXED

(Core :: XUL, defect)

Description

[:Paolo Amadini]

Allowing -moz-stack-sizing to work in a single direction only will simplify the layout of panelmultiview.

I'm carrying over Neil's patch from bug 1293242 and adding tests.

Comment 1

[:Paolo Amadini]

Attachment: Bug 1364115 - Allow -moz-stack-sizing to work in a single direction only.

Review commit: https://reviewboard.mozilla.org/r/138434/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/138434/

Comment 2

[:Paolo Amadini]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=6c714f6e480b59c23c653f92e6e7b41265a90454

Comment 3

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

At first (very quick) glance I think this mostly looks good, with the caveats that:

 (1) you should be adding new values to layout/style/test/property_database.js

 (2) at this point in the stylo project, new properties need to be implemented in both the Gecko and Servo style systems so that they work in both.  A bunch of people on #layout or #servo could provide advice.

Comment 4

[:Paolo Amadini]

(In reply to David Baron :dbaron: ⌚️UTC-7 from comment #3)
>  (2) at this point in the stylo project, new properties need to be
> implemented in both the Gecko and Servo style systems so that they work in
> both.  A bunch of people on #layout or #servo could provide advice.

Is this true of properties used in XUL layout too?

Comment 5

[:Paolo Amadini]

Comment on attachment 8866842 [details]
Bug 1364115 - Allow -moz-stack-sizing to work in a single direction only.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/138434/diff/1-2/

Comment 6

[:Paolo Amadini]

When running layout/style/test locally some seemingly unrelated tests failed, so let's see what automation says about the property_database.js changes:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=9426f58c5115e827cc0ab167b048e542d8b463e0

Comment 7

[:Paolo Amadini]

Ah, I've seen "error: no field `mStretchStack` on type `gecko_bindings::structs::root::nsStyleXUL`" in the tryserver build.

Comment 8

[:Paolo Amadini]

Comment on attachment 8866842 [details]
Bug 1364115 - Allow -moz-stack-sizing to work in a single direction only.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/138434/diff/2-3/

Comment 9

[:Paolo Amadini]

I guess this is all there is to do as the property is only parsed and not implemented in Stylo, let me know if there is anything else to do, or if I should ask review to someone else for the Stylo parts. If I understand correctly, once this is reviewed in Bugzilla it needs a special landing process.

https://treeherder.mozilla.org/#/jobs?repo=try&revision=39d3f87ffadf42aceae4d38689a5ac0258acd713

Comment 10

[:Paolo Amadini]

There is a devtools test to fix, but Stylo builds and passes tests.

Comment 11

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

Comment on attachment 8866842 [details]
Bug 1364115 - Allow -moz-stack-sizing to work in a single direction only.

https://reviewboard.mozilla.org/r/138434/#review142278

r=dbaron with the following comments, but you should ask someone else (SimonSapin?  Manish?) to review the stylo bits.

::: layout/style/nsStyleConsts.h:233
(Diff revision 3)
>  // stack-sizing
>  #define NS_STYLE_STACK_SIZING_IGNORE         0
>  #define NS_STYLE_STACK_SIZING_STRETCH_TO_FIT 1
> +#define NS_STYLE_STACK_SIZING_IGNORE_HORIZONTAL 2
> +#define NS_STYLE_STACK_SIZING_IGNORE_VERTICAL 3

These should be removed now that you have an enum instead.  You'll need to switch to the enum values in a few other places as well.

::: layout/xul/nsStackLayout.cpp:68
(Diff revision 3)
>  {
>    nsSize prefSize (0, 0);
>  
>    nsIFrame* child = nsBox::GetChildXULBox(aBox);
>    while (child) {
> -    if (child->StyleXUL()->mStretchStack) {
> +    if (child->StyleXUL()->mStackSizing != StyleStackSizing::Ignore) {

In all 4 of these functions, please use:

  auto stackSizing = child->StyleXUL()->mStackSizing;

so you don't call StyleXUL() three times (in each function) when you could call it once.

Comment 12

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

Adding dev-doc-needed to document the new ignore-horizontal and ignore-vertical values of this property.

(And yes, see above, somebody else should review the stylo bits.)

Comment 13

[:Paolo Amadini]

Comment on attachment 8866842 [details]
Bug 1364115 - Allow -moz-stack-sizing to work in a single direction only.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/138434/diff/3-4/

Comment 14

[:Paolo Amadini]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=ca10159f205bd40838141801b6d8fb90c80d5f51

Comment 15

[Manish Goregaokar [:manishearth]]

Comment on attachment 8866842 [details]
Bug 1364115 - Allow -moz-stack-sizing to work in a single direction only.

https://reviewboard.mozilla.org/r/138434/#review142614

Comment 16

[Simon Sapin (:SimonSapin)]

Comment on attachment 8866842 [details]
Bug 1364115 - Allow -moz-stack-sizing to work in a single direction only.

https://reviewboard.mozilla.org/r/138434/#review143004

Deferring to Manish’s review.

Comment 17

[:Paolo Amadini]

Comment on attachment 8866842 [details]
Bug 1364115 - Allow -moz-stack-sizing to work in a single direction only.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/138434/diff/4-5/

Comment 18

[:Paolo Amadini]

https://github.com/servo/servo/pull/16892

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/c1ca4c3d5273
Allow -moz-stack-sizing to work in a single direction only. r=dbaron,manishearth

Comment 20

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/c1ca4c3d5273