Skip to content
This repository has been archived by the owner on Feb 2, 2023. It is now read-only.

[ASStackLayoutSpec] Refactor baseline alignment algorithm #2892

Merged

Conversation

nguyenhuy
Copy link
Contributor

@nguyenhuy nguyenhuy commented Jan 12, 2017

The current baseline alignment algorithm is implemented as an extra step that is executed after our main stack algorithm finished. While it has served us well for a long time, this implementation has a few problems that should be addressed:

  • It diverges from CSS Flexbox algorithm (here).
  • It's not extensible. For example, implementing flex-wrap requires adding a new multi-line logic to multiple places.
  • It's hard to maintain. Changes in the main algorithm need to be ported to the baseline algorithm as well. Failing to do will likely introduce bugs that only occur when the new changes are used together with baseline alignment. [ASStackLayoutSpec] Baseline Alignment + Space Between Justification Does Not Space Correctly #1604 is an example.
  • It doesn't correctly handle stretched items (failed snapshot test). This problem happens because these items are stretched in the main algorithm, before baseline alignment is applied and increases the cross size of the stack. Those stretched items are also baseline-aligned!

This PR does a few things:

@nguyenhuy
Copy link
Contributor Author

nguyenhuy commented Jan 12, 2017

Since this PR touches a core component of the layout system, I really appreciate reviews from @maicki, @rcancro, @appleguy, @garrettmoon and @Adlai-Holler.

@nguyenhuy
Copy link
Contributor Author

Hm, the new snapshot test cases are failing on Jenkins. They pass locally! Now sure what is going on and how to see image differences on a Jenkins machine?

@nguyenhuy nguyenhuy force-pushed the HNRefactorBaselineAlignment branch 2 times, most recently from 3f3e22e to 40bc71b Compare January 16, 2017 23:55
@nguyenhuy
Copy link
Contributor Author

Tests are all green now! It turned out text rasterization (antialiasing in particular) works a bit different on iOS 10 and that causes my new snapshot test cases which were captured on iOS 9 to fail.

I will try to clean up our reference images tomorrow in a separate PR.

Copy link
Contributor

@Adlai-Holler Adlai-Holler left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@nguyenhuy Wow this is an great improvement that came out of nowhere. It may be a pre-existing issue and I'm definitely willing to land as-is if need be, but the baselineLast case seems to be off by a pixel.

If this is caused by inconsistent rounding after the layout pass, it may be pretty tough to fix, and it's not really related to this change.

Thoughts?
screen shot 2017-01-17 at 5 13 00 pm

@nguyenhuy
Copy link
Contributor Author

nguyenhuy commented Jan 18, 2017

@Adlai-Holler: Great catch on the misalignment. It's indeed a pre-existing issue. These snapshots were generated based on the current implementation on master.

It has something to do with the baseline difference between the texts that should be floored but instead (indirectly) ceiled. I'm investigating this but don't think it should block this PR.

@Adlai-Holler
Copy link
Contributor

OK I created the linked issue to track the rounding error. It's time to merge this! Awesome work @nguyenhuy – improvements in many dimensions with a single diff ✨

@Adlai-Holler Adlai-Holler merged commit 6736367 into facebookarchive:master Jan 18, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants