Closed Bug 261037 Opened 20 years ago Closed 11 years ago

overflow property not implemented on fieldset

Categories

(Core :: Layout: Form Controls, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla27
Tracking Status
firefox24 - ---
firefox25 - ---
firefox27 --- disabled
firefox28 --- fixed
relnote-firefox --- 28+

People

(Reporter: bjoern.graf, Assigned: roc)

References

Details

(Keywords: dev-doc-needed, testcase, Whiteboard: [DocArea=CSS][DocArea=HTML])

Attachments

(6 files, 4 obsolete files)

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".
Attached file Fieldset Overflow Testcase (obsolete) —
A simple HTML document to demonstrate overflow: scroll
its simply that in nsCSSFrameConstructor:ConstructFieldSetFrame should be a call
to buildscrollframe
Status: UNCONFIRMED → NEW
Ever confirmed: true
Keywords: testcase
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.
Due to public demand to demonstrate the severe impact this testcase shows
overflowing content.
Attachment #159772 - Attachment is obsolete: true
>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.
(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
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
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?
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.
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)
Still not working on FF 4.0b10...
I hit this one today. 
Pitty that this has not been looked at yet

Additional markup is NOT a solution, just a work around.
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.
Lets try this with the correct content type.
Attachment #562954 - Attachment is obsolete: true
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.
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?
Since I had to spend some time tracking this bug down myself, I'll bump this up to a "please fix", too.
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
Attachment #562955 - Attachment mime type: text/plain → text/html
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 :)
8 years and this still hasn't been fixed. Firefox is the new IE.
@Steven Vachon - Your comment shows a complete lack of knowledge in such things. It's immature and stupid.
(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.
Getting bit by this bug in version 18.0.2.
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?
EIGHT YEARS ARE YOU FICKING KIDDING ME????????????????????????????????????????????????????????????????????????????????????????????????????????????????????????????????????????????????????????????????????????

WHAT THE **** MAN
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.
Flags: needinfo?(roc)
Attached patch fix (obsolete) — Splinter Review
Assignee: nobody → roc
Attachment #816998 - Flags: review?(matspal)
Flags: needinfo?(roc)
Attached patch fix v2 (obsolete) — Splinter Review
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-
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.
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.
Attached patch fix v3Splinter Review
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/mozilla-central/rev/44de05b3239b
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla27
*gasp*
Depends on: 931464
Depends on: 931460
Depends on: 935765
Depends on: 939846
Depends on: 942341
Depends on: 951529
No longer blocks: 960277
Depends on: 960277
Flags: in-testsuite?
Whiteboard: [DocArea=CSS][DocArea=HTML]
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
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)
Flags: needinfo?(sledru)
Depends on: 971655
Setting relnote to ? so that we make sure to add this to the FF28 release notes
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.
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.
Depends on: 1429349
No longer depends on: 1429349
You need to log in before you can comment on or make changes to this bug.