Bug 61415

I had some experiments. I think we can remove most of the width calculation code by using flex-box. However we can't remove height-related calculation for some reasons.

The current layout code for the outer-spin-button is really tricky and doesn't work correctly in some cases. I'd like to move the spin button into the border/padding even in Mac... (Yes, I made the outer-spin-button!!)

Created attachment 95883 [details] WIP

Created attachment 97553 [details] Patch

Attachment 97553 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast..." exit_code: 1 LayoutTests/platform/chromium/test_expectations.txt:3762: Duplicate or ambiguous expectation. fast/speech/input-appearance-numberandspeech.html [test/expectations] [5] LayoutTests/platform/chromium/test_expectations.txt:3764: Duplicate or ambiguous expectation. fast/speech/input-appearance-speechbutton.html [test/expectations] [5] Total errors found: 2 in 39 files If any of these errors are false positives, please file a bug against check-webkit-style.

Created attachment 97555 [details] Patch 2 Correct test_expectations.txt

Comment on attachment 97555 [details] Patch 2 View in context: https://bugs.webkit.org/attachment.cgi?id=97555&action=review > LayoutTests/ChangeLog:15 > + * platform/mac/fast/css/input-search-padding-expected.png: I think the new -expected.png is correct. These search fields has padding-bottom. We should apply it. > LayoutTests/ChangeLog:42 > + * platform/mac/fast/table/colspanMinWidth-vertical-expected.png: The width (logical height) of the input becomes correct.

Comment on attachment 97555 [details] Patch 2 Attachment 97555 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/8878441

Created attachment 97557 [details] Patch 3 Fix a build warning

Comment on attachment 97557 [details] Patch 3 Attachment 97557 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/8881477 New failing tests: fast/forms/search-cancel-button-mouseup.html fast/forms/search-abs-pos-cancel-button.html

Created attachment 97565 [details] Archive of layout-test-results from ec2-cr-linux-03 The attached test failures were seen while running run-webkit-tests on the chromium-ews. Bot: ec2-cr-linux-03 Port: Chromium Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

Created attachment 97569 [details] Patch 4 Try to fix Chromium-linux test failures

Created attachment 97570 [details] Patch 5 Try to fix Chromium-linux test failures

Comment on attachment 97570 [details] Patch 5 View in context: https://bugs.webkit.org/attachment.cgi?id=97570&action=review This looks like a great incremental clean-up! > Source/WebCore/html/HTMLInputElement.cpp:135 > +HTMLElement* HTMLInputElement::containerElement() const All these shadow DOM accessors on HTMLInputElement are going away eventually, right? > Source/WebCore/html/TextFieldInputType.cpp:151 > + bool shouldHaveSpinButton = RenderTheme::themeForPage(document->page())->shouldHaveSpinButton(element()); This is super-weird. Why create a new theme here, rather than just querying document->page()->theme()? > Source/WebCore/rendering/RenderTextControlSingleLine.cpp:278 > + innerTextRenderer->layoutIfNeeded(); This seems weird too... Are you going to eventually remove all this code? If you don't intend to, I am afraid we are not better off here. > Source/WebCore/rendering/RenderTextControlSingleLine.cpp:312 > bool RenderTextControlSingleLine::nodeAtPoint(const HitTestRequest& request, HitTestResult& result, const IntPoint& pointInContainer, const IntPoint& accumulatedOffset, HitTestAction hitTestAction) nodeAtPoint looks completely unnecessary after shadow DOM conversion. All elements should correctly react to the hit testing. Are you planning to remove this altogether? > Source/WebCore/rendering/RenderTextControlSingleLine.cpp:-437 > - if (!event->isMouseEvent()) { > - RenderTextControl::forwardEvent(event); > - return; > - } > - > -#if ENABLE(INPUT_SPEECH) > - if (RenderBox* speechBox = speechButtonElement() ? speechButtonElement()->renderBox() : 0) { > - RenderBox* parent = innerTextRenderer ? innerTextRenderer : this; > - FloatPoint pointInTextControlCoords = parent->absoluteToLocal(static_cast<MouseEvent*>(event)->absoluteLocation(), false, true); > - if (speechBox->frameRect().contains(roundedIntPoint(pointInTextControlCoords))) { > - speechButtonElement()->defaultEventHandler(event); > - return; > - } > - } > -#endif > - > - FloatPoint localPoint = innerTextRenderer->absoluteToLocal(static_cast<MouseEvent*>(event)->absoluteLocation(), false, true); > - int textRight = innerTextRenderer->borderBoxRect().maxX(); > - > - HTMLElement* resultsButton = resultsButtonElement(); > - HTMLElement* cancelButton = cancelButtonElement(); > - if (resultsButton && localPoint.x() < innerTextRenderer->borderBoxRect().x()) > - resultsButton->defaultEventHandler(event); > - else if (cancelButton && localPoint.x() > textRight) > - cancelButton->defaultEventHandler(event); > - else > - RenderTextControl::forwardEvent(event); Wooo weee! :)

