Last Comment Bug 261037 - overflow property not implemented on fieldset
: overflow property not implemented on fieldset
Status: RESOLVED FIXED
[DocArea=CSS][DocArea=HTML]
: dev-doc-needed, testcase
Product: Core
Classification: Components
Component: Layout: Form Controls (show other bugs)
: Trunk
: All All
: -- normal with 32 votes (vote)
: mozilla27
Assigned To: Robert O'Callahan (:roc) (email my personal email if necessary)
:
:
Mentors:
: 663874 755667 (view as bug list)
Depends on: 942341 931460 931464 935765 939846 951529 960277 971655
Blocks: 770645
  Show dependency treegraph
 
Reported: 2004-09-22 11:31 PDT by Bjoern Graf
Modified: 2014-03-15 16:55 PDT (History)
35 users (show)
anthony.s.hughes: in‑testsuite?
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
-
-
disabled
fixed
28+


Attachments
Fieldset Overflow Testcase (376 bytes, text/html)
2004-09-22 11:33 PDT, Bjoern Graf
no flags Details
Fieldset Overflow Testcase 2 (1.40 KB, text/html)
2004-09-22 14:42 PDT, Bjoern Graf
no flags Details
Fieldset overflow:hidden testcase (1.11 KB, text/html)
2010-06-05 02:34 PDT, Kai Tallafus
no flags Details
Fieldset overflow and dynamic content testcase (3.24 KB, text/plain)
2011-09-27 19:48 PDT, Tim Heap
no flags Details
Fieldset overflow and dynamic content testcase (3.11 KB, text/html)
2011-09-27 19:55 PDT, Tim Heap
no flags Details
fix (20.38 KB, patch)
2013-10-14 23:54 PDT, Robert O'Callahan (:roc) (email my personal email if necessary)
no flags Details | Diff | Splinter Review
fix v2 (33.41 KB, patch)
2013-10-15 19:46 PDT, Robert O'Callahan (:roc) (email my personal email if necessary)
mats: review-
Details | Diff | Splinter Review
some overflow:auto reftests (3.42 KB, patch)
2013-10-16 13:24 PDT, Mats Palmgren (:mats)
no flags Details | Diff | Splinter Review
dynamic <legend> insertions (778 bytes, text/html)
2013-10-16 13:27 PDT, Mats Palmgren (:mats)
no flags Details
fix v3 (40.35 KB, patch)
2013-10-22 15:45 PDT, Robert O'Callahan (:roc) (email my personal email if necessary)
mats: review+
Details | Diff | Splinter Review

Description Bjoern Graf 2004-09-22 11:31:16 PDT
User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; rv:1.7.3) Gecko/20040913 Firefox/0.10
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; rv:1.7.3) Gecko/20040913 Firefox/0.10

When assigning a value other than visible to the overflow property of a fixed
height/width fieldset its content is rendered as if overflow: visible was specified.

Reproducible: Always
Steps to Reproduce:
1. add a fieldset element with its style set to "overflow: scroll; height: 2em;"
2. open the document and notice the lack of (disabled) scrollbars
Actual Results:  
The overflowing content of the fieldset is visible.

Expected Results:  
The overflowing content of the fieldset should be clipped and, in case of
scroll/auto, scrollbars should be added to the fieldset.

bernd on IRC requested to add "its probably a frame construction bug".
Comment 1 Bjoern Graf 2004-09-22 11:33:36 PDT
Created attachment 159772 [details]
Fieldset Overflow Testcase

A simple HTML document to demonstrate overflow: scroll
Comment 2 Bernd 2004-09-22 12:19:47 PDT
its simply that in nsCSSFrameConstructor:ConstructFieldSetFrame should be a call
to buildscrollframe
Comment 3 Bernd 2004-09-22 12:29:53 PDT
Mats, didn't you already work in frame construction and form controls?
If that isn't the class of bugs that you are interested in (CC), just drop me an
mail I'll stop.
Comment 4 Bjoern Graf 2004-09-22 14:42:32 PDT
Created attachment 159793 [details]
Fieldset Overflow Testcase 2

