261037 - overflow property not implemented on fieldset

Status: RESOLVED FIXED

(Core :: Layout: Form Controls, defect)

Description

[Bjoern Graf]

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]

Attachment: Fieldset Overflow Testcase

A simple HTML document to demonstrate overflow: scroll

Comment 2

[Bernd]

its simply that in nsCSSFrameConstructor:ConstructFieldSetFrame should be a call
to buildscrollframe

Comment 3

[Bernd]

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]

Attachment: Fieldset Overflow Testcase 2

Due to public demand to demonstrate the severe impact this testcase shows
overflowing content.

Comment 5

[Bernd]

>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]

(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]]

Setting platform and os to All.

Opera and IE have this, KHTML sort of, but very buggy.

Comment 8

[Magnus Melin [:mkmelin]]

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]

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]

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 12

[Kai Tallafus]

Attachment: 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]

Still not working on FF 4.0b10...

Comment 14

[Geoffrey Knutzen]

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 16

[Tim Heap]

Attachment: 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]

Attachment: Fieldset overflow and dynamic content testcase

Lets try this with the correct content type.

Comment 18

[patrick]

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.]

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]

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

Comment 22

[Tim Heap]

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]

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]

http://codepen.io/joe/pen/GmAxq

Comment 25

[Steven Vachon]

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

Comment 26

[drhowarddrfine]

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

Comment 27

[Steven Vachon]

(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]]

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]

Getting bit by this bug in version 18.0.2.

Comment 30

[Force Flow]

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]

EIGHT YEARS ARE YOU FICKING KIDDING ME????????????????????????????????????????????????????????????????????????????????????????????????????????????????????????????????????????????????????????????????????????

WHAT THE **** MAN

Comment 32

[Steven Vachon]

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]]

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)]

Attachment: fix

Comment 35

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: fix v2

Fixes some major issues. Green on try: https://tbpl.mozilla.org/?tree=Try&rev=6f24609379af

Comment 36

[Mats Palmgren (inactive)]

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 (inactive)]

Attachment: 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 (inactive)]

Attachment: dynamic <legend> insertions

Dynamic <legend> insertions are broken when there's a scroll frame.

Comment 39

[Mats Palmgren (inactive)]

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)]

(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 (inactive)]

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)]

(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)]

(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)]

Attachment: 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 (inactive)]

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)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/ffd9c7bbd2fe

Comment 47

[Ed Morley [:emorley]]

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

Comment 48

[Robert O'Callahan (:roc) (email my personal email if necessary)]

http://hg.mozilla.org/integration/mozilla-inbound/rev/ce20f4b16d56

Comment 49

[Ed Morley [:emorley]]

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

Comment 50

[Robert O'Callahan (:roc) (email my personal email if necessary)]

https://tbpl.mozilla.org/?tree=Try&rev=c52e80b74d53
https://hg.mozilla.org/integration/mozilla-inbound/rev/44de05b3239b

Comment 51

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/44de05b3239b

Comment 52

[Steven Vachon]

*gasp*

Comment 53

[Ryan VanderMeulen [:RyanVM]]

Backed out from beta (Fx27) due to bug 960277.
https://hg.mozilla.org/releases/mozilla-beta/rev/00b274fbebf3

Comment 54

[Rik]

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]

Why is this still in the release notes of FF27?
See comment 53

Comment 56

[Jean-Yves Perrier [:teoli]]

Removing feedback from bbajaj and notifying Sylvestre, the new Fx Release Manager.

Comment 57

[Lukas Blakk [:lsblakk] use ?needinfo]

Setting relnote to ? so that we make sure to add this to the FF28 release notes

Comment 58

[Sylvestre Ledru [:Sylvestre]]

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 [:bzbarsky]]

> 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]]

Are you sure about the change of Target Milestone ?
OK for the description. I have updated the nucleus db.
Thanks

Comment 61

[Boris Zbarsky [: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.

Comment 62

[u279076]

(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 [:bzbarsky]]

No idea when to use "disabled" vs "wontfix"....

Comment 64

[u279076]

(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.

Comment 65

[Nadia]

(In reply to Force Flow from comment #30)
> 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?

(In reply to mokoko from comment #31)
> EIGHT YEARS ARE YOU FICKING KIDDING
> ME???????????????????????????????????????????????????????????????????????????
> ?????????????????????????????????????????????????????????????????????????????
> ????????????????
> 
> WHAT THE **** MAN

█████████████████████ FOURTEEN YEARS!!! ████████████████████████


██████████████████ Yeah, we are fycking kidding all users. █████████████████

Here a test-case showing the problem is not resolved. 

HTML:

<!DOCTYPE html>
<html><head><title>Test case</title></head>
<body>
<fieldset>
<p>
   ItIsALongTextForFieldsetToTestTheOverflowHiddenAndBorderRadius
</p>
</fieldset>
<div>
<p>
   ItIsALongTextToCompareItWithProperDisplaingThis.WorksInMSIE...
</p>
</div>
</body>
</html>

CSS:

*
{
  margin:0;
  padding:0;
  border:none;
  border-radius:0;
  box-sizing:border-box;
  outline:none;
  box-shadow:none;
  padding:2px;
}
fieldset,div
{
  border:2px solid #00f;
  width:200px;
  border-radius:50%;
  background:#0f0;
  display:block;
  overflow:hidden;
}
p
{
  background:#f00;
  display:block;
}


██████████ FIXED? Really? This fix doesn't work for me like for millions of people from Stack Overflow. ██████████
It is high time to fix it because Chrome and part other browsers made special handling of fieldset especially for compatibility with Firefox. Additionally big part of solutions from Stack Overflow for this problem stopped to work...

Comment 66

[Ed Morley [:emorley]]

This bug was closed 4 years ago, so if it's regressed since, or if there's remaining issues with the implementation it's best to file a new bug with the testcase, mark it blocking this bug, and if needs be add a comment referencing it here.

(PS: I'm very rarely the person you want to be needinfo-ing, my username just happened to be the one that used to run the bug update tool that syncs bugs with commits that land in the repository - I have zero knowledge about this bug or the layout component)

Comment 67

[Nadia]

>  mark it blocking this bug

How to do it? How I can point without privileged this bug was not fixed? 
I dont know how it to do but this bug is still not fixed or returned.

Comment 68

[Nadia]

BTW. I needinfo-ing you because you changed the status to FIXED but problem is not fixed. Please check the testcase. Both object should be due specification shown identical.

Comment 69

[Ed Morley [:emorley]]

I explained in comment 66 why I'm not the correct person to ask about this.

(In reply to [e-mail] from comment #67)
> >  mark it blocking this bug
> 
> How to do it? How I can point without privileged this bug was not fixed? 
> I dont know how it to do but this bug is still not fixed or returned.

When you file a new bug, there is a "blocks" field. Add the bug number of this bug (261037) to it. If you can't see a "blocks" field it might be because you're in the simple-bug-creation view. I forget whether non-editbugs users can see the advanced bug creation view - if not, don't worry about setting the blocks field - just file a new bug and comment with the bug number here (but please don't needinfo me again, since I have nothing to do with the layout code).

Comment 70

[Boris Zbarsky [:bzbarsky]]

I will follow up in bug 1429349, but for the record this looks like it works fine.  In particular, the testcase from comment 65, which I uploaded as attachement 8941449, shows the div and fieldset behaving identically as far as I can tell.

It's possible Nadia is using an old version of Firefox (52 or earlier) and running into bug 504622, with the "min-width:0" not having any effect on the fieldset...   None of that has to do with this bug.