Alter layout engine to conform closer to W3C spec

[rigdern]

In my team's usage of css-layout, we've found it to behave differently than what is described in the W3C spec for flexbox. We've made some changes to make the implementation more closely conform to the W3C spec and to clearly define where its behavior differs from the spec.

The primary goals of this change are:

• Better conformance to the W3C flexbox standard (https://www.w3.org/TR/css-flexbox-1/)
  and a clear articulation of the areas where it deviates from the spec.
• Support for flex-shrink.
• Conformance with layout effects of "overflow: hidden".

Specifically, here are the limitations of this implementation as compared to the W3C
flexbox standard (this is also documented in Layout.js):

• Display property is always assumed to be 'flex' except for Text nodes, which
  are assumed to be 'inline-flex'.
• The 'zIndex' property (or any form of z ordering) is not supported. Nodes are
  stacked in document order.
• The 'order' property is not supported. The order of flex items is always defined
  by document order.
• The 'visibility' property is always assumed to be 'visible'. Values of 'collapse'
  and 'hidden' are not supported.
• The 'wrap' property supports only 'nowrap' (which is the default) or 'wrap'. The
  rarely-used 'wrap-reverse' is not supported.
• Rather than allowing arbitrary combinations of flexGrow, flexShrink and
  flexBasis, this algorithm supports only the three most common combinations:
  • flex: 0 is equivalent to flex: 0 0 auto
  • flex: n (where n is a positive value) is equivalent to flex: n 0 0
  • flex: -1 (or any negative value) is equivalent to flex: 0 1 auto
• Margins cannot be specified as 'auto'. They must be specified in terms of pixel
  values, and the default value is 0.
• The 'baseline' value is not supported for alignItems and alignSelf properties.
• Values of width, maxWidth, minWidth, height, maxHeight and minHeight must be
  specified as pixel values, not as percentages.
• There is no support for calculation of dimensions based on intrinsic aspect ratios
  (e.g. images).
• There is no support for forced breaks.
• It does not support vertical inline directions (top-to-bottom or bottom-to-top text).

And here is how the implementation deviates from the standard (this is also documented in
Layout.js):

• Section 4.5 of the spec indicates that all flex items have a default minimum
  main size. For text blocks, for example, this is the width of the widest word.
  Calculating the minimum width is expensive, so we forego it and assume a default
  minimum main size of 0.
• Min/Max sizes in the main axis are not honored when resolving flexible lengths.
• The spec indicates that the default value for 'flexDirection' is 'row', but
  the algorithm below assumes a default of 'column'.

[vjeux]

Thanks a lot for doing this! For context, the way I built this is by running randomly generated examples and trying to match how Chrome behaved by reverse engineering it. I didn't look at the spec for this as I found it very hard to understand.

I'm glad that you were able to align the implementation more closely to the specifications!

Since it's a pretty big pull request, it may take some time to review it and break some things internally but I'll make sure that we handle it! Thanks!

[sahrens]

Wow, awesome work!

I love how you enumerated all the deviations from the spec - would you mind providing some explanations as to why it's still divergent? It's going to take a lot of work to get this landed (tons of testing and fixing of broken internal apps) so I'd rather rip off as much of the bandaid at once by being intentional about the bahavior we do/don't change. We of course don't want the perfect to be the enemy of the good, however.

Looking forward to seeing this go in!

[emilsjolander]

This is awesome. Thank you!

This pull request includes a number of changes which seem to be independent of one another. Would you mind splitting this up into smaller atomic commits which can be merged individually? This would greatly improve the quality of review we can perform on the code as well as ease up the process of figuring out what parts of the changes are possibly incompatible with our internal usage and address those.

We will also be running some performance benchmarks on internal products running this new code, if there are any major regressions it would help us greatly to be able to locate a smaller revision which caused those regressions.

Overall the changes are very welcome and I would love to see them merged 🍻

[erictraut]

@sahrens - You asked about why we didn't implement the spec fully. We tried to take a pragmatic approach. By ignoring some of the more esoteric aspects of the spec, we were able to reduce complexity and code size and minimize performance impact. If the community decides that some of these features would be useful, they can generally be added in the future with little or no risk of breaking existing apps. The two exceptions I can think of are 1) support for auto min sizes and 2) honoring min/max sizes in the main axis when resolving flexible lengths. We decided not to implement these for performance reasons. If they were added in the future, they could affect the layout and therefore have backward compatibility impact.

Eric Traut
Microsoft Corp.

[rigdern]

@emilsjolander Did you have any ideas in mind about how to split up the commit? Although I listed several goals of the PR above, I'm not sure there are obvious independent pieces to break it up into. In achieving our primary goals of having the implementation more closely conform to the spec and making it clear where it deviates from the standard, we significantly reworked the implementation of layoutNodeImpl by closely following the algorithm described in the spec. As we went through the spec, there were behaviors we chose to omit and new features we chose to include.

I could go back and split a new feature like flex-shrink into its own commit. However, I'm not sure how helpful that would be because even without flex-shrink, the implementation of layoutNodeImpl in our PR is significantly different from the current version.

If you have any suggestions about how to break up the commit, I'd be happy to try them out.

[sahrens]

I totally agree that a pragmatic approach is best and we don't need to fully implement the spec, I was just hoping to get the reasoning behind the choices we're making to reduce the chance of using wrong assumptions when making decisions.

Not doing something because of perf is totally reasonable, for example, but we should make sure the perf is a real problem before declaring it so. Do you have more specifics?

On Apr 27, 2016, at 11:09 AM, Adam Comella <[email protected]mailto:[email protected]> wrote:

@emilsjolanderhttps://github.com/emilsjolander Did you have any ideas in mind about how to split up the commit? Although I listed several goals of the PR above, I'm not sure there are obvious independent pieces to break it up into. In achieving our primary goals of having the implementation more closely conform to the spec and making it clear where it deviates from the standard, we significantly reworked the implementation of layoutNodeImpl by closely following the algorithm described in the spechttps://urldefense.proofpoint.com/v2/url?u=https-3A__www.w3.org_TR_css-2Dflexbox-2D1_-23layout-2Dalgorithm&d=CwMCaQ&c=5VD0RTtNlTh3ycd41b3MUw&r=tYMi0MxDhl6quxlbQ2USOQ&m=oUfSf_oMH7qUqAd2FMBQI7b0y65wl11rt6OovFmOaOo&s=zuYjNuHR8AyqIctS_CM4Vn014DLQJW6sD9crqGKrP-w&e=. As we went through the spec, there were behaviors we chose to omit and new features we chose to include.

I could go back and split a new feature like flex-shrink into its own commit. However, I'm not sure how helpful that would be because even without flex-shrink, the implementation of layoutNodeImpl in our PR is significantly different from the current version.

If you have any suggestions about how to break up the commit, I'd be happy to try them out.

—
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHubhttps://github.com//pull/185#issuecomment-215176384

[emilsjolander]

@rigdern Sure i'll try to describe what parts I think could become good separate diffs. You know more about your changes so feel free to fill me in on any aspects which i get wrong.

1. Change implementation of layoutNodeImpl to be able to accommodate future changes. This commit should make no functional changes, this makes it very easy to test and validate performance.
2. Add documentation stating what certain features are missing compared to the flexbox spec and describe any other deviations.
3. Add support for min/max height/width.
4. Add support for flex-shrink.
5. etc.

In other words I would prefer starting by refactoring thins in a way with improves upon the current code base and also allows for feature additions we wish to make. Then implement each feature one at a time.

[rigdern]

@emilsjolander This was along the lines I was thinking of trying to split it. However, the reworking of layoutNodeImpl is so significant and the code to add the new functionality is so small that I don't think it's worth trying to split up the commits. I don't think splitting will provide much value in terms of understanding or testing the PR. In splitting, we'd have to test each commit to ensure the act of splitting the PR didn't introduce a regression. Here are some specific details about the numbered list you created above:

1. In this step, we essentially rewrote the guts of layoutNodeImpl by closely following the algorithm listed in the spec. This is where most of the work went. At this point, the implementation of layoutNodeImpl is already significantly different from the original.
2. Documentation for limitations and deviations of the implementation. It seems like this would go well with (1). What would be the value of splitting it out?
3. Support for min/max. This is achieved by calling the boundAxis functions throughout the implementation similar to what the original implementation did. This one would be difficult to split out because it appears all over the implementation and there were pre-existing unit tests validating this functionality.
4. Support for flex-shrink. This is achieved by this code block which is analogous to the code for handling the flex-grow case. This is such a small amount of code compared to the rest of the PR that I don't see the value in splitting it out.
5. Support for overflow:hidden. This is achieved by this code block. This is such a small amount of code compared to the rest of the PR that I don't see the value in splitting it out.

Overall, my point is that (1) is such a large change and the others are such small changes that I don't believe the effort of splitting up the PR would make the PR much easier to understand or validate. Hopefully that gives you some insight into my thought process.

Adam Comella
Microsoft Corp.

[erictraut]

@sahrens Here are some specifics about the perf/complexity tradeoffs.

1. Supporting auto-min sizes requires a separate "min-content" measurement step for all text nodes to find the smallest width that accommodates the widest unbreakable word in the string. This is a significant amount of extra runtime calculation.
2. The full spec requires n passes to resolve flex sizes in the main axis. The value of n depends on how many times you need to loop before the result converges. We decided to stick with a two-pass solution. This is consistent with @vjeux's earlier implementation (and therefore matches the existing unit tests). In practice, you need to create very contrived layouts to ever go beyond two passes.

Eric Traut
Microsoft Corp.

[emilsjolander]

@erictraut These tradeoffs make sense. They are something we have had in mind while developing css-layout so it is great to know that they have been kept in mind while performing these changes. Thanks for explicitly stating them.

[sahrens]

That makes a lot of sense, thanks for the specifics!

On Apr 27, 2016, at 4:52 PM, Emil Sjölander <[email protected]mailto:[email protected]> wrote:

@erictrauthttps://github.com/erictraut These tradeoffs make sense. They are something we have had in mind while developing css-layout so it is great to know that they have been kept in mind while performing these changes. Thanks for explicitly stating them.

—
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHubhttps://github.com//pull/185#issuecomment-215266989

[emilsjolander]

@rigdern I still see worth in splitting up new features from (1) even though (1) cannot be split up into smaller pieces. These other pull requests will be very easy to review and will go through very smoothly. It also gives us a bit less to test and worry about when merging (1).

We should discuss if we are able to split (1) into smaller pieces though. I would love to here more about what the workflow was to implement this pull request. My assumption is that you went through the code and at every stage compared it to the spec. If it deviated from the spec without good reason you updated the code to comply. If these assumptions are correct I think that (1) could be split up into smaller pull requests which each change one aspect of css-layout to better comply with the spec.

[emilsjolander]

@rigdern Some of our internal tests break due to the measure function now being called to an EXACTLY measure mode but a size of 0 when it was previously called with UNDEFINED measure mode. By hacking a work around in our internal code to convert this case to be UNDEFINED measure mode the tests pass again.

[emilsjolander]

@rigdern When running some performance tests there was one thing i noticed which i think could be optimized. When calculating the flex basis of a test node without set dimensions a measurement is performed. Later when laying out the children another measurement is performed even though (i think) it is not needed. I am testing on a layout without any flex defined.

I noticed that you are caching the measurement results of nodes. However as the first call is called with performLayout = false and the second layout is called with performLayout = true it looks like this might cause the measurement called from performing the flex layout not to hit the cached value from basis calculation. Does this make sense?

The tree i am testing with is a very simple stack of text nodes:

- container (direction = Column)
--- Text
--- Text
--- Text
--- Text

As each text node's measurement does not rely on any of its siblings measurements i would assume each text node to only be measured once. However with this implementation they are all measured twice.

[erictraut]

@emilsjolander Good find. Yes, I agree that there's an opportunity to eliminate some of these measure() calls. I'll dig into this and try to identify a clean way to short-circuit the redundant measurements.

[sahrens]

Another opportunistic thing to take a look at while you're digging into this perf stuff if you have time: switching a component from position: 'absolute' to position: 'relative' should do minimal recalculation of layout of the container dimensions don't change. I think we've come across some issues where this is not the case.

On May 6, 2016, at 3:04 PM, Eric Traut <[email protected]mailto:[email protected]> wrote:

@emilsjolanderhttps://github.com/emilsjolander Good find. Yes, I agree that there's an opportunity to eliminate some of these measure() calls. I'll dig into this and try to identify a clean way to short-circuit the redundant measurements.

—
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHubhttps://github.com//pull/185#issuecomment-217572142

[rigdern]

@rigdern Some of our internal tests break due to the measure function ...

@emilsjolander If you provide me with an example layout that exhibits this behavior, I can look into this to try to understand why the behavior has changed and determine what the right fix might be.

[erictraut]

@sahrens Does that situation (switching from absolute to relative or vice versa) come up often in practice? We don't see that behavior in any of our code.

That's a difficult case to optimize without some specific hackery because a style change like this dirties the modified node and all of the nodes between it and the root. That means we discard any cached measurements and layout information for those nodes.

[sahrens]

Ideally we can make the layout cache/dirty mechanism smart enough (generally) to not invalidate things if the parent constraints are unchanged.

This type of interaction might happen for example when flying out a photo from inside a list view to a full screen modal, or when doing incremental offscreen rendering, then dropping the finished view into a list (this is how the experimental WindowedListView works).

On May 6, 2016, at 3:23 PM, Eric Traut <[email protected]mailto:[email protected]> wrote:

@sahrenshttps://github.com/sahrens Does that situation (switching from absolute to relative or vice versa) come up often in practice? We don't see that behavior in any of our code.

That's a difficult case to optimize without some specific hackery because a style change like this dirties the modified node and all of the nodes between it and the root. That means we discard any cached measurements and layout information for those nodes.

—
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHubhttps://github.com//pull/185#issuecomment-217578023

[erictraut]

@emilsjolander Based on your suggestion, I've played around with some additional caching rules for text nodes. In my tests so far, the results are very promising. The generic caching rules that were previously in place allowed us to avoid measurements about 17% of the time that we encounter a text node. The new rules allow us to avoid measurements 72% of the time. In other words, this optimization eliminates over half of the text measurements performed with the new layout algorithm. I think that will go a long way toward closing the performance gap.

[emilsjolander]

@erictraut This is great news. I'm looking forward to testing the updated code.

[emilsjolander]

@rigdern I'll try to isolate a minimal example of a layout exhibiting this behavior.

[emilsjolander]

@rigdern I was able to reproduce the problem of a node being measured with height = 0 and heightMode = EXACTLY with the following very simple layout:

<View>
   <Text/> <!-- Or anything else with a measure function -->
</View>

No styles were needed to be applied to the elements. Just using the default styles given by css-layout.

[rigdern]

@emilsjolander I just pushed 2 new commits. One fixes a bug with the positioning of elements that have a min/max width/height. The other introduces the optimizations Eric mentioned for skipping calls to measure.

It's important to note that the optimizations in that commit assume that measure functions are only used to measure horizontal text. For example, the heuristics assume that changing the width may affect the height but assume that changing the height will not affect the width. Consequently, the introduction of these heuristics may break measuring functions that are not intended for measuring horizontal text. For such functions, cached results may be used when in fact the measure function needed to be called.

Is it safe to assume that measure functions are only used for horizontal text? If not, here are a couple of options:

1. Add a flag to layout nodes that indicates that their measure functions are for measuring horizontal text. Then we can restrict these heuristics to those nodes.
2. Move the heuristics inside of React Native's text measure functions. One potential issue is that the heuristics currently utilize the 16 (CSS_MAX_CACHED_RESULT_COUNT) cache slots built into the layout engine whereas, as far as I know, React Native's text measure functions currently only have 1 cache slot.

[emilsjolander]

@rigdern This is not an assumption we can make. The measure function should work as expected for any sorts of custom measured elements, not just text. Can we look into making this optimization more general? I'll have a look at the code myself and see what I can find.

Currently (with this PR) elements are being measured twice even though no flex is specified. This seems like a bug. If i'm missing something please inform me but I don't see a reason as to why we need to measure an element twice when no flex or stretch alignment is specified.

[rigdern]

@emilsjolander An implementation like the following should avoid the double measure issue without assuming that the measure function is only for horizontal text:

  function canUseCachedTextMeasurement(availableWidth, availableHeight,
    marginRow, marginColumn,
    widthMeasureMode, heightMeasureMode,
    cachedLayout) {

    // Is it an exact match?
    if (cachedLayout.availableWidth === availableWidth &&
        cachedLayout.availableHeight === availableHeight &&
        cachedLayout.widthMeasureMode === widthMeasureMode &&
        cachedLayout.heightMeasureMode === heightMeasureMode) {
      return true;
    }

    if (cachedLayout.availableWidth === availableWidth &&
        cachedLayout.widthMeasureMode === widthMeasureMode &&
        heightMeasureMode === CSS_MEASURE_MODE_EXACTLY &&
        availableHeight - marginColumn === cachedLayout.computedHeight) {
      return true;
    }

    if (cachedLayout.availableHeight === availableHeight &&
        cachedLayout.heightMeasureMode === heightMeasureMode &&
        widthMeasureMode === CSS_MEASURE_MODE_EXACTLY &&
        availableWidth - marginRow === cachedLayout.computedWidth) {
      return true;
    }

    return false;
  }

This only catches a small subset of the cases that the other implementation caught.

I only spent a few minutes coming up with this to try to provide you with some kind of answer now. I'll give this more thought tomorrow.

[rigdern]

We did some testing in our app using the relaxed heuristics above. They eliminated ~60% of the measure calls. The stricter heuristics which made too many assumptions eliminated ~67% of the measure calls. So it seems like the relaxed heuristics are pretty good. I just pushed an update which uses the relaxed heuristics. Let me know what you think.

[rigdern]

@emilsjolander Interesting, I've never seen that error. It doesn't repro when I run the unit tests or when I build React Native.

Anyway, I updated the PR to make canUseCachedMeasurement static. Thanks for reporting this.

[emilsjolander]

@rigdern Awesome thanks. I think this is very close to being able to be merged now. I noticed that a react native test for text input on android is broken with this code.

https://github.com/facebook/react-native/blob/master/ReactAndroid/src/test/java/com/facebook/react/views/textinput/TextInputTest.java#L89

The above test breaks on the height assertion. I've been debugging it and it seems like measure is never called for that text node. Running the same test prior to this pull request the measure function is called as expected which ensures the height is set and the test passes. Something in the new heuristic is causing the node to never be measured.

[rigdern]

The measure function isn't being called for that text node due to an optimization where we assume that if the available width (for example, the parent's width) is less than or equal to 0, then we don't need to call the node's measure function and can assume that the node's size is 0x0.

