Bug 110654 – Empty <button>s should collapse; empty <input type="button"> should not collapse

• Status: RESOLVED
• Resolution: FIXED
• Component: Forms

[Tony Chang]

I've attached a screenshot from IE9, Firefox, Chrome 26, and Chrome Canary.

I think we want to match IE and Firefox's behavior of collapsing <button> and not collapsing <input type="button"> when there is no button text.  Previous, we would not collapse either and after r143643, we now collapse both.  I bet there are some websites that depend on <input type="button"> not collapsing.

[Tony Chang]

Created attachment 189852 [details]
browsers on Windows

[Christian Biesinger]

I'll note that previously, we would _sometimes_ collapse them :) See https://bugs.webkit.org/attachment.cgi?id=188679

But I'll look into how to get the uncollapsed behaviour.

[Tony Chang]

(In reply to comment #2)
> I'll note that previously, we would _sometimes_ collapse them :) See https://bugs.webkit.org/attachment.cgi?id=188679
> 
> But I'll look into how to get the uncollapsed behaviour.

Yeah, Ojan pointed that out to me as well.  That seems like a bug to me :)

[Ojan Vafai]

There's a method somewhere that controls whether empty elements collapse. For example, if the element is editable, then it doesn't collapse. I can't seem to find it at the moment.

[Ryosuke Niwa]

(In reply to comment #4)
> There's a method somewhere that controls whether empty elements collapse. For example, if the element is editable, then it doesn't collapse. I can't seem to find it at the moment.

Maybe hasLineIfEmpty() ?

[Christian Biesinger]

Hmm that one already returns true for buttons:

./rendering/RenderButton.h:    virtual bool hasLineIfEmpty() const { return true; }


However, I notice that the deprecated flexbox has code like this:
        if (!iterator.first() && hasLineIfEmpty())
            setHeight(height() + lineHeight(true, style()->isHorizontalWritingMode() ? HorizontalLine : VerticalLine, PositionOfInteriorLineBoxes));

Does new flexbox need something like this?

[Ojan Vafai]

(In reply to comment #6)
> Does new flexbox need something like this?

Yes, I think that's the issue. Although, interestingly, it will mean that we will not collapse both <button> and <input type="button">, which doesn't match other browsers. :(

[Christian Biesinger]

We could conceivably make RenderButton::hasLineIfEmpty smart enough

[Christian Biesinger]

Created attachment 189882 [details]
Testcase

[Christian Biesinger]

Created attachment 189883 [details]
patch?

This is clearly missing changelogs and tests, but with this patch, empty buttons (of all kinds) become 16px high. This compares to 6px when they are collapsed and to 22px when they have actual text.

Is that correct behaviour?

[Christian Biesinger]

Created attachment 189885 [details]
Screenshot with patch

[Ojan Vafai]

Comment on attachment 189883 [details]
patch?

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

Looks good. We should probably make RenderButton::hasLineIfEmpty so that we match other browsers, but that should probably be a separate patch.

> Source/WebCore/rendering/RenderFlexibleBox.cpp:734
> +        setHeight(height() + lineHeight(true, style()->isHorizontalWritingMode() ? HorizontalLine : VerticalLine, PositionOfInteriorLineBoxes));

Also, I know the deprecated flexbox code did this but there a few things I'd change:
-Just call isHorizontalWritingMode(). It caches the style value.
-s/setHeight/setLogicalHeight/
-s/height()/borderAndPaddingLogicalHeight()/ I'm not 100% sure about this, but I think that's what the old code was trying to achieve and it's more explicit this way.

> Source/WebCore/rendering/RenderFlexibleBox.cpp:735
> +    if (!haveLine && hasLineIfEmpty()) {
> +        setHeight(height() + lineHeight(true, style()->isHorizontalWritingMode() ? HorizontalLine : VerticalLine, PositionOfInteriorLineBoxes));
> +    }

One line if shouldn't have braces.

[Ojan Vafai]

You might also need to add scrollbarLogicalHeight(). Would be good to add a test case for this while you're changing it.

[Christian Biesinger]

(In reply to comment #12)
> Looks good. We should probably make RenderButton::hasLineIfEmpty so that we match other browsers, but that should probably be a separate patch.

I sort of agree, but I don't want to regenerate PNGs twice :(

[Christian Biesinger]

(In reply to comment #10)
> This is clearly missing changelogs and tests, but with this patch, empty buttons (of all kinds) become 16px high. This compares to 6px when they are collapsed and to 22px when they have actual text.

With all these suggested changes, empty buttons do become 22px high, same as non-empty ones. So that now sounds definitely correct.

[Christian Biesinger]

Created attachment 190166 [details]
Patch

[Ojan Vafai]

Comment on attachment 190166 [details]
Patch

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

Looks great.

> Source/WebCore/rendering/RenderFlexibleBox.cpp:733
> +        // We need to make sure to have a line. We might not have a line even if we entered the above loop,
> +        // because all our children might be positioned. Therefore, we use this approach to ensure the
> +        // right height.

This could be a comment more focused on the "why" and less on the "what", e.g.,

// Even if computeNextFlexLine returns true, the flexbox might not have a line because all our
// children might be out of flow positioned. Instead of just checking if we have a line,
// make sure the flexbox has at least a line's worth of height to cover this case.

[Christian Biesinger]

Created attachment 190181 [details]
Patch

[WebKit Review Bot]

Comment on attachment 190181 [details]
Patch

Rejecting attachment 190181 [details] from commit-queue.

Failed to run "['/mnt/git/webkit-commit-queue/Tools/Scripts/webkit-patch', '--status-host=webkit-commit-queue.appspot.com', '--bot-id=gce-cq-02', 'apply-attachment', '--no-update', '--non-interactive', 190181, '--port=chromium-xvfb']" exit_code: 2 cwd: /mnt/git/webkit-commit-queue

Last 500 characters of output:
utTests/platform/gtk/TestExpectations
patching file LayoutTests/platform/mac/TestExpectations
Hunk #1 succeeded at 1440 (offset 3 lines).
patching file LayoutTests/platform/qt/TestExpectations
Hunk #1 FAILED at 2680.
1 out of 1 hunk FAILED -- saving rejects to file LayoutTests/platform/qt/TestExpectations.rej
patching file LayoutTests/platform/win/TestExpectations

Failed to run "[u'/mnt/git/webkit-commit-queue/Tools/Scripts/svn-apply', '--force']" exit_code: 1 cwd: /mnt/git/webkit-commit-queue

Full output: http://webkit-commit-queue.appspot.com/results/16743962

[Christian Biesinger]

Created attachment 190330 [details]
Patch

[Christian Biesinger]

Created attachment 190331 [details]
Patch

[WebKit Review Bot]

Comment on attachment 190331 [details]
Patch

Clearing flags on attachment: 190331

Committed r144096: <http://trac.webkit.org/changeset/144096>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Ryosuke Niwa]

Mac rebaselines done in http://trac.webkit.org/changeset/146409.

[Ryosuke Niwa]

(In reply to comment #24)
> Mac rebaselines done in http://trac.webkit.org/changeset/146409.

Oops, somehow I messed it up. Done in http://trac.webkit.org/changeset/146410.