Add scrollbar logical width while computing intrinsic logical width

Add scrollbar logical width while computing intrinsic logical width

This CL does the following:
1. takes into account the intrinsic scrollbar logical width while
   computing min and max logical widths.
2. fixes the typo i.e. fixes "instrinsicScrollbarLogicalWidth"
   to "intrinsicScrollbarLogicalWidth"

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=185999

[Sunil Ratnu, 2014-09-03 11:19:59]

sunil.ratnu@samsung.com changed reviewers:
+ jchaffraix@chromium.org

[Sunil Ratnu, 2014-09-03 11:19:59]

I have also written a test case where grid SHOULD NOT add scrollbar width to the
intrinsic width of fixed-width items here
https://codereview.chromium.org/532203002/ .

I've kept that test case separately because this change should have a test case
which is related to taking into account the scrollbar logical width. I am trying
to come up with a test case for this. Please review this. Thanks!

[Sunil Ratnu, 2014-09-05 13:45:05]

Hi Julien, I've written the test case where we should take into account the
scrollbar width. Please review this. Thanks!

[Sunil Ratnu, 2014-09-09 08:46:55]

sunil.ratnu@samsung.com changed reviewers:
+ jfernandez@igalia.com

[Julien - ping for review, 2014-09-09 14:39:40]

https://codereview.chromium.org/535913002/diff/20001/LayoutTests/fast/css-gri...
File
LayoutTests/fast/css-grid-layout/grid-container-width-should-include-scroll-bar-width.html
(right):

https://codereview.chromium.org/535913002/diff/20001/LayoutTests/fast/css-gri...
LayoutTests/fast/css-grid-layout/grid-container-width-should-include-scroll-bar-width.html:37:
});
The scrollbar width is 16px so no need for this logic. Instead, let's just put
the appropriate data-expected-width.

https://codereview.chromium.org/535913002/diff/20001/Source/core/rendering/Re...
File Source/core/rendering/RenderGrid.cpp (right):

https://codereview.chromium.org/535913002/diff/20001/Source/core/rendering/Re...
Source/core/rendering/RenderGrid.cpp:379: maxLogicalWidth =
std::max(minLogicalWidth, maxLogicalWidth);
Is this max required as we ensure that minTrackBreadth is less than
maxTrackBreadth above?

https://codereview.chromium.org/535913002/diff/20001/Source/core/rendering/Re...
Source/core/rendering/RenderGrid.cpp:381: LayoutUnit scrollbarWidth =
instrinsicScrollbarLogicalWidth();
Let's correct the typo in the function name while touching this code (it should
be intrinsicScrollbarLogicalWidth).

[Sunil Ratnu, 2014-09-09 15:22:55]

https://codereview.chromium.org/535913002/diff/20001/Source/core/rendering/Re...
File Source/core/rendering/RenderGrid.cpp (right):

https://codereview.chromium.org/535913002/diff/20001/Source/core/rendering/Re...
Source/core/rendering/RenderGrid.cpp:379: maxLogicalWidth =
std::max(minLogicalWidth, maxLogicalWidth);
On 2014/09/09 14:39:40, Julien Chaffraix - PST wrote:
> Is this max required as we ensure that minTrackBreadth is less than
> maxTrackBreadth above?

In this case, its not needed as before calling computeIntrinsicLogicalWidths(),
we have
minLogicalWidth = maxLogicalWidth = 0; But I was unsure that this will always
remain equal or 0 because of the FIXME present in
computePreferredLogicalWidths(), hence thought of keeping std::max here (Can be
handy in cases when we have different values for these).

I will upload the next patch including this as well other changes once we can be
sure of what to with std::max.

Please do let me know whether we should still keep it or not?

[Sunil Ratnu, 2014-09-09 17:28:30]

Hi Julien, I've made the suggested changes.

Only one thing remains whether to have "maxLogicalWidth =
std::max(minLogicalWidth, maxLogicalWidth)" or not. Please have a look. Thanks!

https://codereview.chromium.org/535913002/diff/20001/LayoutTests/fast/css-gri...
File
LayoutTests/fast/css-grid-layout/grid-container-width-should-include-scroll-bar-width.html
(right):

https://codereview.chromium.org/535913002/diff/20001/LayoutTests/fast/css-gri...
LayoutTests/fast/css-grid-layout/grid-container-width-should-include-scroll-bar-width.html:37:
});
On 2014/09/09 14:39:40, Julien Chaffraix - PST wrote:
> The scrollbar width is 16px so no need for this logic. Instead, let's just put
> the appropriate data-expected-width.

Done.

https://codereview.chromium.org/535913002/diff/20001/Source/core/rendering/Re...
File Source/core/rendering/RenderGrid.cpp (right):

https://codereview.chromium.org/535913002/diff/20001/Source/core/rendering/Re...
Source/core/rendering/RenderGrid.cpp:381: LayoutUnit scrollbarWidth =
instrinsicScrollbarLogicalWidth();
On 2014/09/09 14:39:40, Julien Chaffraix - PST wrote:
> Let's correct the typo in the function name while touching this code (it
should
> be intrinsicScrollbarLogicalWidth).

