Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Flexline width exceeds container width #521

Merged
merged 9 commits into from
Dec 24, 2019

Conversation

AlexBalo
Copy link
Contributor

@AlexBalo AlexBalo commented Dec 3, 2019

In this PR I am trying to address an issue that occurs when the calculation of flexlines exceeds the permitted size of their container and some items can be shrunk.

After successfully calculating the number of flexline the FlexboxLayout proceeds its measurement by determining the main size through the determineMainSize() method. In certain situations, the width of some flexlines might be bigger than the size of their container. In this cases, we need to restrict their calculated width by retriggering a second layout calculation and enforcing their width as the one of their containers.

Before the change
The example below shows how the TextView width is calculated based on the full screen width causing its text to be cut out. In that specific case the total width of the flexline is: ImageView width (which has fixed size) + the TextView width (which is equals to the screen size). No re-measurement happens after the calculation of the number of flexlines since the main size calculated in the determineMainSize() is based on the widest flexline, which in this case exceeds the container width:

Screenshot 2019-12-04 at 10 49 58

After the change
After changing the determineMainSize() method to disallow lines to be wider than their container, the flexlines are correctly recalculated and laid out. The pictures below show the same example with both a single ImageView or with two ImageView on the left and right of the TextView. The end results are showing a correctly recalculated layout:

Screenshot 2019-12-04 at 10 49 10

Screenshot 2019-12-04 at 10 50 35

This Gist contains the classes that have been used to reproduce the scenarios described above.

@googlebot
Copy link

Thanks for your pull request. It looks like this may be your first contribution to a Google open source project (if not, look below for help). Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

📝 Please visit https://cla.developers.google.com/ to sign.

Once you've signed (or fixed any issues), please reply here with @googlebot I signed it! and we'll verify it.


What to do if you already signed the CLA

Individual signers
Corporate signers

ℹ️ Googlers: Go here for more info.

@AlexBalo AlexBalo changed the title [WIP, Do not merge] Flexline exceeding container [WIP, Do not merge] Flexline width exceeding container width Dec 3, 2019
@AlexBalo AlexBalo changed the title [WIP, Do not merge] Flexline width exceeding container width [WIP] Flexline width exceeds container width Dec 4, 2019
@AlexBalo AlexBalo changed the title [WIP] Flexline width exceeds container width Flexline width exceeds container width Dec 5, 2019
@AlexBalo
Copy link
Contributor Author

AlexBalo commented Dec 5, 2019

@googlebot I signed it!

@googlebot
Copy link

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

@AlexBalo AlexBalo changed the title Flexline width exceeds container width [WIP] Flexline width exceeds container width Dec 6, 2019
@AlexBalo
Copy link
Contributor Author

AlexBalo commented Dec 6, 2019

I updated the Gist to also include a Checkbox widget, because I noticed a different behavior happening in this case. In fact, when Checkbox is allowed to shrink (flexShrink = 1f), during the recalculation can end up being shrunk more than what I would consider its "minimum size" since the negative space is calculated for both the TextView and the Checkbox in the shrinkFlexItems() method.

The image below shows the behavior explained above:

Screenshot 2019-12-06 at 18 39 30

I am trying to understand if there is a viable way to take scenarios like this one into account during measurement. In the case of Checkbox specifically, the widget has a ButtonDrawable that has a minimumWidth and minimumHeight and I am wondering if this information should be taken into account. At least on web, a widget like a checkbox does not seem to be shrinkable.

A possible solution can be something like the snippet below where users can decide to use the buttonDrawable information into account in their custom Checkbox widgets, but I am not sure this is the best solution?

Code

override fun onAttachedToWindow() {
    super.onAttachedToWindow()
    if (layoutParams is FlexboxLayout.LayoutParams) {
      val flexboxLayoutParams = layoutParams as FlexboxLayout.LayoutParams
      val minWidth = flexboxLayoutParams.minWidth
      val drawableMinWidth = if (buttonDrawable != null) buttonDrawable!!.minimumWidth else 0
      flexboxLayoutParams.minWidth = if (minWidth == 0) drawableMinWidth else minWidth
    }
  }

Result

Screenshot 2019-12-06 at 20 03 57

@AlexBalo AlexBalo changed the title [WIP] Flexline width exceeds container width Flexline width exceeds container width Dec 20, 2019
@thagikura
Copy link
Contributor

Thanks for the PR! Sorry for the delayed reply...
The fix looks good to me! Can you please add a test to FlexboxHelperTest to verify the fix.

Looks like the issue for the ButtonDrawable can be fixed in another PR.
The code you suggested looks too specific for FlexboxLayout if it's part of a CustomView. So I'd look into if a View's minimum width/height can be used for determing the size to be shrunk.

@AlexBalo
Copy link
Contributor Author

Hi @thagikura, thank you for reviewing the diff.

I added a couple of tests to verify the fix. I agree we can address the ButtonDrawable in a separate PR.

Copy link
Contributor

@thagikura thagikura left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks!

@thagikura thagikura merged commit 9f9d07d into google:master Dec 24, 2019
@Ribesg
Copy link

Ribesg commented Mar 5, 2020

This "fix" broke my UI. Please don't publish breaking changes as fix versions in the future.

@AlexBalo
Copy link
Contributor Author

AlexBalo commented Mar 5, 2020

This "fix" broke my UI. Please don't publish breaking changes as fix versions in the future.

Hi @Ribesg, sorry to hear this fix broke your UI. I had a problem with text rendering and based on the investigation in the Gist in the description, it looked like during onMeasure the Text widget were given the full-screen width even when they did not have it available.

May I ask you if you can share more information about your scenario that got broken after this fix?

Thank you in advance.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants