292284 - Scrollbar disappears with very small window/div with overflow:scroll

Status: REOPENED

(Core :: Layout, defect)

Description

[Martijn Wargers (dead)]

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8b2) Gecko/20050426 Firefox/1.0+
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.8b2) Gecko/20050426 Firefox/1.0+

See upcoming testcase.
When a window/overflow scroll div is very small, the horizontal/vertical
scrollbar in Mozilla disappears.
This is not what IE6 is doing. There, the scrollbarbuttons shrink to fit in the
remaining space left.

Reproducible: Always

Steps to Reproduce:
1. Make window very small
2.
3.

Actual Results:  
Vertical scrollbar of the content area disappears when the window is at a
certain size.

Expected Results:  
Make the scrollbarbuttons smaller to fit in the remaining space of the window.

Comment 1

[Martijn Wargers (dead)]

Attachment: Testcase

Comment 2

[Martijn Wargers (dead)]

Attachment: patch

This patch fixes it. It makes Mozilla behave the same as IE6.

Comment 3

[Martijn Wargers (dead)]

But that patch is incomplete. Also things need to happen for Seamonkey and Linux
and Mac themes, I guess.

Comment 4

[Robert O'Callahan (:roc) (email my personal email if necessary)]

You don't want to remove the scrollbar removal code from nsGfxScrollFrame. We
still want that on platforms where the scroll arrows can't or shouldn't shrink.
You just need to set the scrollbar button minimum height or width to 0 in
nsNativeThemeWin (but note that, e.g., a scrollarrow-right should still have a
minimum *height*), and fix the themes to not set the minimum size in CSS, and
that should solve the problem.

Comment 5

[Robert O'Callahan (:roc) (email my personal email if necessary)]

You'd also need to verify that the arrows do paint correctly when they have a
small size.

Comment 6

[Martijn Wargers (dead)]

Ah, ok. Thanks for the info. I'll try that. Hopefully I can manage it before my
holiday.

Comment 7

[Martijn Wargers (dead)]

Attachment: patch2

Like this, I guess.
I haven't checked the print related changes in the patch (I don't know how to
check).

Comment 8

[Martijn Wargers (dead)]

Attachment: Testcase2

Comment 9

[Martijn Wargers (dead)]

Attachment: Rendering of testcase2 in browsers

This is the rendering of testcase2 in IE6/Firefox/patched Firefox.
Three issues:
- when height of the overflow:scroll div is smaller than 4px, then the vertical
scrollbar overlaps (which is hardly noticable). Could have something to do with
the borders that are set on the scrollbarbutton.
- IE6 also shrinks the height of the horizontal scrollbar when there is no room
for it anymore.
- The patched Firefox shrinks the height of the scrollbarbuttons too soon
(which you can see in the second to last overflow:scroll div). From bug 276204,
comment 1, I would think that height would act as min-height here, not?

Comment 10

[José Jeria]

I filed bug 184998 for this issue a while ago, i guess it can be duped to this
one since it has a patch. Though note that it set to block bug 180144

Comment 11

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Retest after 292312 has landed.

Comment 12

[[e-mail]]

Attachment: Rendering in Mozilla 1.6, Retro theme

Comment 13

[[e-mail]]

Attachment: Rendering in post-292312 Linux trunk, Modern theme

Comment 14

[[e-mail]]

Comment on attachment 182894 [details] [diff] [review]
patch2

>     case NS_THEME_SCROLLBAR_BUTTON_UP:
>     case NS_THEME_SCROLLBAR_BUTTON_DOWN:
>       (*aResult).width = ::GetSystemMetrics(SM_CXVSCROLL);
>-      (*aResult).height = ::GetSystemMetrics(SM_CYVSCROLL);
>+      (*aResult).height = 0;
>       *aIsOverridable = PR_FALSE;
Don't you need to leave the default height, but allow it to reduce?
(from something bryner told me it's *aIsOverridble that controls that)

Comment 15

[Martijn Wargers (dead)]

Attachment: Testcase3, scrollbar

(In reply to comment #11)
> Retest after 292312 has landed.
I still get the same results after bug 292312.

(In reply to comment #14)
> Don't you need to leave the default height, but allow it to reduce?
> (from something bryner told me it's *aIsOverridble that controls that)
I tried that (only setting *IsOverridable to PR_TRUE), but that way I keep the
current behavior of Mozilla (like without the patch).

The behavior of this testcase3 changes when the patch is applied: without the
patch, the width of the scrollbar is equal to the width of the browser. With
the patch the width of the scrollbar stays at 16px. 
I don't know if that is an issue.
More in general, I'm afraid the patch is going to break themes.

The third issue I mentioned in comment 9: "The patched Firefox shrinks the
height of the scrollbarbuttons too soon (which you can see in the second to
last overflow:scroll div)."
I think this happens because of min-width/min-height of thumb. When I set this
to 50px for example, I get the issue much earlier (at 50px).
I think this has got something to do with: 
http://bonsai.mozilla.org/cvsblame.cgi?file=mozilla/layout/xul/base/src/nsSliderFrame.cpp&rev=1.131&root=/cvsroot#289

So the slider is not painted anymore when the thumb gets larger than the
available area, but it still takes part in layout (at least that's what it
looks like to me).

Comment 16

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Martijn, why don't you set the arrow minimum-size to be zero in both directions?
It looks like IE does that.

Comment 17

[Martijn Wargers (dead)]

Attachment: Extra hackery

Yes, that helps and makes it compatible with IE6 (although I find it personally
more useless, since you'll get a horizontal scrollbar that overlaps the
content).
This is in fact the second issue I mentioned in comment 9.

With this extra hackery I did here, I sort of resolve issue 3 in comment 9, I
don't know/think if this is any good, though.

Comment 18

[[e-mail]]

Try setting the slider's preferred size to zero; this for Modern:
slider {
  width: 0px;
  height: 15px;
  background: url("...") repeat-x;
}
slider[orient="vertical"] {
  width: 15px;
  height: 0px;
  background: url("...") repeat-y;
}

Comment 19

[Martijn Wargers (dead)]

Attachment: patch4

Thanks Neil! That works (now I understand those attached screenshots of yours).

But I have no idea why it works, maybe it should not even work.
This patch also addresses comment 16.

Comment 20

[Martijn Wargers (dead)]

Attachment: Testcase4

Strange, patch4 doesn't work for horizontal scrollbars. See my upcoming
screenshot of testcase 4 with a build with patch4.

Comment 21

[Martijn Wargers (dead)]

Attachment: Screenshot of testcase4 with "patch4" patched Firefox

Comment 22

[José Jeria]

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

Comment 23

[Martijn Wargers (dead)]

Comment on attachment 183758 [details] [diff] [review]
patch4

So is this ready?
There's still this issue left for horizontal scrollbars, but I think that
situation happens much more seldom.

Comment 24

[[e-mail]]

(In reply to comment #20)
>Created an attachment (id=183764)
>Testcase4
Eww, this attachment really confuses Mozilla 1.6 :-(

Comment 25

[[e-mail]]

Comment on attachment 183758 [details] [diff] [review]
patch4

I shouldn't be doing reviews at this time of night, so I'll just point out a
couple of oversights for now:

>Index: gfx/src/windows/nsNativeThemeWin.cpp
I'd be surprised if the other native themes don't need patching.

>   *aIsOverridable = PR_TRUE;
>   switch (aWidgetType) {
>     case NS_THEME_RADIO:
>     case NS_THEME_CHECKBOX: 
>       (*aResult).width = (*aResult).height = 13;
>       break;
>     case NS_THEME_SCROLLBAR_BUTTON_UP:
>     case NS_THEME_SCROLLBAR_BUTTON_DOWN:
>-      (*aResult).width = ::GetSystemMetrics(SM_CXVSCROLL);
>-      (*aResult).height = ::GetSystemMetrics(SM_CYVSCROLL);
>-      *aIsOverridable = PR_FALSE;
>+      (*aResult).width = 0;
>+      (*aResult).height = 0;
>+      *aIsOverridable = PR_TRUE;
Conveniently this is set by default at the beginning of the diff.

Comment 26

[Martijn Wargers (dead)]

Sorry that I didn't let respond for so long.
(In reply to comment #25)
> I'd be surprised if the other native themes don't need patching.
Yes, but I don't know which other native themes are doing, and when looking at:
http://lxr.mozilla.org/seamonkey/source/gfx/src/
there seem to be quite a bit of themes that maybe need patching (gtk, mac, qt
and shared ?)

Comment 27

[Martijn Wargers (dead)]

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

Comment 28

[Martijn Wargers (dead)]

http://wargers.org/mozilla/Bug292284/konqueror.png
http://wargers.org/mozilla/Bug292284/safari.png
Screenshots of testcase4 with Safari an Konqueror. They seem to handle this differently, but they seem both like buggy solutions to me.

Comment 29

[[e-mail]]

Comment on attachment 183758 [details] [diff] [review]
patch4

Sorry for the ridiculous delay, keep poking me next time ;-)

This works very nicely, but only if I want 16px scrollbars; it ignores the OS setting :-(

Comment 30

[Martijn Wargers (dead)]

No problem with the delay (if I really wanted to, I would have poked you :-)
Hmm, I see what you mean.
I guess I would need some magic css value (-moz-scrollbarbutton-width or something) to get this working.

Comment 31

[[e-mail]]

Either that or perhaps the native theme code could be altered so that it can set the maximum and the preferred size rather than just the minimum size.

Comment 32

[Martijn Wargers (dead)]

Attachment: patch5

(In reply to comment #31)
> Either that or perhaps the native theme code could be altered so that it can
> set the maximum and the preferred size rather than just the minimum size.
Ok, I did that with this patch (I think). Not sure if this is the correct way to do this.

Comment 33

[Martijn Wargers (dead)]

One thing I remembered about patch5 is that I abused canOverride. It's also only a windows fix.

Comment 34

[Martijn Wargers (dead)]

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

Comment 37

[Hideo Saito]

Attachment: another approach

How about this approach?
The small size is got from nsBoxFrame::GetMinSize through nsBoxLayoutStat.

Comment 38

[Hideo Saito]

Comment on attachment 447083 [details] [diff] [review]
another approach

roc, could you review this patch?

I use BoxLayoutState to hand the small size to nsBoxFrame. GetMinSize hands the value to LayoutScrollbars from BoxLayoutState instead of intrinsic size. I think this patch influences only the layout of the scrollbars.

Comment 39

[Allan Jardine]

Its been a while since this bug has been updated, so I thought I'd just note that this behaviour is still present in Firefox 43 Windows. I encountered it while using `overflow:scroll` to make scrollbars show, and while these worked everywhere else, it doesn't in Firefox Win if the height of the scrolling element is <34px.

This behaviour is not the same as described on MDN ( https://developer.mozilla.org/en/docs/Web/CSS/overflow ):

> scroll - The content is clipped and desktop browsers use scrollbars, whether or not any content is clipped. This avoids any problem with scrollbars appearing and disappearing in a dynamic environment. Printers may print overflowing content.

Comment 41

[BugBot [:suhaib / :marco/ :calixte]]

The severity field for this bug is relatively low, S3. However, the bug has 6 duplicates and 12 votes.
:dholbert, could you consider increasing the bug severity?

For more information, please visit auto_nag documentation.

Comment 42

[BugBot (nomail) [:suhaib / :marco/ :calixte]]

The last needinfo from me was triggered in error by recent activity on the bug. I'm clearing the needinfo since this is a very old bug and I don't know if it's still relevant.