Bug 39284 – Incorrect position of the vertical scrollbar after temporarily setting overflow:hidden

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Nickolay]

Attaching the test case. It fails in Chrome for Linux (5.0.375.3 dev) and Safari for Windows (4.0.5 531.22.7). Works in FF 3.6 and IE7

Steps to reproduce:

Test case contains a long text with 2 buttons near the bottom. Scroll the window to those buttons, so that vertical scrollbar will have some non-zero "scrollTop" value.

Then press "mask", "unmask" (add/remove "overflow : hidden !important" CSS rule). After that the vertical scroll bar position will be reset to the top, though the content
of the viewable area won't change. If you start scrolling, scrolling will begin from top. 

Expected behavior is that position of the scroll bar should be preserved as it was before clicking "mask" (works in FF, IE)


See also: https://bugs.webkit.org/show_bug.cgi?id=36461

Regards, Nickolay

[Nickolay]

Created attachment 56361 [details]
Test case

Test case

[Alexey Proskuryakov]

See also: bug 19852.

[SFFC]

Safari 5.0 (6533.16) on Mac

I encountered an instance of this same bug in a different context.  On a block div with "overflow:hidden" as default but "overflow:auto" on hover, the scrollbar jumps back to the top when the user hovers out and then back over the div again.  The content of the div jumps back up only when the user starts to scroll.

Firefox exhibits the behavior that I would expect; Opera exhibits a completely different behavior.  In Firefox, the scrollbar remains in the previous place on the second hover.

I will attach a test case to demonstrate.

[SFFC]

Created attachment 59823 [details]
Example div using overflow:auto on hover

This is an example div with "overflow:hidden" and "overflow:auto" on hover.

[SFFC]

To clarify, this bug still exists in the newest build, 534.2+ (r61924)

[SFFC]

On a side note, the overflow:hidden to overflow:auto swap is now being used by major sites like Google.  Search for an image on Google Images and then make your browser window "shorter" (i.e. so it has not much height).  Now, hover over the left panel (the one with links for "Everything", "Images", "Shopping", "More", "Any size", etc).  A scrollbar appears.  Scroll down, hover out of the area, and then over it again.  Lo and behold, bug 39284 rears its ugly head.

[Robin Qiu]

I'm going to have a look at this bug, hope I can fix it.

[Robin Qiu]

It seems caused by this block of code:

http://trac.webkit.org/browser/trunk/WebCore/rendering/RenderLayer.cpp#L2086

m_hBar calls setValue() while m_vBar doesn't and the comment here is strange....

I added setValue() for m_vBar and this bug was fixed, however, it brought some regressions in layout tests.

["Cowboy" Ben Alman]

I've just encountered this bug, here's another test case:

http://jsfiddle.net/cowboy/JHn95/show/

[Nickolay]

This bug was reported for Chrome 5, and its still active even that now I'm using Chrome 10.

Someone, please fix it.

[Nickolay]

The similar problems arise, when the container of the <div> with scroller gets "display" style changed.

See: Bug 18017 and Bug 25835

[Robin Qiu]

I found the root cause and made a patch which worked well and passed the layout test. Going to write a layout test case.

[Nickolay]

Good news

["Cowboy" Ben Alman]

This bug appears to be fixed in the latest WebKit nightly!

http://jsfiddle.net/cowboy/JHn95/show/

[Robin Qiu]

