All users were logged out of Bugzilla on October 13th, 2018

Fieldsets never shrink below their min-intrinsic width, even if min-width is explicitly set to 0

RESOLVED FIXED in Firefox 53

Status

()

RESOLVED FIXED
9 years ago
8 months ago

People

(Reporter: riksoft, Assigned: bzbarsky)

Tracking

(Blocks: 1 bug, {DevAdvocacy, testcase})

Trunk
mozilla53
DevAdvocacy, testcase
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox53 fixed)

Details

(Whiteboard: [DevRel:P2])

Attachments

(4 attachments, 4 obsolete attachments)

(Reporter)

Description

9 years ago
User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.0.11) Gecko/2009060215 Firefox/3.0.11
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.0.11) Gecko/2009060215 Firefox/3.0.11

This example show the problem. IE and Opera, correctly show a fieldset containing a div with horizontal scrollbar. Firefox and Chrome (even worse) instead dont show the scroll.

Removing <fieldset> make all working ok also in Firefox.
Another way to have the scrollbar is writing the width of the div first div in pixel... but I want 100% because I need auto-resizing.

<!DOCTYPE html PUBLIC "-//W3C//DTD XHTML 1.1//EN" "xhtml11.dtd">
<html>
<head>
  <meta http-equiv="Content-Type" content="text/html; charset=windows-1252" />
  <title>fieldset</title>
</head>
<body>
<fieldset>
  <div style="width:100%;height:100px; overflow:scroll;">
    <div style="width:2000px;height:10px;background-color:red">test    test     test    test</div>
  </div>
</fieldset>
</body>
</html>


Reproducible: Always

Steps to Reproduce:
1.Try removing fieldset on the above example.
2.
3.


Expected Results:  
The expected result is having horizontal scrollbar appear.

The fieldset should act as other containers, e.g. a div. and don't have to change the way an inside div work.

I don't found a workaround and I'm going to using styled div to resemble as a fieldset with legend.
(Reporter)

Updated

9 years ago
Version: unspecified → 3.0 Branch

Comment 1

9 years ago
Created attachment 388957 [details]
testcase from comment 0

Updated

9 years ago
Status: UNCONFIRMED → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → DUPLICATE
Duplicate of bug: 261037
(Reporter)

Comment 3

9 years ago
Maybe inside the C code could be the same bug, but but 261037 is quite differente from this.

bug 261037 reported that a fieldset can't have am overflow scroll. Instead I reported that a DIV inside a fieldset loses it's capability of overflow:scroll.

So, while the 261037 could be even interpreted as a lack of feature of the fieldset, I report an interference of a fieldset over a DIV element.

The fieldset modifify the behaviour of a div in the particular situation explained in my example.
Status: RESOLVED → UNCONFIRMED
Resolution: DUPLICATE → ---

Comment 4

8 years ago
Confirmed with Mozilla/5.0 (Windows NT 6.1; Win64; x64; rv:2.0b7pre) Gecko/20101005 Firefox/4.0b7pre
Status: UNCONFIRMED → NEW
Component: General → Layout: Form Controls
Ever confirmed: true
Keywords: testcase
Product: Firefox → Core
QA Contact: general → layout.form-controls
Version: 3.0 Branch → Trunk
(Assignee)

Comment 5

8 years ago
Fieldsets in Gecko never shrink in width below their min intrinsic width, which is the min intrinsic width of their content area (ignoring legends).  And the intrinsic min width of a scrollframe is the intrinsic min width of its content, for web compat.  In particular, they do NOT do the same sizing as normal blocks, which this bug seems to expect; this is needed for web compat as well.

Note that you'd get the same behavior if you put your scrollable thing inside an auto-width float, say.

I agree that the result here is somewhat undesirable.  Perhaps we should allow fieldsets to shrink below their min-intrinsic width... that needs some study about what web pages it would break.
Summary: Fieldset makes div overflow not working → Fieldsets never shrink below their min-intrinsic width
(Reporter)

Comment 6

8 years ago
The problem is not fieldsets that never shrink: I don't bother fieldset dimension because fieldset should do NOTHING.

