Bug 197814 - Computed width of non-replaced inline incorrectly returns "auto" instead of the computed value.
Summary: Computed width of non-replaced inline incorrectly returns "auto" instead of t...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Joonghun Park
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2019-05-11 06:46 PDT by Emilio Cobos Álvarez (:emilio)
Modified: 2019-05-24 18:43 PDT (History)
11 users (show)

See Also:


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, Build Bot
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, Build Bot
no flags Details
Archive of layout-test-results from ews210 for win-future (13.68 MB, application/zip)
2019-05-18 11:00 PDT, Build Bot
no flags Details
Archive of layout-test-results from ews116 for mac-highsierra (2.99 MB, application/zip)
2019-05-18 11:02 PDT, Build Bot
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, Build Bot
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, Build Bot
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

Note You need to log in before you can comment on or make changes to this bug.
Description Emilio Cobos Álvarez (:emilio) 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".
Comment 1 Joonghun Park 2019-05-18 09:12:56 PDT
Created attachment 370191 [details]
To check ews result
Comment 2 Build Bot 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
Comment 3 Build Bot 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
Comment 4 Build Bot 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
Comment 5 Build Bot 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
Comment 6 Build Bot 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
Comment 7 Build Bot 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
Comment 8 Build Bot 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
Comment 9 Build Bot 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
Comment 10 Build Bot 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
Comment 11 Build Bot 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
Comment 12 Joonghun Park 2019-05-18 16:56:14 PDT
Created attachment 370205 [details]
Update LayoutTest
Comment 13 Joonghun Park 2019-05-18 17:45:31 PDT
Created attachment 370210 [details]
Patch
Comment 14 Joonghun Park 2019-05-18 17:52:29 PDT
Created attachment 370211 [details]
Patch
Comment 15 Build Bot 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
Comment 16 Build Bot 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
Comment 17 Joonghun Park 2019-05-18 19:01:17 PDT
Created attachment 370213 [details]
Patch
Comment 18 Joonghun Park 2019-05-18 19:16:33 PDT
Created attachment 370214 [details]
Patch
Comment 19 Joonghun Park 2019-05-18 20:06:15 PDT
Created attachment 370217 [details]
Update legacy-animation-engine layout test
Comment 20 Joonghun Park 2019-05-18 21:56:43 PDT
Could you please review this change?
Comment 21 Emilio Cobos Álvarez (:emilio) 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);
Comment 22 Emilio Cobos Álvarez (:emilio) 2019-05-19 05:22:03 PDT
(disclaimer: I'm not a WebKit reviewer)
Comment 23 Joonghun Park 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:)
Comment 24 Joonghun Park 2019-05-19 06:22:04 PDT
Created attachment 370221 [details]
Patch
Comment 25 Daniel Bates 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?
Comment 26 Daniel Bates 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
Comment 27 Joonghun Park 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.
Comment 28 Joonghun Park 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.
Comment 29 Daniel Bates 2019-05-19 20:35:09 PDT
Cool
Comment 30 Joonghun Park 2019-05-20 19:36:00 PDT
Hi, Could this patch get a review mark?
Comment 31 WebKit Commit Bot 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
Comment 32 Joonghun Park 2019-05-24 13:46:02 PDT
Created attachment 370585 [details]
Patch
Comment 33 WebKit Commit Bot 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>
Comment 34 WebKit Commit Bot 2019-05-24 18:42:25 PDT
All reviewed patches have been landed.  Closing bug.
Comment 35 Radar WebKit Bug Importer 2019-05-24 18:43:56 PDT
<rdar://problem/51129484>