Bug 4334 – REGRESSION: Flickering when css-hover should change opacity on floating elements

• Component: Layout and Rendering
• Status: RESOLVED FIXED

[Jonas Munk]

When hovering over an element that has been assigned a css-float-value AND should change opacity it 
"flickers" instead.

See the URL for a minimal testcase.

The problem is found in Safari Version 2.0 (412.2). I remember that it worked in Safari 1.2 or so. I also 
remember seeing someone else found it as a regression on Hyatts old blog.

Hep hey - my first bug :-)

[Mark Rowe (bdash)]

Confirmed with both Safari 412.2 and ToT WebKit.

[Jonas Munk]

Created attachment 4897 [details]
Same file as the URL

Attached the test just to be sure it doesn't go away.

[Jonas Munk]

Can anyone tell me if this bug has any priority or maybe it is dependant on some other actvity?

[Maciej Stachowiak]

It's unassigned, so no one is actively working on it. Priority would go up if it affects a major site, is shown 
to be a regression, otherwise become higher profile than standard.

[Jonas Munk]

I understand.

Actually I believe it is a regression (it worked a long time ago / when you first started supporting opacity) 
- but I'm unable to prove that :-)

As of nightly 2006-01-17 the problem no longer occurs on the news-box on jonasmunk.dk ("Nyheder").
But it still occurs on www.in2isoft.dk (the links in the top-right corner)

[Dave Hyatt]

Yeah I'm sure this is a regression.

[Jonas Munk]

Just tried it in different versions of OmniWeb. It worked in OmniWeb 5.1 - should be AppleWebKit/125.4 
according to http://www.omnigroup.com/applications/omniweb/developer/

Can be downloaded here: http://browsers.evolt.org/?omniweb/MacOSX/5.1

So I guess it's a regression

I think this is what's happening: when the float is first inserted into its containing block, because it's 
transparent it has a layer, and therefore the containing block sets noPaint to true. When it becomes 
opaque, it loses the layer but the containing block doesn't update the noPaint flag, so nobody paints the 
float.

I suspect that in the opposite situation, the float is painted twice.

[Joost de Valk (AlthA)]

Upping to P1 because this is a regression.

[Darin Adler]

I thin Mitz's analysis is right. But I couldn't figure out the right way to get the noDraw bit updated properly when a child's layer changes in RenderBox::setStyle. Need to talk to Dave about this one.

[Alice Liu]

<rdar://problem/4424077>

Created attachment 8720 [details]
Update the noPaint flag as necessary

This is work in progress.

Created attachment 8727 [details]
Patch, including change log and pixel test

[Darin Adler]

Comment on attachment 8727 [details]
Patch, including change log and pixel test

This patch probably needs to be reviewed by Hyatt. I don't understand the issues well enough to review its behavior.

+                RenderObject* o = parent();
+                while (o && (!lastBlock || !o->layer())) {
+                    if (o->isRenderBlock())
+                        lastBlock = o;
+                    o = o->parent();
+                }

I think this reads better as a for loop.  I'd also make lastBlock be a RenderBlock* to put the cast to RenderBlock closer to the isRenderBlock call.

    for (RenderObject* o = parent(); o; o = o->parent()) {
        if (lastBlock && o->layer())
            break;
        if (o->isRenderBlock())
            lastBlock = static_cast<RenderBlock*>(o);
    }

For me, it's easier to see what the loop is doing in this form. And since this code appears twice, I think it should be factored out into a helper function. It probably doesn't need to be a member function.

[Dave Hyatt]

Comment on attachment 8727 [details]
Patch, including change log and pixel test

r=me

[Dave Hyatt]

Comment on attachment 8727 [details]
Patch, including change log and pixel test

Minusing so you can address Darin's comments.

Created attachment 8745 [details]
Patch, including change log and pixel test

Addressed Darin's comments.

[Darin Adler]

Comment on attachment 8745 [details]
Patch, including change log and pixel test

r=hyatt :-)

[Darin Adler]

Committed revision 14759.

[Dave Hyatt]

Reopening.

[Dave Hyatt]

Created attachment 13886 [details]
Patch that fixes the mezzoblue version of this bug

[Dave Hyatt]

Changed the URL field to the test case that fails.

[Dave Hyatt]

Created attachment 13887 [details]
Test case to go with patch

Comment on attachment 13886 [details]
Patch that fixes the mezzoblue version of this bug

I'm told there's a test to go with this patch, so r=me!

[Dave Hyatt]

Fixed in r20609.