Due to public demand to demonstrate the severe impact this testcase shows
overflowing content.
Comment 5 Bernd 2004-09-22 22:01:57 PDT
>severe impact
Hey come on, look at the bug number, this has never worked. It took 6 years of
the project to get this bug filed, before you nobody noticed the "severe
impact". I am not arguing against the bug or against fixing it, but severe is
something else.
Comment 6 Bjoern Graf 2004-09-22 22:22:23 PDT
(In reply to comment #5)
> >severe impact
That should have read 'the "severe impact"', and if I really felt this bug to be
a showstopper I would have set the Severity to either critical or blocker. I was
quite happy with the inital testcase as it did show the issue in the least lines
of code but someone convinced me to changing it to display data falling out of
it. I couldn't care less whether or not this bug is ever fixed, knowing that it
exists and the workaround (adding an overflowing div around the content) is
enough for me. To reiterate: "severe impact" wasn't meant as a Severity rating
on this bug, only a out-of-context quote. Same applies to the "public demand".
Comment 7 Marek Stępień [:marcoos, inactive] 2005-06-16 05:52:57 PDT
Setting platform and os to All.

Opera and IE have this, KHTML sort of, but very buggy.
Comment 8 Magnus Melin 2005-09-29 10:01:45 PDT
overflow:hidden seem to work now. well kind of work... text rows are not cut off
at correct point if they should be cut off in the middle of the row).
overflow:scroll still broken. 

Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8b5) Gecko/20050929 Firefox/1.4
ID:2005092905
Comment 9 Clint Hall 2006-07-20 11:09:31 PDT
Adding a DIV around it is not a good work-around.  The fieldset should scroll if overflow is set to auto.

Are there no plans to fix this?
Comment 10 Tim Cameron Ryan 2008-08-04 22:42:21 PDT
I just triggered this bug today; reading scrollHeight from an overflowing fieldset didn't return any useful information, since the fieldset never overflowed! Obscure enough that it'd rarely be an issue, but major enough that I need to specifically code around it. I'd also like to see this bug fixed.
Comment 11 Kevin Brosnan 2009-07-16 12:32:39 PDT
*** Bug 504622 has been marked as a duplicate of this bug. ***
Comment 12 Kai Tallafus 2010-06-05 02:34:53 PDT
Created attachment 449431 [details]
Fieldset overflow:hidden testcase

I came across this bug today. In a form i have a fieldset with fixed height and overflow: hidden but the content does not get clipped. This problem is reproduceable sice Firefox 3.6.x. In older Versions like 3.5.8 oder 3.0.16 the problem does not exist for me. I attached another testcase.

Mozilla/5.0 (Windows; U; Windows NT 5.1; de; rv:1.9.2.3) Gecko/20100401 Firefox/3.6.3 (.NET CLR 3.5.30729)
Comment 13 Marc-Aurel 2011-02-02 03:06:36 PST
Still not working on FF 4.0b10...
Comment 14 Geoffrey Knutzen 2011-02-10 11:22:32 PST
I hit this one today. 
Pitty that this has not been looked at yet

Additional markup is NOT a solution, just a work around.
Comment 15 j.j. 2011-06-13 13:53:41 PDT
*** Bug 663874 has been marked as a duplicate of this bug. ***
Comment 16 Tim Heap 2011-09-27 19:48:51 PDT
Created attachment 562954 [details]
Fieldset overflow and dynamic content testcase

This test case shows the problem to be even worse when there is dynamic content involved. Not only does the fieldset not crop its contents with overflow: hidden, but any protruding content is not erased and redrawn correctly if the fieldset moves. This can lead to artefacts being left on the page as the fieldset is moved around.
Comment 17 Tim Heap 2011-09-27 19:55:32 PDT
Created attachment 562955 [details]
Fieldset overflow and dynamic content testcase

Lets try this with the correct content type.
Comment 18 patrick 2011-10-10 18:34:01 PDT
I came across this today on OS X version 7.0.1

Like others, if I replaced the <fieldset> with <div> with overflow:hidden the overflowing content was hidden correctly, otherwise it was shown as visible.
Comment 19 Eric Sh. 2011-11-05 15:14:16 PDT
I am experiencing the same bug on latest nightly, overflow:hidden should work.
This has been going for seven years, isn't there anyone that can fix this?
Comment 20 drhowarddrfine 2011-12-06 14:17:51 PST
Since I had to spend some time tracking this bug down myself, I'll bump this up to a "please fix", too.
Comment 21 Daniel.S 2012-05-16 09:54:59 PDT
*** Bug 755667 has been marked as a duplicate of this bug. ***
Comment 22 Tim Heap 2012-05-28 21:11:44 PDT
Dynamically changing page content above a fieldset no longer leaves artefacts on the page, as mentioned in my comment #16 above.

