Bug 37514 - [CSS3 Backgrounds and Borders] Implement CSS3 background-position offsets
: [CSS3 Backgrounds and Borders] Implement CSS3 background-position offsets
Status: RESOLVED FIXED
: WebKit
CSS
: 528+ (Nightly build)
: Macintosh Intel Mac OS X 10.6
: P2 Normal
Assigned To:
:
: InRadar, WebExposed
: 86703 102104 103440 103877 103879 104014 104133 104253 104539
: 27569 79073
  Show dependency treegraph
 
Reported: 2010-04-13 12:01 PST by
Modified: 2012-12-10 10:27 PST (History)


Attachments
background-position test case (1.13 KB, text/html)
2010-04-13 12:01 PST, mathuaerknedam
no flags Details
patch (35.81 KB, patch)
2011-01-24 11:11 PST, qi
no flags Review Patch | Details | Formatted Diff | Diff
patch2 (35.84 KB, patch)
2011-01-24 11:16 PST, qi
eric: review-
Review Patch | Details | Formatted Diff | Diff
Proposed patch (31.49 KB, patch)
2012-04-03 23:31 PST, Uday Kiran
no flags Review Patch | Details | Formatted Diff | Diff
Archive of layout-test-results from ec2-cr-linux-04 (6.54 MB, application/zip)
2012-04-04 00:46 PST, WebKit Review Bot
no flags Details
Proposed patch (32.54 KB, patch)
2012-04-04 05:02 PST, Uday Kiran
no flags Review Patch | Details | Formatted Diff | Diff
Draft patch (22.63 KB, patch)
2012-04-20 06:52 PST, Uday Kiran
no flags Review Patch | Details | Formatted Diff | Diff
WIP patch with two position values (8.74 KB, patch)
2012-05-07 01:01 PST, Uday Kiran
no flags Review Patch | Details | Formatted Diff | Diff


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2010-04-13 12:01:39 PST
Created an attachment (id=53269) [details]
background-position test case

CSS3 allows background position offsets of the variety "right 3em bottom 10px". It's described at Example XII at http://www.w3.org/TR/css3-background/#the-background-position.

I'll attach a testcase.
------- Comment #1 From 2011-01-24 11:11:05 PST -------
Created an attachment (id=79940) [details]
patch

Proposal for css3 background-position.
------- Comment #2 From 2011-01-24 11:13:29 PST -------
Attachment 79940 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'LayoutTests/css3/css3-background-position-..." exit_code: 1

Source/WebCore/rendering/style/FillLayer.h:180:  Extra space between int and m_xOrigin  [whitespace/declaration] [3]
Source/WebCore/rendering/style/FillLayer.h:181:  Extra space between int and m_yOrigin  [whitespace/declaration] [3]
Source/WebCore/rendering/style/FillLayer.cpp:23:  Found other header before a header this file implements. Should be: config.h, primary header, blank line, and then alphabetically sorted.  [build/include_order] [4]
Total errors found: 3 in 9 files


If any of these errors are false positives, please file a bug against check-webkit-style.
------- Comment #3 From 2011-01-24 11:16:53 PST -------
Created an attachment (id=79942) [details]
patch2

fix style issue.
------- Comment #4 From 2011-04-06 10:19:11 PST -------
(From update of attachment 79942 [details])
View in context: https://bugs.webkit.org/attachment.cgi?id=79942&action=review

> Source/WebCore/css/CSSParser.cpp:2567
> +static bool isBackgroundPositionCKeyword(RefPtr<CSSPrimitiveValue> value)

You mean to use a raw pointer here.  Please read http://www.webkit.org/coding/RefPtr.html
------- Comment #5 From 2011-04-06 10:20:42 PST -------
(From update of attachment 79942 [details])
View in context: https://bugs.webkit.org/attachment.cgi?id=79942&action=review

WE need to find ways to share more code here.  You have so many copy/paste code paths.

> Source/WebCore/css/CSSParser.cpp:2601
> +    } else {
> +        valueX = CSSPrimitiveValue::create(Pair::create(CSSPrimitiveValue::createIdentifier(CSSValueLeft), value));
> +    }

No {} on one line clauses.  Seems we might want to wrap this in a helper method, since you seem to do the same thing in all 3 of these cases, and at least the CSSPrimativeValue::create call could be shared.

> Source/WebCore/css/CSSParser.cpp:2605
> +static PassRefPtr<CSSPrimitiveValue> createBackgroundPositionY(PassRefPtr<CSSPrimitiveValue> pValue, bool usePair = true)

