485149 - Using box-shadow on a <fieldset> with <legend> does not follow box shape

Status: RESOLVED FIXED

(Core :: Layout: Block and Inline, defect)

Description

[Martijn Otto]

User-Agent:       Mozilla/5.0 (X11; U; Linux x86_64; en-US; rv:1.9.1b3) Gecko/20090307 Ubuntu/8.10 (intrepid) Shiretoko/3.1b3
Build Identifier: Mozilla/5.0 (X11; U; Linux x86_64; en-US; rv:1.9.1b3) Gecko/20090307 Ubuntu/8.10 (intrepid) Shiretoko/3.1b3

Using -moz-box-shadow on a fieldset will show shadow as if the legend continues to the right of the fieldset.

Reproducible: Always

Steps to Reproduce:
1. Build a form with a fieldset
2. Add -moz-box-shadow style rule
Actual Results:  
Shadow flows too wide around the fieldset

Expected Results:  
Shadow flows nicely around the fieldset

Comment 1

[Martijn Otto]

Attachment: testcase

Comment 2

[development]

This bug is also present in the Windows version of Firefox.

Comment 3

[Emil Ivanov]

Confirmed with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.3a1pre) Gecko/20090920 Minefield/3.7a1pre

Comment 4

[Emil Ivanov]

Changing the summary to describe the problem better.

When fieldset is used with legend the border updates its position. Maybe if the shadow follows the border this problem will gone.

Comment 5

[Emil Ivanov]

Attachment: testcase 2

Testcase with backgrounds for detailed view.
The body background is above the shadow, which looks very strange.

Comment 6

[brucomela]

Attachment: simple shadow glitch

top-right shadow is wrong

Comment 9

[Darren Kalck [:D-Kalck]]

Is somebody interested in fixing this one ?

Comment 10

[Al]

Not capable of fixing this, but it presumably relates to the way the legend gets rendered. It can be fixed in css by positioning the legend absolutely (relative to the fieldset), so that the legend doesn't stretch the background up the top.

fieldset { position: relative; }
fieldset legend { position: absolute; top: -10px; left: 0; }

top & left values depend on fieldset padding.

The shadow needs to be rendered with the border's frame not the fieldset frame, which gets stretched by the legend. Hope this helps???

Comment 11

[gabriel]

This bug is still present in 8.0

Comment 12

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Note: when we drop -moz-box-shadow in favor of box-shadow (which we've supported for a while), the testcases will need to be modified to omit the prefix.

Comment 14

[j.j.]

Attachment: testcase

Comment 15

[j.j.]

(In reply to David Baron [:dbaron] from comment #12)
> testcases will need to be modified to omit the prefix.
done

Comment 16

[Drew Matamales]

Well, -moz-box-shadow has apparently been dropped as of version 14 but this bug still hasn't been fixed.  I'm not going to use position:absolute for my legends as that will affect layout.  I shouldn't have to, because Webkit has handled this correctly for years now.  Without -moz-box-shadow, there is no straightforward way to fix this anymore.  Please either bring back -moz-box-shadow or fix this bug.

Comment 18

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Probably the way to fix this is to change our implementation of fieldset so that the border box corresponds to where the border is, and the legend's overhang is in the margin area (which we ensure exists).  (Hopefully the legend is already correctly added to the overflow areas, which ensures that scrolling and repainting work correctly, though there's a chance it's not.)

Comment 19

[Robert Utasi [:hunboy]]

Attachment: testcase + layout emulation

What can be the concept of shadow around the legend element?

Comment 20

[Robert Utasi [:hunboy]]

Attachment: browser comparison screenshot (Win)

Comment 21

[Mats Palmgren (inactive)]

Attachment: overflow:hidden test

(In reply to David Baron [:dbaron] (don't cc:, use needinfo? instead) from comment #18)
> Probably the way to fix this is to change our implementation of fieldset so
> that the border box corresponds to where the border is, and the legend's
> overhang is in the margin area (which we ensure exists).

Seeing how other UAs clip overflow:hidden, the border boxes looks
the same in Gecko, Trident, Presto, WebKit.  It looks like they just
paint the shadow with the same negative offset as the border.
Wouldn't that be quite simple to implement for us too?

Comment 22

[Mats Palmgren (inactive)]

Attachment: WIP

This gives the desired rendering for the shadow.

Comment 23

[Mats Palmgren (inactive)]

Attachment: WIP

Forgot to include the header file.

Comment 24

[Mats Palmgren (inactive)]

Attachment: fix

This renders the box-shadow in the same way as other UAs.
https://tbpl.mozilla.org/?tree=Try&rev=a1eb793ef0a0

(let's sort out the clipping / border box issues in bug 770645)

Comment 25

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

Comment on attachment 731563 [details] [diff] [review]
fix

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

::: layout/forms/nsFieldSetFrame.cpp
@@ +575,5 @@
> +  if (topBorder < mLegendRect.height) {
> +    yoff = (mLegendRect.height - topBorder)/2;
> +  }
> +  return yoff;
> +}

How about instead creating a new nsIFrame method GetVisualBorderRect which returns mRect normally but is modified for nsFieldSetFrame? Then we don't need to check for nsFieldSetFrame specifically.

Comment 26

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

Comment on attachment 731563 [details] [diff] [review]
fix

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

Sorry, didn't mean to r+ yet.

Also, I think nsLayoutUtils::GetBoxShadowRectForFrame doesn't need to include this treatment.

Comment 27

[Mats Palmgren (inactive)]

Attachment: fix+test

https://tbpl.mozilla.org/?tree=Try&rev=8da8879637d8
(the JP orange is unrelated), the Windows orange is a few anti-aliased
pixels at the curved border edges.  I added some fuzz to allow that:
https://tbpl.mozilla.org/?tree=Try&rev=f2315c64a863

Comment 28

[Mats Palmgren (inactive)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/9f58ed47cee2

Comment 29

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/9f58ed47cee2

Comment 30

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

It might be good to have a test for inset shadows as well.

Comment 31

[Robert Utasi [:hunboy]]

Attachment: inset shadow testcase

Comment 32

[Robert Utasi [:hunboy]]

(In reply to David Baron [:dbaron] (don't cc:, use needinfo? instead) from comment #30)
> It might be good to have a test for inset shadows as well.

passed, and passed by cross UA compatibility as well at me.

Comment 33

[Robert Utasi [:hunboy]]

Attachment: inset UA comparison screenshot

Comment 34

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

I meant a reftest committed to the tree and running in our automated test suite.

Comment 35

[Mats Palmgren (inactive)]

Added a reftest with inset box-shadow.
https://hg.mozilla.org/integration/mozilla-inbound/rev/8cee060901b8

Comment 36

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/8cee060901b8