overflow property not implemented on fieldset

RESOLVED FIXED in Firefox 28

Status

()

Core
Layout: Form Controls
RESOLVED FIXED
13 years ago
3 years ago

People

(Reporter: Bjoern Graf, Assigned: roc)

Tracking

(Depends on: 1 bug, {dev-doc-needed, testcase})

Trunk
mozilla27
dev-doc-needed, testcase
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite ?

Firefox Tracking Flags

(firefox24-, firefox25-, firefox27 disabled, firefox28 fixed, relnote-firefox 28+)

Details

(Whiteboard: [DocArea=CSS][DocArea=HTML])

Attachments

(6 attachments, 4 obsolete attachments)

(Reporter)

Description

13 years ago
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".
(Reporter)

Comment 1

13 years ago
Created attachment 159772 [details]
Fieldset Overflow Testcase

A simple HTML document to demonstrate overflow: scroll

Comment 2

13 years ago
its simply that in nsCSSFrameConstructor:ConstructFieldSetFrame should be a call
to buildscrollframe
Status: UNCONFIRMED → NEW
Ever confirmed: true

Updated

13 years ago
Keywords: testcase

Comment 3

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

Comment 4

13 years ago
Created attachment 159793 [details]
Fieldset Overflow Testcase 2

Due to public demand to demonstrate the severe impact this testcase shows
overflowing content.
Attachment #159772 - Attachment is obsolete: true

Comment 5

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

Comment 6

13 years ago
(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".
Setting platform and os to All.

Opera and IE have this, KHTML sort of, but very buggy.
OS: Windows XP → All
Hardware: PC → All

Comment 8

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

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

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

Updated

8 years ago
Duplicate of this bug: 504622

Comment 12

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

7 years ago
Still not working on FF 4.0b10...

Comment 14

7 years ago
I hit this one today. 
Pitty that this has not been looked at yet

Additional markup is NOT a solution, just a work around.

Updated

6 years ago
Duplicate of this bug: 663874

Comment 16

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

6 years ago
Created attachment 562955 [details]
Fieldset overflow and dynamic content testcase

Lets try this with the correct content type.
Attachment #562954 - Attachment is obsolete: true

Comment 18

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

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

6 years ago
Since I had to spend some time tracking this bug down myself, I'll bump this up to a "please fix", too.

Updated

5 years ago
Duplicate of this bug: 755667

Comment 22

5 years ago
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.
Blocks: 770645

Updated

5 years ago
Attachment #562955 - Attachment mime type: text/plain → text/html

Comment 23

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

5 years ago
http://codepen.io/joe/pen/GmAxq

Comment 25

5 years ago
8 years and this still hasn't been fixed. Firefox is the new IE.

Comment 26

5 years ago
@Steven Vachon - Your comment shows a complete lack of knowledge in such things. It's immature and stupid.

Comment 27

5 years ago
(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.
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

4 years ago
Getting bit by this bug in version 18.0.2.

Comment 30

4 years ago
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?

Updated

4 years ago
tracking-firefox24: --- → ?
tracking-firefox25: --- → ?

Comment 31

4 years ago
EIGHT YEARS ARE YOU FICKING KIDDING ME????????????????????????????????????????????????????????????????????????????????????????????????????????????????????????????????????????????????????????????????????????

WHAT THE **** MAN

Comment 32

4 years ago
Yup, Firefox is ****. I treat it like I do IE: for testing-only.

(In reply to mokoko from comment #31)
Tracking is only used for new regressions/issues. Perhaps roc will have an update on if/when this bug may be fixed.
tracking-firefox24: ? → -
tracking-firefox25: ? → -
Flags: needinfo?(roc)
Created attachment 816998 [details] [diff] [review]
fix
Assignee: nobody → roc
Attachment #816998 - Flags: review?(matspal)
Flags: needinfo?(roc)
Created attachment 817599 [details] [diff] [review]
fix v2

Fixes some major issues. Green on try: https://tbpl.mozilla.org/?tree=Try&rev=6f24609379af
Attachment #816998 - Attachment is obsolete: true
Attachment #816998 - Flags: review?(matspal)
Attachment #817599 - Flags: review?(matspal)
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.
Attachment #817599 - Flags: review?(matspal) → review-
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.
Created attachment 818036 [details]
dynamic <legend> insertions

Dynamic <legend> insertions are broken when there's a scroll frame.
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 ;-)
(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 :-)
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...
(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.
(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.
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.
Attachment #817599 - Attachment is obsolete: true
Attachment #820669 - Flags: review?(matspal)
Comment on attachment 820669 [details] [diff] [review]
fix v3

Looks good! r=mats
Attachment #820669 - Flags: review?(matspal) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/ffd9c7bbd2fe
Backed out for:
https://tbpl.mozilla.org/php/getParsedLog.php?id=29549074&tree=Mozilla-Inbound
https://tbpl.mozilla.org/php/getParsedLog.php?id=29548883&tree=Mozilla-Inbound
https://tbpl.mozilla.org/php/getParsedLog.php?id=29548849&tree=Mozilla-Inbound
etc

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/6817b1c1439f
http://hg.mozilla.org/integration/mozilla-inbound/rev/ce20f4b16d56
Push backed out for reftest failures:
https://tbpl.mozilla.org/?tree=Mozilla-Inbound&jobname=reftest&rev=75ee2a0bc5d3

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/efe4b4053304
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/e33a8cab1096
https://tbpl.mozilla.org/?tree=Try&rev=c52e80b74d53
https://hg.mozilla.org/integration/mozilla-inbound/rev/44de05b3239b
https://hg.mozilla.org/mozilla-central/rev/44de05b3239b
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla27

Comment 52

4 years ago
*gasp*
relnote-firefox: --- → ?
Keywords: dev-doc-needed

Updated

4 years ago
Depends on: 931464

Updated

4 years ago
Depends on: 931460

Updated

4 years ago
relnote-firefox: ? → 27+

Updated

4 years ago
Depends on: 935765

Updated

4 years ago
Depends on: 939846
Depends on: 942341

Updated

4 years ago
Depends on: 951529
Backed out from beta (Fx27) due to bug 960277.
https://hg.mozilla.org/releases/mozilla-beta/rev/00b274fbebf3
Blocks: 960277
status-firefox27: --- → wontfix
status-firefox28: --- → fixed
No longer blocks: 960277
Depends on: 960277
Flags: in-testsuite?
Whiteboard: [DocArea=CSS][DocArea=HTML]

Comment 54

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

3 years ago
Why is this still in the release notes of FF27?
See comment 53
Flags: needinfo?(bbajaj)
Removing feedback from bbajaj and notifying Sylvestre, the new Fx Release Manager.
Flags: needinfo?(bbajaj) → needinfo?(sledru)
relnote-firefox: 27+ → 28+
Flags: needinfo?(sledru)
Depends on: 971655
Setting relnote to ? so that we make sure to add this to the FF28 release notes
relnote-firefox: 28+ → ?
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.
relnote-firefox: ? → 28+
Target Milestone: mozilla27 → mozilla28
> 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"?
Flags: needinfo?(sledru)
Target Milestone: mozilla28 → mozilla27
Are you sure about the change of Target Milestone ?
OK for the description. I have updated the nucleus db.
Thanks
Flags: needinfo?(sledru) → needinfo?(bzbarsky)
> 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.
Flags: needinfo?(bzbarsky)
(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?
No idea when to use "disabled" vs "wontfix"....
(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.
status-firefox27: wontfix → disabled
You need to log in before you can comment on or make changes to this bug.