Bug 99273 - in a column flexbox, input overflows the box when stretched
Summary: in a column flexbox, input overflows the box when stretched
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Layout and Rendering (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Tony Chang
URL: http://plexode.com/eval3/#ht=%3Cdiv%2...
Keywords:
Depends on:
Blocks: 62048
  Show dependency treegraph
 
Reported: 2012-10-14 13:17 PDT by Tony Chang
Modified: 2012-10-16 11:44 PDT (History)
3 users (show)

See Also:


Attachments
Patch (6.33 KB, patch)
2012-10-15 15:57 PDT, Tony Chang
no flags Details | Formatted Diff | Diff
Patch (8.75 KB, patch)
2012-10-16 10:47 PDT, Tony Chang
no flags Details | Formatted Diff | Diff
Patch for landing (9.12 KB, patch)
2012-10-16 11:18 PDT, Tony Chang
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Tony Chang 2012-10-14 13:17:01 PDT
Example in the URL.  On Chromium Mac, the input box overflows the flexbox.
Comment 1 Tony Chang 2012-10-15 15:57:18 PDT
Created attachment 168796 [details]
Patch
Comment 2 Darin Adler 2012-10-15 20:45:14 PDT
Comment on attachment 168796 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=168796&action=review

> Source/WebCore/ChangeLog:15
> +        (WebCore):

Please omit lines like this even when the script generates them.

> Source/WebCore/rendering/RenderBox.cpp:1775
> +static bool stretchingVerticalFlexboxChild(const RenderObject* child)

This function name sounds like a function that returns the “stretching vertical flexbox child”. Probably best to add the word “is” or “should” to the name to disambiguate.

> Source/WebCore/rendering/RenderBox.cpp:1783
> +    if (parent->isFlexibleBox() && parent->style()->flexWrap() == FlexWrapNone && parent->style()->isColumnFlexDirection()
> +        && (child->style()->alignSelf() == AlignStretch || (child->style()->alignSelf() == AlignAuto && parent->style()->alignItems() == AlignStretch)))
> +        return true;

I think this could use a comment to explain why this is the correct set of checks. And maybe even some helper functions.
Comment 3 Tony Chang 2012-10-16 10:47:37 PDT
Created attachment 168972 [details]
Patch
Comment 4 Ojan Vafai 2012-10-16 11:09:05 PDT
Comment on attachment 168972 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=168972&action=review

> Source/WebCore/rendering/RenderBox.cpp:1775
> +static bool flexboxChildHasStretchAlignment(const RenderObject* child)

Naming nit: how about flexItemHasStretchAlignment?

> Source/WebCore/rendering/RenderBox.cpp:1781
> +static bool isStretchingVerticalFlexboxChild(const RenderObject* child)

Naming nit: isStretchingColumnFlexItem

> Source/WebCore/rendering/RenderBox.cpp:1822
>          if (!parent()->style()->isColumnFlexDirection() || parent()->style()->flexWrap() != FlexWrapNone)

If you added isFlexboxRowOrMultiline(parent), then you could use it here and in isStretchingVerticalFlexboxChild. I think it would make the code a bit more readable.
Comment 5 Tony Chang 2012-10-16 11:18:06 PDT
Created attachment 168984 [details]
Patch for landing
Comment 6 WebKit Review Bot 2012-10-16 11:44:40 PDT
Comment on attachment 168984 [details]
Patch for landing

Clearing flags on attachment: 168984

Committed r131481: <http://trac.webkit.org/changeset/131481>
Comment 7 WebKit Review Bot 2012-10-16 11:44:44 PDT
All reviewed patches have been landed.  Closing bug.