215567 - Absolute positioned element doesn't paint correctly when 'clip' changes

Status: RESOLVED FIXED

(Core :: Web Painting, defect, P2)

Description

[Christian Mattar]

Open the testcase.
After the image has loaded, the clip-property of the image is changed by
Javascript, but the now visible area isn't completely painted.

Comment 1

[Christian Mattar]

Attachment: Testcase

Comment 2

[Christian Mattar]

It seems the bug only appears when doing a Shift-Reload of the testcase.

Comment 3

[Mats Palmgren (inactive)]

Bug also occurs in trunk build 2003-08-04-05 and with Mozilla 1.4 on Linux.
It does NOT seem to occur with Mozilla 1.3.1 on Linux

Comment 4

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

Attachment: fix

This turned out to be a subtle one. 'auto' clip values were being stored as
zero in the mClip rect, and in the testcase both the old and new rects have
'auto' as the top and bottom values. Thus the old and new mClip both appeared
to be zero height and so old mClip == new mClip since they're both empty! thus
we failed to compute the right change hint.

I think the right solution is to change mClip to be something other than
nsRect, so we never accidentally treat 0 values are indicating an empty rect.

Comment 5

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

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

This is a simple review, I think.

Comment 6

[Boris Zbarsky [:bzbarsky]]

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

>Index: content/shared/public/nsStyleStruct.h
>+  nsClipRect       mClip;       // [reset] offsets from upper-left border edge;
>+  // not an nsRect because 0 values don't always mean zero (see mClipFlags),
>+  // so IsEmpty(), == etc don't do the right thing

I'd put the comment before the var.  r+sr=bzbarsky with that.

On the other hand, wouldn't it be simpler to use 0 for auto top and left while
using NS_UNCONSTRAINEDSIZE or something for auto right and bottom?  That should
also solve the problem (and the values are more meaningful too), no?

Comment 7

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

> That should also solve the problem (and the values are more meaningful too),
> no?

You mean use NS_UNCONSTRAINEDSIZE for auto width/height and then keep mClip as
an nsRect?

I guess that would work. The important property is that if the actual clip rect
changes, then mClip != other.mClip or mClipFlags != other.mClipFlags. Proving
that that's so is actually a little subtle. You have the underlying style rect
Shape = (Z U {auto})^4, and two functions, f : Shape x nsSize -> nsRect
interpreting the style for a particular frame's size and g : Shape -> nsRect
storing the style in mClip, and you have to show that for all size, shape,
shape', f(shape, size).nsRect::operator!=(f(shape', size)) implies
g(shape).nsRect::operator!=(g(shape')) or flags(shape) != flags(shape') where
flags : Shape -> {auto, nonauto}^4.

That's fairly easy to prove if you start by assuming
g(shape).nsRect::operator==(g(shape')) and flags(shape) == flags(shape') and
show f(shape, size).nsRect::operator==(f(shape', size)).

Suppose g(shape).IsEmpty(). Then g(shape').IsEmpty(). Also, (shape.bottom !=
auto && shape.bottom <= shape.top (or 0 if shape.top is auto)) || (shape.right
!= auto && shape.right <= shape.left (or 0 if shape.left is auto)). If
(shape.bottom != auto && shape.bottom <= shape.top (or 0 if shape.top is auto)),
then f(shape, size).height <= 0. Likewise on the other hand f(shape, size).width
<= 0. Then f(shape, size).IsEmpty(). Similarly f(shape', size).IsEmpty(). We're
done.

Now suppose !g(shape).IsEmpty(). Then !g(shape').IsEmpty() && g(shape).x ==
g(shape').x && g(shape).y == g(shape').y && g(shape).width == g(shape').width &&
g(shape').height == g(shape').height. Clearly shape == shape' and therefore
f(shape, size) == f(shape', size).

And people wonder why program verification never got off the ground :-).

Note that we'll still need mClipFlags, or at least two bits of it, with this
approach, to distinguish auto from 0 in top and left, which I presume we need to
do so computed style values are returned correctly.

Comment 8

[Boris Zbarsky [:bzbarsky]]

> Note that we'll still need mClipFlags, or at least two bits of it, with this
> approach, to distinguish auto from 0 in top and left

And to distinguish auto right/bottom from people actually setting a huge value
there....

Anyway, whichever approach you prefer is fine with me, really.  I guess the
approach you took in your patch is a lot easier to prove correct... ;)

Comment 9

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

don't we bound CSS values somehow? I notice that the CSS spec doesn't restrict
min/max values on integers :-(

Comment 10

[Boris Zbarsky [:bzbarsky]]

We have a bug somewhere that a large positive pixel width in a CSS sheet will
overflow to a negative number of twips (since it will be positive at parse time,
hence will not get dropped, but the number of twips is bigger).

So no, we don't bound.

Comment 11

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

Why don't we bound?

Comment 12

[Boris Zbarsky [:bzbarsky]]

Presumably because, as you pointed out, the spec does not.

Comment 13

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

Attachment: fix #2

Okay, I think NS_MAXSIZE is the right way to go. It has the advantage that if
you change from one genuinely empty rect to another, we don't generate any
style change hint. And the patch is trivial.

Comment 14

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 142805 [details] [diff] [review]
fix #2

r+sr=bzbarsky

Comment 15

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

Checked in. Thanks bz!