72747 - implement overflow-y and overflow-x properties

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, enhancement)

Description

[Cory Prowse]

From Bugzilla Helper:
User-Agent: Mozilla/5.0 (Windows; U; Windows NT 5.0; en-US; 0.8.1) Gecko/20010319
BuildID:    2001031904

When assigning style="OVERFLOW-Y: scroll; HEIGHT:?px;" on a <div> it doesn't
automatically scroll the contents, they overflow outside the original div.

At
  http://www.uq.net.au/~zzcprows/Development/mozilla_issues.html
the text should scroll within the black border of the div and not overflow
outside as it does now.

Reproducible: Always
Steps to Reproduce:
1. Goto:  http://www.uq.net.au/~zzcprows/Development/mozilla_issues.html
2. Notice that the text overflows outside of the black border


Actual Results:  The text overflows outside of the black border.

Expected Results:  The text should scroll within the black border.

Comment 1

[Boris Zbarsky [:bzbarsky]]

mozilla does not implement the overflow-y property, which is not yet a part of
any CSS standard.... are we planning to do this sometime?  changing this to an
rfe, but I am guessing this is invalid.

Comment 2

[Hixie (not reading bugmail)]

We have our own extensions to do this -- overflow: -moz-horizontal or something.

I suggest WONTFIX.

Comment 3

[Pierre Saslawsky]

Our extensions to 'overflow' are -moz-scrollbars-none, -moz-scrollbars-
horizontal, and -moz-scrollbars-vertical.

I would close the bug as WontFix in its current description but I think that we 
should ask the WG to legitimize our extensions or propose a similar mechanism to 
handle the overflow.  Reassigned to Ian to promote the idea at the next meeting.
Then reassign to me for the implementation...

Comment 4

[Hixie (not reading bugmail)]

Since I do not represent Netscape or Mozilla at the WGs, it is out of line to
ask me to promote an idea for Netscape or Mozilla. Reassigning to Daniel.

Note. I personally am agnostic on this issue; I would not be averse to seeing it
in the specs. I believe Microsoft would love it, so it should be easy to get in.

Comment 5

[alex.dent]

imho overflow-x  and -y should be mapped to -moz-scrollbars-horizontal 
and -vertical.

because microsoft already uses the names, it is not probable that they 
would be defined in any other way. maybe it even becomes part of the 
spec one day. 

and because overflow-x and -y are already used on some sites for internet 
explorer, it would be an increase in real world compatibility. it would not 
break anything, at least.

Comment 6

[Kai Lahmann (is there, where MNG is)]

I suggest WONTFIX, this isn't part of any standard and is not even in the
CSS3-Drafts. Currently it's only used to fix some bugs in IE.

Comment 7

[Josh Johnson]

This is in the working draft for CSS3
http://www.w3.org/TR/2002/WD-css3-box-20021024/#the-overflow-x

I would suggest that until the CSS3 spec is approved, there should be individual
attributes called: -moz-overflow-x and -moz-overflow-y, where the possible
values are visible, auto, scroll, and hidden.

That would allow the logic an usage to be put in place now, and then when the
CSS3 spec is approved, it can be implemented along their guidelines.

Comment 8

[Boris Zbarsky [:bzbarsky]]

Note that the suggested CSS3 spec is not at all what IE implements and not what
we have either....  (eg overflow-x: scroll; overflow-y: auto;)

Comment 9

[Hixie (not reading bugmail)]

How does what IE does differ from the CSS3 proposal?

Comment 10

[Boris Zbarsky [:bzbarsky]]

I was under the impression that the example in my comment did not work in IE...
that's the only explanation I can see for the:

overflow: scroll;
overflow-x: none;

thing I see on so many sites...

I could well be mistaken, of course.  Someone who actually has reliable access
to an IE that is not 5.0 (which is what I have) should test all 16 combinations.

Comment 11

[Josh Johnson]

Here's a link to the IE reference for overflow-x.  Obviously overflow-y is 
similar.

http://msdn.microsoft.com/library/default.asp?
url=/workshop/author/dhtml/reference/properties/overflowx.asp

It looks like the only difference that I notice is IE doesn't support "inherit" 
as a value.

Comment 12

[Madhur Bhatia]

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

Comment 13

[Boris Zbarsky [:bzbarsky]]

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

Comment 14

[Anne (:annevk)]

Attachment: basic test case

Comment 15

[Mats Palmgren (inactive)]

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

Comment 16

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

Attachment: CSS half of patch (untested)

Comment 17

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

Attachment: patch (untested)

Comment 18

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

Attachment: patch (partly tested)

Comment 19

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

Attachment: test showing all combinations

Though this doesn't test the 'auto' cases with x overflow and y overflow
separately...

Comment 20

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

Attachment: patch

Comment 21

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

Attachment: patch

Comment 22

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

Comment on attachment 156620 [details] [diff] [review]
patch

