Compact the flex item sizing property table a bit

RESOLVED FIXED in Firefox 65

Status

enhancement
P2
normal
RESOLVED FIXED
6 months ago
4 months ago

People

(Reporter: pbro, Assigned: pbro)

Tracking

unspecified
Firefox 65

Firefox Tracking Flags

(firefox65 fixed)

Details

Attachments

(5 attachments)

(Assignee)

Description

6 months ago
Posted image 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.
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
(Assignee)

Comment 2

6 months ago
(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.
(Assignee)

Updated

6 months ago
Assignee: nobody → pbrosset
Severity: normal → enhancement
Status: NEW → ASSIGNED
Priority: -- → P2
(Assignee)

Comment 4

6 months ago
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?
Flags: needinfo?(victoria)
(Assignee)

Comment 5

6 months ago
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.
Depends on: 1503206
> 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.
Flags: needinfo?(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
(Assignee)

Comment 8

6 months ago
(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.
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.
(Assignee)

Comment 11

6 months ago
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
Keywords: leave-open
(Assignee)

Comment 12

6 months ago
(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

6 months ago
Pushed by pbrosset@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/63975abdead1
Making the flex item sizing UI more compact and simple; r=gl
(Assignee)

Comment 16

6 months ago
(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.
(Assignee)

Comment 17

6 months ago
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

6 months ago
Pushed by pbrosset@mozilla.com:
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

6 months ago
Pushed by pbrosset@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/ef979c11ccc7
Part 3 - Move the clamping reasons to the min and max sections; r=mtigley
(Assignee)

Comment 20

6 months ago
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.
(Assignee)

Comment 23

5 months ago
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.
No longer depends on: 1503206
(Assignee)

Updated

5 months ago
Keywords: leave-open

Comment 25

5 months ago
Pushed by pbrosset@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/be33905d0aca
Part 4 - Give the sizing section names the right colors; r=mtigley

Comment 26

5 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/be33905d0aca
Status: ASSIGNED → RESOLVED
Last Resolved: 5 months ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 65
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"
(Assignee)

Comment 28

5 months ago
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?
Flags: needinfo?(victoria)
Your example works as expected, but youtube has the same thing I saw before - I'll file a new bug.
Flags: needinfo?(victoria)
You need to log in before you can comment on or make changes to this bug.