The strange is that a div don't understand width:100% if inside a fieldset.

Let's look at this:
<fieldset style="width:100px;background-color:#00FF00">
<!--  
  <div style="width:100%;height:100px; overflow:scroll;background-color:#808080">
    <div style="width:2000px;height:10px;background-color:red">test test</div>
  </div>
</fieldset>
-->

fieldset behave right, adjusting its dimensione to 100px

Now let's remove the rem.

<fieldset style="width:100px;background-color:#00FF00">
  <div style="width:100%;height:100px; overflow:scroll;background-color:#808080">
    <div style="width:2000px;height:10px;background-color:red">test test</div>
  </div>
</fieldset>

Now the logic is completely reversed: childs dictate what parents must do and:
1) the deeper div give the dimension to the parent div
2) for strange reason overflow inside fieldset don't work and 1st div adapt itself to its child div dimension
3) fieldset adapts itself to its child dimension (the 1st div)

Where is the logic?
Let's suppose that is right that a fieldset adapt its dimension based on that of a child div: why its 1st child div don't behave as if it was outside a fieldset since that fieldset is submitted to it?
And if fieldset is not submitted to it, then why div width:100% don't see the limits of the document/viewport as it happen when is outside the fieldset?

So the question is: the fieldset is or not a container? Why adapt to its child? 
And if a div is "stronger" than fieldset, why this don't work?

<div style="width:100%">
	<fieldset style="width:100px;background-color:#00FF00">
		<div style="width:100%;height:100px; overflow:scroll;background-color:#808080">
		  <div style="width:2000px;height:10px;background-color:red">test test</div>
		</div>
	</fieldset>
<div>

I don't see any consistency. On this point is obviuosly more consistent and logic, Opera and IE.


Note: Now this example works also in Chrome.
The only browser where don't work is Firefox

<fieldset style="width:100px;background-color:#00FF00">
  <div style="width:100%;height:100px; overflow:scroll;background-color:#808080">
    <div style="width:2000px;height:10px;background-color:red">test test</div>
  </div>
</fieldset>
(Assignee)

Comment 7

8 years ago
> The strange is that a div don't understand width:100% if inside a fieldset.

It does.  It means 100% of the width of the fieldset, whatever that is.  That's why the fieldset's width behavior matters.

> Where is the logic?

You did read comment 5, right?  It completely describes the behavior.

> and 1st div adapt itself to its child div dimension

No, it just sets its width to the width of the fieldset, whatever that is.

> the fieldset is or not a container?

That's a meaningless question; there is no such concept in CSS terms.

> Why adapt to its child?

I mentioned this in comment 5, no?  There are situations where fieldsets need to expand to the min-intrinsic width of their kids.  Now perhaps these are not all situations; see the end of comment 5.

> I don't see any consistency.

It's really a very simple behavior.  The width of the fieldset in this case is the maximum of its width as an auto-width block and the minimal intrinsic width of its descendants.  See http://hg.mozilla.org/mozilla-central/file/dee1e01fd8ed/layout/forms/nsFieldSetFrame.cpp#l394 for the code if you care

And yes, I agree that the behavior for fixed-width fieldsets is not desirable; it's there for compat with something in some cases, though.  The question is what cases.  Again, see end of comment 5.
(Reporter)

Comment 8