Done.

[Julien - ping for review, 2014-09-10 16:06:54]

lgtm with the following changes

https://codereview.chromium.org/535913002/diff/40001/LayoutTests/fast/css-gri...
File
LayoutTests/fast/css-grid-layout/grid-container-width-should-include-scroll-bar-width.html
(right):

https://codereview.chromium.org/535913002/diff/40001/LayoutTests/fast/css-gri...
LayoutTests/fast/css-grid-layout/grid-container-width-should-include-scroll-bar-width.html:19:
<div class='grid' style='top: 100px' data-expected-width='115'>
I don't think we need the position: absolute and the top/left offsets in both
grid to test data-expected-width so we should remove them.

https://codereview.chromium.org/535913002/diff/40001/Source/core/rendering/Re...
File Source/core/rendering/RenderGrid.cpp (right):

https://codereview.chromium.org/535913002/diff/40001/Source/core/rendering/Re...
Source/core/rendering/RenderGrid.cpp:379: maxLogicalWidth =
std::max(minLogicalWidth, maxLogicalWidth);
I would still remove it. If we fix the FIXME in computePreferredLogicalWidths,
we should still ensure that minLogicalWidth <= maxLogicalWidth. If we don't, the
whole logic in computeIntrinsicLogicalWidths is bogus.

[Julien - ping for review, 2014-09-10 16:08:44]

Another comment: The last sentence of your description is above 80 columns. We
try to keep the description below 80 characters per
https://groups.google.com/a/chromium.org/forum/#!topic/blink-dev/rqGhY7PfkYY (I
advise 72 based on the rationale in the thread but it's a matter of taste)

[Sunil Ratnu, 2014-09-11 10:59:44]

https://codereview.chromium.org/535913002/diff/40001/LayoutTests/fast/css-gri...
File
LayoutTests/fast/css-grid-layout/grid-container-width-should-include-scroll-bar-width.html
(right):

