Bug 39284

Summary: Incorrect position of the vertical scrollbar after temporarily setting overflow:hidden
Product: WebKit Reporter: Nickolay <nickolay8>
Component: Layout and RenderingAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: cowboy, hamaji, mehmet.sahin, omega, robin.qiu.dev, robin.qiu, rwlbuis, tonikitoo, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Attachments:
Description Flags
Test case
none
Example div using overflow:auto on hover
none
Patch
none
Test case explaination.
none
Patch v2
none
Patch V3
none
Patch v4 none

Description Nickolay 2010-05-18 04:46:32 PDT
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
Comment 1 Nickolay 2010-05-18 04:47:32 PDT
Created attachment 56361 [details]
Test case

Test case
Comment 2 Alexey Proskuryakov 2010-05-18 13:37:02 PDT
See also: bug 19852.
Comment 3 SFFC 2010-06-25 23:13:27 PDT
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.
Comment 4 SFFC 2010-06-25 23:26:55 PDT
Created attachment 59823 [details]
Example div using overflow:auto on hover

This is an example div with "overflow:hidden" and "overflow:auto" on hover.
Comment 5 SFFC 2010-06-25 23:43:20 PDT
To clarify, this bug still exists in the newest build, 534.2+ (r61924)
Comment 6 SFFC 2010-07-26 00:19:58 PDT
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.
Comment 7 Robin Qiu 2010-09-08 20:14:00 PDT
I'm going to have a look at this bug, hope I can fix it.
Comment 8 Robin Qiu 2010-09-10 00:41:58 PDT
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.
Comment 9 "Cowboy" Ben Alman 2010-09-22 11:40:45 PDT
I've just encountered this bug, here's another test case:

http://jsfiddle.net/cowboy/JHn95/show/
Comment 10 Nickolay 2011-03-23 03:34:11 PDT
This bug was reported for Chrome 5, and its still active even that now I'm using Chrome 10.

Someone, please fix it.
Comment 11 Nickolay 2011-03-23 03:39:33 PDT
The similar problems arise, when the container of the <div> with scroller gets "display" style changed.

See: Bug 18017 and Bug 25835
Comment 12 Robin Qiu 2011-04-02 01:20:31 PDT
I found the root cause and made a patch which worked well and passed the layout test. Going to write a layout test case.
Comment 13 Nickolay 2011-04-02 01:47:51 PDT
Good news
Comment 14 "Cowboy" Ben Alman 2011-04-02 06:26:42 PDT
This bug appears to be fixed in the latest WebKit nightly!

http://jsfiddle.net/cowboy/JHn95/show/
Comment 15 Robin Qiu 2011-04-05 23:33:41 PDT
(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.
Comment 16 Robin Qiu 2011-05-31 04:35:54 PDT
Created attachment 95417 [details]
Patch

This is a simple patch. Only adds 2 lines code.
Comment 17 Robin Qiu 2011-05-31 04:37:33 PDT
Created attachment 95418 [details]
Test case explaination.
Comment 18 Rob Buis 2011-05-31 10:20:50 PDT
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.
Comment 19 Robin Qiu 2011-05-31 19:48:37 PDT
(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.
Comment 20 Robin Qiu 2011-05-31 19:55:04 PDT
Created attachment 95534 [details]
Patch v2

Made a small adjustment in change logs according to Rob's comment.
Comment 21 Antonio Gomes 2011-06-03 11:28:05 PDT
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.
Comment 22 Robin Qiu 2011-06-06 03:29:57 PDT
(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?
Comment 23 Robin Qiu 2011-06-07 07:58:19 PDT
Created attachment 96246 [details]
Patch V3

Modified the test case to avoid platform difference. Used git format-patch to generate an easy-merge patch.
Comment 24 Antonio Gomes 2011-06-07 08:12:41 PDT
Comment on attachment 96246 [details]
Patch V3

Does this fix http://jsfiddle.net/cowboy/JHn95/show/ ?
Comment 25 Robin Qiu 2011-06-07 18:48:31 PDT
(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.
Comment 26 Robin Qiu 2011-06-09 06:14:54 PDT
Created attachment 96578 [details]
Patch v4

Changelog adjustment.
Comment 27 Antonio Gomes 2011-06-09 06:58:29 PDT
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?
Comment 28 Robin Qiu 2011-06-09 17:15:19 PDT
(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.
Comment 29 Robin Qiu 2011-06-23 02:00:55 PDT
Is the latest patch OK?
Comment 30 Robin Qiu 2011-07-13 06:26:20 PDT
r? r? r? ...

:)
Comment 31 Antonio Gomes 2011-07-13 07:02:24 PDT
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?
Comment 32 Robin Qiu 2011-07-13 08:09:53 PDT
(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.
Comment 33 Leo Yang 2011-07-19 02:06:13 PDT
Comment on attachment 96578 [details]
Patch v4

cq+
Comment 34 WebKit Review Bot 2011-07-19 02:21:15 PDT
Comment on attachment 96578 [details]
Patch v4

Clearing flags on attachment: 96578

Committed r91244: <http://trac.webkit.org/changeset/91244>
Comment 35 WebKit Review Bot 2011-07-19 02:21:21 PDT
All reviewed patches have been landed.  Closing bug.