8 years ago
(In reply to comment #7)
> > The strange is that a div don't understand width:100% if inside a fieldset.
> 
> It does.  It means 100% of the width of the fieldset, whatever that is.  That's
> why the fieldset's width behavior matters.

This seems a paradox; I say: div 100% of what? Of the fieldset. What is fieldset 100%? You say is the 100% of the div. But that's a nonsense loop especially with overflow:scroll

I can understand that:
fieldset with 100% or no width specified can expand to meet child div needs.
BUT I don't understand why the inner div needs are to be 2000px when overflow:scroll! WHY? I can understand fieldset expansion if no overflow:scroll.
But why overflow:scroll doesn't work if inside a fieldset? You say: because filedset adapt to div. Its like Einstein paradox of child and father in a time travel! :-)

Worst yet tha using %, is the behaviuor with px
If I say: fieldset 120px; and div 100%, I expect div to be 120px (as all the browser in the world do). So if then change 120px into 100% I expect fieldset and div to be the same width of the container of the fieldset (e.g. un upper div o the viewport).

> 
> > Where is the logic?
> 
> You did read comment 5, right?  It completely describes the behavior.

Yes but I find it unreal. Especially, but not limited, to this case:

<div style="width:100px">
    <fieldset style="width:100px;background-color:#00FF00">
        <div style="width:100%;height:100px;
overflow:scroll;background-color:#808080">
          <div style="width:2000px;height:10px;background-color:red">test
test</div>
        </div>
    </fieldset>
<div>

Why, 
- despite the px, 
- despite the first div container of 100px
- despite the fieldset 100px
- the everything enlarge to 2000px? =8-o

I don't find a real, complete explanation in comment 5. It sounds like a cat that bites its tail.
(Reporter)

Comment 9

8 years ago
> No, it just sets its width to the width of the fieldset, whatever that is.

But nor fieldset to 100px, nor fieldset contained in a div if 100px, locks the fieldset to 100px or worst yet to 100% of the parent. So, where is the master, the lord of the width? :-)  I find all quite illogical.

 
> > the fieldset is or not a container?
> 
> That's a meaningless question; there is no such concept in CSS terms.

But it's a meaningful question for logical and consistent use (especially when 1 browser behave differetly against 1000 others that behave the same).


> 
> > Why adapt to its child?
> 
> I mentioned this in comment 5, no?  There are situations where fieldsets need
> to expand to the min-intrinsic width of their kids.  Now perhaps these are not
> all situations; see the end of comment 5.

OK, I consider that on comment 6 and here again, but, consistency apart, don't explain why it expand in that situation where it hasn't any need to do that: why expand if is not needed?!

We can reason:
overflow:scroll dont' act because fieldset expands.
 why it expands?
Because overflow:scroll don't work
 why don't work?
Because fieldset expand!
 why it expands?
Because overflow:scroll don't work
 why don't work?
Because fieldset expand!
 why it expands?
Because overflow:scroll don't work
.
.
.
PARADOX/loop


> 
> > I don't see any consistency.
> 
> It's really a very simple behavior.  The width of the fieldset in this case is
> the maximum of its width as an auto-width block and the minimal intrinsic width

But why it expand if not required?!
Because of overflow:scroll not working.
Why not working?! Because of what? Fieldset expanded? Noooo because that should be a paradox. So because of what? My response is: because of a bug in the project: here we have that the inner div is 2000px; its  parent div 100% should BEHAVE LIKE WITHOUT A Fieldset since fieldset is auto-inc and so should be ininfluent. So 100% should be 100% of the DOCUMENT, not of the universe. Instead we have a bud influence: fieldset make its child div to think the document width is infinite and overflow:scroll don't work.
Not good!


> of its descendants.  See
> http://hg.mozilla.org/mozilla-central/file/dee1e01fd8ed/layout/forms/nsFieldSetFrame.cpp#l394
> for the code if you care
> 
> And yes, I agree that the behavior for fixed-width fieldsets is not desirable;
> it's there for compat with something in some cases, though.  The question is
> what cases.  Again, see end of comment 5.

We should see how work GetMinWidth and what is result.width here
nscoord minWidth = GetMinWidth(aRenderingContext);
if (minWidth > result.width)

anyway, code apart is a very strange behaviour and differ at least from Opera, Chrome and IE. There is no way to cope with this problem. I don't know any workaround other than avoid using fieldset.

It's ok, not ok, correct, not correct... I don't know, but I'm sure that to me doesn't sound very good. :-)

I don't test with 100% but at least with fixed-width Firefox 2.0 worked OK. The problem arose with 3.x

Some browsers without this problem:
Opera, IE, Chrome, Seamonkey 1 (not 2), Firefox 2, Iceape
> I don't understand why the inner div needs are to be 2000px when
> overflow:scroll!

