[Tabs] Fix tab indicator size

[pubiqq]

Fixes #3335

[drchen]

Thanks for the PR! Can you explain what's the root cause of the issue and how the PR fixes it?

[pubiqq]

textView and iconView are not direct children of TabView on API 17 and below (see inflateAndAddDefaultTextView() and inflateAndAddDefaultIconView()), so getLeft()/getRight()/getTop()/getBottom() methods cannot be used to determine the position of these views relative to TabView. This PR fixes the issue by replacing incorrect views with direct TabView children.

Also, in the PR, view.getVisibility() == View.VISIBLE is replaced with view.getVisibility() != View.GONE, because invisible elements also take up space, and therefore should be taken into account when determining the content size.

[drchen]

The original implementation is kind of error-prone, therefore I'm a bit worried about just checking all children can introduce different unexpected bugs.

Instead of that, for now can we fix this in a more conservative way? Like checking BadgeUtils.USE_COMPAT_PARENT to decide if we should use textView.getLeft() or textViewParent.getLeft()? (Same for iconView)

And if customView is not null, maybe we should just skip checking iconView and textView.

Let me know what you think.

[pubiqq]

I don't think it's worth doing.

TabView can have only one of the three view hierarchy configurations:

// #1 For custom view
TabView { CustomView() }

// #2 For icon and/or text (API 18+)
TabView { Icon(), Text() }

// #3 For icon and/or text (API 17-)
TabView { 
    FrameLayout(wrap_content, wrap_content) { Icon() }, 
    FrameLayout(wrap_content, wrap_content) { Text() } 
}

The "PR" way and the "conservative" way will work the same in these cases, but the "PR" way will have a simpler implementation, it's more logically correct, and it's consistent with the behavior of a tab with a custom view set.

[drchen]

The behavior is the same in the current happy flow, yes. : )

However clients can actually add any random view to a tab view, and the implementation of tab view might also changed in the future as well.

If you insist, I can bring this to the internal review and see how the team thinks.

[pubiqq]

However clients can actually add any random view to a tab view, and the implementation of tab view might also changed in the future as well.

This is unlikely, but even if so, the added views will become part of the TabView content and logically should be accounted for in the getContentWidth() and getContentHeight() methods (just as it happens with the custom view). So changing behavior in such an unlikely case is more of a fix than a breakdown.

[drchen]

Dan, can you provide your thoughts here?

[dsn5ft]

The fix seems reasonable to me, but I think we don't need it anymore now that the minSdkVersion is 19 for our library (and all androidx libraries).

Closing the PR but feel free to comment or reopen if you feel it is still necessary.