Closed Bug 460240 Opened 12 years ago Closed 12 years ago

dynamic changes to box-shadow not invalidated correctly

Categories

(Core :: Layout, defect)

defect
Not set
normal

Tracking

()

VERIFIED FIXED
mozilla1.9.1b2

People

(Reporter: dao, Assigned: roc)

References

(Depends on 1 open bug)

Details

(Keywords: testcase, verified1.9.1)

Attachments

(6 files)

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:09-dgottwald@mozilla.com-try-224473da0bc/
Michael might be busy. Maybe someone in gfx-land can look at this, the code is not all that deep.
Attached file 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.
Note that I don't see the bug on the initially selected .ctrlTab-thumbnail. It fails when I ctrl-tab to the next one.
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.
Attached file testcase
Keywords: testcase
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.
Probably something to do with overflow areas and invalidation I guess
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?
In unrelated news, we don't set *aAnyOutlineOrEffects in ComputeOutlineAndEffectsRect when we have a box shadow. We need to fix that.
Er, how are we getting in there? Nothing in this testcase uses -moz-transform, so how can GetStyleDisplay()->HasTransform() return true?
Hmm, I guess it isn't the cause since I tried it again and nothing happened.
I don't know what's causing this and don't have time to do a deep debugging.
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.
(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.
Component: GFX → Layout
Depends on: 323255
QA Contact: general → layout
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.
Attached patch fixSplinter Review
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.
Assignee: nobody → roc
Attachment #343695 - Flags: superreview?(dbaron)
Attachment #343695 - Flags: review?(dbaron)
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.
(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 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.  :(
Attachment #343695 - Flags: superreview?(dbaron)
Attachment #343695 - Flags: superreview+
Attachment #343695 - Flags: review?(dbaron)
Attachment #343695 - Flags: review+
(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 ? :-)
pushed b2f370786bc9
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
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
Status: RESOLVED → VERIFIED
Hardware: PC → All
Target Milestone: --- → mozilla1.9.1b2
Attached file still failing testcase
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.
Status: VERIFIED → REOPENED
Resolution: FIXED → ---
Summary: Multiple box-shadows not rendered correctly → dynamic changes to box-shadow not invalidated correctly
Attachment #345722 - Flags: superreview?(dbaron)
Attachment #345722 - Flags: review?(dbaron)
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 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?
Attachment #345722 - Flags: superreview?(dbaron)
Attachment #345722 - Flags: superreview-
Attachment #345722 - Flags: review?(dbaron)
Attachment #345722 - Flags: review-
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?
Attachment #345722 - Flags: superreview-
Attachment #345722 - Flags: superreview+
Attachment #345722 - Flags: review-
Attachment #345722 - Flags: review+
Status: REOPENED → ASSIGNED
Target Milestone: mozilla1.9.1b2 → ---
Pushed 90efdbe2c69a. Please file a new bug if there are any remaining issues.
Status: ASSIGNED → RESOLVED
Closed: 12 years ago12 years ago
Resolution: --- → FIXED
Flags: blocking1.9.1? → blocking1.9.1+
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?
Status: RESOLVED → VERIFIED
Flags: in-testsuite?
Target Milestone: --- → mozilla1.9.1b2
Not until bug 451332 is fixed.
Depends on: 451332
Attached patch reftestSplinter Review
Attachment #384549 - Flags: review?(roc)
Comment on attachment 384549 [details] [diff] [review]
reftest

Nice!
Attachment #384549 - Flags: review?(roc) → review+
Attachment #384549 - Attachment description: reftest → reftest (checkin-needed to 1.9.1 and trunk)
pushed the test: http://hg.mozilla.org/mozilla-central/rev/20120cba8f0b
Flags: in-testsuite? → in-testsuite+
Whiteboard: [test needs landing on 1.9.1]
Attachment #384549 - Attachment description: reftest (checkin-needed to 1.9.1 and trunk) → reftest (checkin-needed to 1.9.1)
http://hg.mozilla.org/releases/mozilla-1.9.1/rev/0d0298519a11
Keywords: checkin-needed
Whiteboard: [test needs landing on 1.9.1]
Attachment #384549 - Attachment description: reftest (checkin-needed to 1.9.1) → reftest
(In reply to comment #36)
> http://hg.mozilla.org/releases/mozilla-1.9.1/rev/0d0298519a11

Backed out, as it times out on branch.
Er, yeah, the MozReftestInvalidate event isn't supported on branch.
You need to log in before you can comment on or make changes to this bug.