460240 - dynamic changes to box-shadow not invalidated correctly

Status: VERIFIED FIXED

(Core :: Layout, defect)

Description

[Dão Gottwald [:dao]]

With the patch for bug 456088, some parts of the box-shadow(s) for the selected ctrl-tab preview are missing. This applies to OS X and Linux but doesn't seem to be an issue on Windows.

The CSS that adds the box-shadows:
> .ctrlTab-thumbnail[selected="true"] {
>   -moz-box-shadow: 1px 2px 3px black, 0 0 10px white, 0 0 20px rgb(50, 170, 255), 0 0 30px rgb(50, 170, 255);
> }

Screenshot:
https://bugzilla.mozilla.org/attachment.cgi?id=342473
Looks like the shadows are clipped at the innermost black shadow.

Tryserver builds:
https://build.mozilla.org/tryserver-builds/2008-10-15_08:[e-mail]-try-224473da0bc/

Comment 1

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

Michael might be busy. Maybe someone in gfx-land can look at this, the code is not all that deep.

Comment 2

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

Attachment: NOT a testcase

This testcase doesn't show the bug for me, so I guess we need a reduced testcase from the XUL that's failing.

Comment 3

[Dão Gottwald [:dao]]

Note that I don't see the bug on the initially selected .ctrlTab-thumbnail. It fails when I ctrl-tab to the next one.

Comment 4

[Michael Ventnor]

I am pretty busy at the moment; but if you're not seeing this issue on all platforms then it smells like another Cairo bug.

Comment 5

[Dão Gottwald [:dao]]

Attachment: testcase

Comment 6

[Michael Ventnor]

Hm? When I load the testcase the problem appears, but when I switch to a different tab then back to that tab the problem is gone.

Comment 7

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

Probably something to do with overflow areas and invalidation I guess

Comment 8

[Michael Ventnor]

This code in nsIFrame::FinishAndStoreOverflow is causing it:

  /* If we're transformed, transform the overflow rect by the current transformation. */
  if ((mState & NS_FRAME_MAY_BE_TRANSFORMED_OR_HAVE_RENDERING_OBSERVERS) && 
      GetStyleDisplay()->HasTransform()) {
    // Save overflow area before the transform
    SetRectProperty(this, nsGkAtoms::preTransformBBoxProperty, *aOverflowArea);

    /* Since our size might not actually have been computed yet, we need to make sure that we use the
     * correct dimensions by overriding the stored bounding rectangle with the value the caller has
     * ensured us we'll use.
     */
    nsRect newBounds(nsPoint(0, 0), aNewSize);
    *aOverflowArea = nsDisplayTransform::TransformRect(*aOverflowArea, this, nsPoint(0, 0), &newBounds);
  }

Removing the line that replaces the overflow rect with the TransformRect fixes this bug.
The question is, how to integrate the existing overflow rect while still solving whatever problem this code solves?

Comment 9

[Michael Ventnor]

In unrelated news, we don't set *aAnyOutlineOrEffects in ComputeOutlineAndEffectsRect when we have a box shadow. We need to fix that.

Comment 10

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

Er, how are we getting in there? Nothing in this testcase uses -moz-transform, so how can GetStyleDisplay()->HasTransform() return true?

Comment 11

[Michael Ventnor]

Hmm, I guess it isn't the cause since I tried it again and nothing happened.

Comment 12

[Michael Ventnor]

I don't know what's causing this and don't have time to do a deep debugging.

Comment 13

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

The overflow areas look correct. I think what's causing this is just that changing the box-shadow isn't invalidating enough. We only reflow, but the element size isn't changing so even if XUL boxes called CheckInvalidateSizeChange (which they don't, but should), that wouldn't do anything.

It's not obvious how to fix this. The RepaintFrame style change hint invalidates the old overflow rect, so that's no help. For style changes that change just the overflow rect (outline, SVG effects, box-shadows, text-shadows I guess), we don't have a good mechanism to force the right invalidation. See bug 323255, which this is basically a dup of.

Comment 14

[Dão Gottwald [:dao]]

(In reply to comment #0)
> With the patch for bug 456088, some parts of the box-shadow(s) for the selected
> ctrl-tab preview are missing. This applies to OS X and Linux but doesn't seem
> to be an issue on Windows.

Interestingly, this is still no issue for Ctrl+Tab on Windows although the testcase fails.

Comment 15

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

So my best idea that doesn't involve rewriting a lot of layout is for FinishAndStoreOverflow to invalidate the overflow area if it changed and there are outline/SVG effects/box-shadows present. Of course it's possible to change, say, outline and box-shadow at the same time so that the overflow area doesn't change, but that's OK because the RepaintFrame style change hint will invalidate enough in that case.

Comment 16

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

Attachment: fix

Not really sure who should review this ... David, if you don't want it, suggest another recipient.

-- Fix ComputeOutlineAndEffectsRect so that we set aAnyOutlineOrEffects to PR_TRUE for box-shadows.

-- Change CheckInvalidateSizeChange 3-arg version to take aNewDesiredSize as an nsSize instead of an nsHTMLReflowMetrics. This simplifies one caller (there are only two), and it would be helpful if we called it from XUL where we don't have a suitable nsHTMLReflowMetrics. (This patch doesn't fix XUL to call CheckInvalidateSizeChange, though.)

-- Do what I said in the last comment and repaint the overflow area if the overflow area changes and there are outlines/effects present.