Because it turns out that web pages depend on that behavior, in general.  We tried making the min intrinsic width of scrollframes be 0 at one point, and all sorts of things on the web broke.

> I don't find a real, complete explanation in comment 5. 

That seems to be your problem... comment 5 describes exactly the algorithm the browser is following here.
And it case it wasn't clear, the behavior here is very broken, I agree.  The question is how to fix it without breaking other things.  For example, changing the min-intrinsic width of scrollframes is a no-go.
(Reporter)

Comment 12

8 years ago
> That seems to be your problem... comment 5 describes exactly the algorithm the
browser is following here.

Reading again comment 5 I probably found the tiny element of disturb: If I correctly understand, the fieldset expands based on the content of the scrollframe (the content of the div) and not based on the div width. This causes the div to see a parent of 2000px and expand also its width. Correct?


>And it case it wasn't clear, the behavior here is very broken, I agree.  The
>question is how to fix it without breaking other things.  For example, changing
>the min-intrinsic width of scrollframes is a no-go.

The compat has been already broken from 2.x to 3.x, so at least could be fixed the problem about fixed-width. About % width I don't know. On my part, the only workaround with Firefox is using JS or not using fieldset.

The problem is to decide if it is worst to broken this compat, or keeping this strange behaviour by design that give another unwanted cross browsing problem  to the developers. One solution could be the compatibility string into the page, as microzozz do in IE.
> the fieldset expands based on the content of the scrollframe

Which in this case is a <div style="width: 2000px">, yes.

> The compat has been already broken from 2.x to 3.x

There were behavior changes from 2.x to 3.x; they by and large did not break sites, though.  Compat means not breaking sites, not keeping the same behavior; else we'd never fix bugs.  ;)
(Reporter)

Comment 14

8 years ago
(In reply to comment #13)
> There were behavior changes from 2.x to 3.x; they by and large did not break
> sites, though.  Compat means not breaking sites, not keeping the same behavior;
> else we'd never fix bugs.  ;)
 :-)
I'm not too accustomed to philosophy or I'm dumb; in both case I don't understand the difference from "breaking sites" and "keeping the same behaviour".
I only understand that if a site use:

<div style="width:100px">
  <fieldset style="width:100px;background-color:#00FF00">
    <div style="width:100%;height:100px; overflow:scroll;background-color:#808080">
      <div style="width:2000px;height:10px;background-color:red">test test</div>
    </div>
  </fieldset>
<div>

with 2.0 has a behaviour (100px), 
with 3.0 another one (2000px).

This 2 different behaviour break very much a site because 100px is very different from 2000px and is surely devastating for any site. So it looks like a compat problem.

However, do what you want. FF its not on me. I only hilight problems.
Developers, as usual, will use some trickery, writing down another compatibility and cross browsing problem to be aware of.
(Assignee)

Updated

8 years ago
Duplicate of this bug: 648253
(Assignee)

Updated

8 years ago
Duplicate of this bug: 648253

Comment 17

7 years ago
You can add style="display:table-column;" to Fildset as a Workaround!

regards,
sufnoK
(Assignee)

Updated

6 years ago
Blocks: 823483

Comment 18

4 years ago
Parallel WebKit bug: https://bugs.webkit.org/show_bug.cgi?id=123507

Comment 19

4 years ago
See also: http://stackoverflow.com/questions/17408815/fieldset-resizes-wrong-appears-to-have-unremovable-min-width-min-content

