Bug 61415

Summary: Avoid custom layout code of RenderTextControlSingleLine
Product: WebKit Reporter: Kent Tamura <tkent>
Component: Layout and RenderingAssignee: Kent Tamura <tkent>
Status: RESOLVED FIXED    
Severity: Normal CC: aroben, dglazkov, hyatt, mitz, morrita, simon.fraser, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Bug Depends on: 61845, 63157, 63168, 63182, 63511    
Bug Blocks: 62096, 53961    
Attachments:
Description Flags
WIP
none
Patch
none
Patch 2
none
Patch 3
none
Archive of layout-test-results from ec2-cr-linux-03
none
Patch 4
none
Patch 5
none
Patch for landing
none
Patch for landing 2 none

Description Kent Tamura 2011-05-24 20:55:56 PDT
WARNING: This will need drastic rebaseline.  I'll change rendering tree structure.
Comment 1 Kent Tamura 2011-05-25 03:43:50 PDT
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.
Comment 2 Kent Tamura 2011-05-31 02:16:39 PDT
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!!)
Comment 3 Kent Tamura 2011-06-03 03:57:27 PDT
Created attachment 95883 [details]
WIP
Comment 4 Kent Tamura 2011-06-17 00:39:56 PDT
Created attachment 97553 [details]
Patch
Comment 5 WebKit Review Bot 2011-06-17 00:41:57 PDT
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.
Comment 6 Kent Tamura 2011-06-17 00:45:59 PDT
Created attachment 97555 [details]
Patch 2

Correct test_expectations.txt
Comment 7 Kent Tamura 2011-06-17 00:51:00 PDT
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 8 WebKit Review Bot 2011-06-17 00:51:52 PDT
Comment on attachment 97555 [details]
Patch 2

Attachment 97555 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/8878441
Comment 9 Kent Tamura 2011-06-17 00:54:08 PDT
Created attachment 97557 [details]
Patch 3

Fix a build warning
Comment 10 WebKit Review Bot 2011-06-17 02:15:04 PDT
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
Comment 11 WebKit Review Bot 2011-06-17 02:15:08 PDT
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
Comment 12 Kent Tamura 2011-06-17 03:18:57 PDT
Created attachment 97569 [details]
Patch 4

Try to fix Chromium-linux test failures
Comment 13 Kent Tamura 2011-06-17 03:21:44 PDT
Created attachment 97570 [details]
Patch 5

Try to fix Chromium-linux test failures
Comment 14 Dimitri Glazkov (Google) 2011-06-17 08:50:16 PDT
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 15 Kent Tamura 2011-06-20 20:07:22 PDT
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.
Comment 16 Kent Tamura 2011-06-21 01:30:28 PDT
Created attachment 97949 [details]
Patch for landing
Comment 17 WebKit Review Bot 2011-06-21 01:33:36 PDT
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.
Comment 18 Kent Tamura 2011-06-21 21:36:45 PDT
Committed r89407: <http://trac.webkit.org/changeset/89407>
Comment 19 Adam Roben (:aroben) 2011-06-22 10:57:12 PDT
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?
Comment 20 Adam Roben (:aroben) 2011-06-22 12:58:57 PDT
This caused bug 63157.

This change was rolled out in r89460 for other reasons.
Comment 21 Kent Tamura 2011-06-23 23:35:05 PDT
Reopened because the patch was rolled out.
Comment 22 Kent Tamura 2011-06-24 05:40:28 PDT
(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.)
Comment 23 Kent Tamura 2011-06-29 01:05:32 PDT
Created attachment 99053 [details]
Patch for landing 2
Comment 24 Kent Tamura 2011-06-29 01:14:50 PDT
(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.
Comment 25 Kent Tamura 2011-06-29 23:30:38 PDT
Committed r90089: <http://trac.webkit.org/changeset/90089>