RLSA needsLayout on customscrollbar thickness change.

RLSA needsLayout on CustomScrollbar thickness change.

RLSA RenderBlock needs layouts excluding the CustomScrollbar thickness.
Set children of RenderBlock width is changed upon CustomScrollbar 
thickness change. This ensures to re-layout RLSA RenderBlock 
considering the CustomScrollbar thickness.

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

[MuVen, 2014-11-08 15:16:55]

sataya.m@samsung.com changed reviewers:
+ rune@opera.com

[MuVen, 2014-11-08 15:20:07]

Hi Rune,

PTAL.


Thanks,
MuVen.

[MuVen, 2014-11-08 15:21:20]

sataya.m@samsung.com changed reviewers:
+ skobes@chromium.org

[MuVen, 2014-11-08 15:21:36]

+ skobes

[skobes, 2014-11-10 20:26:09]

https://codereview.chromium.org/708283002/diff/60001/Source/core/rendering/Re...
File Source/core/rendering/RenderLayerScrollableArea.cpp (right):

https://codereview.chromium.org/708283002/diff/60001/Source/core/rendering/Re...
Source/core/rendering/RenderLayerScrollableArea.cpp:642: if
(box().style()->overflowX() == OAUTO || box().style()->overflowY() == OAUTO ||
m_verticalScrollbarRectChanged || m_horizontalScrollbarRectChanged) {
I think this codepath is intended for the case where we need a second layout
because the first layout caused a scrollbar to be added or removed.  But for a
change in scrollbar style, we don't need a second layout; the normal layout
should handle it.

Instead, updateAfterStyleChange should just mark the box as needing layout, if
the scrollbar frame rect has changed.  Can you see if that fixes the issue?

[MuVen, 2014-11-11 14:45:04]

https://codereview.chromium.org/708283002/diff/60001/Source/core/rendering/Re...
File Source/core/rendering/RenderLayerScrollableArea.cpp (right):

https://codereview.chromium.org/708283002/diff/60001/Source/core/rendering/Re...
Source/core/rendering/RenderLayerScrollableArea.cpp:772: 
    if (box().isRenderBlock()) {
        RenderBlock& block = toRenderBlock(box());
        block.scrollbarsChanged(m_horizontalScrollbarRectChanged,
m_verticalScrollbarRectChanged);
        block.setNeedsLayout();
    } else {
        box().setNeedsLayout();
    }

I have tried earlier with above snippet, but i still see the issue. This
provoked me to go for force layout.

[MuVen, 2014-11-11 15:08:18]

I see a nominal layout of renderbox in testcase 
http://css-tricks.com/examples/WebKitScrollbars/ when i change the 
custom scrollbar width, but when the width is increased to large number
scrollbar overlaps the content. Though i have added above mentioned snippet
it didnt work.Due to which i have done a force layout.
Please share your views/ideas to proceed further. 

Thanks,
MuVen.

[skobes, 2014-11-11 19:53:36]

On 2014/11/11 15:08:18, muven wrote:
> I see a nominal layout of renderbox in testcase 
> http://css-tricks.com/examples/WebKitScrollbars/ when i change the 
> custom scrollbar width, but when the width is increased to large number
> scrollbar overlaps the content. Though i have added above mentioned snippet
> it didnt work.Due to which i have done a force layout.
> Please share your views/ideas to proceed further. 

I think you'll have to do more investigation to understand what is going on. 
For example, do we fail to layout the block after updateAfterStyleChange, or
does the layout see the wrong width, or do we layout correctly but fail to
repaint?

Also, your test case passes for me on Linux with a tip of trunk content_shell
build, so I suspect it is not actually testing the issue you're trying to fix.

[MuVen, 2014-11-12 15:01:17]

Patchset #5 (id:80001) has been deleted

[MuVen, 2014-11-12 15:17:18]

Patchset #5 (id:100001) has been deleted

[MuVen, 2014-11-12 15:39:31]

Patchset #6 (id:140001) has been deleted

[MuVen, 2014-11-12 15:41:18]

Patchset #5 (id:120001) has been deleted

[MuVen, 2014-11-12 17:39:21]

Patchset #5 (id:160001) has been deleted

[MuVen, 2014-11-12 17:41:13]

Hi Skobes,

I see that the logical width/height of the custom scrollbar is not been
calculated. I have added that code by implementing the 
scrollbarsChanged API.

PTAL.

Thanks,
MuVen.

[MuVen, 2014-11-12 18:33:28]

I have modified logical width/height in the renderScrollbar.cpp
to minize code, as in the RenderScrollbar itself owningRender
needsLayout is set.

Removed testcase 
custom-scrollbar-changing-style-relayout-scrollablearea-by-resizing-p.html
as i have upload it by mistake, i have written it to check overflow-x:scroll
functionality.

let me know your inputs.

[skobes, 2014-11-13 04:49:36]

On 2014/11/12 18:33:28, muven wrote:
> I have modified logical width/height in the renderScrollbar.cpp
> to minize code, as in the RenderScrollbar itself owningRender
> needsLayout is set.
> 
> Removed testcase 
> custom-scrollbar-changing-style-relayout-scrollablearea-by-resizing-p.html
> as i have upload it by mistake, i have written it to check overflow-x:scroll
> functionality.
> 
> let me know your inputs.

The logical width and height of the scrollable element should be computed during
layout, by RenderBlockFlow::layoutBlockFlow.  Why isn't that happening here?

[MuVen, 2014-11-13 06:11:52]

layoutBlock(true); makes to relayout children even.
may be we can add conditions to make it true if the custom scrollbar
is changed.

[MuVen, 2014-11-13 11:21:50]

Patchset #7 (id:220001) has been deleted

[MuVen, 2014-11-13 11:22:34]

Patchset #8 (id:260001) has been deleted

[MuVen, 2014-11-13 11:22:51]

Patchset #8 (id:280001) has been deleted

[MuVen, 2014-11-13 11:23:03]

Patchset #8 (id:300001) has been deleted

[MuVen, 2014-11-13 12:49:21]

Patchset #9 (id:340001) has been deleted

[MuVen, 2014-11-13 12:49:45]

Patchset #8 (id:320001) has been deleted

[MuVen, 2014-11-13 15:58:03]

PTAL.

[skobes, 2014-11-13 18:14:26]

https://codereview.chromium.org/708283002/diff/360001/Source/core/rendering/R...
File Source/core/rendering/RenderBlock.cpp (right):

https://codereview.chromium.org/708283002/diff/360001/Source/core/rendering/R...
Source/core/rendering/RenderBlock.cpp:1394: relayoutChildren |=
layer()->scrollableArea()->didCustomScrollbarRectChanged();
RenderBlockFlow::layoutBlockFlow has logic to force relayout of children when
the block's width has changed.  I guess the logicalWidth() includes scrollbars,
so it doesn't change when the scrollbar width changes, therefore we don't
relayout the children when we should?  If that's the root cause then the best
place for the fix might be in RenderBlock::widthAvailableToChildrenHasChanged().

In that case I would try to use m_hasBorderOrPaddingLogicalWidthChanged to track
it, instead of adding state to RenderLayerScrollableArea.  You can rename the
field m_widthAvailableToChildrenChanged since it is affected by more than just
border or padding changes.

[MuVen, 2014-11-13 20:27:57]

Patchset #9 (id:380001) has been deleted

[MuVen, 2014-11-13 20:30:39]

Patchset #9 (id:400001) has been deleted

[MuVen, 2014-11-13 20:34:10]

PTAL.

https://codereview.chromium.org/708283002/diff/360001/Source/core/rendering/R...
File Source/core/rendering/RenderBlock.cpp (right):

https://codereview.chromium.org/708283002/diff/360001/Source/core/rendering/R...
Source/core/rendering/RenderBlock.cpp:1394: relayoutChildren |=
layer()->scrollableArea()->didCustomScrollbarRectChanged();
I have added a variable in the RenderBox to inform customscrollbar thickness
changed, as change in RenderScrollbarPart width will not trigger
widthAvailableToChildrenHasChanged.

[skobes, 2014-11-14 03:32:11]

Thanks, this is looking better.

https://codereview.chromium.org/708283002/diff/420001/Source/core/rendering/R...
File Source/core/rendering/RenderBox.h (right):

https://codereview.chromium.org/708283002/diff/420001/Source/core/rendering/R...
Source/core/rendering/RenderBox.h:453: void
setCustomScrollbarThicknessChanged(bool thicknessChanged) {
m_customScrollbarThicknessChanged = thicknessChanged; }
Instead of adding a new boolean, can you put this method in RenderBlock and have
it set m_widthAvailableToChildrenChanged directly?  Also I would suggest calling
it notifyScrollbarThicknessChanged.

You can downcast at the call site, e.g.

if (box->isRenderBlock())
  toRenderBlock(box)->notifyScrollbarThicknessChanged();

I don't think any other subclass of RenderBox would be affected by this.

[MuVen, 2014-11-14 06:10:25]

PTAL.

https://codereview.chromium.org/708283002/diff/420001/Source/core/rendering/R...
File Source/core/rendering/RenderBox.h (right):

https://codereview.chromium.org/708283002/diff/420001/Source/core/rendering/R...
Source/core/rendering/RenderBox.h:453: void
setCustomScrollbarThicknessChanged(bool thicknessChanged) {
m_customScrollbarThicknessChanged = thicknessChanged; }
On 2014/11/14 03:32:10, skobes wrote:
> Instead of adding a new boolean, can you put this method in RenderBlock and
have
> it set m_widthAvailableToChildrenChanged directly?  Also I would suggest
calling
> it notifyScrollbarThicknessChanged.
> 
> You can downcast at the call site, e.g.
> 
> if (box->isRenderBlock())
>   toRenderBlock(box)->notifyScrollbarThicknessChanged();
> 
> I don't think any other subclass of RenderBox would be affected by this.

Done.

[skobes, 2014-11-14 20:07:42]

lgtm

[MuVen, 2014-11-14 20:24:27]

The CQ bit was checked by sataya.m@samsung.com

[commit-bot: I haz the power, 2014-11-14 20:24:59]

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

[MuVen, 2014-11-14 20:35:17]

sataya.m@samsung.com changed reviewers:
+ pdr@chromium.org

[MuVen, 2014-11-14 20:35:18]

@skobes, thanks for the review, 
@pdr, Need owners LGTM. PTAL.

Thanks,
MuVen.

[commit-bot: I haz the power, 2014-11-14 20:41:26]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2014-11-14 20:41:27]

Try jobs failed on following builders:
  blink_presubmit on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/blink_presubmit/builds/1...)

[pdr., 2014-11-14 21:21:24]

On 2014/11/14 at 20:41:27, commit-bot wrote:
> Try jobs failed on following builders:
>   blink_presubmit on tryserver.blink
(http://build.chromium.org/p/tryserver.blink/builders/blink_presubmit/builds/1...)

Skobes isn't a core owner yet? We should fix that mister 75 patches :)

LGTM

[MuVen, 2014-11-14 21:30:07]

The CQ bit was checked by sataya.m@samsung.com

[MuVen, 2014-11-14 21:30:19]

On 2014/11/14 21:21:24, pdr wrote:
> On 2014/11/14 at 20:41:27, commit-bot wrote:
> > Try jobs failed on following builders:
> >   blink_presubmit on tryserver.blink
>
(http://build.chromium.org/p/tryserver.blink/builders/blink_presubmit/builds/1...)
> 
> Skobes isn't a core owner yet? We should fix that mister 75 patches :)
> 
> LGTM

thanks pdr :)

[commit-bot: I haz the power, 2014-11-14 21:30:47]

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

[commit-bot: I haz the power, 2014-11-14 22:52:09]

Committed patchset #10 (id:440001) as 185395