Bug 110654 - Empty <button>s should collapse; empty <input type="button"> should not collapse
Summary: Empty <button>s should collapse; empty <input type="button"> should not collapse
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Forms (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Christian Biesinger
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-02-22 15:52 PST by Tony Chang
Modified: 2013-03-20 16:44 PDT (History)
4 users (show)

See Also:


Attachments
browsers on Windows (37.19 KB, image/png)
2013-02-22 15:53 PST, Tony Chang
no flags Details
Testcase (200 bytes, text/html)
2013-02-22 17:58 PST, Christian Biesinger
no flags Details
patch? (1.68 KB, patch)
2013-02-22 18:02 PST, Christian Biesinger
no flags Details | Formatted Diff | Diff
Screenshot with patch (37.52 KB, image/png)
2013-02-22 18:03 PST, Christian Biesinger
no flags Details
Patch (62.94 KB, patch)
2013-02-25 17:37 PST, Christian Biesinger
no flags Details | Formatted Diff | Diff
Patch (63.00 KB, patch)
2013-02-25 19:08 PST, Christian Biesinger
no flags Details | Formatted Diff | Diff
Patch (63.16 KB, patch)
2013-02-26 11:13 PST, Christian Biesinger
no flags Details | Formatted Diff | Diff
Patch (63.16 KB, patch)
2013-02-26 11:17 PST, Christian Biesinger
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 2013-02-22 15:52:35 PST
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.
Comment 1 Tony Chang 2013-02-22 15:53:12 PST
Created attachment 189852 [details]
browsers on Windows
Comment 2 Christian Biesinger 2013-02-22 15:56:54 PST
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.
Comment 3 Tony Chang 2013-02-22 16:00:10 PST
(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 :)
Comment 4 Ojan Vafai 2013-02-22 16:36:08 PST
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.
Comment 5 Ryosuke Niwa 2013-02-22 16:42:01 PST
(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() ?
Comment 6 Christian Biesinger 2013-02-22 16:46:11 PST
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?
Comment 7 Ojan Vafai 2013-02-22 16:47:49 PST
(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. :(
Comment 8 Christian Biesinger 2013-02-22 16:49:50 PST
We could conceivably make RenderButton::hasLineIfEmpty smart enough
Comment 9 Christian Biesinger 2013-02-22 17:58:18 PST
Created attachment 189882 [details]
Testcase
Comment 10 Christian Biesinger 2013-02-22 18:02:39 PST
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?
Comment 11 Christian Biesinger 2013-02-22 18:03:58 PST
Created attachment 189885 [details]
Screenshot with patch
Comment 12 Ojan Vafai 2013-02-22 18:30:18 PST
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.
Comment 13 Ojan Vafai 2013-02-22 18:31:09 PST
You might also need to add scrollbarLogicalHeight(). Would be good to add a test case for this while you're changing it.
Comment 14 Christian Biesinger 2013-02-25 12:04:14 PST
(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 :(
Comment 15 Christian Biesinger 2013-02-25 12:27:24 PST
(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.
Comment 16 Christian Biesinger 2013-02-25 17:37:53 PST
Created attachment 190166 [details]
Patch
Comment 17 Ojan Vafai 2013-02-25 18:25:26 PST
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.
Comment 18 Christian Biesinger 2013-02-25 19:08:49 PST
Created attachment 190181 [details]
Patch
Comment 19 WebKit Review Bot 2013-02-26 02:35:11 PST
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
Comment 20 Christian Biesinger 2013-02-26 11:13:36 PST
Created attachment 190330 [details]
Patch
Comment 21 Christian Biesinger 2013-02-26 11:17:02 PST
Created attachment 190331 [details]
Patch
Comment 22 WebKit Review Bot 2013-02-26 13:10:46 PST
Comment on attachment 190331 [details]
Patch

Clearing flags on attachment: 190331

Committed r144096: <http://trac.webkit.org/changeset/144096>
Comment 23 WebKit Review Bot 2013-02-26 13:10:50 PST
All reviewed patches have been landed.  Closing bug.
Comment 24 Ryosuke Niwa 2013-03-20 16:34:20 PDT
Mac rebaselines done in http://trac.webkit.org/changeset/146409.
Comment 25 Ryosuke Niwa 2013-03-20 16:44:05 PDT
(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.