implement overflow-y and overflow-x properties

RESOLVED FIXED in Future

Status

()

Core
CSS Parsing and Computation
--
enhancement
RESOLVED FIXED
16 years ago
11 years ago

People

(Reporter: Cory Prowse, Assigned: dbaron)

Tracking

({css3, testcase})

Trunk
Future
css3, testcase
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: WG[patch], URL)

Attachments

(3 attachments, 5 obsolete attachments)

(Reporter)

Description

16 years ago
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.
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.
Assignee: jst → pierre
Severity: major → enhancement
Status: UNCONFIRMED → NEW
Component: DOM Style → Style System
Ever confirmed: true
OS: Windows 2000 → All
Hardware: PC → All
Summary: style="OVERFLOW-Y: scroll; HEIGHT:?px;" on a <div> not scrolling → implement overflow-y and overflow-x properties
We have our own extensions to do this -- overflow: -moz-horizontal or something.

I suggest WONTFIX.
Whiteboard: WONTFIX ? -- non standards compliant

Comment 3

16 years ago
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...
Assignee: pierre → ian
Target Milestone: --- → Future
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.
Assignee: ian → glazman
Keywords: css3
Whiteboard: WONTFIX ? -- non standards compliant → WG

Comment 5

16 years ago
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.
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

15 years ago
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.
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;)
How does what IE does differ from the CSS3 proposal?
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

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

14 years ago
*** Bug 204309 has been marked as a duplicate of this bug. ***
*** Bug 220289 has been marked as a duplicate of this bug. ***

Comment 14

14 years ago
Created attachment 137767 [details]
basic test case

Updated

14 years ago
Keywords: testcase
Blocks: 42492
*** Bug 241902 has been marked as a duplicate of this bug. ***

Updated

13 years ago
Blocks: 72540
(Assignee)

Comment 16

13 years ago
Created attachment 156556 [details] [diff] [review]
CSS half of patch (untested)
Assignee: daniel → dbaron
Status: NEW → ASSIGNED
(Assignee)

Comment 17

13 years ago
Created attachment 156572 [details] [diff] [review]
patch (untested)
Attachment #156556 - Attachment is obsolete: true
(Assignee)

Updated

13 years ago
Whiteboard: WG → WG[patch]
(Assignee)

Comment 18

13 years ago
Created attachment 156582 [details] [diff] [review]
patch (partly tested)
(Assignee)

Updated

13 years ago
Attachment #156572 - Attachment is obsolete: true
(Assignee)

Comment 19

13 years ago
Created attachment 156617 [details]
test showing all combinations

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

Comment 20

13 years ago
Created attachment 156619 [details] [diff] [review]
patch
Attachment #156582 - Attachment is obsolete: true
(Assignee)

Comment 21

13 years ago
Created attachment 156620 [details] [diff] [review]
patch
(Assignee)

Updated

13 years ago
Attachment #156619 - Attachment is obsolete: true
(Assignee)

Comment 22

13 years ago
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 .
Attachment #156620 - Flags: superreview?(roc)
Attachment #156620 - Flags: review?(roc)
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.
(Assignee)

Comment 24

13 years ago
Comment on attachment 156620 [details] [diff] [review]
patch

Actually, hold on.  I've regressed attachment 145385 [details].
Attachment #156620 - Flags: superreview?(roc)
Attachment #156620 - Flags: review?(roc)
(Assignee)

Comment 25

13 years ago
Created attachment 156629 [details] [diff] [review]
patch
Attachment #156620 - Attachment is obsolete: true
(Assignee)

Updated

13 years ago
Attachment #156629 - Flags: superreview?(roc)
Attachment #156629 - Flags: review?(roc)
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.
Attachment #156629 - Flags: superreview?(roc)
Attachment #156629 - Flags: superreview+
Attachment #156629 - Flags: review?(roc)
Attachment #156629 - Flags: review+

Comment 27

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

13 years ago
mOverflowX and mOverflowY should be used insted of mOverflow.

Comment 29

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

13 years ago
Bug 76197.
(Assignee)

Comment 31

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

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

Comment 33

13 years ago
For the record, my standard set of testcases is bug 69355 comment 34.
(Assignee)

Comment 34

13 years ago
I think part of the problem was the text input changes.
(Assignee)

Comment 35

13 years ago
The frame / iframe changes also, to my surprise, seem to be a part of the
regression.
(Assignee)

Comment 36

13 years ago
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_*.
Status: ASSIGNED → RESOLVED
Last Resolved: 13 years ago
Resolution: --- → FIXED

Comment 37

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

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

Comment 40

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

13 years ago
This fix caused bug 258228.
(Assignee)

Comment 42

13 years ago
This caused regression bug 258300.
This patch appears to have caused bug 282750
You need to log in before you can comment on or make changes to this bug.