LibCSS: Flexbox property support review

[Michael Drake]

Hello,

This is a review of the lcneves/flexbox-tidy branch.

First off I think this is really good work. Almost all the
things I spotted range from minor to trivial.

Aside from the unset values for unit and length in the
min-{width|height} computed style getters, and the fact
that NetSurf is about to do a release[1], I'd be happy
to merge this as-is.

[1] I think NetSurf will need some minor changes to cope
with new values for the display property and the
min-{width|height} properties.



Add codes to flexbox properties
===============================

http://source.netsurf-browser.org/libcss.git/commit/?h=lcneves/flexbox-tidy&id=b1c9ff5a957db2ff86ebacb4b4e735458f7a60ab

+72 - align-content
+ <value> (14bits) :
+ 0 => stretch,
+ 1 => flex-start,
+ 2 => flex-end,
+ 3 => center,
+ 4 => space-between,
+ 5 => space-around,
+ 6 => space-evenly
+ other => Reserved for future expansion.

I'm a bit unsure about the space-evenly value.

CSS Flexible Box Layout Module Level 1 doesn't have this value:
https://www.w3.org/TR/css-flexbox-1/#propdef-align-content

CSS Box Alignment Module Level 3 does have this value and
a few others:
https://www.w3.org/TR/css-align-3/#alignment-values

I think its OK for us to have space-evenly. Same for the
justify-content property.


Parsing: Add support for parsing the flexbox properties
=======================================================

http://source.netsurf-browser.org/libcss.git/commit/?h=lcneves/flexbox-tidy&id=fd73ec6bf364fec71309528a823474533c73a226

src/parse/properties/flex.c:
http://source.netsurf-browser.org/libcss.git/diff/src/parse/properties/flex.c?h=lcneves/flexbox-tidy&id=fd73ec6bf364fec71309528a823474533c73a226

