Bug 197814

Summary:

Computed width of non-replaced inline incorrectly returns "auto" instead of the computed value.

Product:

WebKit

Reporter:

Emilio Cobos Álvarez (:emilio) <emilio>

Component:

CSS

Assignee:

Joonghun Park <jh718.park>

Status:

RESOLVED FIXED

Severity:

Normal

CC:

commit-queue, darin, dbates, ews-watchlist, gyuyoung.kim, jh718.park, koivisto, rniwa, simon.fraser, webkit-bug-importer, zalan

Priority:

Keywords:

InRadar

Version:

WebKit Nightly Build

Hardware:

Unspecified

OS:

Unspecified

Attachments:

Description	Flags
To check ews result	none
Archive of layout-test-results from ews102 for mac-highsierra	none
Archive of layout-test-results from ews106 for mac-highsierra-wk2	none
Archive of layout-test-results from ews210 for win-future	none
Archive of layout-test-results from ews116 for mac-highsierra	none
Archive of layout-test-results from ews123 for ios-simulator-wk2	none
Update LayoutTest	none
Patch	none
Patch	none
Archive of layout-test-results from ews101 for mac-highsierra	none
Patch	none
Patch	none
Update legacy-animation-engine layout test	none
Patch	none
Patch	none

Emilio Cobos Álvarez (:emilio)

Reported 2019-05-11 06:46:22 PDT

<!doctype html> <span id="inline" style="width: 10%">A</span> <pre> <script> document.writeln(getComputedStyle(inline).width); </script> </pre> All engines but WebKit show "10%" instead of "auto".

Attachments
To check ews result (5.37 KB, patch) 2019-05-18 09:12 PDT, Joonghun Park	no flags	Details Formatted Diff Diff
Archive of layout-test-results from ews102 for mac-highsierra (3.11 MB, application/zip) 2019-05-18 10:16 PDT, EWS Watchlist	no flags	Details
Archive of layout-test-results from ews106 for mac-highsierra-wk2 (2.64 MB, application/zip) 2019-05-18 10:31 PDT, EWS Watchlist	no flags	Details
Archive of layout-test-results from ews210 for win-future (13.68 MB, application/zip) 2019-05-18 11:00 PDT, EWS Watchlist	no flags	Details
Archive of layout-test-results from ews116 for mac-highsierra (2.99 MB, application/zip) 2019-05-18 11:02 PDT, EWS Watchlist	no flags	Details
Archive of layout-test-results from ews123 for ios-simulator-wk2 (8.35 MB, application/zip) 2019-05-18 11:06 PDT, EWS Watchlist	no flags	Details
Update LayoutTest (13.71 KB, patch) 2019-05-18 16:56 PDT, Joonghun Park	no flags	Details Formatted Diff Diff
Patch (13.15 KB, patch) 2019-05-18 17:45 PDT, Joonghun Park	no flags	Details Formatted Diff Diff
Patch (13.71 KB, patch) 2019-05-18 17:52 PDT, Joonghun Park	no flags	Details Formatted Diff Diff
Archive of layout-test-results from ews101 for mac-highsierra (4.16 MB, application/zip) 2019-05-18 18:50 PDT, EWS Watchlist	no flags	Details
Patch (13.15 KB, patch) 2019-05-18 19:01 PDT, Joonghun Park	no flags	Details Formatted Diff Diff
Patch (13.71 KB, patch) 2019-05-18 19:16 PDT, Joonghun Park	no flags	Details Formatted Diff Diff
Update legacy-animation-engine layout test (18.99 KB, patch) 2019-05-18 20:06 PDT, Joonghun Park	no flags	Details Formatted Diff Diff
Patch (18.43 KB, patch) 2019-05-19 06:22 PDT, Joonghun Park	no flags	Details Formatted Diff Diff
Patch (18.45 KB, patch) 2019-05-24 13:46 PDT, Joonghun Park	no flags	Details Formatted Diff Diff
Show Obsolete (8) View All Add attachment proposed patch, testcase, etc.

Joonghun Park

Comment 1 2019-05-18 09:12:56 PDT

Created attachment 370191 [details] To check ews result

EWS Watchlist

Comment 2 2019-05-18 10:16:43 PDT

