405952 - shrink-wrap overflow:auto elements leave space for scrollbar

Status: RESOLVED FIXED

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

Description

[Daniel Kirsch]

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

Comment 1

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

Not sure what we should do about this.

Comment 2

[Georg Maaß]

Gecko 1.9 should do, what all the other browsers do. Not adding the padding for not necessary scrollbars.

Comment 3

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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?

Comment 4

[Georg Maaß]

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.

Comment 5

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Maybe we should drop the  scrollbar from pref, but max() with min?  Need to look at other browsers.

Comment 6

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

I'll look at this for a bit

Comment 7

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

See bug 402567 for a discussion of the issues involved.

Comment 8

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

Attachment: 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

Comment 9

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

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.

Comment 10

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

Attachment: trivial patch

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.

Comment 11

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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 &&

?

Comment 12

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(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?)

Comment 13

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

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.

Comment 14

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

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

Comment 15

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

Attachment: updated patch, still fairly trivial

Updated. Reftesting this is a bit harder because we don't know how wide the native scrollbar is. So I clip it off entirely.

Comment 16

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 305921 [details] [diff] [review]
updated patch, still fairly trivial

r+sr=dbaron

Comment 17

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

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.

Comment 18

[Daniel Holbert [:dholbert]]

(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.

Comment 19

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

Attachment: testcase

This is the testcase causing that reftest failure ... adding "width:0" to the select's container makes the select wider :-)

Comment 20

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

Actually, it's the unconstrained case that's changed. It's narrower with the patch than on trunk.

Comment 21

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

Attachment: better testcase

This makes it easier to see what's going on

Comment 22

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

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?

Comment 23

[Daniel Holbert [:dholbert]]

(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

Comment 24

[Daniel Holbert [:dholbert]]

(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.

Comment 25

[Daniel Holbert [:dholbert]]

(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.

Comment 26

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

Attachment: fix v3

Updated with the fix I mentioned above. Passes reftests on Mac.

Comment 27

[Daniel Holbert [:dholbert]]

Comment on attachment 306169 [details] [diff] [review]
fix v3

Looks good.

Comment 28

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 306169 [details] [diff] [review]
fix v3

sr=dbaron

Comment 29

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

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.

Comment 30

[Boris Zbarsky [:bzbarsky]]

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?

Comment 31

[Boris Zbarsky [:bzbarsky]]

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.

Comment 32

[Boris Zbarsky [:bzbarsky]]

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 33

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 306169 [details] [diff] [review]
fix v3

a1.9b4=beltzner

Comment 34

[Boris Zbarsky [:bzbarsky]]

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.

Comment 35

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

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.

Comment 36

[Boris Zbarsky [:bzbarsky]]

Listboxes always show a vertical scrollbar, as you pointed out.  So you'll only be overriding the fix for combobox dropdowns, really.

Comment 37

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

Right. I meant "just for nsListControlFrames".

Comment 38

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

Attachment: low-risk version

Boris, this OK by you?

Comment 39

[Boris Zbarsky [: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...

Comment 40

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

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.

Comment 41

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

checked in

Comment 42

[Daniel Kirsch]

Thanks a lot for the efforts and good work.