Overflow is still not implemented for fieldsets, so this bug is still valid.
Comment 23 Damian Senn 2012-11-14 08:12:10 PST
Just ran into this issue, easily to workaround by putting a div inside the fieldset... but well... a fix would of course be welcome too :)
Comment 24 synpp 2012-12-05 02:33:01 PST
http://codepen.io/joe/pen/GmAxq
Comment 25 Steven Vachon 2013-01-08 06:35:34 PST
8 years and this still hasn't been fixed. Firefox is the new IE.
Comment 26 drhowarddrfine 2013-01-08 07:28:30 PST
@Steven Vachon - Your comment shows a complete lack of knowledge in such things. It's immature and stupid.
Comment 27 Steven Vachon 2013-01-08 07:52:53 PST
(In reply to drhowarddrfine from comment #26)
> @Steven Vachon - Your comment shows a complete lack of knowledge in such
> things. It's immature and stupid.

Hi.
Comment 28 Marek Stępień [:marcoos, inactive] 2013-01-10 14:53:34 PST
Steven, drhowarddrfine - please read Bugzilla etiquette at https://bugzilla.mozilla.org/page.cgi?id=etiquette.html and refrain from posting useless comments (comment 25) and personal attacks (comment 26).

If you want to show that you care about this bug getting fixed, please use the voting feature, or help writing code, that's always welcome.
Comment 29 Robert McKee 2013-02-06 10:56:33 PST
Getting bit by this bug in version 18.0.2.
Comment 30 Force Flow 2013-10-03 14:16:12 PDT
I encountered this bug in version 24.

This was first reported in 2004. Other browsers (Chrome, IE, Safari, Opera) do not exhibit this issue.

Why has this not been fixed yet?
Comment 31 mokoko 2013-10-06 11:15:35 PDT
EIGHT YEARS ARE YOU FICKING KIDDING ME????????????????????????????????????????????????????????????????????????????????????????????????????????????????????????????????????????????????????????????????????????

WHAT THE **** MAN
Comment 32 Steven Vachon 2013-10-06 11:41:43 PDT
Yup, Firefox is ****. I treat it like I do IE: for testing-only.

(In reply to mokoko from comment #31)
Comment 33 Alex Keybl [:akeybl] 2013-10-08 06:29:48 PDT
Tracking is only used for new regressions/issues. Perhaps roc will have an update on if/when this bug may be fixed.
Comment 34 Robert O'Callahan (:roc) (email my personal email if necessary) 2013-10-14 23:54:03 PDT
Created attachment 816998 [details] [diff] [review]
fix
Comment 35 Robert O'Callahan (:roc) (email my personal email if necessary) 2013-10-15 19:46:59 PDT
Created attachment 817599 [details] [diff] [review]
fix v2

Fixes some major issues. Green on try: https://tbpl.mozilla.org/?tree=Try&rev=6f24609379af
Comment 36 Mats Palmgren (:mats) 2013-10-16 13:21:27 PDT
Comment on attachment 817599 [details] [diff] [review]
fix v2

>layout/base/nsCSSFrameConstructor.cpp
>+  fieldsetFrame->AddStateBits(NS_FRAME_CAN_HAVE_ABSPOS_CHILDREN);
>+  if (fieldsetFrame->IsPositioned()) {
>+    aState.PushAbsoluteContainingBlock(fieldsetFrame, fieldsetFrame, absoluteSaveState);
>   }

Hmm, I think we want a positioned scrolled fieldset to behave the same
as an ordinary positioned scroll frame where the inner scrolled frame
acts as the abs.pos. containing block.

If the fieldset frame is the containing block then overflow contributions
from positioned children are not taken into account by the scroll frame.
See attached reftest, which I think should pass.


>-      (!aParentFrame ||
>-       (aParentFrame->GetType() != nsGkAtoms::fieldSetFrame &&
>-        aParentFrame->StyleContext()->GetPseudo() !=
>-          nsCSSAnonBoxes::fieldsetContent) ||
>+      (!IsFrameForFieldSet(aParentFrame) ||

I would prefer to keep the explicit "!aParentFrame" here and remove
the null-check in IsFrameForFieldSet.


>layout/forms/nsFieldSetFrame.cpp
>+  virtual nsIScrollableFrame* GetScrollTargetFrame() MOZ_OVERRIDE
>+  {
>+    return do_QueryFrame(GetInner());
>+  }

Doesn't this require nsIScrollableFrame.h to compile? If so, I think we
should add that explicitly to the list of #includes instead of depending
on some other header doing so.  (I might mistaken though)


>layout/generic/nsHTMLReflowState.cpp

Seems unnecessary to touch this file just to remove a space.
Comment 37 Mats Palmgren (:mats) 2013-10-16 13:24:30 PDT
Created attachment 818033 [details] [diff] [review]
some overflow:auto reftests

I thought it would be good to have some overflow:auto tests as well,
so I wrote some.  This also demonstrates the abs.pos. error described
above.  Please add this to the patch set.
Comment 38 Mats Palmgren (:mats) 2013-10-16 13:27:48 PDT
Created attachment 818036 [details]
dynamic <legend> insertions

Dynamic <legend> insertions are broken when there's a scroll frame.
Comment 39 Mats Palmgren (:mats) 2013-10-16 13:30:41 PDT
FYI, while testing this I also discovered that we get the fieldset height wrong
in an edge case, bug 927484.  It's unrelated to your changes though, but feel
free to have a look while you're here ;-)
Comment 40 Robert O'Callahan (:roc) (email my personal email if necessary) 2013-10-16 13:41:25 PDT
(In reply to Mats Palmgren (:mats) from comment #39)
> FYI, while testing this I also discovered that we get the fieldset height
> wrong
> in an edge case, bug 927484.  It's unrelated to your changes though, but feel
> free to have a look while you're here ;-)

I'd rather not turn this into a can of worms, thanks :-)
Comment 41 Mats Palmgren (:mats) 2013-10-16 14:01:39 PDT
Yeah, I didn't mean to suggest it should be fixed in this bug.  Just that it might be easy
to fix next while you have this code fresh in your head.  It's an unimportant edge case though
so it can wait...
Comment 42 Robert O'Callahan (:roc) (email my personal email if necessary) 2013-10-22 15:22:16 PDT
(In reply to Mats Palmgren (:mats) from comment #36)
> Hmm, I think we want a positioned scrolled fieldset to behave the same
> as an ordinary positioned scroll frame where the inner scrolled frame
> acts as the abs.pos. containing block.
> 
> If the fieldset frame is the containing block then overflow contributions
> from positioned children are not taken into account by the scroll frame.
> See attached reftest, which I think should pass.

OK, I've fixed that.

> >-      (!aParentFrame ||
> >-       (aParentFrame->GetType() != nsGkAtoms::fieldSetFrame &&
> >-        aParentFrame->StyleContext()->GetPseudo() !=
> >-          nsCSSAnonBoxes::fieldsetContent) ||
> >+      (!IsFrameForFieldSet(aParentFrame) ||
> 
> I would prefer to keep the explicit "!aParentFrame" here and remove
> the null-check in IsFrameForFieldSet.

OK.

> >layout/forms/nsFieldSetFrame.cpp
> >+  virtual nsIScrollableFrame* GetScrollTargetFrame() MOZ_OVERRIDE
> >+  {
> >+    return do_QueryFrame(GetInner());
> >+  }
> 
> Doesn't this require nsIScrollableFrame.h to compile? If so, I think we
> should add that explicitly to the list of #includes instead of depending
> on some other header doing so.  (I might mistaken though)

OK.

> >layout/generic/nsHTMLReflowState.cpp
> 
> Seems unnecessary to touch this file just to remove a space.

OK.
Comment 43 Robert O'Callahan (:roc) (email my personal email if necessary) 2013-10-22 15:24:08 PDT
(In reply to Mats Palmgren (:mats) from comment #37)
> Created attachment 818033 [details] [diff] [review]
> some overflow:auto reftests
> 
> I thought it would be good to have some overflow:auto tests as well,
> so I wrote some.  This also demonstrates the abs.pos. error described
> above.  Please add this to the patch set.

OK, added.
Comment 44 Robert O'Callahan (:roc) (email my personal email if necessary) 2013-10-22 15:45:24 PDT
Created attachment 820669 [details] [diff] [review]
fix v3

Fixed the things I mentioned above. Also fixed the dynamic legend issues by calling IsFrameForFieldSet in a couple more places in nsCSSFrameConstructor. I made it take aFrame's type as a parameter to save a virtual call for the new callers.
Comment 45 Mats Palmgren (:mats) 2013-10-23 01:37:17 PDT
Comment on attachment 820669 [details] [diff] [review]
fix v3

Looks good! r=mats
Comment 46 Robert O'Callahan (:roc) (email my personal email if necessary) 2013-10-23 06:35:53 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/ffd9c7bbd2fe
Comment 48 Robert O'Callahan (:roc) (email my personal email if necessary) 2013-10-24 00:48:05 PDT
http://hg.mozilla.org/integration/mozilla-inbound/rev/ce20f4b16d56
Comment 50 Robert O'Callahan (:roc) (email my personal email if necessary) 2013-10-25 00:47:36 PDT
https://tbpl.mozilla.org/?tree=Try&rev=c52e80b74d53
https://hg.mozilla.org/integration/mozilla-inbound/rev/44de05b3239b
Comment 51 Ed Morley [:emorley] 2013-10-25 09:55:09 PDT
https://hg.mozilla.org/mozilla-central/rev/44de05b3239b
Comment 52 Steven Vachon 2013-10-25 16:45:27 PDT
*gasp*
Comment 53 Ryan VanderMeulen [:RyanVM] 2014-01-16 09:29:40 PST
Backed out from beta (Fx27) due to bug 960277.
https://hg.mozilla.org/releases/mozilla-beta/rev/00b274fbebf3
Comment 54 Rik 2014-02-04 00:54:43 PST
I think the parent of this bug is 504622, still unresolved.
"Fieldsets never shrink below their min-intrinsic width"
https://bugzilla.mozilla.org/show_bug.cgi?id=504622
Comment 55 sjw 2014-02-08 14:07:33 PST
Why is this still in the release notes of FF27?
See comment 53
Comment 56 Jean-Yves Perrier [:teoli] 2014-02-08 14:16:58 PST
Removing feedback from bbajaj and notifying Sylvestre, the new Fx Release Manager.
Comment 57 Lukas Blakk [:lsblakk] use ?needinfo 2014-02-19 16:30:38 PST
Setting relnote to ? so that we make sure to add this to the FF28 release notes
Comment 58 Sylvestre Ledru [:sylvestre] 2014-03-06 01:25:46 PST
Added in the nucleus database for 28.
I used the commit description for the release note: "Support scrolled fieldsets"
Let me know if someone has a better wording.
Comment 59 Boris Zbarsky [:bz] (still a bit busy) 2014-03-12 22:27:52 PDT
> Let me know if someone has a better wording.

It's not just scrolling; it's also supporting overflow:hidden...  Maybe "support the 'overflow' property on fieldsets"?
Comment 60 Sylvestre Ledru [:sylvestre] 2014-03-13 02:43:55 PDT
Are you sure about the change of Target Milestone ?
OK for the description. I have updated the nucleus db.
Thanks
Comment 61 Boris Zbarsky [:bz] (still a bit busy) 2014-03-13 06:55:50 PDT
> Are you sure about the change of Target Milestone ?

Yeah, I'd just changed it from 27 to 28, then realized this has been in nightlies all through 27, but got backed out at beta, so 27 is the right TM but we're not shipping this until 28.
Comment 62 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2014-03-13 08:56:44 PDT
(In reply to Boris Zbarsky [:bz] from comment #61)
> > Are you sure about the change of Target Milestone ?
> 
> Yeah, I'd just changed it from 27 to 28, then realized this has been in
> nightlies all through 27, but got backed out at beta, so 27 is the right TM
> but we're not shipping this until 28.

So then this should be "disabled" for status-firefox27, shouldn't it?
Comment 63 Boris Zbarsky [:bz] (still a bit busy) 2014-03-13 11:39:52 PDT
No idea when to use "disabled" vs "wontfix"....
Comment 64 Anthony Hughes (:ashughes) [GFX][QA][Mentor] 2014-03-13 12:53:50 PDT
(In reply to Boris Zbarsky [:bz] from comment #63)
> No idea when to use "disabled" vs "wontfix"....

Wontfix applies when a code has not and will not be landed/uplifted for the respective Firefox version. Disabled applies when code was landed for that version but disabled, usually behind a pref.

I think in this case firefox27:disabled applies. Please correct me if you think otherwise.

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