(In reply to comment #14)
> This bug appears to be fixed in the latest WebKit nightly!
> 
> http://jsfiddle.net/cowboy/JHn95/show/

Yes, this test case works all right in the latest code.
However, the test cases attached still don't work.

[Robin Qiu]

Created attachment 95417 [details]
Patch

This is a simple patch. Only adds 2 lines code.

[Robin Qiu]

Created attachment 95418 [details]
Test case explaination.

[Rob Buis]

Hi Robin

(In reply to comment #16)
> Created an attachment (id=95417) [details]
> Patch
> 
> This is a simple patch. Only adds 2 lines code.

Just a few observations (I don't know about Scrollbar, so no official review).

The ChangeLogs usually have a different format, for example:

        [Texmap][Qt] Enable TextureMapper by default
        https://bugs.webkit.org/show_bug.cgi?id=61740

        Make CONFIG+=texmap an opt-out instead of an opt-in.

I didn't find in the style guide that it must be like that, but why not stick to it?

What about calling offsetDidChange instead? It also adjusts the thumb.
Cheers,

Rob.

[Robin Qiu]

(In reply to comment #18)
> Hi Robin
> 
> (In reply to comment #16)
> > Created an attachment (id=95417) [details] [details]
> > Patch
> > 
> > This is a simple patch. Only adds 2 lines code.
> 
> Just a few observations (I don't know about Scrollbar, so no official review).
> 
> The ChangeLogs usually have a different format, for example:
> 
>         [Texmap][Qt] Enable TextureMapper by default
>         https://bugs.webkit.org/show_bug.cgi?id=61740
> 
>         Make CONFIG+=texmap an opt-out instead of an opt-in.
> 
> I didn't find in the style guide that it must be like that, but why not stick to it?
> 
OK. I'll modify the changelog. 
The patch is platform independent, so, I guess it's not necessary to add a tag like [Qt]? 

> What about calling offsetDidChange instead? It also adjusts the thumb.

offsetDidChange() also updates the thumb and do some painting, but here, we only need to initialize current position.

> Cheers,
> 
> Rob.

Thanks for your review.

[Robin Qiu]

Created attachment 95534 [details]
Patch v2

Made a small adjustment in change logs according to Rob's comment.

[Antonio Gomes]

Comment on attachment 95534 [details]
Patch v2

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

Could you rebase your patch against ToT, so we can move it forward? Thanks

> LayoutTests/scrollbars/scrollbar-initial-position-expected.txt:8
> +  RenderView at (0,0) size 800x600
> +layer at (0,0) size 800x118
> +  RenderBlock {HTML} at (0,0) size 800x118
> +    RenderBody {BODY} at (8,8) size 784x102
> +layer at (8,8) size 102x102 clip at (9,9) size 80x100 scrollY 300 scrollHeight 400
> +  RenderBlock {DIV} at (0,0) size 102x102 [bgcolor=#0000FF] [border: (1px solid #000000)]
> +    RenderBlock {DIV} at (1,1) size 50x200 [bgcolor=#FF0000]

what platform was this result generated at? I think you will have different platform with different font-pixel values here.

[Robin Qiu]

(In reply to comment #21)
> (From update of attachment 95534 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=95534&action=review
> 
> Could you rebase your patch against ToT, so we can move it forward? Thanks
> 
> > LayoutTests/scrollbars/scrollbar-initial-position-expected.txt:8
> > +  RenderView at (0,0) size 800x600
> > +layer at (0,0) size 800x118
> > +  RenderBlock {HTML} at (0,0) size 800x118
> > +    RenderBody {BODY} at (8,8) size 784x102
> > +layer at (8,8) size 102x102 clip at (9,9) size 80x100 scrollY 300 scrollHeight 400
> > +  RenderBlock {DIV} at (0,0) size 102x102 [bgcolor=#0000FF] [border: (1px solid #000000)]
> > +    RenderBlock {DIV} at (1,1) size 50x200 [bgcolor=#FF0000]
> 
> what platform was this result generated at? I think you will have different platform with different font-pixel values here.

Hi, Antonio,
Thanks for your review.

I generated this result in linux-Qt platform. How can I avoid font influence? Setting explicit margin?

[Robin Qiu]

Created attachment 96246 [details]
Patch V3

Modified the test case to avoid platform difference. Used git format-patch to generate an easy-merge patch.

[Antonio Gomes]

Comment on attachment 96246 [details]
Patch V3

Does this fix http://jsfiddle.net/cowboy/JHn95/show/ ?

[Robin Qiu]

(In reply to comment #24)
> (From update of attachment 96246 [details])
> Does this fix http://jsfiddle.net/cowboy/JHn95/show/ ?

This test case had already been fixed by other patches.

see:

Comment #15 From Robin Qiu 2011-04-05 23:33:41 PST (-) [reply]

(In reply to comment #14)
> This bug appears to be fixed in the latest WebKit nightly!
> 
> http://jsfiddle.net/cowboy/JHn95/show/

Yes, this test case works all right in the latest code.
However, the test cases attached still don't work.

[Robin Qiu]

Created attachment 96578 [details]
Patch v4

Changelog adjustment.

[Antonio Gomes]

Comment on attachment 96578 [details]
Patch v4

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

> LayoutTests/scrollbars/scrollbar-initial-position-expected.txt:1
> +

is the expected result file an empty line?

[Robin Qiu]

(In reply to comment #27)
> (From update of attachment 96578 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=96578&action=review
> 
> > LayoutTests/scrollbars/scrollbar-initial-position-expected.txt:1
> > +
> 
> is the expected result file an empty line?

Yes. This test case can only be tested by pixel test. So I dump an empty line by calling dumpAsTest() to avoid platform font differences.

[Robin Qiu]

Is the latest patch OK?

[Robin Qiu]

r? r? r? ...

:)

[Antonio Gomes]

Comment on attachment 96578 [details]
Patch v4

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

>>> LayoutTests/scrollbars/scrollbar-initial-position-expected.txt:1
>>> +
>> 
>> is the expected result file an empty line?
> 
> Yes. This test case can only be tested by pixel test. So I dump an empty line by calling dumpAsTest() to avoid platform font differences.

is the expected result file an empty line?

[Robin Qiu]

(In reply to comment #31)
> (From update of attachment 96578 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=96578&action=review
> 
> >>> LayoutTests/scrollbars/scrollbar-initial-position-expected.txt:1
> >>> +
> >> 
> >> is the expected result file an empty line?
> > 
> > Yes. This test case can only be tested by pixel test. So I dump an empty line by calling dumpAsTest() to avoid platform font differences.
> 
> is the expected result file an empty line?

Yes, it is expected.

[Leo Yang]

Comment on attachment 96578 [details]
Patch v4

cq+

[WebKit Review Bot]

Comment on attachment 96578 [details]
Patch v4

Clearing flags on attachment: 96578

Committed r91244: <http://trac.webkit.org/changeset/91244>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.