Bug 41201 - CSS3 background: Number of layers should be determined by background-image element count
Summary: CSS3 background: Number of layers should be determined by background-image el...
Status: RESOLVED DUPLICATE of bug 47159
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: 528+ (Nightly build)
Hardware: All All
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on: 42173 43592
Blocks:
  Show dependency treegraph
 
Reported: 2010-06-25 02:23 PDT by Andreas Kling
Modified: 2011-03-11 12:17 PST (History)
9 users (show)

See Also:


Attachments
Proposed patch (1.89 KB, patch)
2010-06-25 02:27 PDT, Andreas Kling
no flags Details | Formatted Diff | Diff
Proposed patch v2 (3.95 KB, patch)
2010-07-12 15:56 PDT, Andreas Kling
no flags Details | Formatted Diff | Diff
Proposed patch v3 (5.82 KB, patch)
2010-07-16 06:09 PDT, Andreas Kling
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Comment 1 Andreas Kling 2010-06-25 02:27:58 PDT
Created attachment 59745 [details]
Proposed patch

Slightly altered culling logic in FillLayer to discard all layers after (and including) the first one with no image set.

Would like some suggestions on how to test this.
Comment 2 WebKit Review Bot 2010-06-25 02:32:00 PDT
Attachment 59745 [details] did not pass style-queue:

Failed to run "['WebKitTools/Scripts/check-webkit-style', '--no-squash']" exit_code: 1
WebCore/rendering/style/FillLayer.cpp:253:  Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side.  [whitespace/operators] [4]
WebCore/rendering/style/FillLayer.cpp:254:  Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side.  [whitespace/operators] [4]
Total errors found: 2 in 2 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 3 Andreas Kling 2010-07-12 15:56:56 PDT
Created attachment 61280 [details]
Proposed patch v2

I spoke with Antti about this on IRC, he suggested I make the logic a bit more readable.
I've also included a manual test.
Comment 4 WebKit Commit Bot 2010-07-13 04:05:06 PDT
Comment on attachment 61280 [details]
Proposed patch v2

Clearing flags on attachment: 61280

Committed r63192: <http://trac.webkit.org/changeset/63192>
Comment 5 WebKit Commit Bot 2010-07-13 04:05:10 PDT
All reviewed patches have been landed.  Closing bug.
Comment 6 Antonio Gomes 2010-07-13 12:56:52 PDT
Should this be re-opened since bug 42173 reverted it.
Comment 7 Andreas Kling 2010-07-13 13:28:46 PDT
Yeah, reopening.
Comment 8 Andreas Kling 2010-07-16 05:50:26 PDT
We need to fillUnsetProperties() before we cullEmptyLayers()

Consider the following scenario:

#foo {
    background-image: url('Layer1'), ('Layer2');
    background-repeat: no-repeat;
}

The previous version of the patch would discard Layer2 since it had no corresponding properties. If we instead fillUnsetProperties() first, we get the correct result:

#foo {
    background-image: url('Layer1'), ('Layer2');
    background-repeat: no-repeat, no-repeat;
}

Now for the reverse scenario:

#foo {
    background-image: url('Layer1');
    background-repeat: no-repeat, no-repeat;
}

cullEmptyLayers() would previously remove the second 'no-repeat' before fillUnsetProperties() had a chance to repeat Layer1. If we swap the order of fillUnsetProperties() and cullEmptyLayers() we need to alter fillUnsetProperties() a bit - it shouldn't repeat image properties.
Comment 9 Andreas Kling 2010-07-16 06:09:53 PDT
Created attachment 61796 [details]
Proposed patch v3
Comment 10 WebKit Commit Bot 2010-07-19 07:06:15 PDT
Comment on attachment 61796 [details]
Proposed patch v3

Clearing flags on attachment: 61796

Committed r63653: <http://trac.webkit.org/changeset/63653>
Comment 11 WebKit Commit Bot 2010-07-19 07:06:22 PDT
All reviewed patches have been landed.  Closing bug.
Comment 12 Simon Fraser (smfr) 2010-08-05 15:51:11 PDT
This broke multiple background image: bug 43581.
Comment 13 Simon Fraser (smfr) 2010-08-05 16:59:46 PDT
Rolling this out via bug 43592.
Comment 14 Simon Fraser (smfr) 2010-09-26 08:38:06 PDT
Reopening, since this was rolled out.
Comment 15 Andreas Kling 2011-03-11 12:17:53 PST

*** This bug has been marked as a duplicate of bug 47159 ***