Comment on attachment 370191 [details] To check ews result Attachment 370191 [details] did not pass mac-ews (mac): Output: https://webkit-queues.webkit.org/results/12225010 New failing tests: legacy-animation-engine/fast/css/getComputedStyle/getComputedStyle-with-pseudo-element.html fast/css/getComputedStyle/getComputedStyle-resolved-values.html fast/css/getComputedStyle/getComputedStyle-with-pseudo-element.html

EWS Watchlist

Comment 3 2019-05-18 10:16:44 PDT

Created attachment 370192 [details] Archive of layout-test-results from ews102 for mac-highsierra The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews102 Port: mac-highsierra Platform: Mac OS X 10.13.6

EWS Watchlist

Comment 4 2019-05-18 10:31:18 PDT

Comment on attachment 370191 [details] To check ews result Attachment 370191 [details] did not pass mac-wk2-ews (mac-wk2): Output: https://webkit-queues.webkit.org/results/12225030 New failing tests: legacy-animation-engine/fast/css/getComputedStyle/getComputedStyle-with-pseudo-element.html fast/css/getComputedStyle/getComputedStyle-resolved-values.html fast/css/getComputedStyle/getComputedStyle-with-pseudo-element.html

EWS Watchlist

Comment 5 2019-05-18 10:31:20 PDT

Created attachment 370193 [details] Archive of layout-test-results from ews106 for mac-highsierra-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews106 Port: mac-highsierra-wk2 Platform: Mac OS X 10.13.6

EWS Watchlist

Comment 6 2019-05-18 11:00:25 PDT

Comment on attachment 370191 [details] To check ews result Attachment 370191 [details] did not pass win-ews (win): Output: https://webkit-queues.webkit.org/results/12225104 New failing tests: fast/css/getComputedStyle/getComputedStyle-resolved-values.html fast/css/getComputedStyle/getComputedStyle-with-pseudo-element.html

EWS Watchlist

Comment 7 2019-05-18 11:00:28 PDT

Created attachment 370194 [details] Archive of layout-test-results from ews210 for win-future The attached test failures were seen while running run-webkit-tests on the win-ews. Bot: ews210 Port: win-future Platform: CYGWIN_NT-10.0-17763-3.0.5-338.x86_64-x86_64-64bit

EWS Watchlist

Comment 8 2019-05-18 11:02:25 PDT

Comment on attachment 370191 [details] To check ews result Attachment 370191 [details] did not pass mac-debug-ews (mac): Output: https://webkit-queues.webkit.org/results/12225059 New failing tests: legacy-animation-engine/fast/css/getComputedStyle/getComputedStyle-with-pseudo-element.html fast/css/getComputedStyle/getComputedStyle-resolved-values.html fast/css/getComputedStyle/getComputedStyle-with-pseudo-element.html

EWS Watchlist

Comment 9 2019-05-18 11:02:27 PDT

Created attachment 370195 [details] Archive of layout-test-results from ews116 for mac-highsierra The attached test failures were seen while running run-webkit-tests on the mac-debug-ews. Bot: ews116 Port: mac-highsierra Platform: Mac OS X 10.13.6

EWS Watchlist

Comment 10 2019-05-18 11:06:49 PDT

Comment on attachment 370191 [details] To check ews result Attachment 370191 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: https://webkit-queues.webkit.org/results/12225073 New failing tests: legacy-animation-engine/fast/css/getComputedStyle/getComputedStyle-with-pseudo-element.html fast/css/getComputedStyle/getComputedStyle-resolved-values.html fast/css/getComputedStyle/getComputedStyle-with-pseudo-element.html

EWS Watchlist

Comment 11 2019-05-18 11:06:51 PDT

Created attachment 370196 [details] Archive of layout-test-results from ews123 for ios-simulator-wk2 The attached test failures were seen while running run-webkit-tests on the ios-sim-ews. Bot: ews123 Port: ios-simulator-wk2 Platform: Mac OS X 10.14.5

Joonghun Park

Comment 12 2019-05-18 16:56:14 PDT

Created attachment 370205 [details] Update LayoutTest

Joonghun Park

Comment 13 2019-05-18 17:45:31 PDT

Created attachment 370210 [details] Patch

Joonghun Park

Comment 14 2019-05-18 17:52:29 PDT

Created attachment 370211 [details] Patch

EWS Watchlist

Comment 15 2019-05-18 18:50:19 PDT