I confirmed with Emil that this is a valid assumption. I added a commit to generalize the optimization to cover the case where the available height is less than or equal to 0. Emil will fix the React Native unit tests by running them in a root view which has a non-zero height and width.

[emilsjolander]

@rigdern Why does POSITIVE_FLEX_IS_AUTO exists? It seems to never be set to anything else than false

[rigdern]

@emilsjolander the value of POSITIVE_FLEX_IS_AUTO affects the behavior of the style flex: n (where n is a positive value). Specifically:

1. If POSITIVE_FLEX_IS_AUTO is 1, then css-layout flex: n is equivalent to W3C flex: n 1 auto
2. If POSITIVE_FLEX_IS_AUTO is 0, then css-layout flex: n is equivalent to W3C flex: n 0 0

The tradeoff between these two options is between performance and flexibility. Option (2) is faster because the content doesn't need to be measured but it's less flexible because the basis is always 0 and can't be overriden with the width/height attributes. This information is described in a comment in the engine.

While working on the engine, we experimented with both options. We ultimately went with option (2) because it enabled us to achieve all of our app's scenarios while giving us better performance.

That's the background I'm familiar with. Perhaps @erictraut has additional information to add.

[erictraut]

Yes, @rigdern is correct. POSITIVE_FLEX_IS_AUTO provides more flexibility. It allows for some layouts that are not possible otherwise. The downside is that it adds more overhead because "auto" requires additional measurements to compute the initial flex basis value based on the content size. By switching from "flex n 1 auto" to "flex n 0 0", we eliminate some flexibility but gain some performance. I left it in the code in case someone felt they needed the extra flexibility in the future. If you think that it's confusing to leave that compile-time switch in the code, feel free to delete it.

