Last Comment Bug 72747 - implement overflow-y and overflow-x properties
: implement overflow-y and overflow-x properties
Status: RESOLVED FIXED
WG[patch]
: css3, testcase
Product: Core
Classification: Components
Component: CSS Parsing and Computation (show other bugs)
: Trunk
: All All
: -- enhancement with 5 votes (vote)
: Future
Assigned To: David Baron :dbaron: ⌚️UTC-7 (busy September 14-25)
: Hixie (not reading bugmail)
:
Mentors:
http://www.uq.net.au/~zzcprows/Develo...
: 204309 220289 241902 (view as bug list)
Depends on:
Blocks: 42492 72540
  Show dependency treegraph
 
Reported: 2001-03-20 17:26 PST by Cory Prowse
Modified: 2006-06-27 22:56 PDT (History)
31 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
basic test case (747 bytes, text/html)
2003-12-20 10:14 PST, Anne (:annevk)
no flags Details
CSS half of patch (untested) (51.34 KB, patch)
2004-08-19 18:59 PDT, David Baron :dbaron: ⌚️UTC-7 (busy September 14-25)
no flags Details | Diff | Splinter Review
patch (untested) (127.53 KB, patch)
2004-08-20 00:49 PDT, David Baron :dbaron: ⌚️UTC-7 (busy September 14-25)
no flags Details | Diff | Splinter Review
patch (partly tested) (158.75 KB, patch)
2004-08-20 03:12 PDT, David Baron :dbaron: ⌚️UTC-7 (busy September 14-25)
no flags Details | Diff | Splinter Review
test showing all combinations (6.13 KB, text/html; charset=UTF-8)
2004-08-20 11:27 PDT, David Baron :dbaron: ⌚️UTC-7 (busy September 14-25)
no flags Details
patch (175.72 KB, patch)
2004-08-20 11:37 PDT, David Baron :dbaron: ⌚️UTC-7 (busy September 14-25)
no flags Details | Diff | Splinter Review
patch (176.23 KB, patch)
2004-08-20 11:40 PDT, David Baron :dbaron: ⌚️UTC-7 (busy September 14-25)
no flags Details | Diff | Splinter Review
patch (175.38 KB, patch)
2004-08-20 13:38 PDT, David Baron :dbaron: ⌚️UTC-7 (busy September 14-25)
roc: review+
roc: superreview+
Details | Diff | Splinter Review

Description Cory Prowse 2001-03-20 17:26:07 PST
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 [:bz] (still a bit busy) 2001-03-20 18:04:42 PST
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) 2001-03-21 00:21:45 PST
We have our own extensions to do this -- overflow: -moz-horizontal or something.

I suggest WONTFIX.
Comment 3 Pierre Saslawsky 2001-04-25 01:52:34 PDT
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) 2001-05-02 23:24:53 PDT
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 2001-08-15 13:10:28 PDT
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) 2002-04-05 16:01:42 PST
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 2002-10-29 15:41:12 PST
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 [:bz] (still a bit busy) 2002-10-29 16:17:53 PST
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) 2002-10-29 21:25:16 PST
How does what IE does differ from the CSS3 proposal?
Comment 10 Boris Zbarsky [:bz] (still a bit busy) 2002-10-29 21:27:29 PST
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 2002-10-30 10:38:13 PST
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 2003-05-05 13:14:01 PDT
*** Bug 204309 has been marked as a duplicate of this bug. ***
Comment 13 Boris Zbarsky [:bz] (still a bit busy) 2003-09-25 13:58:03 PDT
*** Bug 220289 has been marked as a duplicate of this bug. ***
Comment 14 Anne (:annevk) 2003-12-20 10:14:33 PST
Created attachment 137767 [details]
basic test case
Comment 15 Mats Palmgren (:mats) 2004-04-27 18:07:47 PDT
*** Bug 241902 has been marked as a duplicate of this bug. ***
Comment 16 David Baron :dbaron: ⌚️UTC-7 (busy September 14-25) 2004-08-19 18:59:14 PDT
Created attachment 156556 [details] [diff] [review]
CSS half of patch (untested)
Comment 17 David Baron :dbaron: ⌚️UTC-7 (busy September 14-25) 2004-08-20 00:49:43 PDT
Created attachment 156572 [details] [diff] [review]
patch (untested)
Comment 18 David Baron :dbaron: ⌚️UTC-7 (busy September 14-25) 2004-08-20 03:12:46 PDT
Created attachment 156582 [details] [diff] [review]
patch (partly tested)
Comment 19 David Baron :dbaron: ⌚️UTC-7 (busy September 14-25) 2004-08-20 11:27:22 PDT
Created attachment 156617 [details]
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-7 (busy September 14-25) 2004-08-20 11:37:29 PDT
Created attachment 156619 [details] [diff] [review]
patch
Comment 21 David Baron :dbaron: ⌚️UTC-7 (busy September 14-25) 2004-08-20 11:40:28 PDT
Created attachment 156620 [details] [diff] [review]
patch
Comment 22 David Baron :dbaron: ⌚️UTC-7 (busy September 14-25) 2004-08-20 12:02:52 PDT
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) 2004-08-20 12:31:14 PDT
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-7 (busy September 14-25) 2004-08-20 12:55:17 PDT
Comment on attachment 156620 [details] [diff] [review]
patch

Actually, hold on.  I've regressed attachment 145385 [details].
Comment 25 David Baron :dbaron: ⌚️UTC-7 (busy September 14-25) 2004-08-20 13:38:21 PDT
Created attachment 156629 [details] [diff] [review]
patch
Comment 26 Robert O'Callahan (:roc) (email my personal email if necessary) 2004-08-25 21:28:37 PDT
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 2004-08-26 11:40:27 PDT
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 2004-08-26 11:45:38 PDT
mOverflowX and mOverflowY should be used insted of mOverflow.
Comment 29 neil@parkwaycc.co.uk 2004-08-26 16:12:15 PDT
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 2004-08-26 16:38:27 PDT
Bug 76197.
Comment 31 David Baron :dbaron: ⌚️UTC-7 (busy September 14-25) 2004-08-26 18:27:15 PDT
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 2004-08-27 18:30:13 PDT
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-7 (busy September 14-25) 2004-09-03 14:56:21 PDT
For the record, my standard set of testcases is bug 69355 comment 34.
Comment 34 David Baron :dbaron: ⌚️UTC-7 (busy September 14-25) 2004-09-03 23:39:54 PDT
I think part of the problem was the text input changes.
Comment 35 David Baron :dbaron: ⌚️UTC-7 (busy September 14-25) 2004-09-04 12:29:26 PDT
The frame / iframe changes also, to my surprise, seem to be a part of the
regression.
Comment 36 David Baron :dbaron: ⌚️UTC-7 (busy September 14-25) 2004-09-04 21:09:49 PDT
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) 2004-09-05 03:29:17 PDT
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) 2004-09-05 05:00:09 PDT
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) 2004-09-05 05:19:23 PDT
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-7 (busy September 14-25) 2004-09-05 11:19:15 PDT
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) 2004-09-06 23:56:22 PDT
This fix caused bug 258228.
Comment 42 David Baron :dbaron: ⌚️UTC-7 (busy September 14-25) 2004-09-07 18:45:35 PDT
This caused regression bug 258300.
Comment 43 Ryan VanderMeulen [:RyanVM] 2005-06-29 18:12:47 PDT
This patch appears to have caused bug 282750

Note You need to log in before you can comment on or make changes to this bug.