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

REOPENED
Unassigned

Status

()

Core
Layout
REOPENED
13 years ago
7 months ago

People

(Reporter: Martijn Wargers (dead), Unassigned)

Tracking

(Depends on: 1 bug, Blocks: 2 bugs, {testcase})

Trunk
testcase
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(10 attachments, 4 obsolete attachments)

(Reporter)

Description

13 years ago
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.
(Reporter)

Comment 1

13 years ago
Created attachment 182115 [details]
Testcase
(Reporter)

Comment 2

13 years ago
Created attachment 182118 [details] [diff] [review]
patch

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

Comment 3

13 years ago
But that patch is incomplete. Also things need to happen for Seamonkey and Linux
and Mac themes, I guess.
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.
You'd also need to verify that the arrows do paint correctly when they have a
small size.
(Reporter)

Comment 6

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

Updated

13 years ago
Keywords: testcase
(Reporter)

Comment 7

13 years ago
Created attachment 182894 [details] [diff] [review]
patch2

Like this, I guess.
I haven't checked the print related changes in the patch (I don't know how to
check).
Attachment #182118 - Attachment is obsolete: true
(Reporter)

Comment 8

13 years ago
Created attachment 182897 [details]
Testcase2
(Reporter)

Comment 9

13 years ago
Created attachment 182902 [details]
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

13 years ago
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
Retest after 292312 has landed.
Depends on: 292312

Comment 12

13 years ago
Created attachment 182981 [details]
Rendering in Mozilla 1.6, Retro theme

Comment 13

13 years ago
Created attachment 182985 [details]
Rendering in post-292312 Linux trunk, Modern theme

Comment 14

13 years ago
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)
(Reporter)

Comment 15

13 years ago
Created attachment 183096 [details]
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).
Martijn, why don't you set the arrow minimum-size to be zero in both directions?
It looks like IE does that.
(Reporter)

Comment 17

13 years ago
Created attachment 183211 [details] [diff] [review]
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

13 years ago
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;
}
(Reporter)

Comment 19

13 years ago
Created attachment 183758 [details] [diff] [review]
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.
Attachment #182894 - Attachment is obsolete: true
Attachment #183211 - Attachment is obsolete: true
(Reporter)

Comment 20

13 years ago
Created attachment 183764 [details]
Testcase4

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

Comment 21

13 years ago
Created attachment 183765 [details]
Screenshot of testcase4 with "patch4" patched Firefox

Comment 22

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

Updated

13 years ago
Blocks: 180144, 297055
(Reporter)

Comment 23

13 years ago
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.
Attachment #183758 - Flags: review?(neil.parkwaycc.co.uk)

Comment 24

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

Comment 25

13 years ago
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.
(Reporter)

Comment 26

13 years ago
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 ?)
(Reporter)

Updated

12 years ago
No longer blocks: 297055
(Reporter)

Comment 27

12 years ago
*** Bug 297055 has been marked as a duplicate of this bug. ***
(Reporter)

Comment 28

12 years ago
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

12 years ago
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 :-(
Attachment #183758 - Flags: review?(neil) → review-
(Reporter)

Comment 30

12 years ago
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

12 years ago
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.
(Reporter)

Comment 32

12 years ago
Created attachment 219859 [details] [diff] [review]
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.
Attachment #183758 - Attachment is obsolete: true
(Reporter)

Comment 33

12 years ago
One thing I remembered about patch5 is that I abused canOverride. It's also only a windows fix.
(Reporter)

Comment 34

12 years ago
*** Bug 341236 has been marked as a duplicate of this bug. ***
(Reporter)

Updated

11 years ago
Blocks: 109016
(Reporter)

Updated

11 years ago
Depends on: 48798

Updated

10 years ago
Duplicate of this bug: 398529
(Assignee)

Updated

9 years ago
Product: Core → SeaMonkey

Updated

8 years ago
QA Contact: themes

Updated

8 years ago
Blocks: 342970

Updated

8 years ago
Assignee: themes → nobody
Component: Themes → Layout
Product: SeaMonkey → Core
QA Contact: themes → layout
Duplicate of this bug: 342970

Updated

8 years ago
Blocks: 33654

Comment 37

8 years ago
Created attachment 447083 [details] [diff] [review]
another approach

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

Comment 38

8 years ago
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.
Attachment #447083 - Flags: review?(roc)

Updated

7 years ago
OS: Windows XP → All
Hardware: x86 → All

Comment 39

2 years ago
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.
Attachment #447083 - Flags: review?(roc)
(Reporter)

Updated

7 months ago
Status: NEW → RESOLVED
Last Resolved: 7 months ago
Resolution: --- → WORKSFORME
Status: RESOLVED → REOPENED
Resolution: WORKSFORME → ---
You need to log in before you can comment on or make changes to this bug.