Comment on attachment 370211 [details] Patch Attachment 370211 [details] did not pass mac-ews (mac): Output: https://webkit-queues.webkit.org/results/12227288 New failing tests: fast/media/mq-any-hover-styling.html fast/text/crash-font-family-parsed.html fast/css-generated-content/pseudo-animation.html fast/css/getComputedStyle/getComputedStyle-length-unit.html imported/w3c/web-platform-tests/css/cssom/getComputedStyle-pseudo.html legacy-animation-engine/fast/css/getComputedStyle/getComputedStyle-with-pseudo-element.html fast/css/getComputedStyle/zoom-on-display-none.html inspector/css/pseudo-element-matches.html fast/dom/length-attribute-mapping.html fast/css/computed-width-without-renderer.html imported/w3c/web-platform-tests/infrastructure/assumptions/html-elements.html fast/css/getComputedStyle/getComputedStyle-zoom-and-background-size.html fast/media/mq-any-pointer-styling.html fast/media/mq-hover-styling.html fast/css/getComputedStyle/getComputedStyle-resolved-values.html fast/css/getComputedStyle/getComputedStyle-with-pseudo-element.html fast/history/visited-generated-content-test.html fast/css/getComputedStyle/pending-stylesheet.html fast/media/mq-pointer-styling.html fast/css/getComputedStyle/computed-style-without-renderer.html media/modern-media-controls/tracks-support/tracks-support-captions-offset-with-controls-bar.html jquery/css.html

EWS Watchlist

Comment 16 2019-05-18 18:50:21 PDT

Created attachment 370212 [details] Archive of layout-test-results from ews101 for mac-highsierra The attached test failures were seen while running run-webkit-tests on the mac-ews. Bot: ews101 Port: mac-highsierra Platform: Mac OS X 10.13.6

Joonghun Park

Comment 17 2019-05-18 19:01:17 PDT

Created attachment 370213 [details] Patch

Joonghun Park

Comment 18 2019-05-18 19:16:33 PDT

Created attachment 370214 [details] Patch

Joonghun Park

Comment 19 2019-05-18 20:06:15 PDT

Created attachment 370217 [details] Update legacy-animation-engine layout test

Joonghun Park

Comment 20 2019-05-18 21:56:43 PDT

Could you please review this change?

Emilio Cobos Álvarez (:emilio)

Comment 21 2019-05-19 05:21:26 PDT

Comment on attachment 370217 [details] Update legacy-animation-engine layout test View in context: https://bugs.webkit.org/attachment.cgi?id=370217&action=review > Source/WebCore/css/CSSComputedStyleDeclaration.cpp:3190 > + if ((renderer && style.display() != DisplayType::Contents) && !renderer->isRenderSVGModelObject()) { How can you end up with a box but be display: contents? That check seems wrong / unnecessary. Probably just: if (renderer && !isNonReplacedInline(*renderer) && !renderer->isRenderSVGModelObject()) return zoomAdjustedPixelValue(sizingBox(*renderer).height(), style); return zoomAdjustedPixelValueForLength(style.height(), style);

Emilio Cobos Álvarez (:emilio)

Comment 22 2019-05-19 05:22:03 PDT