Comment on attachment 97570 [details] Patch 5 View in context: https://bugs.webkit.org/attachment.cgi?id=97570&action=review Thank you for reviewing the large change! >> Source/WebCore/html/HTMLInputElement.cpp:135 >> +HTMLElement* HTMLInputElement::containerElement() const > > All these shadow DOM accessors on HTMLInputElement are going away eventually, right? Yeah, they should be going away. We can't do it for now because RenderTextControlSingleLine needs to access renderers of these nodes. >> Source/WebCore/html/TextFieldInputType.cpp:151 >> + bool shouldHaveSpinButton = RenderTheme::themeForPage(document->page())->shouldHaveSpinButton(element()); > > This is super-weird. Why create a new theme here, rather than just querying document->page()->theme()? You're right. document->page()->theme() should work. I'll correct this line before landing. >> Source/WebCore/rendering/RenderTextControlSingleLine.cpp:278 >> + innerTextRenderer->layoutIfNeeded(); > > This seems weird too... Are you going to eventually remove all this code? If you don't intend to, I am afraid we are not better off here. This is a part of the height hack in layout() and styleDidChange(), and it's required for now. We should remove it in the future. I'll add a FIXME comment in layout() about the height hack. >> Source/WebCore/rendering/RenderTextControlSingleLine.cpp:312 >> bool RenderTextControlSingleLine::nodeAtPoint(const HitTestRequest& request, HitTestResult& result, const IntPoint& pointInContainer, const IntPoint& accumulatedOffset, HitTestAction hitTestAction) > > nodeAtPoint looks completely unnecessary after shadow DOM conversion. All elements should correctly react to the hit testing. Are you planning to remove this altogether? I think we still need this because we'd like to assume the inner editable text is clicked when <input> border/padding is clicked.

Created attachment 97949 [details] Patch for landing

Attachment 97949 [details] did not pass style-queue: Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast..." exit_code: 1 LayoutTests/platform/chromium/test_expectations.txt:930: Path does not exist. svg/custom/svg-fonts-fallback.xhtml [test/expectations] [2] LayoutTests/platform/chromium/test_expectations.txt:932: Path does not exist. svg/custom/svg-fonts-segmented.xhtml [test/expectations] [2] LayoutTests/platform/chromium/test_expectations.txt:933: Path does not exist. svg/custom/svg-fonts-word-spacing.html [test/expectations] [2] LayoutTests/platform/chromium/test_expectations.txt:937: Path does not exist. svg/text/text-overflow-ellipsis-svgfont.html [test/expectations] [2] Total errors found: 4 in 41 files If any of these errors are false positives, please file a bug against check-webkit-style.

Committed r89407: <http://trac.webkit.org/changeset/89407>

Comment on attachment 97949 [details] Patch for landing View in context: https://bugs.webkit.org/attachment.cgi?id=97949&action=review > LayoutTests/ChangeLog:11 > + Slight position changes for search result buttons and search cancel > + buttons are expected. Does the layout still match the equivalent Cocoa controls? > LayoutTests/ChangeLog:45 > + * fast/css/text-input-with-webkit-border-radius-expected.txt: > + * platform/chromium/test_expectations.txt: > + * platform/mac/fast/css/input-search-padding-expected.png: > + * platform/mac/fast/css/input-search-padding-expected.txt: > + * platform/mac/fast/css/pseudo-cache-stale-expected.txt: > + * platform/mac/fast/forms/box-shadow-override-expected.txt: > + * platform/mac/fast/forms/control-restrict-line-height-expected.txt: > + * platform/mac/fast/forms/input-appearance-height-expected.txt: > + * platform/mac/fast/forms/input-appearance-spinbutton-disabled-readonly-expected.txt: > + * platform/mac/fast/forms/input-appearance-spinbutton-expected.txt: > + * platform/mac/fast/forms/input-appearance-spinbutton-layer-expected.txt: > + * platform/mac/fast/forms/input-appearance-spinbutton-up-expected.txt: > + * platform/mac/fast/forms/input-appearance-spinbutton-visibility-expected.txt: > + * platform/mac/fast/forms/placeholder-position-expected.txt: > + * platform/mac/fast/forms/placeholder-pseudo-style-expected.txt: > + * platform/mac/fast/forms/placeholder-set-value-expected.txt: > + * platform/mac/fast/forms/search-cancel-button-style-sharing-expected.txt: > + * platform/mac/fast/forms/search-display-none-cancel-button-expected.txt: > + * platform/mac/fast/forms/search-placeholder-value-changed-expected.txt: > + * platform/mac/fast/forms/search-rtl-expected.txt: > + * platform/mac/fast/forms/search-styled-expected.txt: > + * platform/mac/fast/forms/search-transformed-expected.txt: > + * platform/mac/fast/forms/search-vertical-alignment-expected.png: > + * platform/mac/fast/forms/search-vertical-alignment-expected.txt: > + * platform/mac/fast/forms/search-zoomed-expected.txt: > + * platform/mac/fast/forms/searchfield-heights-expected.txt: > + * platform/mac/fast/repaint/search-field-cancel-expected.png: > + * platform/mac/fast/repaint/search-field-cancel-expected.txt: > + * platform/mac/fast/replaced/width100percent-searchfield-expected.txt: > + * platform/mac/fast/table/colspanMinWidth-vertical-expected.png: > + * platform/mac/fast/table/colspanMinWidth-vertical-expected.txt: > + * fast/forms/search-cancel-button-mouseup.html: Adjust click position for the cancel button. > + * fast/forms/search-abs-pos-cancel-button.html: ditto. Do none of these need new pixel results?