+/**
+ * Parse list-style
+ *

s/list-style/flex/


In css__parse_flex():
+ parserutils_vector_iterate(vector, ctx);
+ }
+
+ /* Handle auto, equivalent of flex: 1 1 auto; */
+ else if ((token->type == CSS_TOKEN_IDENT) &&
+ (lwc_string_caseless_isequal(
+ token->idata, c->strings[AUTO],
+ &match) == lwc_error_ok && match)) {
+ error = css__stylesheet_style_appendOPV(grow_style,
+ CSS_PROP_FLEX_GROW, 0, FLEX_GROW_SET);

Our code style is } else { on a single line, and the comment
would be inside the else block before the first line.

Could do something like:
parserutils_vector_iterate(vector, ctx);

} else if ((token->type == CSS_TOKEN_IDENT) &&
(lwc_string_caseless_isequal(
token->idata, c->strings[AUTO],
&match) == lwc_error_ok && match)) {
/* Handle auto, equivalent of flex: 1 1 auto; */
error = css__stylesheet_style_appendOPV(grow_style,
CSS_PROP_FLEX_GROW, 0, FLEX_GROW_SET);

Also I think we could simplfy handling of auto and none
by creating a copule more bools at the start of the function:

bool auto = false;
bool none = false;

Set those true if we get either of those tokens here, and
then below, in the /* defaults */ section, choose the
defaults that get set based on whether:

1. auto is set
2. none is set
3. neither are set


src/parse/properties/flex_flow.c:
http://source.netsurf-browser.org/libcss.git/diff/src/parse/properties/flex_flow.c?h=lcneves/flexbox-tidy&id=fd73ec6bf364fec71309528a823474533c73a226

+/**
+ * Parse list-style
+ *

s/list-style/flex-flow/


Selection: Add support for the flexbox properties
=================================================

http://source.netsurf-browser.org/libcss.git/commit/?h=lcneves/flexbox-tidy&id=2fd135cd43d6821e944110bbdf8f77bea5cc40fb


In src/select/computed.c:
http://source.netsurf-browser.org/libcss.git/diff/src/select/computed.c?h=lcneves/flexbox-tidy&id=2fd135cd43d6821e944110bbdf8f77bea5cc40fb


In css_computed_min_height():
- return get_min_height(style, length, unit);
+ uint8_t min_height = get_min_height(style, length, unit);
+ uint8_t display = get_display(style);
+
+ if (display != CSS_DISPLAY_FLEX && display !=
CSS_DISPLAY_INLINE_FLEX &&
+ min_height == CSS_MIN_HEIGHT_AUTO)
+ {

Spaces used for indent instead of tab on the first and last
inserted likes above. Also out code style doesn't put the
opening brace on a new line.

Same in css_computed_min_width().

For both css_computed_min_height() css_computed_min_width() we
could rearrange to avoid getting display unless the value is
auto. Something like:


uint8_t css_computed_min_height(const css_computed_style *style,
css_fixed *length, css_unit *unit)
{
uint8_t min_height = get_min_height(style, length, unit);

if (min_height == CSS_MIN_HEIGHT_AUTO) {
uint8_t display = get_display(style);

if (display != CSS_DISPLAY_FLEX &&
display != CSS_DISPLAY_INLINE_FLEX) {
min_height = CSS_MIN_HEIGHT_SET;
}
}

return min_height;
}

Finally, I don't think it's OK to override the value
to *_SET and not set appropriate values for length and unit
at the same time. When the computed value is set to *_AUTO
in the cascade it doesn't clear any values for length and
unit. So we should be them up for 0px where we override
min_height.

Same for both css_computed_min_height() and for
css_computed_min_width().


In css_computed_display():
+ } else if (display == CSS_DISPLAY_INLINE_FLEX) {
+ return CSS_DISPLAY_FLEX;

Spaces used for indent on the first inserted line.


In src/select/computed.h:
http://source.netsurf-browser.org/libcss.git/diff/src/select/computed.h?h=lcneves/flexbox-tidy&id=2fd135cd43d6821e944110bbdf8f77bea5cc40fb

+ * 35 yyybbbaa overflow-y | background-repeat | align-content
+ * 36 bbbbbbaj flex-basis | align-content | justify_content
+ * 37 fffcccjj flex-direction | clear | justify_content

Since align-content and justify-content are split over two
areas, I would say somthing to hint at that in the comments,
e.g.:

* 35 yyybbbaa overflow-y | background-repeat | align-content1
* 36 bbbbbbaj flex-basis | align-content2 | justify_content1
* 37 fffcccjj flex-direction | clear | justify_content2

In the long term perhaps we could consider using a uint32_t
array for the bit data than uint8_t, so its easier to pack
things in without splitting stuff.


+ css_fixed flex_basis;
+
+ css_fixed flex_grow;
+
+ css_fixed flex_shrink;
+
+ int32_t order;

Spaces used for indent.


In src/select/dispatch.c:
http://source.netsurf-browser.org/libcss.git/diff/src/select/dispatch.c?h=lcneves/flexbox-tidy&id=2fd135cd43d6821e944110bbdf8f77bea5cc40fb

+ },
+ {
+ PROPERTY_FUNCS(align_content),

Spaces for indent on the middle inserted line above.


In src/select/propget.h
http://source.netsurf-browser.org/libcss.git/diff/src/select/propget.h?h=lcneves/flexbox-tidy&id=2fd135cd43d6821e944110bbdf8f77bea5cc40fb

-#define BOX_SIZING_INDEX 34
+#define BOX_SIZING_INDEX 23

Good spot! Ditto for the similar change in propset.h.


In get_align_content():
+ /* Most significant bit out of three */
+ bits_b <<= 2;
+
+ uint8_t bits = bits_a | bits_b;

Spaces for indent on the first and last lines above.
Similar in get_justify_content().


Selection: Logic for the flexbox properties
===========================================

http://source.netsurf-browser.org/libcss.git/commit/?h=lcneves/flexbox-tidy&id=587926f812375472962628845226d18f5df95cf1


In src/select/properties/flex_basis.c:
http://source.netsurf-browser.org/libcss.git/diff/src/select/properties/flex_basis.c?h=lcneves/flexbox-tidy&id=587926f812375472962628845226d18f5df95cf1

In css__cascade_flex_basis():
+ uint16_t value = CSS_FLEX_BASIS_INHERIT;
+ css_fixed length = 0;
+ uint32_t unit = UNIT_PX;

Spaces for indent on last line above.


In src/select/properties/order.c:
http://source.netsurf-browser.org/libcss.git/diff/src/select/properties/order.c?h=lcneves/flexbox-tidy&id=587926f812375472962628845226d18f5df95cf1

+ /* Undo the <<10, because this is an integer */
+ order = *((css_fixed *) style->bytecode) >> 10;

Use CSS_RADIX_POINT instead of 10. (From include/libcss/fpmath.h)
Or use the FIXTOINT macro from include/libcss/fpmath.h.

[Tim Hill]

Post by Michael Drake
Hello,
This is a review of the lcneves/flexbox-tidy branch.

[Snip]

This is great news even though I suspect you didn't mean to post it to
'users' :).

I use flex containers a lot to allow blobs of content to flow to fit
mobile to desktop sizes and I became so wound up by NetSurf's inability
to render flex on my own screen, I implemented browser detection and you
can guess what netsurf-style/css is for: it uses display:inline-block
instead of display:flex and while that does the job after a fashion, it's
not quite as pretty. No vertical alignment and no 'intelligent'
distribution across the page.

Will my browser detection please be able to notice a version number
change when flex is added? Otherwise NetSurf users (okay, probably only
me) will be stuck with inline blocks.

[Michael Drake]

Post by Tim Hill
This is great news even though I suspect you didn't mean to post it to
'users' :).

Yes, I meant to send it to the dev list, sorry.

This work adds handling of flexbox properties to LibCSS, but there
has currently been no work to implement the Flexbox layout algorithms
in NetSurf.

We only change version numbers when we do a release.

Cheers,

[Tim Hill]

Post by Michael Drake

Post by Tim Hill
This is great news even though I suspect you didn't mean to post it
to 'users' :).

Yes, I meant to send it to the dev list, sorry.
This work adds handling of flexbox properties to LibCSS, but there has
currently been no work to implement the Flexbox layout algorithms in
NetSurf.
We only change version numbers when we do a release.
Cheers,

Thanks for the clarifications.