REGRESSION: Incomplete repaint of CSS image substitution

• Component: Layout and Rendering
• Status: NEW ---

[Philip Timmermann]

This URL contains CSS image-substitution on hover in the navigation-bar on top of the page and on the images of houses further down the page (just check in any other browser to see how it should look)...

The hover-effect works correctly in virtually any browser: IE5 (even IE5:mac), IE6, IE7, Firefox 1+, Opera, Netscape 6+ (etc.) and Safari-versions before Safari 3...

The image-substitution basically works by changing the upper padding of a fixed size <a> (display:block) containing an <img>, on hover. This <a> has overflow:hidden properties, so by changing its upper padding, the contained image gets shifted into the hidden overflow-area, revealing an image below it (layering is done via negative margins)...

This might sound a little complicated for simple image-substitution, but avoids flicker in old IE versions, doesn't need preloading, is css-only and works in virtually any browser except for Safari 3...

[Matt Lilek]

The actual image substitution works, but it isn't repainted.  It seems to repaint the first link you hover over fine, but from there you need to force a repaint.

[Matt Lilek]

Created attachment 17286 [details]
Reduction

This is partly fixed in ToT from Safari 3, but is still a definite regression from Safari 2.

In Safari 2, both squares change to blue when hovered over.

In Safari 3.0.4 (Tiger), the green square changes to blue on hover, the red square never becomes blue unless a repaint is forced.

In ToT (r27761), the green square changes to blue on hover, as does the red square, but if you move the cursor from red square -> green square, the red square stays blue until a repaint is forced.

WebKit actually repaints the wrong rectangle because it applies a translated clip. The reason for that is that the repainting happens under a non-zero layoutDelta but clip computation ignores that. I have a one-line fix for the LayoutState code path - when pushing state and adding clipping, translate the additional clip by the current layout delta. But I don't know what to do in the non-LayoutState code path.

Created attachment 17299 [details]
Fix for the LayoutState-enabled code path

[Darin Adler]

Comment on attachment 17299 [details]
Fix for the LayoutState-enabled code path

r=me

Comment on attachment 17299 [details]
Fix for the LayoutState-enabled code path

Landed in r27869. Clearing the review flag.

[Philip Timmermann]

'just checked r27875... looks great to me...

thanks guys... looking forward to the next safari release-update...

[Robert Blaut]

The test case works fine. I suppose this bug is fixed, isn't it?

(In reply to comment #8)
> The test case works fine. I suppose this bug is fixed, isn't it?

Not completely.

[Gavin Sherlock]

The test case seems to work correctly now (in r48518, and Safari 4.03), so maybe this can be closed?

[Benjamin Poulain]

(In reply to comment #9)
> (In reply to comment #8)
> > The test case works fine. I suppose this bug is fixed, isn't it?
> 
> Not completely.

What do you mean? What is missing?

(In reply to comment #11)
> (In reply to comment #9)
> > (In reply to comment #8)
> > > The test case works fine. I suppose this bug is fixed, isn't it?
> > 
> > Not completely.
> 
> What do you mean? What is missing?

At the time, what I meant was that the bug was still present in the unoptimized code path (where LayoutState is not used). I don’t know if that’s still the case—someone could make a version of the test that take the unoptimized code path in order to find out.

[Benjamin Poulain]

Created attachment 51025 [details]
Reduction without LayoutState

> At the time, what I meant was that the bug was still present in the unoptimized
> code path (where LayoutState is not used). I don’t know if that’s still the
> case—someone could make a version of the test that take the unoptimized code
> path in order to find out.

You are right, the bug is still there for the unoptimized code path. I have attached a test case, I will try to have a look at the problem today.

[Benjamin Poulain]

Created attachment 51080 [details]
Fix the issue

First attempt to fix the issue.

It think there is still a missing case. If I understand correctly, the layoutDelta is relative to the enclosing layer(?).
If in the slow path, but with a layer(), layoutDelta should be transformed before applied to the clipping rect.

Is that right?

[Benjamin Poulain]

Created attachment 51272 [details]
Patch and test

Here is a patch that can be integrated.

I am surprised by the two little white rects on the expected image. Someone might know where they come from.

[Simon Hausmann]

(In reply to comment #15)
> Created an attachment (id=51272) [details]
> Patch and test

You may want to skip the newly added test on the other platforms until they have pixel test results :)

[Adam Barth]

This patch has been up for review for almost two months and Hyatt isn't even CCed.  According to my understanding, he's one of the few people on earth who can review repainting patches.

[Adam Barth]

Comment on attachment 51272 [details]
Patch and test

I'm sorry to have to r- your patch.  In order to make changes like this the codebase, you need to socialize your change with folks who can review them.  In this case, there are only a handful of people who can (or are willing to) review repainting bugs, mostly because the code is subtle, fragile, and poorly tested.  If current trends continue, your patch will be up for review indefinitely, which is bad for the project.  Feel free to renominate your patch for review (or to post an updated patch) when you've found someone who is willing and able to review your patch.

Again, my apologies that you're running up against some of the brokenness in our review system.  Hopefully this won't discourage you from contributing more patches in the future.