This caused bug 63157. This change was rolled out in r89460 for other reasons.

Reopened because the patch was rolled out.

(In reply to comment #19) > (From update of attachment 97949 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=97949&action=review > > > LayoutTests/ChangeLog:11 > > + Slight position changes for search result buttons and search cancel > > + buttons are expected. > > Does the layout still match the equivalent Cocoa controls? > > > LayoutTests/ChangeLog:45 > > + * fast/css/text-input-with-webkit-border-radius-expected.txt: > > + * platform/chromium/test_expectations.txt: > > + * platform/mac/fast/css/input-search-padding-expected.png: > > + * platform/mac/fast/css/input-search-padding-expected.txt: > > + * platform/mac/fast/css/pseudo-cache-stale-expected.txt: > > + * platform/mac/fast/forms/box-shadow-override-expected.txt: > > + * platform/mac/fast/forms/control-restrict-line-height-expected.txt: > > + * platform/mac/fast/forms/input-appearance-height-expected.txt: > > + * platform/mac/fast/forms/input-appearance-spinbutton-disabled-readonly-expected.txt: > > + * platform/mac/fast/forms/input-appearance-spinbutton-expected.txt: > > + * platform/mac/fast/forms/input-appearance-spinbutton-layer-expected.txt: > > + * platform/mac/fast/forms/input-appearance-spinbutton-up-expected.txt: > > + * platform/mac/fast/forms/input-appearance-spinbutton-visibility-expected.txt: > > + * platform/mac/fast/forms/placeholder-position-expected.txt: > > + * platform/mac/fast/forms/placeholder-pseudo-style-expected.txt: > > + * platform/mac/fast/forms/placeholder-set-value-expected.txt: > > + * platform/mac/fast/forms/search-cancel-button-style-sharing-expected.txt: > > + * platform/mac/fast/forms/search-display-none-cancel-button-expected.txt: > > + * platform/mac/fast/forms/search-placeholder-value-changed-expected.txt: > > + * platform/mac/fast/forms/search-rtl-expected.txt: > > + * platform/mac/fast/forms/search-styled-expected.txt: > > + * platform/mac/fast/forms/search-transformed-expected.txt: > > + * platform/mac/fast/forms/search-vertical-alignment-expected.png: > > + * platform/mac/fast/forms/search-vertical-alignment-expected.txt: > > + * platform/mac/fast/forms/search-zoomed-expected.txt: > > + * platform/mac/fast/forms/searchfield-heights-expected.txt: > > + * platform/mac/fast/repaint/search-field-cancel-expected.png: > > + * platform/mac/fast/repaint/search-field-cancel-expected.txt: > > + * platform/mac/fast/replaced/width100percent-searchfield-expected.txt: > > + * platform/mac/fast/table/colspanMinWidth-vertical-expected.png: > > + * platform/mac/fast/table/colspanMinWidth-vertical-expected.txt: > > + * fast/forms/search-cancel-button-mouseup.html: Adjust click position for the cancel button. > > + * fast/forms/search-abs-pos-cancel-button.html: ditto. > > Do none of these need new pixel results? I confirmed some tests passed with --pixel --tolerance 0. So there are no pixel differences in Mac. (Some other tests didn't pass with --tolerance 0 even without the patch.)

Created attachment 99053 [details] Patch for landing 2

(In reply to comment #23) > Created an attachment (id=99053) [details] > Patch for landing 2 The differences from r89407 are: * Removed innerTextRenderer->layoutIfNeeded() It caused Bug 63182. It was needed for a regression by r87067, so we don't need to fix it in this patch. * Fixed Bug 63157 in RendetTextControlSingleLine::layout() We need to put the content on the outside of the content rect of the <input> if an intrinsic height of the content is taller than contentHeight() of the <input> and smaller than height() of the <input>. I'm going to land this tomorrow.

Committed r90089: <http://trac.webkit.org/changeset/90089>