This patch does a bunch of things:

It does the normal stuff for adding a CSS property (without a -moz- prefix,
since IE already implements essentially the same property).  It puts perhaps
slightly more effort into correct serialization of the shorthand when possible
for backwards compatibility with when it wasn't a shorthand (although it never
produces -moz-scrollbars-vertical and -moz-scrollbars-horizontal).  It also
ensures that the computed values of overflow-x and overflow-y are always equal
when one of them is 'visible' or '-moz-hidden-unscrollable' (by moving away
from those values if one of the values is in that set and the other isn't).

It makes the changes needed to propagate two values through the viewport
overflow override.

It adds two convenience methods to nsStyleDisplay that simplify many of the
changes in layout.  Many of the other changes are simple because of the
invariant about mOverflowX and mOverflowY being equal if one of them is
...VISIBLE or ...CLIP.

It fixes the callers of the nsIScrollable API to actually use the enums on that
API (which are the only values that make sense) rather than NS_STYLE_OVERFLOW
values.

It moves ScrollbarStyles to nsPresContext and clearly documents the limitation
on the possible values there (more limited than previously documented), and
enforces that limitation.  (It's the NS_STYLE_OVERFLOW_* values that correspond
to the three values on the nsIScrollable API).

The current nsIScrollable API has "default" scrollbar preferences (persistent
across pages, which are used only for handling the scrolling attribute /
'overflow' property of frame/iframe elements) and "current" scrollbar
preferences (which are used only to suppress scrollframe creation on frameset
documents).  This patch removes the latter (and the reset method that sets the
latter to the former) and handles the suppression of scrolllframe creation on
framesets in a simpler way (a simple API on nsIHTMLDocument).  This also means
that the contents of an iframe with scrolling="no" have the same scroll frame
setup under the root frame as all other documents (I remember hitting bugs
caused by that difference in the past).  It likewise moves the testing of the
scrollbar preferences (now only "default") into nsGfxScrollFrame, which now
makes it easy to implement scrolling="yes" (and equivalents) for FRAME and
IFRAME (which we'd never implemented correctly before -- we treated it as
scrolling="auto").

This patch removes the GetScrollPreference API from nsIScrollableFrame and
makes the callers use GetScrollbarStyles instead (which is better for their
needs anyway).

This patch removes the SetScrollbarVisibility API from nsIScrollableFrame and
replaces it with a single line of CSS for the anonymous div inside non-textarea
text inputs (which was the only use).  Note that the internal variables for
that pref were also used for print preview, but the null-checks of the
scrollbar boxes should be sufficient.

This cleans up a bit of code in nsGfxScrollFrame that did more work than needed
when the overflow style was 'hidden'.

It cleans up overuse of |out| in nsIScrollable.idl .

Comment 23

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

Sounds great, I'll review it soon.

At first I wasn't convinced about making it overflow-x and overflow-y but I
think I've convinced myself that you're right.

Comment 24

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

Comment on attachment 156620 [details] [diff] [review]
patch

Actually, hold on.  I've regressed attachment 145385 [details].

Comment 25

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

Attachment: patch

Comment 26

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

Comment on attachment 156629 [details] [diff] [review]
patch

Basically looks great. I'm not so confident about the style changes since my
style knowledge is fairly weak, and technically I'm not a style peer, but I
think you should just go ahead anyway.

See
http://lxr.mozilla.org/mozilla/source/layout/html/forms/src/nsListControlFrame.
cpp#1252
We can do that later.

+  // if we have 'auto' scrollbars or we dynamically changed to 'hidden'
+  // look at the vertical case
   if (styles.mVertical != NS_STYLE_OVERFLOW_SCROLL) {

I think this comment is incorrect. A dynamic change to 'hidden' would cause a
reframe. The only way styles.mVertical could be HIDDEN here is as a propagated
style from the viewport or imposed by the container preferences.

 [scriptable, uuid(61792520-82C2-11d3-AF76-00A024FFC08C)]
 interface nsIScrollable : nsISupports

Since you changed the interface I think it'd be best to give it a new UUID.

Comment 27

[Dainis Jonitis]

This broke DEBUG build:

g:/Warpzilla\mozilla\content\base\src\nsStyleContext.cpp(88) : warning C4554: '<
<' : check operator precedence for possible error; use parentheses to clarify pr
ecedence
g:/Warpzilla\mozilla\content\base\src\nsStyleContext.cpp(88) : warning C4554: '&
' : check operator precedence for possible error; use parentheses to clarify pre
cedence
g:/Warpzilla\mozilla\content\base\src\nsStyleContext.cpp(751) : error C2039: 'mO
verflow' : is not a member of 'nsStyleDisplay'
        g:\Warpzilla\mozilla\dist\include\content\nsStyleStruct.h(699) : see dec
laration of 'nsStyleDisplay'
../../../dist\include\xpcom\nsCOMPtr.h(228) : warning C4624: 'nsDerivedSafe<T>'
: destructor could not be generated because a base class destructor is inaccessi
ble
        with
        [
            T=nsPresContext
        ]
        g:/Warpzilla\mozilla\content\base\src\nsStyleContext.cpp(908) : see refe
rence to class template instantiation 'nsDerivedSafe<T>' being compiled
        with
        [
            T=nsPresContext
        ]

  fprintf(out, "<display data=\"%d %d %f %d %d %d %d %d %d %ld %ld %ld %ld %s\"
/>\n",
    (int)disp->mPosition,
    (int)disp->mDisplay,
    (float)disp->mOpacity,      
    (int)disp->mFloats,
    (int)disp->mBreakType,
    (int)disp->mBreakBefore,
    (int)disp->mBreakAfter,
    (int)disp->mOverflow,
    (int)disp->mClipFlags,
    (long)disp->mClip.x,
    (long)disp->mClip.y,
    (long)disp->mClip.width,
    (long)disp->mClip.height,
    URICString(disp->mBinding).get()
    );

mOverflowArea should go away

Comment 28

[Dainis Jonitis]

mOverflowX and mOverflowY should be used insted of mOverflow.

Comment 29

[[e-mail]]

So, this means that we're going to support more cases of visibly unscrollable
scrollbars, is there an existing bug on the appearance of such scrollbars?

Comment 30

[Braden]

Bug 76197.

Comment 31

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

Checked in for a short time, but backed out (in 2 pieces) due to pageload time
regression.  I'll try relanding in more finely-grained pieces another time.

Comment 32

[Darin Fisher]

Looks like you may need to rev the UUIDs for nsIDOMCSS2Properties and
nsIScrollable (though you seemed to cover nsIScrollable in your checkin).

Comment 33

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

For the record, my standard set of testcases is bug 69355 comment 34.

Comment 34

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

I think part of the problem was the text input changes.

Comment 35

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

The frame / iframe changes also, to my surprise, seem to be a part of the
regression.

Comment 36

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

Everything is landed except for:
 * the text input changes and the removal of the nsIScrollableFrame API that
they used to suppress scrollbars
 * the change that makes the frame construction around the root more consistent
when in a frame with scrolling="no".

Both of these hurt pageload time, so I'm just skipping them.  I did modify the
latter so it checks both X and Y, though, and fix the comments around it.  I
also fixed up a few comments that I missed earlier:  mostly just changing
NS_STYLE_OVERFLOW_* to nsIScrollable::Scrollbar_*.

Comment 37

[Anne (:annevk)]

Comment on attachment 156629 [details] [diff] [review]
patch

>+           /* Mozilla extensions */
>+           attribute DOMString        overflowX;
>+                                        // raises(DOMException) on setting
>+
>+           attribute DOMString        overflowY;
>+                                        // raises(DOMException) on setting
>+

Shouldn't that read 'MozOverflowX' and 'MozOverflowY'? Since it seems that the
properties are implemented with a -moz- prefix (quite logical, since css3-box
isn't in CR).

By the way, this should probably be mentioned on www-style or mailed directly
to Bert Bos, the editor of css3-box. Two UAs now support the properties so it
would not be very logical to extend 'overflow' instead.

Comment 38

[Hixie (not reading bugmail)]

Those properties were already co-opted by IE so we're not breaking the Web by 
using them again. Hence the lack of -moz- prefixes.

The fact that Mozilla implements this should not be an argument about whether to 
keep this feature in the drafts or not. That would be a very bad precedent (it 
would mean that all someone had to do to get a feature into CSS would be to get 
it implemented in two or more UAs).

Comment 39

[Anne (:annevk)]

In that case, shouldn't the following lines read a little different? (Line
breaks made after 'mOverflowX,' and 'mOverflowY' here should be ignored.)

>+CSS_PROP_DISPLAY(overflow-x, overflow_x, MozOverflowX, Display, mOverflowX,
> eCSSType_Value, PR_FALSE, kOverflowSubKTable)
>+CSS_PROP_DISPLAY(overflow-y, overflow_y, MozOverflowY, Display, mOverflowY,
> eCSSType_Value, PR_FALSE, kOverflowSubKTable)

That third paramter should be 'OverflowX' and 'OverflowY' if I'm not mistaken.
(Actually, I just compare with them other lines.)

Another question: how long will Mozilla have support for the '-moz-scrollbars-*'
values?

Comment 40

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

They do -- that patch didn't compile, since I missed one spot when removing the
Moz, and I removed the Moz long before checking in.

Comment 41

[Anne (:annevk)]

This fix caused bug 258228.

Comment 42

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

This caused regression bug 258300.

Comment 43

[Ryan VanderMeulen [:RyanVM]]

This patch appears to have caused bug 282750