1503180 - Compact the flex item sizing property table a bit

Status: RESOLVED FIXED

(DevTools :: Inspector, enhancement, P2)

Description

[Patrick Brosset <:pbro>]

Attachment: image (1).png

From a UI review from Victoria: 

- Looking at the sizing table now. Re: "Base Size: The item's content size when unconstrained," I wonder if we can just word this as "Content Size" or "Initial Content Size"?
Would be nice to not have the extra row of explanatory text

- Similarly, for the row with Flexibility heading and also `flex-grow: 0;`, I'm tempted to combine those two info bits into one heading

- Another question - instead of using the word Size, should we use Height or Width based on the direction?

Attached is an illustration of the proposal.

Comment 1

[Victoria Wang [:victoria]]

Re: diagram colors
  --magenta-65 at 13% opacity
  --blue40 at 25% opacity
grow/shrink arrow: magenta 65 100%

Sizing table labels: semibold 11px, colors: Blue 50, Magenta 70, and Purple 50 for Padding (assuming that's still one of the types)

Dark mode: labels should use --theme-highlight-blue --theme-highlight-red --theme-highlight-purple, exact diagram colors are on my todo list

Comment 2

[Patrick Brosset <:pbro>]

(In reply to Patrick Brosset <:pbro> from comment #0)
> - Looking at the sizing table now. Re: "Base Size: The item's content size
> when unconstrained," I wonder if we can just word this as "Content Size" or
> "Initial Content Size"?
> Would be nice to not have the extra row of explanatory text
I'm tempted to keep the section title unchanged (Base Size) because that's what this section is about. And the base size may come from various things: a width CSS property, a flex-basis property, or the size of the content.
I like the idea of making it simpler when neither width/height nor flex-basis are defined, but then what do we show when they are?
So, when nothing is defined:
[Initial Content Size         70px]

If flex-basis is defined:
[Base Size (flex-basis:70px)  70px]

If width is defined:
[Base Size (width:70px)       70px]

The idea for having the flex-basis or width listed here on top of the actual value is that those are CSS properties, and it would be nice for them to appear as such, and even in the future become clickable so you can jump to the rule-view to see them in context.

> - Similarly, for the row with Flexibility heading and also `flex-grow: 0;`,
> I'm tempted to combine those two info bits into one heading
Sounds good to me. If we do this, it'd be nice to make all CSS properties appear next to their respective section headers.
Example:

[Flexibility (flex-grow:1)      +500px]
Item was set to grow ....

[Maximum Size (max-width:300px)  300px]

> - Another question - instead of using the word Size, should we use Height or
> Width based on the direction?
I sort of like the word Size because it's independent of things like rtl, writing-mode and CSS transforms which might potentially make what you're seeing in the page not match your mental model of what width or height usually mean.
But I'm not set on this. If you see a strong reason for us to use the actual directions instead, I'm happy for us to change it.

Comment 3

[Patrick Brosset <:pbro>]

Attachment: Bug 1503180 - Making the flex item sizing UI more compact and simple; r?gl!

Comment 4

[Patrick Brosset <:pbro>]

This first patch does not address the color changes from comment 1, but makes the table more compact to match the mockups.
Victoria: do you mind looking over my comment 2 and letting me know your thoughts?

Comment 5

[Patrick Brosset <:pbro>]

I intend to work on a second patch here to align the outline and sizing table colors with what Victoria suggested, but I'll need to wait for bug 1503206 to land first.

Comment 6

[Victoria Wang [:victoria]]

> So, when nothing is defined:
> [Initial Content Size         70px]
> 
> If flex-basis is defined:
> [Base Size (flex-basis:70px)  70px]
> 
> If width is defined:
> [Base Size (width:70px)       70px]

Yes, this plan sounds great!


> [Flexibility (flex-grow:1)      +500px]
> Item was set to grow ....
> 
> [Maximum Size (max-width:300px)  300px]

This seems like a very nice way to follow the pattern as well.


> I sort of like the word Size because it's independent of things like rtl,
> writing-mode and CSS transforms which might potentially make what you're
> seeing in the page not match your mental model of what width or height
> usually mean.
> But I'm not set on this. If you see a strong reason for us to use the actual
> directions instead, I'm happy for us to change it.

Ah, I think I see what you mean - and if we do just mention "max-width" or whatever property, seems fine to stay with Size.

Comment 7

[Victoria Wang [:victoria]]

I'm going through the list of explanation strings and will post some ideas soon.

One thing I saw in this user comment (https://discourse.mozilla.org/t/flexbox-inspector-input-wanted/30611/18) - seems like there's a case currently where it can say three things in Flexibility:

There wasn't enough room available on the flex line
Item was not set to shrink
Item was set to shrink but could not

I'm not sure exactly what these things mean together but maybe there's some way we could condense it into one sentence that's easier to understand

Comment 8

[Patrick Brosset <:pbro>]

(In reply to Victoria Wang [:victoria] from comment #7)
> One thing I saw in this user comment
> (https://discourse.mozilla.org/t/flexbox-inspector-input-wanted/30611/18) -
> seems like there's a case currently where it can say three things in
> Flexibility:
> 
> There wasn't enough room available on the flex line
> Item was not set to shrink
> Item was set to shrink but could not
> 
> I'm not sure exactly what these things mean together but maybe there's some
> way we could condense it into one sentence that's easier to understand
Yes, this is a bug that I just fixed yesterday (bug 1501066). Things should make a lot more sense now.
Unfortunately, this is fixed in 65, but DevEdition 64 out there does not have this fix.

Comment 9

[Victoria Wang [:victoria]]

New ideas for this - https://mozilla.invisionapp.com/share/MZNA228P5WB#/328646784_flexbox_Item_Examples (started conversation in Slack)

Comment 10

[Victoria Wang [:victoria]]

Here's the text for my suggested changes, for copy/paste goodness:

Flexibility
1. The item had room to grow. (Actually I think it may be more clear here to say "The item grew to fit."
2. The item was set to grow, but there wasn't enough room.
3. The item shrank to fit. 

Minimum
1. The item was clamped to its minimum size. (or "minimum content size" for min-content?)
2. The item had room to grow, but was clamped to its minimum size.
3. The item's growth was overridden by a greater minimum size.

Padding
The item's padding increased the size by 20px.

Looking at this now, I wonder if Flexibility #2 and Minimum #2 are a bit too redundant if they're shown together.

Comment 11

[Patrick Brosset <:pbro>]

I've gotten an R+ from Gabriel on the part 1 patch. It does not address recent comments here, but I will mark this bug as "leave-open" so I can work on these in subsequent parts.

Part 1 try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=a75f473d80de01652bd0c6e4e523a038775ea363

Comment 12

[Patrick Brosset <:pbro>]

(In reply to Patrick Brosset <:pbro> from comment #11)
> Part 1 try push:
> https://treeherder.mozilla.org/#/
> jobs?repo=try&revision=a75f473d80de01652bd0c6e4e523a038775ea363
I fixed the failing tests on those and pushed to try again: https://treeherder.mozilla.org/#/jobs?repo=try&revision=d39060504b2dd7aa9e5794165b2d586d650b5b29&group_state=expanded
This one has an ESLint failure that I also fixed since then. I'll update the patch on phabricator and land it.

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/63975abdead1
Making the flex item sizing UI more compact and simple; r=gl

Comment 14

[Patrick Brosset <:pbro>]

Attachment: Bug 1503180 - Part 2 - Simplify the base size section when it's the content size; r?mtigley!

Comment 15

[Patrick Brosset <:pbro>]

Attachment: Bug 1503180 - Part 3 - Move the clamping reasons to the min and max sections; r?mtigley!

Depends on D11047

Comment 16

[Patrick Brosset <:pbro>]

(In reply to Victoria Wang [:victoria] from comment #9)
> New ideas for this -
> https://mozilla.invisionapp.com/share/MZNA228P5WB#/
> 328646784_flexbox_Item_Examples (started conversation in Slack)
I made some changes in the 2 new patches I just attached.
I don't think we can achieve all of the things described in this mockup though.
For instance, displaying whether there was enough room or not for an item on a line is actually quite tricky for reasons described in bug 1496740 comment 4.

Comment 17

[Patrick Brosset <:pbro>]

Try push for parts 2 and 3: https://treeherder.mozilla.org/#/jobs?repo=try&revision=b01edd501b65c2db290a9b95fac04e6dcac3efd5
These parts have been R+'d too, so I'll proceed to land them.

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/7c45e486aaf9
Part 2 - Simplify the base size section when it's the content size; r=mtigley

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/ef979c11ccc7
Part 3 - Move the clamping reasons to the min and max sections; r=mtigley

Comment 20

[Patrick Brosset <:pbro>]

Let's keep this open a bit longer until bug 1503206 lands and then we can work on a part 4 to align the section colors to the colors in  the minimap.

Comment 21

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/63975abdead1

Comment 22

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/7c45e486aaf9
https://hg.mozilla.org/mozilla-central/rev/ef979c11ccc7

Comment 23

[Patrick Brosset <:pbro>]

No need to depend on that minimap bug anymore. Victoria provided colors in bug 1508404. So since I touched the sizing component here, let me apply the right colors to it here, in a part 4 patch, and then we can close this bug.

Comment 24

[Patrick Brosset <:pbro>]

Attachment: Bug 1503180 - Part 4 - Give the sizing section names the right colors; r?mtigley!

Comment 25

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/be33905d0aca
Part 4 - Give the sizing section names the right colors; r=mtigley

Comment 26

[Bogdan Tara[:bogdan_tara | bogdant]]

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

Comment 27

[Victoria Wang [:victoria]]

I was looking at https://bugzilla.mozilla.org/show_bug.cgi?id=1508396#c2 and realized something had changed in how the Content/Base info was displayed. An item that has flex-basis: 0.000000001px; defined was showing "Content Size _____ 0" in its sizing table.

I think it used to say Base Size "(flex-basis: 1e-9px) _____ 0"

I think (based on the idea of this mockup https://mozilla.invisionapp.com/d/#/console/14988591/328646784/preview) I expect it to say something like "Base Size (flex-basis: ~0) ______ 0"

Comment 28

[Patrick Brosset <:pbro>]

That's odd, looking at the code again, the title "Content Size" should only ever appear when no dimension property is defined (flex-basis or width/height).
I just tested on the latest nightly with this test URL:
data:text/html,<div style="display:flex"><div style="flex-basis: 0.000000001px;">test
and I do see "(flex-basis: 1e-9px) _____ 0" as expected (we're fixing the 1e-9px scientific notation right now).
Could you test again and file a new bug if this keeps on happening for you?

Comment 29

[Victoria Wang [:victoria]]

Your example works as expected, but youtube has the same thing I saw before - I'll file a new bug.