Bug 71541 – Unnecessary scroll bar after changing the dimensions of a DIV

• Component: Layout and Rendering
• Status: REOPENED

[satya]

Created attachment 113623 [details]
Unnecessary scroll bar while changing the dimensions of a DIV

Unnecessary scroll bar while changing the dimensions of a DIV.


What steps will reproduce the problem?
1. Have two divs one inside another. Position inner DIV absolutely with top and left "0px" and have more width and height to have the scroll.
2. Now set dimensions on outer DIV dynamically to avoid the scroll with the same width and height as inner DIV
3. Still we get scroll, even though it is not required

4. Use the attached test page to reproduce the issue.

What is the expected result?
Scroll should not be there

What happens instead?
Extra scrolls are coming

[Simon Fraser (smfr)]

Did this regress at some point?

[SravanKumar S(:sravan)]

Hi Simon,

I have no idea about this regressing, but, The latest understandings from debugging are when trying to calculate pixelSnappedClientWidth and pixelSnappedClientHeight in RenderLayer::computeScrollDimensions(bool* needHBar, bool* needVBar), they were taking previous scrollbar's width and heights, as they depend on clientWidth() and clientHeight(), which again depend on verticalScrollbarWidth() and horizontalScrollbarHeight().

I think verticalScrollbarWidth and horizontalScrollbarHeight, should be more smart and hence i am thinking to replace the logic with following conditions.

int RenderLayer::verticalScrollbarWidth(OverlayScrollbarSizeRelevancy relevancy) const
{
    RenderBox* box = renderBox();
    if (!m_vBar || (renderer()->style()->overflowX() == OAUTO && box->width() >= m_scrollSize.width())
        || (m_vBar->isOverlayScrollbar() && relevancy == IgnoreOverlayScrollbarSize))
        return 0;
    return m_vBar->width();
}

int RenderLayer::horizontalScrollbarHeight(OverlayScrollbarSizeRelevancy relevancy) const
{
    RenderBox* box = renderBox();
    if (!m_hBar || (renderer()->style()->overflowY() == OAUTO && box->height() >= m_scrollSize.height())
        || (m_hBar->isOverlayScrollbar() && relevancy == IgnoreOverlayScrollbarSize))
        return 0;
    return m_hBar->height();
}

This is perfectly fixing the issue, and i dont see any regressions on all the overflow tests.

But i would like to know your comments, before i submit it as a patch, as you are suspecting regression on this.

[SravanKumar S(:sravan)]

This looks like a regression from https://bugs.webkit.org/show_bug.cgi?id=51148

Following comments from Mark indicates its a regression.
"Description From Mark Kristensson 2010-12-15 15:56:31 PST (-) [reply]
This is relatively new bug that has cropped up in our application on Safari and Chrome in the last month. I've taken the very complex JS and HTML from our app and reproduced the bug in the attached html page (with some embedded JS). The basic scenario is this:"

but, did not mention from when exactly. I am suspecting it could have been from http://nightly.webkit.org/builds/trunk/mac/27

[Julien Chaffraix]

> but, did not mention from when exactly. I am suspecting it could have been from http://nightly.webkit.org/builds/trunk/mac/27

We usually prefer a tested regression range or the changeset that made us regress. Btw, this means this bug is a duplicate of bug 51148.

