Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
503583002:
Fixed Inconsistent behaviour of custom-scrollbar (Closed)
DescriptionFixed Inconsistent behaviour of custom-scrollbar
Custom-scrollbar scales along with the page, in this process
thickness of the scrollbar is changed. Which sets ownerRender
needslayout. Now the layout is being trigerred on change in
thickness and updating scrollbar for every scale-factor
irrespective of scrollbar presence.
Detailed explanation of the bug mentioned @
https://codereview.chromium.org/503583002/#msg46
This review is merged to
https://codereview.chromium.org/702913002/
BUG=406676, 390895, 375200, 424925
Patch Set 1 #Patch Set 2 : Adding test case #
Total comments: 14
Patch Set 3 : addressed review comments #Patch Set 4 : #Patch Set 5 : #Patch Set 6 : #
Total comments: 3
Patch Set 7 : #Patch Set 8 : #Patch Set 9 : final fix #Patch Set 10 : testexpectations #Patch Set 11 : TOT and changed the Test Name #Patch Set 12 : Invalidate ,adjust custom scrollbar on thickness change #
Total comments: 3
Patch Set 13 : addressed review comments #
Messages
Total messages: 75 (27 generated)
PTAL.Sorry for raising another review request for the same, I closed the previous issue unknowingly.
friendly ping. Thanks.
This CL needs a test.
Patchset #2 (id:20001) has been deleted
Hi Abarth, PTAL. Thanks,
sataya.m@samsung.com changed reviewers: + esprehn@chromium.org
Hi Esprehn, Can you PTAL at the patch. Thanks, MuVen.
On 2014/09/11 16:49:45, muven wrote: > Hi Esprehn, > > Can you PTAL at the patch. > > Thanks, > MuVen. Friendly ping. PTAL.
https://codereview.chromium.org/503583002/diff/40001/LayoutTests/fast/events/... File LayoutTests/fast/events/scale-document.html (right): https://codereview.chromium.org/503583002/diff/40001/LayoutTests/fast/events/... LayoutTests/fast/events/scale-document.html:1: <html> missing doctype, also leave out the html, body and head elements. https://codereview.chromium.org/503583002/diff/40001/LayoutTests/fast/events/... LayoutTests/fast/events/scale-document.html:5: height: 16px; Does height actually do anything here? https://codereview.chromium.org/503583002/diff/40001/LayoutTests/fast/events/... LayoutTests/fast/events/scale-document.html:6: overflow: visible; overflow doesn't do anything on a scrollbar. https://codereview.chromium.org/503583002/diff/40001/LayoutTests/fast/events/... LayoutTests/fast/events/scale-document.html:15: // The factors are the same as those in Chrome settings. just inline this code below, remove the function. https://codereview.chromium.org/503583002/diff/40001/LayoutTests/fast/events/... LayoutTests/fast/events/scale-document.html:22: if (window.testRunner) bad indent. https://codereview.chromium.org/503583002/diff/40001/LayoutTests/fast/events/... LayoutTests/fast/events/scale-document.html:26: if (window.testRunner) { bad indentation. https://codereview.chromium.org/503583002/diff/40001/Source/core/frame/FrameV... File Source/core/frame/FrameView.cpp (right): https://codereview.chromium.org/503583002/diff/40001/Source/core/frame/FrameV... Source/core/frame/FrameView.cpp:924: performLayout(rootForThisLayout, inSubtreeLayout); Other places call adjustViewSize without a second layout, are you sure this is right? Should adjustViewSize do this?
Patchset #3 (id:60001) has been deleted
Patchset #3 (id:80001) has been deleted
PTAL. https://codereview.chromium.org/503583002/diff/40001/LayoutTests/fast/events/... File LayoutTests/fast/events/scale-document.html (right): https://codereview.chromium.org/503583002/diff/40001/LayoutTests/fast/events/... LayoutTests/fast/events/scale-document.html:1: <html> On 2014/09/19 05:06:52, esprehn wrote: > missing doctype, also leave out the html, body and head elements. Done. https://codereview.chromium.org/503583002/diff/40001/LayoutTests/fast/events/... LayoutTests/fast/events/scale-document.html:5: height: 16px; Nothing like that, i see issue with height and width 1px even. On 2014/09/19 05:06:52, esprehn wrote: > Does height actually do anything here? https://codereview.chromium.org/503583002/diff/40001/LayoutTests/fast/events/... LayoutTests/fast/events/scale-document.html:6: overflow: visible; On 2014/09/19 05:06:52, esprehn wrote: > overflow doesn't do anything on a scrollbar. Done. https://codereview.chromium.org/503583002/diff/40001/LayoutTests/fast/events/... LayoutTests/fast/events/scale-document.html:15: // The factors are the same as those in Chrome settings. On 2014/09/19 05:06:52, esprehn wrote: > just inline this code below, remove the function. Done. https://codereview.chromium.org/503583002/diff/40001/LayoutTests/fast/events/... LayoutTests/fast/events/scale-document.html:22: if (window.testRunner) On 2014/09/19 05:06:52, esprehn wrote: > bad indent. Done. https://codereview.chromium.org/503583002/diff/40001/LayoutTests/fast/events/... LayoutTests/fast/events/scale-document.html:26: if (window.testRunner) { On 2014/09/19 05:06:52, esprehn wrote: > bad indentation. Done. https://codereview.chromium.org/503583002/diff/40001/Source/core/frame/FrameV... File Source/core/frame/FrameView.cpp (right): https://codereview.chromium.org/503583002/diff/40001/Source/core/frame/FrameV... Source/core/frame/FrameView.cpp:924: performLayout(rootForThisLayout, inSubtreeLayout); During adjustViewSize call, in the API FrameView::setContentsSize, after calling ScrollView::setContentsSize(size); sets the rootObject needsLayout.
On 2014/09/19 16:19:30, muven wrote: > PTAL. > > https://codereview.chromium.org/503583002/diff/40001/LayoutTests/fast/events/... > File LayoutTests/fast/events/scale-document.html (right): > > https://codereview.chromium.org/503583002/diff/40001/LayoutTests/fast/events/... > LayoutTests/fast/events/scale-document.html:1: <html> > On 2014/09/19 05:06:52, esprehn wrote: > > missing doctype, also leave out the html, body and head elements. > > Done. > > https://codereview.chromium.org/503583002/diff/40001/LayoutTests/fast/events/... > LayoutTests/fast/events/scale-document.html:5: height: 16px; > Nothing like that, i see issue with height and width 1px even. > On 2014/09/19 05:06:52, esprehn wrote: > > Does height actually do anything here? > > https://codereview.chromium.org/503583002/diff/40001/LayoutTests/fast/events/... > LayoutTests/fast/events/scale-document.html:6: overflow: visible; > On 2014/09/19 05:06:52, esprehn wrote: > > overflow doesn't do anything on a scrollbar. > > Done. > > https://codereview.chromium.org/503583002/diff/40001/LayoutTests/fast/events/... > LayoutTests/fast/events/scale-document.html:15: // The factors are the same as > those in Chrome settings. > On 2014/09/19 05:06:52, esprehn wrote: > > just inline this code below, remove the function. > > Done. > > https://codereview.chromium.org/503583002/diff/40001/LayoutTests/fast/events/... > LayoutTests/fast/events/scale-document.html:22: if (window.testRunner) > On 2014/09/19 05:06:52, esprehn wrote: > > bad indent. > > Done. > > https://codereview.chromium.org/503583002/diff/40001/LayoutTests/fast/events/... > LayoutTests/fast/events/scale-document.html:26: if (window.testRunner) { > On 2014/09/19 05:06:52, esprehn wrote: > > bad indentation. > > Done. > > https://codereview.chromium.org/503583002/diff/40001/Source/core/frame/FrameV... > File Source/core/frame/FrameView.cpp (right): > > https://codereview.chromium.org/503583002/diff/40001/Source/core/frame/FrameV... > Source/core/frame/FrameView.cpp:924: performLayout(rootForThisLayout, > inSubtreeLayout); > During adjustViewSize call, in the API FrameView::setContentsSize, after calling > ScrollView::setContentsSize(size); sets the rootObject needsLayout. friendly ping
esprehn@chromium.org changed reviewers: + pdr@chromium.org - abarth@chromium.org
This lgtm, but it does make me sad because we should have adjusted the size before the first layout so we don't need to do multi pass layout I think. pdr@ should look at this, he's the master of the multi pass layout.
> pdr@ should look at this, he's the master of the multi pass layout. Aaack! Libel! :) This fixes the assert but I would like more convincing that this addresses the underlying problem. Can you describe why zoom requires multiple layouts but changing the size of the html element doesn't? Why does the testcase require multiple zooms instead of just one? We should try to remove two-pass algorithms where possible because they become exponential. Two two-pass algorithms can lead to laying out an element 4 times, three two-pass algorithms can lead to laying out an element 8 times, etc. This one may be required but I'd like to understand why.
On 2014/09/26 04:55:50, pdr wrote: > > pdr@ should look at this, he's the master of the multi pass layout. > Aaack! Libel! :) > > > > This fixes the assert but I would like more convincing that this addresses the > underlying problem. Can you describe why zoom requires multiple layouts but > changing the size of the html element doesn't? Why does the testcase require > multiple zooms instead of just one? > > We should try to remove two-pass algorithms where possible because they become > exponential. Two two-pass algorithms can lead to laying out an element 4 times, > three two-pass algorithms can lead to laying out an element 8 times, etc. This > one may be required but I'd like to understand why. Hi pdr, i have simplified the test case with single zoom instead of multiple. When the document gets zoomed at 2.5 Creation of scrollbar is triggered after adjusting the viewsize which is setting setNormalChildNeedsLayout.The test case was simplified from https://support.google.com/chrome/?p=help&ctx=keyboard#topic=3227046 on which the assert occurs as we zoom-in/out. Thanks,
On 2014/09/26 12:04:12, muven wrote: > On 2014/09/26 04:55:50, pdr wrote: > > > pdr@ should look at this, he's the master of the multi pass layout. > > Aaack! Libel! :) > > > > > > > > This fixes the assert but I would like more convincing that this addresses the > > underlying problem. Can you describe why zoom requires multiple layouts but > > changing the size of the html element doesn't? Why does the testcase require > > multiple zooms instead of just one? > > > > We should try to remove two-pass algorithms where possible because they become > > exponential. Two two-pass algorithms can lead to laying out an element 4 > times, > > three two-pass algorithms can lead to laying out an element 8 times, etc. This > > one may be required but I'd like to understand why. > > Hi pdr, i have simplified the test case with single zoom instead of multiple. > When the document gets zoomed at 2.5 Creation of scrollbar is triggered > after adjusting the viewsize which is setting setNormalChildNeedsLayout.The test > case was simplified from > https://support.google.com/chrome/?p=help&ctx=keyboard#topic=3227046 on which > the > assert occurs as we zoom-in/out. Thanks, sorry, small correction, Hi pdr, i have simplified the test case with single zoom instead of multiple. When the document gets zoomed at 2.5 Creation of scrollbar is triggered "during" adjusting the viewsize which is setting setNormalChildNeedsLayout.The test case was simplified from https://support.google.com/chrome/?p=help&ctx=keyboard#topic=3227046 on which the assert occurs as we zoom-in/out. Thanks,
one more point i want to mention, to confirm the scrollbar creation is setting the needslayout, i tried setting newHasHorizontalScrollbar = newHasVerticalScrollbar = false; after computeScrollbarExistence call in the ScrollView::adjustScrollbarExistence API. with this changes i dont see any assert being raised after adjustviewsize, as vertical scrollbar existence is set to false, vertical scrollbar is not created on overflow.
On 2014/09/26 17:03:20, muven wrote: > one more point i want to mention, to confirm the scrollbar creation is > setting the needslayout, i tried setting > newHasHorizontalScrollbar = newHasVerticalScrollbar = false; after > computeScrollbarExistence call in the ScrollView::adjustScrollbarExistence API. > with this changes i dont see any assert being raised after adjustviewsize, > as vertical scrollbar existence is set to false, vertical scrollbar is > not created on overflow. Can you describe why zoom requires an additional 2-pass layout but changing the size of the html element doesn't? Scrollbars already have a 2-pass layout; I'd like to know why zoom is requiring another.
Actually when the page is loaded overflow scrollbars are created and they are set to disable mode(as contentsHeight() < clientHeight) which prevents them being painted. When i keep on zooming on at certain zoom level it sets enable when contentsHeight is greater than clientHeight. @ ScrollView::updateScrollbarGeometry() ====> m_verticalScrollbar->setEnabled(contentsHeight() > clientHeight); This sets the RootNeeds Layout.
On 2014/09/28 11:49:59, muven wrote:
> Actually when the page is loaded overflow scrollbars are created and they are
> set to disable mode(as contentsHeight() < clientHeight) which prevents them
> being painted. When i keep on zooming on at certain zoom level it sets enable
> when contentsHeight is greater than clientHeight.
>
> @ ScrollView::updateScrollbarGeometry()
> ====> m_verticalScrollbar->setEnabled(contentsHeight() > clientHeight);
>
> This sets the RootNeeds Layout.
Scrollbars already have a 2-pass layout; I'd like to know _why_ zoom is
requiring another.
In your test, if you do the following instead of zooming and open in a browser,
the page does not assert:
setTimeout(function() {
document.body.innerHTML += "<style>ul, div { font-size: 75px; }</style>";
}, 1000);
All zoom is doing in your test is increasing the text size. Why does increasing
the text size with zoom cause the assert to fail but increasing the text size
with css doesn't?
Hi Pdr, After debugging i see the reson why it doesnt asserts during JS call and assests on ZOOM. Durring Zoom the When scrollbar is set to enabled, it triggers RenderScrollbar::updateScrollbarParts in which the thickness of the custom scrollbar after the RenderScrollbarPart Layout, is modified to 3. The new thickness is 3 and the oldthickness is 1, due to which it sets box->setChildNeedsLayout() which is asserting in the frameview. May be i have modified and proposed new patch, after updateScrollbarGeometry() we can have one more layout, Or else kindly share inputs of what can be done. Thanks,
On 2014/10/01 at 09:59:13, sataya.m wrote: > Hi Pdr, > > After debugging i see the reson why it doesnt asserts during JS call > and assests on ZOOM. Durring Zoom the When scrollbar is set to enabled, > it triggers RenderScrollbar::updateScrollbarParts in which the thickness > of the custom scrollbar after the RenderScrollbarPart Layout, is modified > to 3. The new thickness is 3 and the oldthickness is 1, due to which it sets > box->setChildNeedsLayout() which is asserting in the frameview. > > May be i have modified and proposed new patch, after updateScrollbarGeometry() > we can have one more layout, Or else kindly share inputs of what can be done. > > Thanks, I feel like we are getting closer but still not quite there. Based on your description, it is the size of the scrollbar that is changing and not the existence. Therefore, adding a call to scrollbarExistenceDidChange() does not sound correct. Furthermore, you added a call to scrollbarExistenceDidChange() above the "if (scrollbarExistenceChanged)" check which you must have noticed is false... Why is the thickness of the scrollbar changing at all?
Hi pdr, Style of the scrollbar thickness is calculated @ RenderObject::getUncachedPseudoStyle Im pasting some of the logs which calculates the style value of scrollbar thickness. RenderObject.cpp(2735)] getUncachedPseudoStyle result Style width,height 3 * 3 node:BODY RenderScrollbarPart.cpp(97)] calcScrollbarThicknessUsing length:3 isAuto:0 type:2 RenderScrollbarPart.cpp(111)] computeScrollbarWidth width:3 As this width 3 != 1 this sets the node needs layout. Kindly share your inputs. Thanks,
On 2014/10/02 at 12:07:14, sataya.m wrote: > Hi pdr, > > Style of the scrollbar thickness is calculated @ RenderObject::getUncachedPseudoStyle > > Im pasting some of the logs which calculates the style value of scrollbar thickness. > > RenderObject.cpp(2735)] getUncachedPseudoStyle result Style width,height 3 * 3 node:BODY > RenderScrollbarPart.cpp(97)] calcScrollbarThicknessUsing length:3 isAuto:0 type:2 > RenderScrollbarPart.cpp(111)] computeScrollbarWidth width:3 > > As this width 3 != 1 this sets the node needs layout. > > Kindly share your inputs. > > Thanks, A general comment about code reviews: due to unfortunate timezone differences, each round of reviews will take us 24 hours. We're working on ways around the speed of light but, until then, this review will take a long time if it continues in this way. To save your time and mine, I encourage you to question the code--does it make sense for the browser to do this? Try to really understand what the code is doing. For a question like what the width should be in RenderScrollbarPart: the only way I can answer it is to read the code or make a simple test with a scrollbar and check if the width changes when I zoom. Sometimes I check Firefox, Safari, or IE as well, or consult specifications. I instrumented the code and I also see computeScrollbarWidth computing new values after zoom (for me it is 0->16 and not 1->3 though) which does not seem like it should happen. My intuition is that the width should not change under zoom. Why is the thickness of the scrollbar changing at all?
Hi Pdr, I have debugged further and found the root cause. When i try to load the page RenderScrollbar is created with a width specified in the test page of width and height 1px.As i keep on zooming at zoom level(of pagescalefactor = 3) 300% contents height is greater than window height, which is setting the vertical Custom RenderScrollbar enabled. At this point the style of the RenderScrollbar is calculated, which is being multiplied by factor 3. The new width and height is 3 x 3( 1*3 , 1*3).As new thickness != old thickness this scrollbar which is child of body element is setting needsLayout. If we have huge contents which triggers vertical scrollbar enabled on 1.75 (175%) scale-factor then the width and height is (1.75 x 1.75). As this is CustomScrollbar style of the scrollbar is scaled by page-scale factor. kindly recommend the correct way to fix this issue. Thanks,
On 2014/10/10 at 13:53:28, sataya.m wrote: > Hi Pdr, > > I have debugged further and found the root cause. When i try to load > the page RenderScrollbar is created with a width specified in the > test page of width and height 1px.As i keep on zooming at zoom > level(of pagescalefactor = 3) 300% contents height is greater than > window height, which is setting the vertical Custom RenderScrollbar > enabled. At this point the style of the RenderScrollbar is calculated, > which is being multiplied by factor 3. The new width and height is > 3 x 3( 1*3 , 1*3).As new thickness != old thickness this scrollbar which > is child of body element is setting needsLayout. If we have huge contents > which triggers vertical scrollbar enabled on 1.75 (175%) scale-factor then > the width and height is (1.75 x 1.75). As this is CustomScrollbar style of > the scrollbar is scaled by page-scale factor. > > kindly recommend the correct way to fix this issue. > > Thanks, Why is the RenderScrollbar being scaled by the zoom factor (3) if the zoom factor has no effect on it? As I said before: My intuition is that the width should not change under zoom. Why is the thickness of the scrollbar changing at all?
solidcolorscrollbar or paintedscrollbarlayer is not being scaled by zoom factor, instead the VisibleToTotalLengthRatio will be changed. But in the case of custom scrollbar it is created as part of document and the renderobject of renderscrollbar is scaled by zoom factor. Please see the example http://css-tricks.com/examples/WebKitScrollbars/ where you see the RED color scrollbar is being scaled when we keep on zooming.
On 2014/10/11 at 19:42:43, sataya.m wrote: > solidcolorscrollbar or paintedscrollbarlayer is not being scaled by zoom factor, > instead the VisibleToTotalLengthRatio will be changed. But in the case of > custom scrollbar it is created as part of document and the renderobject of > renderscrollbar is scaled by zoom factor. > > Please see the example http://css-tricks.com/examples/WebKitScrollbars/ > where you see the RED color scrollbar is being scaled when we keep on zooming. I didn't realize custom scrollbars depended on the zoom level. Are two layouts actually needed or is a layout just needed for the scrollbar parts? I see the scrollbar width changing before performLayout is called. What actually needs layout after perform layout is called, and could we have laid it out during layout instead of after layout? I don't think this is possible to fix without deeply understanding how custom scrollbars work.
As per below statement
if (RenderBox* box = owningRenderer())
box->setChildNeedsLayout();
owningRender(body) needs Layout.
https://support.google.com/chrome/?p=help&ctx=keyboard#topic=3227046
this link has a custom-scrollbar. upon zooming to 50% pageHeight < windowHeight
which sets the scrollbar to be disabled and the thickness changes from 15 to 0,
and requires body layout.(at this instance also it crashes).Again if you try
to increase zoom level at one point scrollbar is enabled so thickness changes
from 0 to 15 (at this point also it crashes). Normally in the release build
as we dont see the crash, and the body is not layouted not able to see the
scrollbar with 15 units thickness. As scrollview is merged to frameview
may be can we have a flag on thickness change, based on the flag
scrollbarExistenceDidChange() can be invoked which does the layout ?
Patchset #6 (id:210001) has been deleted
Patchset #6 (id:230001) has been deleted
PTAL on the changes, If the framerect of the scrollbar is changed, then only layout is been called.
https://codereview.chromium.org/503583002/diff/250001/Source/core/frame/Frame... File Source/core/frame/FrameView.cpp (right): https://codereview.chromium.org/503583002/diff/250001/Source/core/frame/Frame... Source/core/frame/FrameView.cpp:3453: bool scrollbarExistenceChanged = false; Is the scrollbar existence changing, or just the size? https://codereview.chromium.org/503583002/diff/250001/Source/core/frame/Frame... Source/core/frame/FrameView.cpp:3465: oldRect = m_horizontalScrollbar->frameRect(); Why do you need to store the oldRect here instead of using the value from above? https://codereview.chromium.org/503583002/diff/250001/Source/core/frame/Frame... Source/core/frame/FrameView.cpp:3496: if (oldRect != m_verticalScrollbar->frameRect()) I don't understand the values we're seeing here. On my machine, I see the following: zoom at 100%: height is 275px zoom at 125%: height is 275px zoom at 150%: height is 275px zoom at 175%: height is 275px zoom at 200%: height is 275px zoom at 250%: height is 273px zoom at 300%: height is 273px Something doesn't seem right here.
pdr@chromium.org changed reviewers: - esprehn@chromium.org, pdr@chromium.org
I'm afraid I no longer have the bandwidth to continue this review.
Im sorry pdr, Initially when i started this i had only minimum knowldge in this area. after your repeated question "why ?" has pushed me beyond my boundaries and helped me learning something which is important. I didnt reply as i want to understand this area completely and upload solid patch which solves the issue ideal way.
not lgtm, removing my bit since this patch isn't good to land for now. If you do figure it out please upload again and I'll take a look.
esprehn@chromium.org changed reviewers: + esprehn@chromium.org
On 2014/10/16 at 05:55:07, sataya.m wrote: > Im sorry pdr, Initially when i started this i had only minimum knowldge in this area. > after your repeated question "why ?" has pushed me beyond my boundaries and helped me > learning something which is important. I didnt reply as i want to understand this area > completely and upload solid patch which solves the issue ideal way. You learned a lot with this patch and I hope you'll continue working on it! My review load is currently too high and I have to prioritize more critical reviews. rob.buis@samsung may be a good reviewer for this patch.
Patchset #7 (id:270001) has been deleted
sataya.m@samsung.com changed reviewers: + rob.buis@samsung.com
Hi Rob, PTAL. Custom Scrollbar(RenderScrollbar.cpp) is enabled upon zooming 300% and the style of the scrollbar is scaled by zoom factor(*3 for 300% zoom). Which modifies the thickness of the scrollbar to style * 3. When the thickness of the scrollbar is changed new framerect is set for the scrollbar and the owning render of scrollbar is set for setchildneedslayout. This entire calls are being called in adjustviewsize API in Frameview after rootLayout is being done. As box->setChildNeedsLayout(); is set @ RenderScrollbar::updateScrollbarParts this call is causing the assert ASSERT(!rootForThisLayout->needsLayout()); in frameview layout. For this issue im proposing a solution that if the thickness of the scrollbar changes then scrollbarExistenceDidChange() is called which skips the assert.
sataya.m@samsung.com changed reviewers: + pdr@chromium.org
Hi All, Im happy to find the root cause of the issue which led me to fixed 2 pri-1 issues related to custom scrollbar which is being used by google websites. CustomScrollbar is part of the page and the scale factor is being applied to the scrollbar when the page is zoomed. ex:http://css-tricks.com/examples/WebKitScrollbars/ Here are list of issue associated with custom-scrollbars. 390895:Vertical scrollbar in Google Keep is not restored to default size. 406676:ASSERT on Zoom-IN/OUT 375200:Scroll bar is missing on the page after restoring it to default zoom level. RootCause for Issue 406676: ASSERT on Zoom-IN/OUT; This issue when we try to Zoom-in/out of the page scale factor is being applied to the scrollbar style properties. If the vertical scrollbar is having css style of width:10px, when we zoom the page, "width property" which is used to calculating thumb thickness of vertical scrollbar becomes (10px * 3) 30px (on zooming to 300%). When there is a change of thickness value we set owningRenderer->setchildneedslayout(). The thickness value calculations are made once the root layout is completed and adjustviewsize is invoked after rootlayout, which invokes updatescrollbars which trigers verticalscrollbar thickness calculations. As the vertical scrollbar owningRenderer is setneedschildlayout due to this ASSERT(!rootForThisLayout->needsLayout()); call is invoked. Fix for this is on changing the thickness layout is invoked via scrollbarExistenceDidChange(); RootCause for Issue 375200: Scroll bar is missing on the page after restoring it to default zoom level. Upon zooming to 33% scrollbar is disabled.When returned to default zoom scrollbar is enabled and the thickness of the scrollbar is changed.When thickness of the scrollbar is changed this needs a layout. As the layout is not triggered we are not able to see scrollbar. RootCause for Issue 390895: Vertical scrollbar in Google Keep is not restored to default size. In this case also this needs layout of the scrollbar. But along with layout on every page scale scrollbar needs updateScrollbarParts irrespective of scrollbar enabled or disable.As custom scrollbars thickness changes with page scale every time on zooming the page thickness of the scrollbar is changed. Currently we are restricting this change based on below condition. if (wasEnabled != e) updateScrollbarParts(); We also see a inconsistent scrollbar behvaiour in this sites.When we load page scrollbar has 16px upon zooming to 25% scrollbar is disabled, now when zooming to 500% you see a scrollbar of very minute thickness. These inconsistent behaviour is fixed now. Special thanks to pdr for pushing me beyond boundaries to find the real root cause and fix issues related to custom-scrollbar completely. PTAL. Thanks, MuVen.
Patchset #9 (id:330001) has been deleted
Is this CL related to this problem? https://codereview.chromium.org/653423003
I tried on the TOT with https://codereview.chromium.org/653423003 CL. The above mentioned issues are still present.May be these CL is not related this problem.
Patchset #11 (id:390001) has been deleted
Patchset #11 (id:410001) has been deleted
sataya.m@samsung.com changed reviewers: - pdr@chromium.org
sataya.m@samsung.com changed reviewers: - esprehn@chromium.org
sataya.m@samsung.com changed reviewers: + esprehn@chromium.org
sataya.m@samsung.com changed required reviewers: + rob.buis@samsung.com
Hi Rob, PTAL. Thanks, MuVen
On 2014/10/27 12:25:51, muven wrote: > Hi Rob, > > PTAL. > > Thanks, > MuVen Are you sure that the fast/repaint/line-flow-with-floats-9.html regression is caused by your patch? If so can you explain it? Why only a windows change in behavior?
Patchset #14 (id:490001) has been deleted
Patchset #12 (id:450001) has been deleted
Patchset #13 (id:510001) has been deleted
Patchset #12 (id:470001) has been deleted
Patchset #13 (id:550001) has been deleted
Hi Rob, Are you sure that the fast/repaint/line-flow-with-floats-9.html regression is caused by your patch? => Yes. Scrollbars needs invalidation before setting the proportion,enable/disable if the framerect has been changed. I have uploaded a patch#12 which invalidates/layouts customscrollbar on change in thickness. As in CustomScrollbar when setenable is called framerect of the scrollbar is changed, due to which im preserving the oldRect value again. This solves the issue.PTAL.
Patch looks ok to me, but I am not an expert in this area. I suggest esprehn looks at it since he reviewed before. https://codereview.chromium.org/503583002/diff/530001/Source/core/frame/Frame... File Source/core/frame/FrameView.cpp (right): https://codereview.chromium.org/503583002/diff/530001/Source/core/frame/Frame... Source/core/frame/FrameView.cpp:3354: } I see a repeated code pattern, meaning a shared function could be made, but it probably is not needed for this fix...
sataya.m@samsung.com changed required reviewers: + esprehn@chromium.org - rob.buis@samsung.com
On 2014/10/28 14:59:03, rwlbuis wrote: > Patch looks ok to me, but I am not an expert in this area. I suggest esprehn > looks at it since he reviewed before. > > https://codereview.chromium.org/503583002/diff/530001/Source/core/frame/Frame... > File Source/core/frame/FrameView.cpp (right): > > https://codereview.chromium.org/503583002/diff/530001/Source/core/frame/Frame... > Source/core/frame/FrameView.cpp:3354: } > I see a repeated code pattern, meaning a shared function could be made, but it > probably is not needed for this fix... Thanks for the review rob, Esprehn, PTAL.
sataya.m@samsung.com changed reviewers: - rob.buis@samsung.com
sataya.m@samsung.com changed required reviewers: - esprehn@chromium.org
https://codereview.chromium.org/503583002/diff/530001/Source/core/frame/Frame... File Source/core/frame/FrameView.cpp (right): https://codereview.chromium.org/503583002/diff/530001/Source/core/frame/Frame... Source/core/frame/FrameView.cpp:3319: scrollbarExistenceDidChange(); What behavior are you trying to trigger here? The existence didn't change when the height changes, this doesn't seem like the right call.
https://codereview.chromium.org/503583002/diff/530001/Source/core/frame/Frame... File Source/core/frame/FrameView.cpp (right): https://codereview.chromium.org/503583002/diff/530001/Source/core/frame/Frame... Source/core/frame/FrameView.cpp:3319: scrollbarExistenceDidChange(); Upon zooming in, customScrollbar style scales by scaleFactor. When SetEnabled is called, updateScrollbarParts will be called with in the SetEnabled function, which triggers RenderScrollbar::updateScrollbarPart. In this API RenderStyle of the scrollbar is calculated (which is multiplied by scale factor) and the style is set to the RenderScrollbar at the end of the API.When you move back to updateScrollbarParts, it checks for the scrollbar thickness, if the thickness is changed it sets the box->setChildNeedsLayout(); scrollbarExistenceDidChange(will be invoked if thickness of the scrollbar is changed) call will relayout if box->setChildNeedsLayout is set. If Layout is not triggered, Assert ASSERT(!rootForThisLayout->needsLayout()); will be invoked in FrameView::layout function.
On 2014/10/29 09:39:59, muven wrote: > https://codereview.chromium.org/503583002/diff/530001/Source/core/frame/Frame... > File Source/core/frame/FrameView.cpp (right): > > https://codereview.chromium.org/503583002/diff/530001/Source/core/frame/Frame... > Source/core/frame/FrameView.cpp:3319: scrollbarExistenceDidChange(); > Upon zooming in, customScrollbar style scales by scaleFactor. When > SetEnabled is called, updateScrollbarParts will be called with in the SetEnabled > function, which triggers RenderScrollbar::updateScrollbarPart. In this API > RenderStyle of the scrollbar is calculated (which is multiplied by scale factor) > and the style is set to the RenderScrollbar at the end of the API.When you move > back to updateScrollbarParts, it checks for the scrollbar thickness, if the > thickness is changed it sets the > box->setChildNeedsLayout(); scrollbarExistenceDidChange(will be invoked if > thickness of the scrollbar is changed) call will relayout if > box->setChildNeedsLayout is set. If Layout is not triggered, Assert > ASSERT(!rootForThisLayout->needsLayout()); will be invoked in FrameView::layout > function.
Hi Esprehn, I have uploaded a screencast of how these custom scrollbars are behaving inconsistently and why this needs layout @ crbug.com/390895. PTAL. Thanks
I think you're misunderstanding here, the scrollbar existence didn't change. It just changed size, what function should you be calling? I understand the need for layout, but it seems like you either need to rename scrollbarExistenceChanged() or use a different function.
esprehn, as u have suggested i have renamed scrollbarExistenceChanged(), PTAL. Thanks
Closing this review as https://codereview.chromium.org/702913002/ is fixing this issue(started with Updating custom scrollbar style issue, but that provoked to fix this issue without a relayout for scrollbar). |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