The current behavior seems somewhat in line with the spec (modulo an !important, note the `min-width: min-content;` on https://html.spec.whatwg.org/multipage/rendering.html#the-fieldset-and-legend-elements ), although the spec probably only says that due to legacy horribleness.

Comment 20

4 years ago
I disagree that the current behavior is in line with the spec. It would be in line with it if `fieldset` had `min-width: (-moz-)min-content;` by default, but the author could easily override this (as currently WebKit/Blink did). Moreover, this shouldn't break much backwards compatibility (except edge cases like one mentioned in WebKit bug referenced in Comment #18 as "parallel"). Current behavior, when we have to use non-intuitive hacks (e.g. changing `display` to `table-*`, as suggested in Comment #17) in order just to control the width of the fieldset, seems like something to be fixed.
(Reporter)

Comment 21

4 years ago
Oh my gosh... this bug is still here?!
I opened this report in 2009 and in the meantime I've also changed country of residence. :-)
When I was young (born in 1966) was a common thought that in the year 2000 mankind would have been an advanced civilization traveling with UFO, dressing metallic spacesuits with antigravity boots. On the contrary, in the year 2014 we can't even deal with common standards about antediluvian stuff like CSS and HTML! :-)

Comment 22

4 years ago
Created attachment 8581987 [details]
workaround.htm

For me (Win7, FF 36) wrapping the inner divs within an element styled like this
{
 display: table;
 table-layout: fixed;
 width: 100%;
}
fixes the issue.

Comment 23

4 years ago
(In reply to Francesco from comment #22)
> For me (Win7, FF 36) wrapping the inner divs within an element styled like
> this
> {
>  display: table;
>  table-layout: fixed;
>  width: 100%;
> }
> fixes the issue.

There is also a different workaround in the answer to my Stack Overflow question (http://stackoverflow.com/a/17863685/578288):

    @-moz-document url-prefix() {
        fieldset {
            display: table-cell;
        }
    }

This workaround doesn’t require creating an extra wrapper element.

Comment 24

4 years ago
(In reply to Rory O’Kane from comment #23)
> (In reply to Francesco from comment #22)
> > For me (Win7, FF 36) wrapping the inner divs within an element styled like
> > this
> > {
> >  display: table;
> >  table-layout: fixed;
> >  width: 100%;
> > }
> > fixes the issue.
> 
> There is also a different workaround in the answer to my Stack Overflow
> question (http://stackoverflow.com/a/17863685/578288):
> 
>     @-moz-document url-prefix() {
>         fieldset {
>             display: table-cell;
>         }
>     }
> 
> This workaround doesn’t require creating an extra wrapper element.

Your workaround is surely better. However, I'm wondering why 'table-layout: fixed' can be used to work around several, possibly related, Firefox bugs: for instance, bug 823483.
So in trying to understanding the list of bugs related to max-width: 100%
I tested https://bug504622.bugzilla.mozilla.org/attachment.cgi?id=388957
in Firefox, Opera (Blink) and Safari and I'm getting the same results.

So it seems there is interop for this one (webcompat wise) (have not tested IE).
Flags: needinfo?(bzbarsky)
Flags: needinfo?(anthony)
Looks like IE 11 has different behavior.  IE Edge has the same behavior as Gecko/Blink/WebKit.
Flags: needinfo?(bzbarsky)
Thanks bz. Still in the process of understanding all the issues with regards to max-width and intrinsic sizes.

I made this to help me understand, but I guess the bug linkage is messy and the dependencies are not that meaningful There might be a way to clean that up.
http://la-grange.net/2015/07/02/max-width-moz/max-width
Comment hidden (off-topic)

Comment 29

3 years ago
Added relevant testcase to the HTML test suite: https://github.com/w3c/web-platform-tests/pull/1969

Updated

3 years ago
Blocks: 1035091
I have set this to block bug 1035091 because (anecdotally) a major legitimate use of @-moz-document in author sheets is working around this bug.  We generally don't intend to provide CSS constructs that let you apply rules to Gecko but nothing else, but I feel a degree of caution is warranted (especially since 1035091 had to be backed out once already for add-on fallout).

Updated

2 years ago
OS: Windows XP → All
Hardware: x86 → All

Updated

2 years ago
Blocks: 1230801
This is a major interop bug that the creators of popular frameworks have to hack around, including Bootstrap and Drupal. Let's fix this, and get Firefox to behave as other browsers do.
Keywords: DevAdvocacy
Whiteboard: [DevRel:P2]
Jen, the problem is that it's not clear what "fix" means here.  For example, consider this testcase:

<!DOCTYPE html>
<body style="width: 0">
  <fieldset style="width: 0; display: block; background: yellow">
    Thisisaverylongword
  </fieldset>
</body>

This shows the fieldset not shrinking below it's min-intrinsic width in Firefox, Safari, and Chrome; I don't have IE/Edge to test with right this second.

Are you suggesting we change that behavior?  If not, what change are you proposing, exactly?
Flags: needinfo?(jensimmons)
Or put another way, what is there lack of interop with?
Checked Edge on that testcase, and it matches the other browsers.
Ah, I guess one behavior difference is what happens if you explicitly set "min-width: 0" on the fieldset.  That seems like a reasonable behavior change we could make in Gecko.  Is that what you were talking about?

Comment 36

2 years ago
Making `fieldset { min-width: 0; }` disable the weird legacy behavior, like it already does in WebKit/Blink (haven't tested Edge), is all that we at Bootstrap are asking for.

Here's a testcase for your convenience:
http://w3c-test.org/html/rendering/non-replaced-elements/the-fieldset-element-0/min-width-not-important.html
(Assignee)

Updated

2 years ago
Flags: needinfo?(jensimmons)
Summary: Fieldsets never shrink below their min-intrinsic width → Fieldsets never shrink below their min-intrinsic width, even if min-width is explicitly set to 0
Created attachment 8813460 [details] [diff] [review]
part 1.  Rewrite fieldset border drawing to just clip to the area outside the legendm instead of doing it in pieces with different clip rects

This change will allow the border drawing code to deal with the following
changes, which will make us no longer force the fieldset to be wider than the
legend.  Without this change, doing that causes the vertical inline-start-side
and inline-end-side borders of the fieldset to paint under the legend, because
the current code only modifies the painting of the block-start-side border (the
one the legend is positioned on).

This does change behavior in one situation, which the new tests test.  For
relatively positioned legends, we used to use the original vertical location but
the positioned horizontal location of the legend to decide which parts of the
border to not paint.  In the new setup, we use the original location for both.
I did check that this new behavior matches Chrome and Safari.  Edge seems to
have our old behavior.
Attachment #8813460 - Flags: review?(matt.woodrow)
Attachment #8813460 - Flags: review?(dbaron)
(Assignee)

Updated

2 years ago
Assignee: nobody → bzbarsky
Status: NEW → ASSIGNED
> part 1.  Rewrite fieldset border drawing to just clip to the area outside the legendm

Assume I fixed the typo there: "legend".  ;)
Created attachment 8813462 [details] [diff] [review]
part 2.  Allow fieldsets to shrink below their intrinsic min-width and below the width of their legend if their min-width is explicitly overridden

I'm not sure how to write an automated test for the "inline-start/end-side
border doesn't get drawn under the legend" bit that part 1 was fixing, but I did
manually test it.
Attachment #8813462 - Flags: review?(dbaron)
Created attachment 8813491 [details] [diff] [review]
Part 2 with another reftest fix try found
Attachment #8813491 - Flags: review?(dbaron)
(Assignee)

Updated

2 years ago
Attachment #8813462 - Attachment is obsolete: true
Attachment #8813462 - Flags: review?(dbaron)
Comment on attachment 8813460 [details] [diff] [review]
part 1.  Rewrite fieldset border drawing to just clip to the area outside the legendm instead of doing it in pieces with different clip rects

Review of attachment 8813460 [details] [diff] [review]:
-----------------------------------------------------------------

::: layout/forms/nsFieldSetFrame.cpp
@@ +249,5 @@
> +    // to paint within our rect but outside the legend rect.
> +    RefPtr<PathBuilder> pathBuilder =
> +      drawTarget->CreatePathBuilder(FillRule::FILL_WINDING);
> +    int32_t appUnitsPerDevPixel = presContext->AppUnitsPerDevPixel();
> +    AppendRectToPath(pathBuilder,

It wouldn't hurt to add a AppendCounterClockwiseRectToPath helper instead of a bool param, but the comment above is enough.

@@ +258,5 @@
> +                     NSRectToSnappedRect(legendRect, appUnitsPerDevPixel,
> +                                         *drawTarget),
> +                     false);
> +    RefPtr<Path> clipPath = pathBuilder->Finish();
> + 

Whitespace!
Attachment #8813460 - Flags: review?(matt.woodrow) → review+
> Whitespace!

Fixed.
Yes, Boris, I believe the problem is a lack of interop on what happens with `fieldset { min-width: 0; }`. 

Drupal core CSS files are full of things like this:

```
.fieldgroup {
  min-width: 0;
}
/**
 * We've temporarily added this Firefox specific rule here to fix fieldset
 * widths.
 * @todo remove once this Mozilla bug is fixed.
 * See https://bugzilla.mozilla.org/show_bug.cgi?id=504622
 */
@-moz-document url-prefix() {
  .fieldgroup {
    display: table-cell;
  }
}
```

I'm glad to see some action here. Thanks. Exciting.
Comment on attachment 8813460 [details] [diff] [review]
part 1.  Rewrite fieldset border drawing to just clip to the area outside the legendm instead of doing it in pieces with different clip rects

>Bug 504622 part 1.  Rewrite fieldset border drawing to just clip to the area outside the legendm instead of doing it in pieces with different clip rects.  r=mattwoodrow,dbaron

Reminder to "legendm" -> "legend"

>legend.  Without this change, doing that causes the vertical inline-start-side

Maybe change "Without this change, doing that" to be "Without this patch,
allowing the fieldset to be narrower than the legend".

(I misread this sentence the first time, and thought you were only
applying the clip to the top border.  But I agree clipping all borders
sounds like the right thing.)

>This does change behavior in one situation, which the new tests test.  For
>relatively positioned legends, we used to use the original vertical location but
>the positioned horizontal location of the legend to decide which parts of the
>border to not paint.  In the new setup, we use the original location for both.
>I did check that this new behavior matches Chrome and Safari.  Edge seems to
>have our old behavior.

I agree that the new behavior for relatively positioned legends is correct.


>+    // We set up a clip path which has our rect clockwise and the legend rect
>+    // counterclockwise, with FILL_WINDING as the fill rule.  That will allow us
>+    // to paint within our rect but outside the legend rect.

This doesn't seem like it will work right for the cases where the border
extends outside the border-rect.  This can happen with
'border-image-outset'.

Please add a testcase for this, check that it fails with the current patch,
and fix it by making the outer rect of the clip be the fieldset's visual
overflow rect (but you'll need to leave |rect| alone for its other uses).

r=dbaron with that
Attachment #8813460 - Flags: review?(dbaron) → review+
Comment on attachment 8813491 [details] [diff] [review]
Part 2 with another reftest fix try found

>I'm not sure how to write an automated test for the "inline-start/end-side
>border doesn't get drawn under the legend" bit that part 1 was fixing, but I did
>manually test it.

Seems like something minimal would be to use a != reftest:
 * test and reference each have a fieldset with border-top: none but borders other sides (or maybe just a border on the right) and a fixed width, and a legend *with visibility:hidden*
 * the test has enough text in the legend (or, better, an inline-block with width) to obscure part of the right border, but the reference has a small legend


I also wonder if it makes more sense to use &#x200b; (zero width space)
rather than zero width non-joiner in fieldset-min-width-2-ref.html.


r=dbaron, preferably with such a test added
Attachment #8813491 - Flags: review?(dbaron) → review+
> This can happen with 'border-image-outset'.

Exciting.  Note that this is broken without this patch too, as long as we have a legend, because we end up clipping to "rect" or some subset of it in all cases.  So for example this testcase:

  <style>
    fieldset {
      width: 100px;
      height: 100px;
      border: 10px solid;
      border-image: url(https://www.w3.org/2014/10/stdvidthumb.png) 100%/1/1;
   }
  </style>
  <fieldset>
    <legend></legend>
  </fieldset>

doesn't show any border images.  It does if the legend is removed.

I tried using "GetVisualOverflowRect() + aPt" for the clockwise rect and it makes that test pass.  But the documentation for GetVisualOverflowRect() says:

   * @return the rect relative to this frame's origin, but after
   * CSS transforms have been applied (i.e. not really this frame's coordinate
   * system, and may not contain the frame's border-box, e.g. if there
   * is a CSS transform scaling it down)

and indeed if I throw in "transform: scale(0.5)" then the border doesn't get painted at all.

Are there cases other than border-image-outset that can cause borders to paint outside the border-rect?  If not, then inflating rect by GetStyleBorder()->GetImageOutset() seems like the right thing to do.  I'll post a patch doing that.

> Seems like something minimal would be to use a != reftest:

Actually, I found a simple way to test this given that idea.  The test has:

  <style>
    fieldset {
      min-width: 0;
      width: 0;
    }
    legend {
      width: 100px;
      height: 20px;
    }
  </style>
  <fieldset>
    <legend></legend>
  </fieldset>

and the reference is exactly the same except for "background: white" on the legend.
Created attachment 8815320 [details] [diff] [review]
Interdiff addressing part 1 review comments
Attachment #8815320 - Flags: review?(dbaron)
I suppose another pragmatic solution is to simply use aDirtyRect for the "outer" clip rect.  At least assuming it's in the coordinate system of our frame.  That seems like it should really cover all cases...
And assuming that dirty rect is actually correct in this case...
Comment on attachment 8815320 [details] [diff] [review]
Interdiff addressing part 1 review comments

dbaron pointed out GetVisualOverflowRectRelativeToSelf() has the right coordinates to use for our clip.
Attachment #8815320 - Flags: review?(dbaron)
And per discussion, the reason for the random try failures with the border-image-outset is that nsDisplayFieldSetBorderBackground's GetBounds() wasn't including the images in that case.  Fixed that with GetVisualOverflowRectRelativeToSelf() as well.
Created attachment 8815441 [details] [diff] [review]
Part 1 updated to review comments
(Assignee)

Updated

2 years ago
Attachment #8813460 - Attachment is obsolete: true
Created attachment 8815442 [details] [diff] [review]
Part 2 updated to review comments
(Assignee)

Updated

2 years ago
Attachment #8813491 - Attachment is obsolete: true

Comment 54

2 years ago
Pushed by bzbarsky@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/056f728704e7
part 1.  Rewrite fieldset border drawing to just clip to the area outside the legend instead of doing it in pieces with different clip rects.  r=mattwoodrow,dbaron
https://hg.mozilla.org/integration/mozilla-inbound/rev/c024721d9b03
part 2.  Allow fieldsets to shrink below their intrinsic min-width and below the width of their legend if their min-width is explicitly overridden.  r=dbaron
(Assignee)

Updated

2 years ago
Attachment #8815320 - Attachment is obsolete: true

Comment 56

2 years ago
Pushed by bzbarsky@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/694d5dac1dcc
part 1.  Rewrite fieldset border drawing to just clip to the area outside the legend instead of doing it in pieces with different clip rects.  r=mattwoodrow,dbaron
https://hg.mozilla.org/integration/mozilla-inbound/rev/096c7943a1c8
part 2.  Allow fieldsets to shrink below their intrinsic min-width and below the width of their legend if their min-width is explicitly overridden.  r=dbaron
(Assignee)

Updated

2 years ago
Blocks: 424225
That was an unexpected pass because I removed the assertions that crashtest was hitting. Relanded with the test annotations changed to not claim this test asserts.

I filed bug 1321127 on the fact that _only_ Android picked up the fact that the wrong number of assertions was now happening...
Flags: needinfo?(bzbarsky)

Comment 58

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/694d5dac1dcc
https://hg.mozilla.org/mozilla-central/rev/096c7943a1c8
Status: ASSIGNED → RESOLVED
Last Resolved: 9 years ago2 years ago
status-firefox53: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla53

Comment 59

2 years ago
Pushed by bzbarsky@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/1e768923183b
followup for a review comment I missed.  r=dbaron
> I also wonder if it makes more sense to use &#x200b; (zero width space)

Er, I missed this somehow.  Landing that change now.
Depends on: 1326163
(Assignee)

Updated

9 months ago
Duplicate of this bug: 1429349
You need to log in before you can comment on or make changes to this bug.