Soooo much duplicated code :(
------- Comment #6 From 2011-09-19 13:13:45 PST -------
It looks like this patch might be abandoned. Unless someone objects, I'll find someone to take a stab at making the requested changes.
------- Comment #7 From 2011-09-30 16:18:48 PST -------
<rdar://problem/10218267>
------- Comment #8 From 2012-02-23 09:39:29 PST -------
It would be good to make progress on this, because the Transforms spec may end up wanting to re-use the same parsing behavior for transform-origin and perspective-origin.
------- Comment #9 From 2012-04-03 23:31:03 PST -------
Created an attachment (id=135521) [details]
Proposed patch
------- Comment #10 From 2012-04-04 00:46:32 PST -------
(From update of attachment 135521 [details])
Attachment 135521 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/12324576

New failing tests:
ietestcenter/css3/bordersbackgrounds/background_position_three_four_values.htm
------- Comment #11 From 2012-04-04 00:46:39 PST -------
Created an attachment (id=135525) [details]
Archive of layout-test-results from ec2-cr-linux-04

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-04  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick
------- Comment #12 From 2012-04-04 05:02:19 PST -------
Created an attachment (id=135560) [details]
Proposed patch
------- Comment #13 From 2012-04-18 21:13:34 PST -------
(From update of attachment 135560 [details])
View in context: https://bugs.webkit.org/attachment.cgi?id=135560&action=review

It's difficult to actually see all that you changed because this change is huge. I would advise you to split it into a refactoring that aligns the code with what you want and the real change that introduces the new syntax. As-is, it's definitely not a good change due to the un-intended changes, the loss of CSSValuePool and everything I have missed as I haven't spend much time looking at whether the extended grammar matches the spec.

> Source/WebCore/ChangeLog:3
> +        Background-position parsing more than 2 values

This should be the bug number here. If you just run Tools/Scripts/prepare-ChangeLog, it would fill this part.

> Source/WebCore/ChangeLog:24
> +        Create value from keyword left/top - 0%, center - 50%, right/bottom - 100%.

Your ChangeLog is lacking the details: what are you allowing as part of this change?

> Source/WebCore/css/CSSParser.cpp:3283
> +            return CSSPrimitiveValue::createIdentifier(id);

AFAICT you want to use CSSValuePool as much as possible as it does some internal caching so this should go through CSSValuePool.

> Source/WebCore/css/CSSParser.cpp:3291
> +void CSSParser::parseFillPosition(CSSParserValueList* valueList, RefPtr<CSSValue>& value1, RefPtr<CSSValue>& value2, unsigned int numValues)

parseFillPosition is shared with -webkit-mask-position and several other properties are also using the function. This is never mentioned in your change that this is a side effect and the lack of testing makes me think you didn't see that.

I haven't found some standard information about -webkit-mask-position so I cannot tell if this is a desirable change. Each properties needs to be evaluated as you may be deviating from the existing standards.

> Source/WebCore/css/CSSParser.cpp:3301
> +        if (values.size() == numValues)

I am not sure what is the use of numValues here. First the name is so vague as to be confusing (what does it represents? Could you use that to rename the variable to something helping). Then I would have expected it to reflect something (like the size of some vectors or passed in variable) but it bears no relation to anything. Third it seems completely arbitrary that the default value is 0 (for which btw this condition will never be true as you always append something above).

> Source/WebCore/css/CSSParser.cpp:-3297
> -        // Only one value was specified.  If that value was not a keyword, then it sets the x position, and the y position
> -        // is simply 50%.  This is our default.
> -        // For keywords, the keyword was either an x-keyword (left/right), a y-keyword (top/bottom), or an ambiguous keyword (center).
> -        // For left/right/center, the default of 50% in the y is still correct.

Removing useful comments is really NOT OK. This comment explains why the magic constant 50% below in the '1' case.

> Source/WebCore/rendering/style/FillLayer.cpp:25
> +#include "CSSValueKeywords.h"

That's a layering violation. The whole purpose of the rendering/style/ classes is to hold the computed style but also to isolate the rendering code from the CSS code.

The proper way is to introduce an enum in RenderStyleConstants.h to hold the computed values.

> LayoutTests/ChangeLog:14
> +        * css3/calc/resources/smiley.png: Added.

Where did you get this file? You need the right to this image for us to integrate it and I am not sure this is the case here. Preferably re-use an existing image.

> LayoutTests/ChangeLog:15
> +        * platform/chromium/test_expectations.txt: Skip background-position test

Why is that fine to make one of the IE test to fail? You need a good justification here before skipping it. Also such changes need to be done on *all* platforms that have a baseline, not on the only EWS that runs the test.

> LayoutTests/css3/calc/background-position.html:1
> +<!DOCTYPE html PUBLIC "-//W3C//DTD HTML 4.01//EN" "http://www.w3.org/TR/html4/strict.dtd">

We usually prefer the HTML5 doctype for WebKit-specific test: <!DOCTYPE html>

> LayoutTests/css3/calc/background-position.html:2
> +<html lang="en">

No lang please.

> LayoutTests/css3/calc/background-position.html:5
> +  <style type="text/css">

No type.

> LayoutTests/css3/calc/background-position.html:6
> +    .case { width: 128px; height: 128px; float: left; border: solid; margin: 0.2em; }

You could easily remove the case class and use instead the div tag selector (div { .... })

> LayoutTests/css3/calc/background-position.html:7
> +    .a, .b, .c, .d, .e, .f, .g, .h, .i, .j, .k, .l, .m, .n, .o, .p, .r, .s, .t, .u, .v, .w, .x1, .x2, .x3, .x4, .x5, .x6

no .y? Anyway, it's pretty confusing to use just letters for those classes and it doesn't make the test case easy to read.

> LayoutTests/css3/calc/background-position.html:14
> +    .case .a { background-position: left 25% top 25%; }

Not a huge fan of the combined selectors here: .a should be enough no?
------- Comment #14 From 2012-04-20 06:52:06 PST -------
Created an attachment (id=138085) [details]
Draft patch

Thanks for reviewing. This is smaller patch with helper functions and parsing code. I will add testcases in next patch.
------- Comment #15 From 2012-04-27 06:18:01 PST -------
Hi Julien, can you please review the patch?
------- Comment #16 From 2012-05-03 15:49:52 PST -------
(From update of attachment 138085 [details])
View in context: https://bugs.webkit.org/attachment.cgi?id=138085&action=review

To be frank, I think your approach of coming and implementing the whole grammar (even worse the grammars of different CSS values) is going to be painful: you have little experience in WebKit and > 20k patches from someone with little experience in the WebKit way have little change to get landed (also they are a reviewer time sink)

Think about this incrementally: which step as small as possible can you take towards supporting the full grammar while getting your change tested! It's important to keep your changes small and tested: this helps reviewers assess the quality of your change.

My take here would be to add parsing support for one <position> first, then add the needed rendering. Once this is done, scale to 2 <position>, ... Also this bug is about 'background-position', it is good that you are thinking about other values but don't get ahead of your self: getting 'background-position' is enough work for now.

I haven't read the whole patch because I think it should be split and tested.

> Source/WebCore/ChangeLog:11
> +        Currently parser supports only parsing of two values for background-position property.
> +        According to CSS specification http://www.w3.org/TR/css3-background/#the-background-position,
> +        it can take upto four values. This parsing function can also be reused for parsing other
> +        properties like -transform-origin, -perspective-origin, -webkit-mask-position and -radial-gradient.

Saying what grammar you *do* support would be helpful instead of having to extract it from the code.

> Source/WebCore/ChangeLog:13
> +        No functionality change. Tests to be added in next patch.

Technically this is a change in functionality. It's not exposed in JavaScript as you don't implement the getComputedStyle part though. Pushing the tests in another bug is not going to work as we expect changes in functionality to be tested.

> Source/WebCore/css/CSSParser.cpp:3225
> +    if (value->primitiveType() == CSSPrimitiveValue::CSS_IDENT) {
> +        int key = value->getIdent();
> +        return key == CSSValueLeft || key == CSSValueRight
> +            || key == CSSValueTop || key == CSSValueBottom || key == CSSValueCenter;
> +    }
> +    return false;

This should be:

return isBackgroundKeywordX(value) || isBackgroundKeyword(value) || isBackgroundKeywordC(value);

> Source/WebCore/css/CSSParser.cpp:3243
> +    if (value->primitiveType() == CSSPrimitiveValue::CSS_IDENT) {
> +        int key = value->getIdent();
> +        return key == CSSValueTop || key == CSSValueBottom;
> +    }
> +    return false;

We prefer early return (not repeated):
if (!value->isIdent())
   return false;
int identity = value->getIdent();
return identity == ...;

(btw, |key| is not a good variable name as it doesn't bear any meaning)

> Source/WebCore/css/CSSParser.cpp:3246
> +static bool isBackgroundKeywordC(CSSPrimitiveValue* value)

The naming doesn't cut. I guess C as Center but please don't abbreviate needlessly your names.

> Source/WebCore/rendering/style/FillLayer.h:208
> +    bool m_xOriginSet : 1;
> +    bool m_yOriginSet : 1;

The whole pattern of having a bool for being set + the value above makes me think we should have some structure to do that for us. We would need to be smart about it.
------- Comment #17 From 2012-05-03 23:13:47 PST -------
(From update of attachment 138085 [details])
View in context: https://bugs.webkit.org/attachment.cgi?id=138085&action=review

Thanks for reviewing.
Rendering code for one or two <position> is already present. So my approach here was to reduce three or four <position> into two <position>. 
There is a special case when offset in pixels from right or bottom is specified where rendering support is needed.
I will upload a patch which only handles background-position and addressing review comments along with test cases.

>> Source/WebCore/ChangeLog:11
>> +        properties like -transform-origin, -perspective-origin, -webkit-mask-position and -radial-gradient.
> 
> Saying what grammar you *do* support would be helpful instead of having to extract it from the code.

I will make changes such that only background-position specification is handled. Other properties will be handled in different bug to keep the patch smaller and easier for review.

>> Source/WebCore/ChangeLog:13
>> +        No functionality change. Tests to be added in next patch.
> 
> Technically this is a change in functionality. It's not exposed in JavaScript as you don't implement the getComputedStyle part though. Pushing the tests in another bug is not going to work as we expect changes in functionality to be tested.

This is a draft patch where only parsing functions are written but not called from anywhere. As per your previous suggestion, I was splitting up bigger patch into parsing code and real patch that introduces the change. I meant to add tests to this bug itself when real patch would be uploaded as this draft patch itself was getting bigger.

>> Source/WebCore/css/CSSParser.cpp:3225
>> +    return false;
> 
> This should be:
> 
> return isBackgroundKeywordX(value) || isBackgroundKeyword(value) || isBackgroundKeywordC(value);

Done.

>> Source/WebCore/css/CSSParser.cpp:3243
>> +    return false;
> 
> We prefer early return (not repeated):
> if (!value->isIdent())
>    return false;
> int identity = value->getIdent();
> return identity == ...;
> 
> (btw, |key| is not a good variable name as it doesn't bear any meaning)

Done.

>> Source/WebCore/css/CSSParser.cpp:3246
>> +static bool isBackgroundKeywordC(CSSPrimitiveValue* value)
> 
> The naming doesn't cut. I guess C as Center but please don't abbreviate needlessly your names.

Done.

>> Source/WebCore/rendering/style/FillLayer.h:208
>> +    bool m_yOriginSet : 1;
> 
> The whole pattern of having a bool for being set + the value above makes me think we should have some structure to do that for us. We would need to be smart about it.

Right. But shouldn't that be done as different patch as this patch is already getting bigger and we would be touching code related to other fill properties?
------- Comment #18 From 2012-05-07 01:01:10 PST -------
Created an attachment (id=140488) [details]
WIP patch with two position values

WIP patch addressing review comments
------- Comment #19 From 2012-05-16 21:44:40 PST -------
(From update of attachment 140488 [details])
I am going to break full patch down into different bugs instead of doing it incrementally in same bug.
------- Comment #20 From 2012-11-07 12:34:18 PST -------
(In reply to comment #19)
> (From update of attachment 140488 [details] [details])
> I am going to break full patch down into different bugs instead of doing it incrementally in same bug.

This bug has been inactive since May, If nobody objects, I'd like to take it to actually implement the feature. I landed new tests already to backup the existing implementation.
------- Comment #21 From 2012-11-07 19:50:08 PST -------
(In reply to comment #20)
> (In reply to comment #19)
> > (From update of attachment 140488 [details] [details] [details])
> > I am going to break full patch down into different bugs instead of doing it incrementally in same bug.
> 
> This bug has been inactive since May, If nobody objects, I'd like to take it to actually implement the feature. I landed new tests already to backup the existing implementation.

Sure.
------- Comment #22 From 2012-12-10 10:27:16 PST -------
I think we can close this bug now. The support is now enabled by default on all WebKit port.