(disclaimer: I'm not a WebKit reviewer)

Joonghun Park

Comment 23 2019-05-19 06:04:39 PDT

(In reply to Emilio Cobos Álvarez (:emilio) from comment #21) > Comment on attachment 370217 [details] > Update legacy-animation-engine layout test > > View in context: > https://bugs.webkit.org/attachment.cgi?id=370217&action=review > > > Source/WebCore/css/CSSComputedStyleDeclaration.cpp:3190 > > + if ((renderer && style.display() != DisplayType::Contents) && !renderer->isRenderSVGModelObject()) { > > How can you end up with a box but be display: contents? That check seems > wrong / unnecessary. Probably just: > > if (renderer && !isNonReplacedInline(*renderer) && > !renderer->isRenderSVGModelObject()) > return zoomAdjustedPixelValue(sizingBox(*renderer).height(), style); > return zoomAdjustedPixelValueForLength(style.height(), style); That's right. The element with display: contents should disappear so there will be no such render object, means that the check is unnecessary/wrong. As you suggested I will remove the display:content condition check. Thank you for your review, Emilio:)

Joonghun Park

Comment 24 2019-05-19 06:22:04 PDT

Created attachment 370221 [details] Patch

Daniel Bates

Comment 25 2019-05-19 14:32:12 PDT

Comment on attachment 370221 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=370221&action=review > Source/WebCore/css/CSSComputedStyleDeclaration.cpp:3190 > if (renderer && !renderer->isRenderSVGModelObject()) { ^^^ not part of your patch, but just realized this function name is misspelled. "in" should have capital I. Does not need to fixed now. > Source/WebCore/css/CSSComputedStyleDeclaration.cpp:3191 > // According to http://www.w3.org/TR/CSS2/visudet.html#the-height-property, Is this comment still applicable then. > Source/WebCore/css/CSSComputedStyleDeclaration.cpp:3192 > // the "height" property does not apply for non-replaced inline elements. ? Also, is the code change correct for when a non-replaced inline has an explicit height: auto? Anyway, not a css expert, yet. > Source/WebCore/css/CSSComputedStyleDeclaration.cpp:3513 > // According to http://www.w3.org/TR/CSS2/visudet.html#the-width-property, Is comment still applicable?

Daniel Bates

Comment 26 2019-05-19 14:33:30 PDT

Comment on attachment 370221 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=370221&action=review > Source/WebCore/ChangeLog:13 > + 'If the property applies to the element or pseudo-element This seems to contradict the spec undefined nature for width and height of of non-replaced inline elements in CSS 2.2

Joonghun Park

Comment 27 2019-05-19 17:28:06 PDT

Comment on attachment 370221 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=370221&action=review >> Source/WebCore/ChangeLog:13 >> + 'If the property applies to the element or pseudo-element > > This seems to contradict the spec undefined nature for width and height of of non-replaced inline elements in CSS 2.2 This spec(https://drafts.csswg.org/cssom/#resolved-value) section's statement is for many properties e.g. block-size/height/inline-size/margin-block-end/... So, widht/height is belong to "Otherwise" case in below. >> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:3190 >> if (renderer && !renderer->isRenderSVGModelObject()) { > > ^^^ not part of your patch, but just realized this function name is misspelled. "in" should have capital I. Does not need to fixed now. It is just typo, so I will fix it with an separate unreviewed patch. >> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:3191 >> // According to http://www.w3.org/TR/CSS2/visudet.html#the-height-property, > > Is this comment still applicable then. When I see even the draft version of this spec(https://drafts.csswg.org/css-sizing-3/#preferred-size-properties), it's still applicable. It says about width/height, "Applies to: all elements except non-replaced inlines" >> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:3513 >> // According to http://www.w3.org/TR/CSS2/visudet.html#the-width-property, > > Is comment still applicable? ditto.

Joonghun Park

Comment 28 2019-05-19 19:51:55 PDT

Comment on attachment 370221 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=370221&action=review >> Source/WebCore/css/CSSComputedStyleDeclaration.cpp:3192 >> // the "height" property does not apply for non-replaced inline elements. > > ? Also, is the code change correct for when a non-replaced inline has an explicit height: auto? Anyway, not a css expert, yet. I missed out to add comment here in the previous comments. The spec, https://drafts.csswg.org/cssom/#resolved-value says that width and height should return the computed value, so if 'auto' is specified, then it should return 'auto'. So this change is correct, I think.

Daniel Bates

Comment 29 2019-05-19 20:35:09 PDT

Cool

Joonghun Park

Comment 30 2019-05-20 19:36:00 PDT

Hi, Could this patch get a review mark?

WebKit Commit Bot

Comment 31 2019-05-24 07:39:32 PDT

Comment on attachment 370221 [details] Patch Rejecting attachment 370221 [details] from commit-queue. Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-01', 'validate-changelog', '--check-oops', '--non-interactive', 370221, '--port=mac']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit ChangeLog entry in LayoutTests/ChangeLog contains OOPS!. Full output: https://webkit-queues.webkit.org/results/12278678

Joonghun Park

Comment 32 2019-05-24 13:46:02 PDT

Created attachment 370585 [details] Patch

WebKit Commit Bot

Comment 33 2019-05-24 18:42:23 PDT

Comment on attachment 370585 [details] Patch Clearing flags on attachment: 370585 Committed r245768: <https://trac.webkit.org/changeset/245768>

WebKit Commit Bot

Comment 34 2019-05-24 18:42:25 PDT

All reviewed patches have been landed. Closing bug.

Radar WebKit Bug Importer

Comment 35 2019-05-24 18:43:56 PDT

<rdar://problem/51129484>

Note You need to log in before you can comment on or make changes to this bug.