[emilsjolander]

I should have stated my question clearer. I know what it does, was just wondering why it was a compile time flag. It is somewhat confusing when reading the code. Could we instead describe the optimization in a comment on the isFlexBasisAuto() function and remove the flag?

[woehrl01]

How about adding a public static bool flag at the engine level to be able to control this on runtime?

[emilsjolander]

I'm ok with making it a flag but i should not be a static flag. Rather a
flag on the node is that case. Otherwise it would not be possible to turn
off this optimization on only parts of the layout that need it.
On Thu, 26 May 2016 at 12:38, Lukas Wöhrl [email protected] wrote:

How about adding a public static bool flag at the engine level to be able
to control this on runtime?

—
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#185 (comment)

[emilsjolander]

@rigdern Lets leave this as is for now. I'll look into changing it later.

[jordwalke]

What is the meaning of this -1? Shouldn't you make a new enum state to model whatever this special kind of undefined means?

[rigdern]

Thanks for the feedback.

It means that width_measure_mode is uninitialized. The idea is that while a cache entry's width_measure_mode has this value, layoutNodeInternal will never return that cache entry because the widthMeasureMode given to layoutNodeInternal will never match -1.

This pattern was used for layout.last_direction prior to this PR: https://github.com/facebook/css-layout/blob/b0d00ad33850d83450139d994bded89d20ddac32/src/Layout.c#L80

If we were to create an explicit enum value instead of using -1, it might give people the idea that it is okay to call layoutNode with that enum value for widthMeasureMode or heightMeasureMode which is not the case. This could be mitigated with documentation.

That's the rationale. I'm not sure whether or not having an explicit enum value would result in more idiomatic C code.

[jordwalke]

I was mostly just asking for my own understanding of the algorithm, so feel free to disregard any of my suggestions. Especially since this pattern was in use beforehand, and I don't typically write idiomatic C often.

[emilsjolander]

Yeah this pattern is weird and hard to understand what it does (decrementing a type name?). However it was in the code base before this PR so let's not change it here. I'll look into updating the code later to remove it.