Closed Bug 405952 Opened 12 years ago Closed 12 years ago

shrink-wrap overflow:auto elements leave space for scrollbar

Categories

(Core :: Layout: Block and Inline, defect, P2)

defect

Tracking

()

RESOLVED FIXED

People

(Reporter: daniel, Assigned: roc)

References

(Depends on 3 open bugs)

Details

Attachments

(4 files, 3 obsolete files)

User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; de; rv:1.8.1.10) Gecko/20071115 Firefox/2.0.0.10
Build Identifier: 

When using overflow="auto" and a border for an Element, additional padding gets added at the right side. The width of the padding seems to be the width that a scrollbar would take. However the padding gets added even if no scrollbar is visible.
This happens since bug 384876 was fixed. It does not happen in 1.8 releases or other known browsers.

See bug 384876 for history and attachment 290509 [details] for some screenshots (https://bugzilla.mozilla.org/attachment.cgi?id=290509)

Testcase:
https://bugzilla.mozilla.org/attachment.cgi?id=268782

Reproducible: Always
Version: unspecified → Trunk
Not sure what we should do about this.
Flags: blocking1.9?
Gecko 1.9 should do, what all the other browsers do. Not adding the padding for not necessary scrollbars.
Resummarizing to avoid the use of the term "padding" since this doesn't have anything to do with the CSS padding property, as far as I can tell.  (Feel free to correct if that's wrong.)

So do other browsers wrap the text if they do need a scrollbar?
Summary: Additional padding gets added at right side when overflow="auto" is used → shrink-wrap overflow:auto elements leave space for scrollbar
Thanks for introducing a better term than "padding". We know that, what we are talking about is not the css padding but something else, but we did not know a better term.
Flags: blocking1.9? → blocking1.9+
Priority: -- → P4
Status: UNCONFIRMED → NEW
Ever confirmed: true
Maybe we should drop the  scrollbar from pref, but max() with min?  Need to look at other browsers.
Assignee: nobody → dbaron
Priority: P4 → P2
I'll look at this for a bit
Assignee: dbaron → roc
See bug 402567 for a discussion of the issues involved.
Attached file testcase
This testcase tests min and pref widths with overflow:auto and overflow:scroll, both when there is vertical overflow and when there is not.

Summary of the results:

              min-width     pref-width
Safari 3       never         never
IE7            if needed     never
Opera 9.5      never         if needed
Fx2            if needed     if needed
Fx3 trunk      never         always
Strange bunch of results!

We basically can't implement "if needed" with the reflow-branch architecture. So I say let's just do "never" for pref-width as well. Nice and simple, consistent with Safari across the board, consistent with IE7, and no worries about what happens when the height is less than the minimum scrollbar size.
Attached patch trivial patch (obsolete) — Splinter Review
The fix is trivial, if we agree this is the right thing to do.

dholbert, if you've got any comments I'd like to hear them too.
Attachment #305463 - Flags: superreview?(dbaron)
Attachment #305463 - Flags: review?(dbaron)
Whiteboard: [needs review]
Ignoring the scrollbar width for 'overflow:scroll' just seems broken.  You'll end up with things that can fit on one line breaking onto two.  Why not just change:

>  if (ss.mVertical != NS_STYLE_OVERFLOW_HIDDEN && // ideal?

to:

>  if (ss.mVertical == NS_STYLE_OVERFLOW_SCROLL &&

?
(The difference between what you propose in comment 10 and what I propose in comment 11 shouldn't be a problem for compatibility, since Fx2's "if needed" presumably always included it for 'overflow:scroll' since it was always "needed", right?)
That's right, it should be OK for compatibility.

The argument for not adding scrollbar width to pref-width basically mirrors the argument for min-width. In bug 402567, we decided not to add the scrollbar width for min-width, primarily because the scrollbar might not actually be shown if the element is not as tall as the scrollbar min-height, so you can get useless space.

Also, regardless of anything else, matching IE7 and Safari has some merit.
From irc.mozilla.org, #developers:

[26 14:05:57] <dbaron> roc, note that I had comments/questions on 405952
[26 14:07:35] <roc> okay let's thrash it out
[26 14:07:54] <roc> the problem is that even with overflow:scroll we won't show a scrollbar if the element's height is less than the scrollbar min-height
[26 14:08:03] <roc> so we end up with useless space
[26 14:08:38] <roc> the argument in bug 402567 concluded that that was a very bad thing so we shouldn't add scrollbar width for min-size
[26 14:08:46] <roc> the same argument would seem to apply to pref-sie
[26 14:09:33] <roc> even more so, since the element is likely to be taller with min-size than with pref-size
[26 14:10:31] <roc> I'm not totally convinced by the argument in bug 402567 but I don't want to reopen it
[26 14:10:59] <roc> I also think that broken or not, matching IE7 and Safari has merit
[26 14:22:13] <dbaron> roc, I guess I think I prefer patch v.2 over patch v.3
[26 14:22:24] <dbaron> roc, except that in the past we treated min-width as 0
[26 14:22:32] <dbaron> roc, so we were stuck in a situation where nobody did the right thing
[26 14:22:42] <dbaron> roc, so doing the right thing could have broken stuff
[26 14:22:50] <dbaron> roc, here, Fx2 and Opera did the right thing to begin with
[26 14:22:57] <dbaron> roc, so I'd rather stick with that for Fx3
[26 14:23:05] <dbaron> roc, that said, I'd be open to changing min-width as well
[26 14:25:25] <dbaron> roc, I guess I think we probably should change both
[26 14:26:27] <roc> okay so, just checking, you've changed your mind on https://bugzilla.mozilla.org/show_bug.cgi?id=402567#c42 ?
[26 14:26:54] <dbaron> roc, yeah
[26 14:27:00] <dbaron> roc, is that unreasonable?
[26 14:27:09] <roc> no, you're allowed to change your mind :-)
[26 14:27:33] <dbaron> IE7 left room there, right?  At least assuming the element was tall enough?
[26 14:27:49] <dbaron> yeah, that's what comment 41 says, I think
[26 14:27:59] <roc> well there's the thing
[26 14:28:19] <roc> in my tests, IE7 did not leave room there
[26 14:28:24] <roc> let me test dholbert's test
[26 14:28:40] <dbaron> I think people are pretty unlikely to be happy with overflow:scroll when we don't show the scrollbars no matter what we do for intrinsic widths.
[26 14:29:00] <roc> or if you have IE7 handy you can test yourself
[26 14:29:07] <dbaron> And I tend to think overflow:scroll is less likely to be used for really small things
[26 14:29:15] * dbaron tests
[26 14:30:01] <roc> well
[26 14:30:42] <roc> dholbert's tests only test min-width, not pref-width
[26 14:30:46] <dbaron> on dholbert's test, IE7 leaves room for the scrollbar for both auto and scroll
[26 14:30:48] <roc> IE7 adds scrollbar width for min-width, but not pref-width
[26 14:30:51] <dbaron> right
[26 14:30:58] <dbaron> but Fx2 adds it for pref-width
[26 14:31:02] <dbaron> so we should be fine continuing to do so there
[26 14:32:12] <roc> dholbert had two points in comment #41
[26 14:32:21] <roc> "  - When an "overflow: auto" frame has scrollbars, it should behave similarly to an overflow: scroll frame."
[26 14:32:31] <roc> "- It's kind of broken to allocate min-width for a scrollbar, and then let our content fill that space instead because the scrollbar doesn't fit vertically."
[26 14:32:51] <roc> you're willing to ditch both of those?
[26 14:32:54] <dbaron> yes
[26 14:32:57] <roc> ok!
[26 14:32:59] <roc> I'll do another patch
[26 14:33:05] <roc> let me review your table stuff first
[26 14:33:11] <dbaron> I think treating overflow:hidden the same as overflow:scroll is a pretty big requirement
[26 14:33:22] <dbaron> and making overflow:auto behave the same as both dictates that
[26 14:33:28] <dbaron> so I don't think it's one we want to meet
Updated. Reftesting this is a bit harder because we don't know how wide the native scrollbar is. So I clip it off entirely.
Attachment #305463 - Attachment is obsolete: true
Attachment #305921 - Flags: superreview?(dbaron)
Attachment #305921 - Flags: review?(dbaron)
Attachment #305463 - Flags: superreview?(dbaron)
Attachment #305463 - Flags: review?(dbaron)
Comment on attachment 305921 [details] [diff] [review]
updated patch, still fairly trivial

r+sr=dbaron
Attachment #305921 - Flags: superreview?(dbaron)
Attachment #305921 - Flags: superreview+
Attachment #305921 - Flags: review?(dbaron)
Attachment #305921 - Flags: review+
This causes reftest failures locally. In particular
REFTEST UNEXPECTED FAIL: file:///Users/roc/shared/mozilla-checkin/mozilla/layout/ref
tests/bugs/363858-1.html
and others.

I'll need to do some more work on this tomorrow. We should probably still try to get it in for beta4.
Whiteboard: [needs review]
(In reply to comment #10)
> dholbert, if you've got any comments I'd like to hear them too.

FWIW, I like the "updated patch" behavior.
OS: Windows XP → All
Hardware: PC → All
Attached file testcase
This is the testcase causing that reftest failure ... adding "width:0" to the select's container makes the select wider :-)
Actually, it's the unconstrained case that's changed. It's narrower with the patch than on trunk.
Attached file better testcase
This makes it easier to see what's going on
The issue is that nsComboboxControlFrame expects the scrollbar width to always be included in the dropdown frame min/pref size. But now that overflow:auto scrollframes don't get the scrollbar width added to their pref width, that assumption is violated and nsComboboxControlFrame subtracts too much.

This isn't affecting min-widths because nsListControlFrame overrides GetMinWidth to add the scrollbar width.

I think we should just take out the nsListControlFrame override and the code in nsComboboxControlFrame that subtracts the scrollbar width. The dropdown is never going to be adding the scrollbar width anymore. Does that sound OK to everyone?
(In reply to comment #22)
> I think we should just take out the nsListControlFrame override and the code in
> nsComboboxControlFrame that subtracts the scrollbar width. The dropdown is
> never going to be adding the scrollbar width anymore.

Code source:
http://mxr.mozilla.org/seamonkey/source/layout/forms/nsComboboxControlFrame.cpp#594
(In reply to comment #22)
> This isn't affecting min-widths because nsListControlFrame overrides
> GetMinWidth to add the scrollbar width.

Code link:
http://mxr.mozilla.org/seamonkey/source/layout/forms/nsListControlFrame.cpp#529

A bit off-topic, but -- do we still need that overriding function?  The comment there that justifies it is no longer true, after bug 402567's fix:
 531   // Scrollframes typically have an intrinsic min width of 0, but
 532   // that's not how we want to behave.
(In reply to comment #24)
> A bit off-topic, but -- do we still need that overriding function?

Ah, nevermind, that's part of what roc suggested in comment 22. :)

Anyway, I agree with comment 22.
Attached patch fix v3 (obsolete) — Splinter Review
Updated with the fix I mentioned above. Passes reftests on Mac.
Attachment #305921 - Attachment is obsolete: true
Attachment #306169 - Flags: superreview?(dbaron)
Attachment #306169 - Flags: review?(dholbert)
Whiteboard: [needs review]
Comment on attachment 306169 [details] [diff] [review]
fix v3

Looks good.
Attachment #306169 - Flags: review?(dholbert) → review+
Comment on attachment 306169 [details] [diff] [review]
fix v3

sr=dbaron
Attachment #306169 - Flags: superreview?(dbaron) → superreview+
Comment on attachment 306169 [details] [diff] [review]
fix v3

this patch should go into beta4. It changes layout in a way that might affect a significant number of real pages.
Attachment #306169 - Flags: approval1.9b4?
So the point of the list control min width is that we need to satisfy the following criteria, for auto-width selects:

1)  listboxes have the same width no matter what the available width is
2)  listboxes do not have a horizontal scrollbar
3)  combobox dropdowns do not have a horizontal scrollbar
4)  comboboxes have enough space for the display area and the dropmarker no
    matter what option is selected.
5)  comboboxes have the same width no matter what the available width is
6)  comboboxes have the same width no matter which option is selected.

