Bug 59941

Summary: REGRESSION(79985): Changes in fill-opacity should trigger repaint but don't
Product: WebKit Reporter: Nico Weber <thakis>
Component: SVGAssignee: Rob Buis <rwlbuis>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, ademar, eric, krit, rwlbuis, thakis, webkit.review.bot, zimmermann
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: PC   
OS: OS X 10.5   
Bug Depends on: 54800    
Bug Blocks:    
Attachments:
Description Flags
repro
none
Patch
none
Patch darin: review+

Description Nico Weber 2011-05-02 10:02:26 PDT
Created attachment 91934 [details]
repro

Repro:
1. Open attached file
2. Click circle

Expected: Circle opacity change is displayed immediately
Expected: Nothing seems to happen. After resizing the window, a repaint happens and things look correct.

Also see http://crbug.com/80880
Comment 1 Dirk Schulze 2011-05-04 13:55:49 PDT
Regression introduced with bug 54800
Comment 2 Rob Buis 2011-05-11 16:06:17 PDT
Hi,

I already have a fix for this so once I have a testcase I'll post a patch, expect somthing in the next 24h.
Cheers,

Rob.
Comment 3 Rob Buis 2011-05-11 19:21:18 PDT
Created attachment 93235 [details]
Patch
Comment 4 Eric Seidel (no email) 2011-05-11 20:29:58 PDT
Comment on attachment 93235 [details]
Patch

OK.  Do we already do the same for stroke?
Comment 5 Eric Seidel (no email) 2011-05-11 20:30:46 PDT
Comment on attachment 93235 [details]
Patch

Why do we need to do this via a click?  Seems  round-about way to test this.  Why not just change the fill-opacity on load?
Comment 6 Eric Seidel (no email) 2011-05-11 20:31:21 PDT
Comment on attachment 93235 [details]
Patch

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

> LayoutTests/svg/custom/fill-opacity-update.svg:5
> +<!DOCTYPE svg PUBLIC "-//W3C//DTD SVG 1.1//EN" 
> +  "http://www.w3.org/Graphics/SVG/1.1/DTD/svg11.dtd">
> +<svg width="6cm" height="5cm" viewBox="0 0 600 500"
> +     xmlns="http://www.w3.org/2000/svg" version="1.1" onload="setTimeout('runTest()', 0)">

More junk here than needed.  Also, do we need a waitUntilDone since you're using a timeout?
Comment 7 Eric Seidel (no email) 2011-05-11 20:31:51 PDT
Comment on attachment 93235 [details]
Patch

The fix is fine, but the test is bad.  Don't you want this to be a repaint test?
Comment 8 Rob Buis 2011-05-12 06:37:52 PDT
Hi Eric,

(In reply to comment #7)
> (From update of attachment 93235 [details])
> The fix is fine, but the test is bad.  Don't you want this to be a repaint test?

Oops, when I woke up I saw r+ in my inbox so I went ahead with the commit. Will have a look at a better test now, and a separate stroke-opacity test is nice as well (though looking at the code that part was not broken).
Cheers,

Rob.
Comment 9 Ademar Reis 2011-05-12 07:25:24 PDT
For reference, the (first) patch landed in r86331
Comment 10 WebKit Review Bot 2011-05-12 08:45:44 PDT
http://trac.webkit.org/changeset/86331 might have broken Windows 7 Release (Tests)
Comment 11 Rob Buis 2011-05-12 09:06:46 PDT
Created attachment 93291 [details]
Patch
Comment 12 Rob Buis 2011-05-12 10:02:39 PDT
The improved test got landed in r86352, also an added test for stroke-opacity. I don't think we can avoid using notifyDone in this case. So I think this bug can be closed now.
Cheers,

Rob.
Comment 13 Nico Weber 2011-05-12 10:07:36 PDT
Thanks!
Comment 14 WebKit Review Bot 2011-05-12 11:10:26 PDT
http://trac.webkit.org/changeset/86352 might have broken GTK Linux 64-bit Debug
The following tests are not passing:
svg/custom/fill-opacity-update.svg
Comment 15 Ademar Reis 2011-05-13 09:28:53 PDT
Revision r86331 cherry-picked into qtwebkit-2.2 with commit 674fb8f <http://gitorious.org/webkit/qtwebkit/commit/674fb8f>
Revision r86352 cherry-picked into qtwebkit-2.2 with commit d2b42e8 <http://gitorious.org/webkit/qtwebkit/commit/d2b42e8>