Bug 93735 – Positioned Replaced Elements That Aren't RenderReplaced get Incorrect Width

• Component: CSS
• Status: REOPENED

[Robert Hogan]

.

[Robert Hogan]

Created attachment 157784 [details]
Test Case

[Pravin D]

Created attachment 159030 [details]
TestCase

There was a extra script tag in the previous testcase which was causing the page not to render anything on the view port.

[Robert Hogan]

Created attachment 202459 [details]
Reduction - Other browsers do not stretch the input element to fill the viewport

[Robert Hogan]

Created attachment 203004 [details]
Patch

[Robert Hogan]

Created attachment 203006 [details]
Patch

[kov's GTK+ EWS bot]

Comment on attachment 203006 [details]
Patch

Attachment 203006 [details] did not pass gtk-ews (gtk):
Output: http://webkit-queues.appspot.com/results/735126

[EFL EWS Bot]

Comment on attachment 203006 [details]
Patch

Attachment 203006 [details] did not pass efl-wk2-ews (efl-wk2):
Output: http://webkit-queues.appspot.com/results/685337

[Build Bot]

Comment on attachment 203006 [details]
Patch

Attachment 203006 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/655421

[Build Bot]

Comment on attachment 203006 [details]
Patch

Attachment 203006 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/704277

[Build Bot]

Comment on attachment 203006 [details]
Patch

Attachment 203006 [details] did not pass win-ews (win):
Output: http://webkit-queues.appspot.com/results/737193

[Robert Hogan]

Created attachment 203009 [details]
Patch

[Robert Hogan]

Created attachment 203480 [details]
Patch

[Build Bot]

Comment on attachment 203480 [details]
Patch

Attachment 203480 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/679301

New failing tests:
media/media-fullscreen-inline.html
http/tests/security/mixedContent/redirect-https-to-http-iframe-in-main-frame.html
fullscreen/full-screen-iframe-without-allow-attribute-allowed-from-parent.html
fullscreen/empty-anonymous-block-continuation-crash.html
http/tests/fullscreen/fullscreenelement-same-origin.html
plugins/fullscreen-plugins-dont-reload.html
fullscreen/full-screen-css.html
fullscreen/full-screen-fixed-pos-parent.html
fullscreen/full-screen-keyboard-disabled.html
media/video-controls-visible-exiting-fullscreen.html
media/video-controls-fullscreen-volume.html
http/tests/inspector/inspect-element.html
editing/pasteboard/copy-image-with-alt-text.html
fast/regions/full-screen-video-from-region.html
editing/pasteboard/smart-paste-001.html
fullscreen/full-screen-element-stack.html
fullscreen/anonymous-block-merge-crash.html
fast/forms/onselect-textfield.html
fullscreen/full-screen-inline-split-crash.html
fullscreen/full-screen-iframe-with-mixed-allow-webkitallow-attribute.html
fast/forms/option-mouseevents.html
fullscreen/full-screen-crash-offsetLeft.html
fast/forms/button-positioned.html
http/tests/fullscreen/fullscreenelement-different-origin.html
fullscreen/full-screen-cancel.html
fast/forms/input-appearance-preventDefault.html
fast/replaced/width-and-height-of-positioned-replaced-elements.html

[Build Bot]

Created attachment 203511 [details]
Archive of layout-test-results from webkit-ews-11 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-11  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.3

[Robert Hogan]

Created attachment 203537 [details]
Patch

[Build Bot]

Comment on attachment 203537 [details]
Patch

Attachment 203537 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/706691

New failing tests:
fast/forms/button-positioned.html
fast/forms/option-mouseevents.html
fast/forms/input-appearance-preventDefault.html
fast/forms/onselect-textfield.html
fast/replaced/width-and-height-of-positioned-replaced-elements.html

[Build Bot]

Created attachment 203540 [details]
Archive of layout-test-results from webkit-ews-11 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-11  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.3

[Build Bot]

Comment on attachment 203537 [details]
Patch

Attachment 203537 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/691307

New failing tests:
fast/forms/button-positioned.html
fast/forms/option-mouseevents.html
fast/forms/input-appearance-preventDefault.html
fast/forms/onselect-textfield.html
fast/replaced/width-and-height-of-positioned-replaced-elements.html

[Build Bot]

Created attachment 203542 [details]
Archive of layout-test-results from webkit-ews-08 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-08  Port: mac-mountainlion  Platform: Mac OS X 10.8.3

[Dave Hyatt]

Comment on attachment 203537 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=203537&action=review

I would use intrinsicSize() subclassing more. You might want to investigate how intrinsicSize() is called. It may be worthwhile to turn it into intrinsicLogicalSize() to simplify subclassing, especially if nobody is really using the physical version of it.

> Source/WebCore/rendering/RenderBox.cpp:2447
> +    // Some replaced renderers, such as text controls, need to calculate their 'intrinsic' height during layout.
> +    updateIntrinsicLogicalHeight();

I don't get why you had to add this. Can't you just do this inside RenderTextControl's computeLogicalHeight?

> Source/WebCore/rendering/RenderBox.cpp:3052
> +static bool isReplacedElement(const RenderBox* child)
> +{
> +    return child->isReplaced() || (child->node() && child->node()->isElementNode() && toElement(child->node())->isFormControlElement() && !child->isFieldset());
> +}

Let's add a FIXME about how we'd like this to just be isReplaced() eventually and file a followup bug about it too.

> Source/WebCore/rendering/RenderBox.cpp:3056
> -    if (isReplaced()) {
> +    if (isReplacedElement(this)) {

I think it's worth a comment about why we're not just using isReplaced.

> Source/WebCore/rendering/RenderBox.cpp:3398
> -    if (isReplaced()) {
> +    if (isReplacedElement(this)) {

Ditto.

> Source/WebCore/rendering/RenderBox.h:405
> +    virtual LayoutUnit intrinsicLogicalWidth() const { return style()->isHorizontalWritingMode() ? intrinsicSize().width() : intrinsicSize().height(); }
> +    virtual LayoutUnit intrinsicLogicalHeight() const { return style()->isHorizontalWritingMode() ? intrinsicSize().height() : intrinsicSize().width(); }

I don't get why these have to become virtual. Can't you just override the already virtual intrinsicSize()? When someone queries for the height on objects that only subclassed to provide a width, you now have two virtual function calls (one to call intrinsicLogicalHeight(), and then a second virtual function call to intrinsicSize()(.

> Source/WebCore/rendering/RenderBox.h:406
> +    virtual void updateIntrinsicLogicalHeight() { };

Don't get why this is needed.

> Source/WebCore/rendering/RenderButton.h:61
> +protected:
> +    virtual LayoutUnit intrinsicLogicalWidth() const { return maxPreferredLogicalWidth(); }

Just subclass intrinsicSize() instead.

> Source/WebCore/rendering/RenderMenuList.h:61
> +protected:
> +    virtual LayoutUnit intrinsicLogicalWidth() const { return maxPreferredLogicalWidth(); }

Just subclass intrinsicSize() instead.

> Source/WebCore/rendering/RenderTextControl.cpp:146
> -void RenderTextControl::computeLogicalHeight(LayoutUnit logicalHeight, LayoutUnit logicalTop, LogicalExtentComputedValues& computedValues) const
> +void RenderTextControl::updateIntrinsicLogicalHeight()

Seems like you could have just done the update in computeLogicalHeight without an extra function?

> Source/WebCore/rendering/RenderTextControl.h:69
> +    virtual void updateIntrinsicLogicalHeight() OVERRIDE;
> +    virtual LayoutUnit intrinsicLogicalHeight() const { return m_intrinsicLogicalHeight; }
> +    virtual LayoutUnit intrinsicLogicalWidth() const { return maxPreferredLogicalWidth(); }

Definitely could just subclass intrinsicSize() here.

[Robert Hogan]

Created attachment 203884 [details]
Patch

[Darin Adler]

Comment on attachment 203884 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=203884&action=review

> Source/WebCore/rendering/RenderBox.cpp:3050
> +    return child->isReplaced() || (toElement(child->node())->isFormControlElement() && !child->isFieldset());

What guarantees that child->node() is an Element?

Can’t we do this check entirely on the rendering side of things?

> Source/WebCore/rendering/RenderButton.h:60
> +    virtual LayoutSize intrinsicSize() const { return LayoutSize(maxPreferredLogicalWidth(), LayoutUnit()); }

Can this be private instead of public? Can this use OVERRIDE and FINAL?

> Source/WebCore/rendering/RenderListBox.h:60
> +    virtual LayoutSize intrinsicSize() const { return LayoutSize(maxPreferredLogicalWidth(), m_intrinsicLogicalHeight); }

Can this be private instead of public? Can this use OVERRIDE and FINAL?

> Source/WebCore/rendering/RenderMenuList.h:60
> +    virtual LayoutSize intrinsicSize() const { return LayoutSize(maxPreferredLogicalWidth(), logicalHeight()); }

Can this be private instead of public? Can this use OVERRIDE and FINAL?

> Source/WebCore/rendering/RenderTextControl.h:67
> +    virtual LayoutSize intrinsicSize() const { return LayoutSize(maxPreferredLogicalWidth(), m_intrinsicLogicalHeight); }

Can this be private instead of public? Can this use OVERRIDE and FINAL?

[Robert Hogan]

Created attachment 204020 [details]
Patch

[Build Bot]

Comment on attachment 204020 [details]
Patch

Attachment 204020 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/794033

New failing tests:
fast/replaced/width-and-height-of-positioned-replaced-elements.html

[Build Bot]

Created attachment 204028 [details]
Archive of layout-test-results from webkit-ews-04 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-04  Port: mac-mountainlion  Platform: Mac OS X 10.8.3

[Build Bot]

Comment on attachment 204020 [details]
Patch

Attachment 204020 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/734443

New failing tests:
fast/replaced/width-and-height-of-positioned-replaced-elements.html

[Build Bot]

Created attachment 204091 [details]
Archive of layout-test-results from webkit-ews-12 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-12  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.3

[Robert Hogan]

Committed r151451: <http://trac.webkit.org/changeset/151451>

[Beth Dakin]

This change caused a regression on the iCloud.com login page. https://bugs.webkit.org/show_bug.cgi?id=117744

[Simon Fraser (smfr)]

This also broke buttons at http://mint.com (misplaced labels in the buttons in top right corner).

[WebKit Commit Bot]

Re-opened since this is blocked by bug 117848

[Robert Hogan]

Created attachment 205412 [details]
Patch

[Build Bot]

Comment on attachment 205412 [details]
Patch

Attachment 205412 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/978644

New failing tests:
fast/replaced/width-and-height-of-positioned-replaced-elements.html

[Build Bot]

Created attachment 205418 [details]
Archive of layout-test-results from webkit-ews-15 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-15  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.3

[Build Bot]

Comment on attachment 205412 [details]
Patch

Attachment 205412 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/917076

New failing tests:
fast/replaced/width-and-height-of-positioned-replaced-elements.html

[Build Bot]

Created attachment 205422 [details]
Archive of layout-test-results from webkit-ews-06 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-06  Port: mac-mountainlion  Platform: Mac OS X 10.8.3

[Build Bot]

Comment on attachment 205412 [details]
Patch

Attachment 205412 [details] did not pass win-ews (win):
Output: http://webkit-queues.appspot.com/results/848547

New failing tests:
fast/replaced/width-and-height-of-positioned-replaced-elements.html

[Build Bot]

Created attachment 205433 [details]
Archive of layout-test-results from APPLE-EWS-2 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: APPLE-EWS-2  Port: win-future  Platform: CYGWIN_NT-6.1-WOW64-1.7.20-0.266-5-3-i686-32bit

[Robert Hogan]

Created attachment 205827 [details]
Patch

[Build Bot]

Comment on attachment 205827 [details]
Patch

Attachment 205827 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/1012389

New failing tests:
fast/replaced/width-and-height-of-positioned-replaced-elements.html

[Build Bot]

Created attachment 205830 [details]
Archive of layout-test-results from webkit-ews-04 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-04  Port: mac-mountainlion  Platform: Mac OS X 10.8.3

[Build Bot]

Comment on attachment 205827 [details]
Patch

Attachment 205827 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-queues.appspot.com/results/1019033

New failing tests:
fast/replaced/width-and-height-of-positioned-replaced-elements.html

[Build Bot]

Created attachment 205875 [details]
Archive of layout-test-results from webkit-ews-12 for mac-mountainlion-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: webkit-ews-12  Port: mac-mountainlion-wk2  Platform: Mac OS X 10.8.3

[Build Bot]

Comment on attachment 205827 [details]
Patch

Attachment 205827 [details] did not pass win-ews (win):
Output: http://webkit-queues.appspot.com/results/1016453

New failing tests:
fast/replaced/width-and-height-of-positioned-replaced-elements.html

[Build Bot]

Created attachment 205949 [details]
Archive of layout-test-results from APPLE-EWS-5 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: APPLE-EWS-5  Port: win-future  Platform: CYGWIN_NT-6.1-WOW64-1.7.20-0.266-5-3-i686-32bit

[Robert Hogan]

Created attachment 209668 [details]
Patch

[Dave Hyatt]

Comment on attachment 209668 [details]
Patch

r=me

[WebKit Commit Bot]

Comment on attachment 209668 [details]
Patch

Clearing flags on attachment: 209668

Committed r154826: <http://trac.webkit.org/changeset/154826>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Tim Horton]

This change *once again* caused vertical centering issues on the iCloud.com login page (https://bugs.webkit.org/show_bug.cgi?id=117744).

[Robert Hogan]

(In reply to comment #50)
> This change *once again* caused vertical centering issues on the iCloud.com login page (https://bugs.webkit.org/show_bug.cgi?id=117744).

Thanks for checking - I'll roll it out.

[WebKit Commit Bot]

Re-opened since this is blocked by bug 120517

[Ahmad Saleem]

I am still able to reproduce this bug in Safari 15.5 on macOS 12.4 and Safari TP 148 using "Reduction..." test case.

The input field is stretched out while in other browser, Chrome Canary 105 is also stretching it but Firefox is not. Firefox is just showing it is as normal 184*16 computed size.

If I am testing it incorrectly, please retest accordingly and ignore my comment. Thanks!