With this patch, do we satisfy #2?
OK.  We do satisfy #2, as roc points out, because listboxes claim to be overflow:scroll via GetScrollbarStyles.

There is still an issue for comboboxes, though.  If the width of the themed dropmarker is smaller than the scrollbar width, we'll violate #3.  Hard to reftest that, of course.
To be precise, in the situation where bug 404288 comment 19 says that the combobox would end up narrower than the dropdown (before this patch), with this patch we'll end up with scrollbars in the dropdown.

I guess we should just take this and fix bug 404288.
Comment on attachment 306169 [details] [diff] [review]
fix v3

a1.9b4=beltzner
Attachment #306169 - Flags: approval1.9b4? → approval1.9b4+
Actually, fixing bug 404288 will only sort of help.  The thing this patch really breaks is that any time the combobox itself is narrower than the dropdown would be with a scrollbar you get items cut off in the dropdown.  The easiest way to get there is to have a width style on the combobox which is smaller than the width of the longest item plus the width of a vertical scrollbar.

So what we really need is for an unconstrained listbox to always be wide enough to fit any of the options plus a scrollbar.  This means adding the scrollbar width in pref width.  At that point, we should add it for min width too, to make the logic in nsComboboxControlFrame simpler.
Okay, what I plan to do here is to revise the patch to leave comboboxes unchanged, by adding nsListControlFrame::GetPrefWidth which will add the scrollbar width to the pref-width, essentially overriding the main fix here just for listboxes. That's ugly but it will eliminate the risk.
Listboxes always show a vertical scrollbar, as you pointed out.  So you'll only be overriding the fix for combobox dropdowns, really.
Right. I meant "just for nsListControlFrames".
Attached patch low-risk versionSplinter Review
Boris, this OK by you?
Attachment #306169 - Attachment is obsolete: true
Attachment #306225 - Flags: superreview?(bzbarsky)
Attachment #306225 - Flags: review?(bzbarsky)
Comment on attachment 306225 [details] [diff] [review]
low-risk version

>Index: layout/forms/nsListControlFrame.cpp
>+nsListControlFrame::GetPrefWidth(nsIRenderingContext *aRenderingContext)
>+  result += GetDesiredScrollbarSizes(&bls).LeftRight();

This needs to be a saturating add, since pref-width can be unconstrained.  r+sr=bzbarsky with that.

I wish we had a way to reftest the combobox dropdown...
Attachment #306225 - Flags: superreview?(bzbarsky)
Attachment #306225 - Flags: superreview+
Attachment #306225 - Flags: review?(bzbarsky)
Attachment #306225 - Flags: review+
It would be cool if we had a mode where the dropdown just paints in the page. Would be easy to implement, would allow reftesting of dropdowns, and would be nice for certain embedders who don't like having to deal with popup windows.
checked in
Whiteboard: [needs review]
Status: NEW → RESOLVED
Closed: 12 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
Thanks a lot for the efforts and good work.
Depends on: 764076
Depends on: 1509159
You need to log in before you can comment on or make changes to this bug.