Bug 115736 – Changing the style of a hidden div queues a repaint

• Status: NEW
• Component: Layout and Rendering

[José Dapena Paz]

Created attachment 200924 [details]
Test html

Attaching test html.

Test shows how browser updates tiles.
There are 4 divs on page:
one is container and three others are columns with different colors;
right div is hidden(visibility) and placed over two tiles;
Every 3 secs JScript changes colors for left and right divs.
As result, browser updates tiles upper-left(it is place for left div) and upper-right and down-right, BUT nothing changed(visually) on right side.

[José Dapena Paz]

Created attachment 200931 [details]
Patch

[Build Bot]

Comment on attachment 200931 [details]
Patch

Attachment 200931 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.appspot.com/results/266124

New failing tests:
fast/repaint/hidden-div-repaint.html

[Build Bot]

Created attachment 200952 [details]
Archive of layout-test-results from webkit-ews-06 for mac-mountainlion

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: webkit-ews-06  Port: mac-mountainlion  Platform: Mac OS X 10.8.2

[Simon Fraser (smfr)]

Comment on attachment 200931 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=200931&action=review

> Source/WebCore/rendering/style/RenderStyle.cpp:678
> +    if (visibility() == other->visibility() && visibility() == HIDDEN)
> +        return StyleDifferenceEqual;

You can't do this; you'll miss out on other styles tested later, which may be different.

[José Dapena Paz]

(In reply to comment #4)
> (From update of attachment 200931 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=200931&action=review
> 
> > Source/WebCore/rendering/style/RenderStyle.cpp:678
> > +    if (visibility() == other->visibility() && visibility() == HIDDEN)
> > +        return StyleDifferenceEqual;
> 
> You can't do this; you'll miss out on other styles tested later, which may be different.

Let's try to understand the problem better: what you say is something like this scenario, right?:

(Considering change from A to B should do a repaint)
1. style A, visible
2. style A, hidden -> proper update
3. style B, hidden -> we prevent style change
4. style B, visible -> Shouldn't this be at least forcing a repaint? We'll be changing visibility in this style change, so the check for inherited_flags._visibility will mark the diff as paint.

What happens in current code?
1. style A, visible
2. style A, hidden -> proper update
3. style B, hidden -> We schedule a repaint of the area covered by the element. But... it is and was hidden and with same layout, do we need this change?
4. style B, visible -> It will mark diff as paint due to the visibility change.

Guess I am missing something terribly obvious, could you ellaborate more on the issue?

[Simon Fraser (smfr)]

(In reply to comment #5)
> (In reply to comment #4)
> > (From update of attachment 200931 [details] [details])
> > View in context: https://bugs.webkit.org/attachment.cgi?id=200931&action=review
> > 
> > > Source/WebCore/rendering/style/RenderStyle.cpp:678
> > > +    if (visibility() == other->visibility() && visibility() == HIDDEN)
> > > +        return StyleDifferenceEqual;
> > 
> > You can't do this; you'll miss out on other styles tested later, which may be different.
> 
> Let's try to understand the problem better: what you say is something like this scenario, right?:

The problem is how RenderStlye::diff() works. You can't ever return early with a diff type is that is "less" than later ones. i.e. all the "StyleDifferenceLayout" returns have to come before the StyleDifferenceRepaint returns etc. Bailing early with a StyleDifferenceEqual is not an option because you may miss some other part of the style change.