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.
Created attachment 189852 [details] browsers on Windows
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.
(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 :)
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.
(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() ?
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?
(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. :(
We could conceivably make RenderButton::hasLineIfEmpty smart enough
Created attachment 189882 [details] Testcase
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?
Created attachment 189885 [details] Screenshot with patch
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.
You might also need to add scrollbarLogicalHeight(). Would be good to add a test case for this while you're changing it.
(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 :(
(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.
Created attachment 190166 [details] Patch
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.
Created attachment 190181 [details] Patch
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
Created attachment 190330 [details] Patch
Created attachment 190331 [details] Patch
Comment on attachment 190331 [details] Patch Clearing flags on attachment: 190331 Committed r144096: <http://trac.webkit.org/changeset/144096>
All reviewed patches have been landed. Closing bug.
Mac rebaselines done in http://trac.webkit.org/changeset/146409.
(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.