Bug 61415 – Avoid custom layout code of RenderTextControlSingleLine

• Status: RESOLVED
• Resolution: FIXED
• Component: Layout and Rendering

[Kent Tamura]

WARNING: This will need drastic rebaseline.  I'll change rendering tree structure.

[Kent Tamura]

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.

[Kent Tamura]

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!!)

[Kent Tamura]

Created attachment 95883 [details]
WIP

[Kent Tamura]

Created attachment 97553 [details]
Patch

[WebKit Review Bot]

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.

[Kent Tamura]

Created attachment 97555 [details]
Patch 2

Correct test_expectations.txt

[Kent Tamura]

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.

[WebKit Review Bot]

Comment on attachment 97555 [details]
Patch 2

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

[Kent Tamura]

Created attachment 97557 [details]
Patch 3

Fix a build warning

[WebKit Review Bot]

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

[WebKit Review Bot]

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

[Kent Tamura]

Created attachment 97569 [details]
Patch 4

Try to fix Chromium-linux test failures

[Kent Tamura]

Created attachment 97570 [details]
Patch 5

Try to fix Chromium-linux test failures

[Dimitri Glazkov (Google)]

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! :)

[Kent Tamura]

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.

[Kent Tamura]

Created attachment 97949 [details]
Patch for landing

[WebKit Review Bot]

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.

[Kent Tamura]

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

[Adam Roben (:aroben)]

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?

[Adam Roben (:aroben)]

This caused bug 63157.

This change was rolled out in r89460 for other reasons.

[Kent Tamura]

Reopened because the patch was rolled out.

[Kent Tamura]

(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.)

[Kent Tamura]

Created attachment 99053 [details]
Patch for landing 2

[Kent Tamura]

(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.

[Kent Tamura]

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