Bug 110393 – Positioned, replaced elements with intrinsic width keywords compute the wrong width

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Ojan Vafai]

Positioned, replaced elements with intrinsic width keywords compute the wrong width

[Ojan Vafai]

Created attachment 189392 [details]
Patch

[Emil A Eklund]

Comment on attachment 189392 [details]
Patch

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

> Source/WebCore/rendering/RenderBox.cpp:2594
> +            LayoutUnit containerLogicalWidth = 0;

This temporary variable isn't needed. If you feel it is needed for readability reason then at last match the name of the argument (availableLogicalWidth).

> LayoutTests/fast/css-intrinsic-dimensions/intrinsic-sized-replaced-absolutes.html:19
> +    <iframe class="child" style="width: -webkit-max-content;" data-expected-width="310"></iframe>

Where did you get the number 310 from?

[Ojan Vafai]

Comment on attachment 189392 [details]
Patch

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

>> Source/WebCore/rendering/RenderBox.cpp:2594
>> +            LayoutUnit containerLogicalWidth = 0;
> 
> This temporary variable isn't needed. If you feel it is needed for readability reason then at last match the name of the argument (availableLogicalWidth).

I think it's needed for readablity. I'll rename it.

>> LayoutTests/fast/css-intrinsic-dimensions/intrinsic-sized-replaced-absolutes.html:19
>> +    <iframe class="child" style="width: -webkit-max-content;" data-expected-width="310"></iframe>
> 
> Where did you get the number 310 from?

The intrinsic width of an iframe is 300px and it has a 10px border.

[Tony Chang]

Comment on attachment 189392 [details]
Patch

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

> Source/WebCore/rendering/RenderBox.cpp:2617
>          }

Nit: It would be nice to remove the default: case and enumerate the remaining types (Auto, Relative, and Undefined, I think).

[Emil A Eklund]

(In reply to comment #3)
> (From update of attachment 189392 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=189392&action=review
> 
> >> Source/WebCore/rendering/RenderBox.cpp:2594
> >> +            LayoutUnit containerLogicalWidth = 0;
> > 
> > This temporary variable isn't needed. If you feel it is needed for readability reason then at last match the name of the argument (availableLogicalWidth).
> 
> I think it's needed for readablity. I'll rename it.

Fair enough.

> 
> >> LayoutTests/fast/css-intrinsic-dimensions/intrinsic-sized-replaced-absolutes.html:19
> >> +    <iframe class="child" style="width: -webkit-max-content;" data-expected-width="310"></iframe>
> > 
> > Where did you get the number 310 from?
> 
> The intrinsic width of an iframe is 300px and it has a 10px border.

Could you add a variable for that? I.e. defaultIframeWidth = 300 and then use that to compute the expected width? Would make the test a lot easier to understand.

[Elliott Sprehn]

Comment on attachment 189392 [details]
Patch

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

>>>> LayoutTests/fast/css-intrinsic-dimensions/intrinsic-sized-replaced-absolutes.html:19
>>>> +    <iframe class="child" style="width: -webkit-max-content;" data-expected-width="310"></iframe>
>>> 
>>> Where did you get the number 310 from?
>> 
>> The intrinsic width of an iframe is 300px and it has a 10px border.
> 
> Could you add a variable for that? I.e. defaultIframeWidth = 300 and then use that to compute the expected width? Would make the test a lot easier to understand.

The whole point of these new style data-expected tests is there's no need for script running. Lets just use a comment to explain that instead.

[Ojan Vafai]

Comment on attachment 189392 [details]
Patch

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

>>>> LayoutTests/fast/css-intrinsic-dimensions/intrinsic-sized-replaced-absolutes.html:19
>>>> +    <iframe class="child" style="width: -webkit-max-content;" data-expected-width="310"></iframe>
>>> 
>>> Where did you get the number 310 from?
>> 
>> The intrinsic width of an iframe is 300px and it has a 10px border.
> 
> Could you add a variable for that? I.e. defaultIframeWidth = 300 and then use that to compute the expected width? Would make the test a lot easier to understand.

It would make this test considerably more complicated. Now I need script to set an attribute, but some of these widths are different. I mean...I can do it, but it doesn't seem worth it. I could add a comment though and get the same benefit I think.

[Emil A Eklund]

(In reply to comment #6)
> > Could you add a variable for that? I.e. defaultIframeWidth = 300 and then use that to compute the expected width? Would make the test a lot easier to understand.
> 
> The whole point of these new style data-expected tests is there's no need for script running. Lets just use a comment to explain that instead.

It still runs a script. It just happens that you declare the expectations in html instead of js. While it might make sense in many cases it doesn't here as it provides values without any explanation and makes the test harder to understand. Perhaps we should change resources/check-layout.js to allow named constants and simple arithmetic.


(In reply to comment #7)
A comment would do nicely. Thanks.

[Ojan Vafai]

Created attachment 189403 [details]
Patch

[EFL EWS Bot]

Comment on attachment 189403 [details]
Patch

Attachment 189403 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/16651617

[Early Warning System Bot]

Comment on attachment 189403 [details]
Patch

Attachment 189403 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/16660585

[Early Warning System Bot]

Comment on attachment 189403 [details]
Patch

Attachment 189403 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/16656605

[Emil A Eklund]

Comment on attachment 189403 [details]
Patch

Thanks!

[Ojan Vafai]

Committed r143539: <http://trac.webkit.org/changeset/143539>