> if (!m_vBar || (renderer()->style()->overflowX() == OAUTO && box->width() >= m_scrollSize.width())

As mentioned on IRC, this is not the right fix. The issue is that the scrollbars are not destroyed as expected, hiding them is just working around this fact.

[SravanKumar S(:sravan)]

Created attachment 134506 [details]
SnapShot_H

SnapShot for Horizontal Scrollbar Destruction as part of updateScrollInfoAfterLayout().

[SravanKumar S(:sravan)]

Created attachment 134507 [details]
SnapShot_V

SnapShot for Vertical Scrollbar Destruction as part of updateScrollInfoAfterLayout().

[SravanKumar S(:sravan)]

Julien, thanks for the comments, I just wanted to make sure that we both are in sync on what you are assuming, and what i am implementing.

I have attached snapshots to show the scrollbar destruction as part of my fix mentioned.

[Julien Chaffraix]

(In reply to comment #7)
> Julien, thanks for the comments, I just wanted to make sure that we both are in sync on what you are assuming, and what i am implementing.
> 
> I have attached snapshots to show the scrollbar destruction as part of my fix mentioned.

Please avoid to do screenshots of your editor, 3 lines of text would have been way more efficient. I still think you are lucky that the logic is helping you here as what you are doing is really just hiding the scrollbar (ie faking not having one).

RenderLayer::updateScrollInfoAfterLayout has some cases where we want to make sure we have no scrollbars. It looks like this is where you should patch the code. You need also to understand *why* we still have them to solve the root of the problem.

[SravanKumar S(:sravan)]

Sorry for the patches, will keep it in mind.
Now, i am not touching widths/heights but i am directly altering overflows as following in RenderLayer::updateScrollInfoAfterLayout only in auto mode.

     // overflow:auto may need to lay out again if scrollbars got added/removed.

-    bool scrollbarsChanged = (box->hasAutoHorizontalScrollbar() && haveHorizont
alBar != horizontalOverflow) ||
-                             (box->hasAutoVerticalScrollbar() && haveVerticalBa
r != verticalOverflow);
+    bool scrollbarsChanged = (box->hasAutoHorizontalScrollbar() && haveHorizont
alBar != (horizontalOverflow = (box->width() < m_scrollSize.width()))) ||
+                             (box->hasAutoVerticalScrollbar() && haveVerticalBa
r != (verticalOverflow = (box->height() < m_scrollSize.height())));
     if (scrollbarsChanged) {
-        if (box->hasAutoHorizontalScrollbar())
+               if (box->hasAutoHorizontalScrollbar())
             setHasHorizontalScrollbar(horizontalOverflow);
         if (box->hasAutoVerticalScrollbar())
             setHasVerticalScrollbar(verticalOverflow);

Please comment if you see something wrong this fix?

[SravanKumar S(:sravan)]

*** Bug 51148 has been marked as a duplicate of this bug. ***

[SravanKumar S(:sravan)]

Created attachment 134874 [details]
Patch

[WebKit Review Bot]

Comment on attachment 134874 [details]
Patch

Attachment 134874 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12291441

New failing tests:
fast/overflow/hidden-scrollbar-resize.html
scrollbars/scrollbars-on-positioned-content.html

[WebKit Review Bot]

Created attachment 134886 [details]
Archive of layout-test-results from ec2-cr-linux-04

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-04  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[Darin Adler]

Comment on attachment 134874 [details]
Patch

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

> Source/WebCore/rendering/RenderLayer.cpp:2322
> +    if (box->hasAutoHorizontalScrollbar())
> +        *needHBar = box->pixelSnappedWidth() < m_scrollSize.width();
> +    if (box->hasAutoVerticalScrollbar())
> +        *needVBar = box->pixelSnappedHeight() < m_scrollSize.height();

needHBar and needVBar can be 0; this code needs a null check like the code above it

It’s not good to do two different needHBar and needVBar computations. These should be combined with the others above in "if/else" constructions.

There is no good reason to use m_scrollSize.width() when the existing code uses scrollWidth(). That kind of unnecessary difference can easily cause confusion. Similarly, there is no reason to reverse the order of the comparison, putting the scroll width on the right side instead of the left side above.

Finally, this is the kind of code that needs a comment. What is it about automatic scrollbars that causes you to need to use “width” rather than “client width”? This is the kind of “why” issue that needs to be covered by comments.

[SravanKumar S(:sravan)]

Darin, Thanks for the review comments.

Please let me know if following changes makes this fix look better.

    // In automatic scrollbar case if resizing of box takes place such that
    // box width is equal or greater than content width, then existing scroll bars if any should be destroyed.
    // Ideally clientWidth should be used to compare to scrollWidth to decide overflow, but in this case 
    // clientWidth computed already has verticalScrollbar's width, hence it cant be used.
    // If possible we should think of fixing clientWidth/Height in this special case, with out hiding scrollbars.
    if (needHBar && box->hasAutoHorizontalScrollbar())
        *needHBar = scrollWidth() > box->pixelSnappedWidth();
    if (needVBar && box->hasAutoVerticalScrollbar())
        *needVBar = scrollHeight() > box->pixelSnappedHeight();

[SravanKumar S(:sravan)]

More accurate comments and code.

    // In overflow: auto case if resizing of box takes place such that
    // box width/height is equal or greater than content width/height, then existing scroll bars if any should be destroyed.
    // Ideally clientWidth/Height should be used to compare to scrollWidth/Height to decide overflow, but in this case 
    // clientWidth/Height computed already has scrollbar's height/width subtracted from it, hence box width/height after
    // accounting for borders is taken  to decide overflow.
    if (needHBar && box->hasAutoHorizontalScrollbar())
        *needHBar = scrollWidth() > box->pixelSnappedWidth() - box->borderLeft() - box->borderRight();
    if (needVBar && box->hasAutoVerticalScrollbar())
        *needVBar = scrollHeight() > box->pixelSnappedHeight() - box->borderTop() - box->borderBottom();

Please let me know if this is ok for patch.?

[SravanKumar S(:sravan)]

Created attachment 134962 [details]
patch

Updated patch with darin comments, and checking for borders also for setting overflow.

[WebKit Review Bot]

Comment on attachment 134962 [details]
patch

Attachment 134962 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12291814

New failing tests:
fast/overflow/hidden-scrollbar-resize.html
scrollbars/scrollbars-on-positioned-content.html

[WebKit Review Bot]

Created attachment 134965 [details]
Archive of layout-test-results from ec2-cr-linux-02

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-02  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[Julien Chaffraix]

Comment on attachment 134962 [details]
patch

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

> Source/WebCore/rendering/RenderLayer.cpp:2323
> +    // In overflow: auto case if resizing of box takes place such that
> +    // box width/height is equal or greater than content width/height, then existing scroll bars if any should be destroyed.
> +    // Ideally clientWidth/Height should be used to compare to scrollWidth/Height to decide overflow, but in this case 
> +    // clientWidth/Height computed already has scrollbar's height/width subtracted from it, hence box width/height after
> +    // accounting for borders is taken to decide overflow.

This comment is way too verbose and doesn't go to the point. Only overflow: scroll scrollbars actually do need to check the client box as we always remove the scrollbars' size from the content. For other values, it either doesn't matter (overflow: hidden / visible) or the padding box should be used as the scrollbar depends on the actual content.

> Source/WebCore/rendering/RenderLayer.cpp:2324
> +    if (needHBar && box->hasAutoHorizontalScrollbar())

This should be merged into the previous ifs. You could pick the right box by looking at the overflow property:

// Overflow: scroll always uses the client box as we always display the scrollbar and we disable it based on the available area.
// Other value of overflow uses the padding box.
int widthForHorizontalScrollbar = box->style()->overflow() == OSCROLL ? box->pixelSnappedClientWidth() : box->pixelSnappedPaddingBoxWidth();
*needHBar = scrollWidth() > widthForHorizontalScrollbar;

The whole logic is really made more complicated that it should by the fact that we mix together 2 code paths that have slightly different expectations (overflow scroll vs the rest).

> Source/WebCore/rendering/RenderLayer.cpp:2327
> +        *needHBar = scrollWidth() > box->pixelSnappedWidth() - box->borderLeft() - box->borderRight();
> +    if (needVBar && box->hasAutoVerticalScrollbar())
> +        *needVBar = scrollHeight() > box->pixelSnappedHeight() - box->borderTop() - box->borderBottom();

Looks like you need to expose your snapped padding box's width and height in RenderBox.

> LayoutTests/fast/overflow/overflow-auto-destroy-scroll-after-resizing-expected.html:8
> +        <button onclick=changeDim()>Change Red DIV width and Height to 100 and 100</button>

This button is unneeded and harmful as changeDim() doesn't exist.

> LayoutTests/fast/overflow/overflow-auto-destroy-scroll-after-resizing.html:8
> +                setTimeout(increaseOuterDiv, 0);

Why do we need a setTimeout here? It doesn't strike me as needed.

[SravanKumar S(:sravan)]

Hi Julien,

sorry for the delay, i was on something else till today. Looks like RenderLayer.cpp has changed a little bit. Got few doubts in your review comments.

1. "// Overflow: scroll always uses the client box as we always display the scrollbar and we disable it based on the available area.
// Other value of overflow uses the padding box.
int widthForHorizontalScrollbar = box->style()->overflow() == OSCROLL ? box->pixelSnappedClientWidth() : box->pixelSnappedPaddingBoxWidth();
*needHBar = scrollWidth() > widthForHorizontalScrollbar;"

I think with "pixelSnappedPaddingBoxWidth()" you were mentioning about actual content + padding widths", if so then i would like to rename it as pixelSnappedPaddingPlusContentWidth. And, we cant use contentWidth() in that function as contentWidth() is calculated from clientWidth() which after style change also excludes verticalScrollBarWidth(). 

So, again i would like to calculate it as following in RenderLayer.cpp and keep it local to bool RenderLayer::hasVerticalOverflow() const
LayoutUnit pixelSnappedPaddingPlusContentWidth = renderBox()->pixelSnappedWidth() - renderBox()->borderLeft() - renderBox()->borderRight();

2. This button is unneeded and harmful as changeDim() doesn't exist.

I retained this button in -expected.html as to match the output of overflow-auto-destroy-scroll-after-resizing.html

3.Why do we need a setTimeout here? It doesn't strike me as needed.

I am doing this because, we need to change dimensions through javascript, and by setting it to 0, i am automating it, so that we can compare its output with -expected.html output.

[SravanKumar S(:sravan)]

I think as Javascript is there to take care of updating widths and heights, we dont need button. sorry, i will remove the button.

[SravanKumar S(:sravan)]

Created attachment 135987 [details]
Patch

[WebKit Review Bot]

Attachment 135987 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast..." exit_code: 1
LayoutTests/platform/chromium/test_expectations.txt:3220:  Path does not exist. scrollbars/scrollbars-on-positioned-content.htmL  [test/expectations] [5]
Total errors found: 1 in 6 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[SravanKumar S(:sravan)]

Created attachment 135989 [details]
Patch

[Julien Chaffraix]

Comment on attachment 135989 [details]
Patch

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

> Source/WebCore/ChangeLog:9
> +        overflow in auto mode was not properly calculated before in RenderLayer. Now it is taken care
> +        in RenderLayer::hasVerticalOverflow and RenderLayer::hasHorizontalOverflow.

Your ChangeLog is pretty blurry on the details here. You are should merely describing what you did and we prefer you to say 'why' you did it. For example, why was the previous code was wrong? How did you change correct that?

Also in a couple of lines, how would you *precisely* describe your change? (those are guidelines of how to write good ChangeLogs, look at other people's to see some live examples).

> Source/WebCore/rendering/RenderLayer.cpp:2418
> +    LayoutUnit pixelSnappedPaddingBoxWidth = renderBox()->pixelSnappedWidth() - renderBox()->borderLeft() - renderBox()->borderRight();
> +    int widthForHorizontalScrollbar = renderBox()->style()->overflowX() == OSCROLL ? renderBox()->pixelSnappedClientWidth() : pixelSnappedPaddingBoxWidth;

This is really not super readable but we are getting there. It also involves computing |pixelSnappedPaddingBoxWidth| even if we don't use it.

I advised you to add new function on RenderBox for those reasons. As for the naming, padding box has a meaning in CSS and it's what you want (see http://www.w3.org/TR/CSS2/box.html). Your naming doesn't strike me as more precise or better for that matter.

> Source/WebCore/rendering/RenderLayer.cpp:2425
> +    LayoutUnit pixelSnappedPaddingBoxHeight = renderBox()->pixelSnappedHeight() - renderBox()->borderTop() - renderBox()->borderBottom();

Note that this should be an |int| not a LayoutUnit.

> LayoutTests/ChangeLog:12
> +        * platform/chromium/test_expectations.txt:

You need to explain why you disable and why this is a progression here.

Also you need to disable the tests on more platforms as they will need the same rebaseline. Chromium uses test_expectations.txt and other platforms Skipped.

> LayoutTests/fast/overflow/overflow-auto-destroy-scroll-after-resizing.html:9
> +            window.onload = function() {
> +                setTimeout(increaseOuterDiv, 0);
> +            }

I understand the need to remove the scrollbars via JavaScript but I still don't see *why* we need to use setTimeout vs just a plain:

window.addEventListener("load", increaseOuterDiv, false);

Using a load event listener would also remove the need of using waitUntilDone / notifyDone.

> LayoutTests/platform/chromium/test_expectations.txt:3245
> +BUGWK71541 : fast/overflow/hidden-scrollbar-resize.html = FAIL
> +BUGWK71541 : scrollbars/scrollbars-on-positioned-content.html = FAIL

We usually don't use FAIL as it's a blunt hammer. Here the first one is failing with a TEXT difference, the second one IMAGE+TEXT. You can actually rebaseline them on Chromium linux but that would make your entry more complex so it's not mandatory.

[SravanKumar S(:sravan)]

(In reply to comment #26)
> This is really not super readable but we are getting there. It also involves computing |pixelSnappedPaddingBoxWidth| even if we don't use it.
> 
> I advised you to add new function on RenderBox for those reasons. As for the naming, padding box has a meaning in CSS and it's what you want (see http://www.w3.org/TR/CSS2/box.html). Your naming doesn't strike me as more precise or better for that matter.
> 
Please correct me if i my understanding of padding box is incorrect, as i am not able to make out much difference between client box and padding box?, 

client box(clientWidth and clientHeight) is implemented to represent the interior of an object excluding border and scroll bar. Which should have been nothing but content box + padding(which is what i think you and http://www.w3.org/TR/CSS2/box.html are mentioning as padding box).

And following is the reason that you are asking me to write new PaddingBox functions.
But as content box implementation is using client box which excludes scrolls even after re-styling such that with new measurements we don't need a scroll bar and hence does not meet our needs. Hence i think you are asking me to write a separate function that returns actual content + padding measurements. This way it will be independent of previous scrolls. This wont introduce any bugs as scroll bars if any should be placed between borders and padding.


> > LayoutTests/ChangeLog:12
> > +        * platform/chromium/test_expectations.txt:
> 
> You need to explain why you disable and why this is a progression here.

Sure, these tests were also having redundant scrollbars, which is not conflicting with their test pass criterion.

> Also you need to disable the tests on more platforms as they will need the same rebaseline. Chromium uses test_expectations.txt and other platforms Skipped.
> 
> > LayoutTests/fast/overflow/overflow-auto-destroy-scroll-after-resizing.html:9
> > +            window.onload = function() {
> > +                setTimeout(increaseOuterDiv, 0);
> > +            }
> 
> I understand the need to remove the scrollbars via JavaScript but I still don't see *why* we need to use setTimeout vs just a plain:
> 
> window.addEventListener("load", increaseOuterDiv, false);
> 
> Using a load event listener would also remove the need of using waitUntilDone / notifyDone.
> 
> > LayoutTests/platform/chromium/test_expectations.txt:3245
> > +BUGWK71541 : fast/overflow/hidden-scrollbar-resize.html = FAIL
> > +BUGWK71541 : scrollbars/scrollbars-on-positioned-content.html = FAIL
> 
> We usually don't use FAIL as it's a blunt hammer. Here the first one is failing with a TEXT difference, the second one IMAGE+TEXT. You can actually rebaseline them on Chromium linux but that would make your entry more complex so it's not mandatory.

Thanks for the above detailed information.

[SravanKumar S(:sravan)]

Created attachment 136125 [details]
Patch

[SravanKumar S(:sravan)]

Created attachment 136126 [details]
Patch

[SravanKumar S(:sravan)]

Created attachment 136127 [details]
Patch

Forgot to tick the Patch check-box :( .

[WebKit Review Bot]

Attachment 136127 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files']" exit_code: 1
Total errors found: 0 in 0 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Julien Chaffraix]

Comment on attachment 136127 [details]
Patch

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

I would like to see a revised version but the change looks great now. The style bot looked like it was sick when you uploaded your patch.

> Source/WebCore/ChangeLog:8
> +        overflow: auto was being determined based on clientWidth and clientHeight, which during re-styling

We prefer to talk about style change or style application / selection and not re-styling.

> Source/WebCore/ChangeLog:9
> +        scenarios will blindly exclude previous scrollbars while computing overflow after re-styling.

Technically, this makes sense for overflow: scroll as it is used to determine if we should enable / disable our scrollbars. For other values of overflow, this is wrong as it wrongly reduce the available area. It would be nice to speak about this difference (overflow: scroll vs auto / overlay).

> Source/WebCore/rendering/RenderBox.cpp:516
> +LayoutUnit RenderBox::paddingBoxWidth() const
> +{
> +    return width() - borderLeft() - borderRight();
> +}
> +
> +LayoutUnit RenderBox::paddingBoxHeight() const
> +{
> +    return height() - borderTop() - borderBottom();
> +}
> +
> +int RenderBox::pixelSnappedPaddingBoxWidth() const
> +{
> +    return snapSizeToPixel(paddingBoxWidth(), paddingBoxLeft());
> +}
> +
> +int RenderBox::pixelSnappedPaddingBoxHeight() const
> +{
> +    return snapSizeToPixel(paddingBoxHeight(), paddingBoxTop());
> +}

I think those should be inlined into the header as much as possible.

> Source/WebCore/rendering/RenderBox.h:216
> +    LayoutUnit paddingBoxLeft() const { return paddingLeft(); }
> +    LayoutUnit paddingBoxTop() const { return paddingTop(); }

I don't think you need to expose those 2.

[SravanKumar S(:sravan)]

Created attachment 136181 [details]
Patch

Thanks for the comments Julien, updated patch as per requirements.

[SravanKumar S(:sravan)]

Created attachment 136185 [details]
Patch

[WebKit Review Bot]

Attachment 136185 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/ChangeLog', u'LayoutTests/fast..." exit_code: 1
LayoutTests/platform/chromium/test_expectations.txt:3158:  Path does not exist. scrollbars/scrollbars-on-positioned-content.htmL  [test/expectations] [5]
Total errors found: 1 in 11 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[SravanKumar S(:sravan)]

Created attachment 136186 [details]
Patch

Fixed htmL to html.

[Julien Chaffraix]

Comment on attachment 136186 [details]
Patch

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

> Source/WebCore/ChangeLog:9
> +        or style application / selection scenarios will blindly exclude previous scrollbars while computing overflow.

just keep during style change, style application / selection are synonyms here :)

> Source/WebCore/rendering/RenderBox.h:218
> +    int pixelSnappedPaddingBoxWidth() const { return snapSizeToPixel(paddingBoxWidth(), paddingLeft()); }
> +    int pixelSnappedPaddingBoxHeight() const { return snapSizeToPixel(paddingBoxHeight(), paddingTop()); }

After checking with leviw, those offsets should be x() + borderLeft() and y() + borderTop(). The existing code follows the same pattern but is wrong.

> LayoutTests/fast/overflow/overflow-auto-destroy-scroll-after-resizing.html:12
> +                // There should be no scrollbars now
> +                if (window.layoutTestController)
> +                    layoutTestController.notifyDone();

This should be removed.

[Julien Chaffraix]

Created attachment 136248 [details]
Patch for landing

[Julien Chaffraix]

I just went ahead and fixed the small nits I mentioned while I was still in the zone to remove another of review.

[WebKit Review Bot]

Comment on attachment 136248 [details]
Patch for landing

Clearing flags on attachment: 136248

Committed r113611: <http://trac.webkit.org/changeset/113611>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[James Simonsen]

In the future, please don't disable tests on chromium. Just let them fail and the webkit gardener can take care of it. If you want to let them know ahead of time, you can find the name of the gardener on build.chromium.org and notify them on IRC. Thanks.

[Julien Chaffraix]

(In reply to comment #42)
> In the future, please don't disable tests on chromium.

For the record, there is no consensus on that at the moment and I strongly disagree with this proposal as goes against the effort to keep the WebKit bots green. See the on-going discussion on webkit-dev about that: https://lists.webkit.org/pipermail/webkit-dev/2012-April/020213.html

[SravanKumar S(:sravan)]

*** Bug 26438 has been marked as a duplicate of this bug. ***

[satya]

Thanks.

In which version of the Chrome, this solution will be available?

[SravanKumar S(:sravan)]

If you have a svn webkit open source code base, release r113611 and above will have these changes.

If you are asking about the normal chrome browser released for public use, then i think chrome guys have around 11weeks of cycle time for these changes to be out for public.

[SravanKumar S(:sravan)]

*** Bug 21462 has been marked as a duplicate of this bug. ***

[Julien Chaffraix]

> In which version of the Chrome, this solution will be available?

It should be available in the Canary build this week. For the other branch, it will be part of M20.

[Anders Carlsson]

I had to back this out in http://trac.webkit.org/changeset/113756; it broke fast/forms/basic-textareas.html on all Mac ports, and investigating further there was now at least one textarea where there's no longer a vertical scrollbar even though the contents had multiple lines.

[SravanKumar S(:sravan)]

Hi Carlsson,

I took a look at the results through 
http://test-results.appspot.com/dashboards/flakiness_dashboard.html#group=%40ToT%20-%20webkit.org&showExpectations=true&tests=fast%2Fforms%2Fbasic-textareas.html 

after analyzing the results, it is pretty clear that available height in all those cases where vertical scroll bar is missing, is good enough to accommodate the content. Hence we don't need the scroll-bar .

I think its more of re-baselining case than re-opening this bug.
Please let me know if you feel other-wise.

[Simon Fraser (smfr)]

(In reply to comment #50)
> Hi Carlsson,
> 
> I took a look at the results through 
> http://test-results.appspot.com/dashboards/flakiness_dashboard.html#group=%40ToT%20-%20webkit.org&showExpectations=true&tests=fast%2Fforms%2Fbasic-textareas.html 
> 
> after analyzing the results, it is pretty clear that available height in all those cases where vertical scroll bar is missing, is good enough to accommodate the content. Hence we don't need the scroll-bar .
> 
> I think its more of re-baselining case than re-opening this bug.
> Please let me know if you feel other-wise.

Anders and I looked at the behavior with your patch. There are textareas that have more lines of text than are visible, because the last line of text is obscured by the horizontal scrollbar, so in this case a vertical scrollbar should be visible. We think behavior in this case with your patch is wrong.

[SravanKumar S(:sravan)]

I tried re-analyzing the root-cause of the issue, and proposing following patch, let me know if any one among you find it in-correct.

$ svn diff Source/WebCore/rendering/RenderBox.cpp
Index: Source/WebCore/rendering/RenderBox.cpp
===================================================================
--- Source/WebCore/rendering/RenderBox.cpp      (revision 114223)
+++ Source/WebCore/rendering/RenderBox.cpp      (working copy)
@@ -660,13 +660,15 @@
 bool RenderBox::includeVerticalScrollbarSize() const
 {
     return hasOverflowClipWithLayer() && !layer()->hasOverlayScrollbars()
-        && (style()->overflowY() == OSCROLL || style()->overflowY() == OAUTO);
+        && (style()->overflowY() == OSCROLL
+        || (style()->overflowY() == OAUTO && layer()->scrollHeight() > pixelSna
ppedPaddingBoxHeight()));
 }

 bool RenderBox::includeHorizontalScrollbarSize() const
 {
     return hasOverflowClipWithLayer() && !layer()->hasOverlayScrollbars()
-        && (style()->overflowX() == OSCROLL || style()->overflowX() == OAUTO);
+        && (style()->overflowX() == OSCROLL
+        || (style()->overflowX() == OAUTO && layer()->scrollWidth() > pixelSnap
pedPaddingBoxWidth()));
 }

 int RenderBox::verticalScrollbarWidth() const

$ svn diff Source/WebCore/rendering/RenderBox.h
Index: Source/WebCore/rendering/RenderBox.h
===================================================================
--- Source/WebCore/rendering/RenderBox.h        (revision 114223)
+++ Source/WebCore/rendering/RenderBox.h        (working copy)
@@ -216,6 +216,11 @@
     int pixelSnappedClientWidth() const;
     int pixelSnappedClientHeight() const;

+    LayoutUnit paddingBoxWidth() const { return width() - borderLeft() - borderRight(); }
+    LayoutUnit paddingBoxHeight() const { return height() - borderTop() - borderBottom(); }
+    int pixelSnappedPaddingBoxWidth() const { return snapSizeToPixel(paddingBoxWidth(), x() + paddingLeft()); }
+    int pixelSnappedPaddingBoxHeight() const { returnsnapSizeToPixel(paddingBoxHeight(), y() + paddingTop()); }
+
     // scrollWidth/scrollHeight will be the same as clientWidth/clientHeight 




This patch is specific to Auto mode, unlike previous patch.

[SravanKumar S(:sravan)]

Created attachment 137666 [details]
Patch

[WebKit Review Bot]

Comment on attachment 137666 [details]
Patch

Attachment 137666 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12424447

New failing tests:
fast/overflow/hidden-scrollbar-resize.html
scrollbars/scrollbars-on-positioned-content.html
scrollbars/overflow-scrollbar-combinations.html

[WebKit Review Bot]

Created attachment 137670 [details]
Archive of layout-test-results from ec2-cr-linux-02

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-02  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[Anders Carlsson]

Comment on attachment 137666 [details]
Patch

Clearing review flag on patches from before 2014. If this patch is still relevant, please reset the r? flag.

[Ahmad Saleem]

(In reply to satya from comment #0)
> Created attachment 113623 [details]
> Unnecessary scroll bar while changing the dimensions of a DIV
> 
> Unnecessary scroll bar while changing the dimensions of a DIV.
> 
> 
> What steps will reproduce the problem?
> 1. Have two divs one inside another. Position inner DIV absolutely with top
> and left "0px" and have more width and height to have the scroll.
> 2. Now set dimensions on outer DIV dynamically to avoid the scroll with the
> same width and height as inner DIV
> 3. Still we get scroll, even though it is not required
> 
> 4. Use the attached test page to reproduce the issue.
> 
> What is the expected result?
> Scroll should not be there
> 
> What happens instead?
> Extra scrolls are coming

I am able to reproduce this bug in Safari 16.2 and Safari Technology Preview 161 using attached testcase from Comment 0, where clicking on "Change Red DIV.." will expand "Red" DIV but with scrollbar while it is not reproducible in Chrome Canary 111 and Firefox Nightly 111. Just wanted to update the testing results. Thanks!