Bug 104014

Summary: [CSS3 Backgrounds and Borders] Allow the CSS3 background position offset for background shorthand.
Product: WebKit Reporter: Alexis Menard (darktears) <menard>
Component: CSSAssignee: Alexis Menard (darktears) <menard>
Status: RESOLVED FIXED    
Severity: Normal CC: cmarcelo, krit, macpherson, ojan, webkit.review.bot
Priority: P2 Keywords: WebExposed
Version: 528+ (Nightly build)   
Hardware: All   
OS: All   
Bug Depends on: 104131    
Bug Blocks: 37514    
Attachments:
Description Flags
Patch
none
Patch none

Alexis Menard (darktears)
Reported 2012-12-04 10:35:24 PST
[CSS3 Backgrounds and Borders] Allow the CSS3 background position offset for background shorthand.
Attachments
Patch (12.68 KB, patch)
2012-12-04 10:44 PST, Alexis Menard (darktears) 		no flags
DetailsFormatted DiffDiff
Patch (19.88 KB, patch)
2012-12-05 03:46 PST, Alexis Menard (darktears) 		no flags
DetailsFormatted DiffDiff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.
Alexis Menard (darktears)
Comment 1 2012-12-04 10:44:37 PST
Created attachment 177510 [details] Patch
Dirk Schulze
Comment 2 2012-12-04 11:32:58 PST
Comment on attachment 177510 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=177510&action=review > Source/WebCore/css/CSSParser.cpp:3859 > +bool CSSParser::isPositionValue(CSSParserValue* value) can you make this an inline function? Or just put this in the header? > Source/WebCore/css/CSSParser.cpp:4139 > + // parseFillBackgroundPosition advances the m_valueList pointer Make it a sentence please. > LayoutTests/fast/backgrounds/background-position-parsing-2.html:454 > +style.background = "center center center / 10em gray round fixed border-box"; > +shouldBe("style.background", "'left 30% top 10% round fixed border-box border-box gray'"); > +shouldBe("computedStyle.background", "'rgb(128, 128, 128) none round fixed left 30% top 10% / auto border-box border-box'"); > + Do we have negative tests that are supposed to fail? If not, can you add them please?
Dirk Schulze
Comment 3 2012-12-04 11:38:13 PST
Comment on attachment 177510 [details] Patch Looking at it again, it looks like they are missing, since no test needs to be adapted. Please add negative tests as well, where the position is partly between background-position and -size, where you have 1, 2, 3, 4 or fife values. I just can see tests with 4 values. That is not enough. I assume we have tests for 1 and 2 values, but 3 and 5 are missing.
Alexis Menard (darktears)
Comment 4 2012-12-04 12:08:37 PST
Comment on attachment 177510 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=177510&action=review >> LayoutTests/fast/backgrounds/background-position-parsing-2.html:454 >> + > > Do we have negative tests that are supposed to fail? If not, can you add them please? These on top were. Maybe it was not clear but after debug("background-position inside the background shorthand, some invalid cases, no changes expected"); in fact the style didn't change because the background values were incorrect. I made it more clear in the next version.
Alexis Menard (darktears)
Comment 5 2012-12-05 03:46:52 PST
Created attachment 177718 [details] Patch
Dirk Schulze
Comment 6 2012-12-05 06:29:08 PST
Comment on attachment 177718 [details] Patch LGTM. r=me.
WebKit Review Bot
Comment 7 2012-12-05 07:11:27 PST
Comment on attachment 177718 [details] Patch Clearing flags on attachment: 177718 Committed r136683: <http://trac.webkit.org/changeset/136683>
WebKit Review Bot
Comment 8 2012-12-05 07:11:30 PST
All reviewed patches have been landed. Closing bug.
Note You need to log in before you can comment on or make changes to this bug.