Closed Bug 1364115 Opened 3 years ago Closed 3 years ago

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

Categories

(Core :: XUL, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla55
Tracking Status
firefox55 --- fixed

People

(Reporter: Paolo, Assigned: enndeakin)

References

Details

(Keywords: dev-doc-needed)

Attachments

(1 file)

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.
Blocks: 1009116
No longer blocks: 1293242
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.
Flags: needinfo?(paolo.mozmail)
(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?
Flags: needinfo?(paolo.mozmail) → needinfo?(dbaron)
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
Ah, I've seen "error: no field `mStretchStack` on type `gecko_bindings::structs::root::nsStyleXUL`" in the tryserver build.
Flags: needinfo?(dbaron) → needinfo?(paolo.mozmail)
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
Flags: needinfo?(paolo.mozmail) → needinfo?(dbaron)
There is a devtools test to fix, but Stylo builds and passes tests.
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.
Attachment #8866842 - Flags: review?(dbaron) → review+
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.)
Flags: needinfo?(dbaron)
Keywords: dev-doc-needed
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
Attachment #8866842 - Flags: review+
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.
Attachment #8866842 - Flags: review?(simon.sapin)
Pushed by manishearth@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/c1ca4c3d5273
Allow -moz-stack-sizing to work in a single direction only. r=dbaron,manishearth
https://hg.mozilla.org/mozilla-central/rev/c1ca4c3d5273
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
You need to log in before you can comment on or make changes to this bug.