Comment 17

[Boris Zbarsky [:bzbarsky]]

So in this case, we're getting a NS_STYLE_HINT_REFLOW hint, right?  So we invalidate the old overflow area, then reflow, then don't invalidate the new overflow area?

I coulda sworn CheckInvalidateSizeChange looked at the new overflow area, which is why I hadn't just switched it to nsSize...  It doesn't now, though.

Comment 18

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

(In reply to comment #17)
> So in this case, we're getting a NS_STYLE_HINT_REFLOW hint, right?  So we
> invalidate the old overflow area, then reflow, then don't invalidate the new
> overflow area?

That's right.

> I coulda sworn CheckInvalidateSizeChange looked at the new overflow area, which
> is why I hadn't just switched it to nsSize...  It doesn't now, though.

Yeah.

If you're thinking about this, would you like to review the patch?

Comment 19

[Boris Zbarsky [:bzbarsky]]

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

I can't quite convince myself that this gets all the table cases, right (as in, not sure they always call FinishAndStoreOverflow as needed on resizes), but we'll see.

>+++ b/layout/generic/nsFrame.cpp
>     if (mState & NS_FRAME_OUTSIDE_CHILDREN) {
>       // remove the previously stored overflow area 
>       DeleteProperty(nsGkAtoms::overflowAreaProperty);
>+      overflowChanged = PR_TRUE;
>+    } else {
>+      overflowChanged = PR_FALSE;
>     }
>     mState &= ~NS_FRAME_OUTSIDE_CHILDREN;

Could put the &= inside the if, right?

Other than that, looks good.

The other approach would be to try to invalidate after reflow too in response to a repaint hint paired with a reflow hint, but that sounds painful.

We really need to fix invalidation.  :(

Comment 20

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

(In reply to comment #19)
> (From update of attachment 343695 [details] [diff] [review])
> I can't quite convince myself that this gets all the table cases, right (as in,
> not sure they always call FinishAndStoreOverflow as needed on resizes), but
> we'll see.

That would be an existing bug that needs to be fixed, I guess.

> >+++ b/layout/generic/nsFrame.cpp
> >     if (mState & NS_FRAME_OUTSIDE_CHILDREN) {
> >       // remove the previously stored overflow area 
> >       DeleteProperty(nsGkAtoms::overflowAreaProperty);
> >+      overflowChanged = PR_TRUE;
> >+    } else {
> >+      overflowChanged = PR_FALSE;
> >     }
> >     mState &= ~NS_FRAME_OUTSIDE_CHILDREN;
> 
> Could put the &= inside the if, right?

Yeah OK.

> The other approach would be to try to invalidate after reflow too in response
> to a repaint hint paired with a reflow hint, but that sounds painful.

Yeah.

> We really need to fix invalidation.  :(

http://weblogs.mozillazine.org/roc/archives/2008/10/invalidation.html ? :-)

Comment 21

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

pushed b2f370786bc9

Comment 22

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Dao, sorry that I missed to file this bug. I tried the latest try server build and now it works. Thanks.

Verified with Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1b2pre) Gecko/20081025 Minefield/3.1b2pre

Comment 23

[Dão Gottwald [:dao]]

Attachment: still failing testcase

Comment 24

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

The difference between attachment 343513 [details] and attachment 345709 [details] is the addition of "-moz-border-radius: 10px".

Seems like, given that testcase, either this bug should be reopened or a new one should be filed.  For now, reopening this one.

Comment 25

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

Attachment: fix

Comment 26

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

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

This is a simple error in nsStyleBorder, where we were returning prematurely with a VISUAL hint when box-shadows needed a REFLOW hint.

Comment 27

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

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

This looks like there are still many "return NS_STYLE_HINT_VISUAL" before the potential return of shadowDifference.

Why not make CalcShadowDifference return a boolean (and renamed to ShadowsEqual or something like that) so that it's clearer that the hint involved is either none or reflow?  Or does somebody have plans to optimize for changes in the color of the shadow?

Comment 28

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

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

Er, never mind, I see what you're doing now.

But could you at least add a comment about what shadowDifference might be?

Comment 29

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

Sure.

Comment 30

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

Pushed 90efdbe2c69a. Please file a new bug if there are any remaining issues.

Comment 31

[Henrik Skupin [:whimboo][⌚️UTC+2]]

Verified with:

Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10.5; en-US; rv:1.9.1b3pre) Gecko/20081225 Shiretoko/3.1b3pre ID:20081225020447

Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1b3pre) Gecko/20081225 Shiretoko/3.1b3pre ID:20081225034145

Is it possible to have an automated test for that issue?

Comment 32

[Boris Zbarsky [:bzbarsky]]

Not until bug 451332 is fixed.

Comment 33

[Jonathan Griffin (:jgriffin)]

Attachment: reftest

Comment 34

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

Comment on attachment 384549 [details] [diff] [review]
reftest

Nice!

Comment 35

[Markus Stange [:mstange]]

pushed the test: http://hg.mozilla.org/mozilla-central/rev/20120cba8f0b

Comment 36

[Dão Gottwald [:dao]]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/0d0298519a11

Comment 37

[Dão Gottwald [:dao]]

(In reply to comment #36)
> http://hg.mozilla.org/releases/mozilla-1.9.1/rev/0d0298519a11

Backed out, as it times out on branch.

Comment 38

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

Er, yeah, the MozReftestInvalidate event isn't supported on branch.