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

RESOLVED FIXED197814

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

https://bugs.webkit.org/show_bug.cgi?id=197814

Summary Computed width of non-replaced inline incorrectly returns "auto" instead of t...

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.

Status RESOLVED

Resolution FIXED

Priority P2

Severity Normal

Classification Unclassified

Version WebKit Nightly Build

Hardware Unspecified

OS Unspecified

Product WebKit

Component CSS

Assignee

Joonghun Park

Reported

2019-05-11 06:46 PDT

Modified

2019-05-24 18:43 PDT History

CC List

11 users Show

URL

Keywords InRadar

Depends on

Blocks