https://codereview.chromium.org/535913002/diff/40001/LayoutTests/fast/css-gri...
LayoutTests/fast/css-grid-layout/grid-container-width-should-include-scroll-bar-width.html:19:
<div class='grid' style='top: 100px' data-expected-width='115'>
I think we need it because when I tried testing after removing "position:
absolute", the whole div (in our case the container) occupies the screen space,
and the scrollbar appears at the end, and the data-expected-width comes 784
(http://jsbin.com/harezasuhepu/1/edit). But when we use the absolute
positioning, it is seen clearly seen that scrollbar width gets added in the
container size.

https://codereview.chromium.org/535913002/diff/40001/Source/core/rendering/Re...
File Source/core/rendering/RenderGrid.cpp (right):

https://codereview.chromium.org/535913002/diff/40001/Source/core/rendering/Re...
Source/core/rendering/RenderGrid.cpp:379: maxLogicalWidth =
std::max(minLogicalWidth, maxLogicalWidth);
On 2014/09/10 16:06:54, Julien Chaffraix - PST wrote:
> I would still remove it. If we fix the FIXME in computePreferredLogicalWidths,
> we should still ensure that minLogicalWidth <= maxLogicalWidth. If we don't,
the
> whole logic in computeIntrinsicLogicalWidths is bogus.

Done.

[Sunil Ratnu, 2014-09-12 00:39:20]

Hi Julien, It would be great if you could review the final patchset as after
today i will be OOO for 5 days. Thank you!

[Sunil Ratnu, 2014-09-18 03:03:31]

Hi Julien, i am back at office now, can you please review it now? Thanks!

[Sunil Ratnu, 2014-09-23 12:58:44]

On 2014/09/18 03:03:31, Sunil Ratnu wrote:
> Hi Julien, i am back at office now, can you please review it now? Thanks!

Hi Julien, Please have a look.

Thank you,
Sunil

[Sunil Ratnu, 2014-11-06 05:33:09]

On 2014/09/23 12:58:44, Sunil Ratnu wrote:
> On 2014/09/18 03:03:31, Sunil Ratnu wrote:
> > Hi Julien, i am back at office now, can you please review it now? Thanks!
> 
> Hi Julien, Please have a look.
> 
> Thank you,
> Sunil

Hi Julien,

Could you please review this CL. Its been pending for a long time now.

Thanks!
Sunil

[Julien - ping for review, 2014-11-19 23:28:41]

lgtm.

Sorry I didn't see your comments before now.

You had an LGTM so you didn't have to wait on me for landing. The best way to
get my attention is direct email or IRC.

https://codereview.chromium.org/535913002/diff/40001/LayoutTests/fast/css-gri...
File
LayoutTests/fast/css-grid-layout/grid-container-width-should-include-scroll-bar-width.html
(right):

https://codereview.chromium.org/535913002/diff/40001/LayoutTests/fast/css-gri...
LayoutTests/fast/css-grid-layout/grid-container-width-should-include-scroll-bar-width.html:19:
<div class='grid' style='top: 100px' data-expected-width='115'>
On 2014/09/11 10:59:44, Sunil Ratnu wrote:
> I think we need it because when I tried testing after removing "position:
> absolute", the whole div (in our case the container) occupies the screen
space,
> and the scrollbar appears at the end, and the data-expected-width comes 784
> (http://jsbin.com/harezasuhepu/1/edit). But when we use the absolute
> positioning, it is seen clearly seen that scrollbar width gets added in the
> container size. 

You don't *need* position per se. What you want is a consistent size for the
grid container, which can be achieve in other ways.

One is to make it shrink-wrap (which is what position: absolute gives you
instead of width: auto being sized after your containing block's content width):

.grid {
    width: fit-content;
}

Another more simple way is just to set the width and height of the grid:

.grid {
    width: 100px;
    height: 100px;
}

(your children are just a proxy for getting a size as you're shrink-wrap)

The 2nd option is the most appealing for me as it's the most simple too,
especially if we remove the .gridItem that are distracting readers from what the
test stresses.

[Sunil Ratnu, 2014-11-21 07:25:49]

On 2014/11/19 23:28:41, Julien Chaffraix - PST wrote:
> lgtm.
> 
> Sorry I didn't see your comments before now.
> 
> You had an LGTM so you didn't have to wait on me for landing. The best way to
> get my attention is direct email or IRC.
> 
>
https://codereview.chromium.org/535913002/diff/40001/LayoutTests/fast/css-gri...
> File
>
LayoutTests/fast/css-grid-layout/grid-container-width-should-include-scroll-bar-width.html
> (right):
> 
>
https://codereview.chromium.org/535913002/diff/40001/LayoutTests/fast/css-gri...
>
LayoutTests/fast/css-grid-layout/grid-container-width-should-include-scroll-bar-width.html:19:
> <div class='grid' style='top: 100px' data-expected-width='115'>
> On 2014/09/11 10:59:44, Sunil Ratnu wrote:
> > I think we need it because when I tried testing after removing "position:
> > absolute", the whole div (in our case the container) occupies the screen
> space,
> > and the scrollbar appears at the end, and the data-expected-width comes 784
> > (http://jsbin.com/harezasuhepu/1/edit). But when we use the absolute
> > positioning, it is seen clearly seen that scrollbar width gets added in the
> > container size. 
> 
> You don't *need* position per se. What you want is a consistent size for the
> grid container, which can be achieve in other ways.
> 
> One is to make it shrink-wrap (which is what position: absolute gives you
> instead of width: auto being sized after your containing block's content
width):
> 
> .grid {
>     width: fit-content;
> }
> 
> Another more simple way is just to set the width and height of the grid:
> 
> .grid {
>     width: 100px;
>     height: 100px;
> }
> 
> (your children are just a proxy for getting a size as you're shrink-wrap)
> 
> The 2nd option is the most appealing for me as it's the most simple too,
> especially if we remove the .gridItem that are distracting readers from what
the
> test stresses.

Hi Julien, I've made the suggested changes following first approach.

The second one won't work because we are specifying a fixed width
and in case of already specified fixed width cases,scrollbar width will
not get added to the intrinsic width.

Can we land this now?

[Julien - ping for review, 2014-11-26 00:12:51]

lgtm

https://codereview.chromium.org/535913002/diff/100001/LayoutTests/fast/css-gr...
File
LayoutTests/fast/css-grid-layout/grid-container-width-should-include-scroll-bar-width.html
(right):

https://codereview.chromium.org/535913002/diff/100001/LayoutTests/fast/css-gr...
LayoutTests/fast/css-grid-layout/grid-container-width-should-include-scroll-bar-width.html:8:
width: -o-fit-content;
I don't think we want the Opera prefix as it is now using Blink.

[Sunil Ratnu, 2014-11-26 03:37:58]

Thanks for the review Julien.

https://codereview.chromium.org/535913002/diff/100001/LayoutTests/fast/css-gr...
File
LayoutTests/fast/css-grid-layout/grid-container-width-should-include-scroll-bar-width.html
(right):

https://codereview.chromium.org/535913002/diff/100001/LayoutTests/fast/css-gr...
LayoutTests/fast/css-grid-layout/grid-container-width-should-include-scroll-bar-width.html:8:
width: -o-fit-content;
On 2014/11/26 00:12:51, Julien Chaffraix - PST wrote:
> I don't think we want the Opera prefix as it is now using Blink.

Done.

[Sunil Ratnu, 2014-11-26 03:38:24]

The CQ bit was checked by sunil.ratnu@samsung.com

[commit-bot: I haz the power, 2014-11-26 03:38:43]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/535913002/120001

[commit-bot: I haz the power, 2014-11-26 05:00:32]

Committed patchset #7 (id:120001) as
https://src.chromium.org/viewvc/blink?view=rev&revision=185999