Added backround-repeat-x and -y as keyword properties.

Added backround-repeat-x and -y as keyword properties.

Now code like <element>.style['background-repeat-x'] = 'no-repeat' would work.

BUG=352140
Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=171568

[Dmitry Zvorygin, 2014-04-09 16:27:19]

Please take a look.

[eseidel, 2014-04-09 17:14:35]

Please add a test.  Presumably this matchds the spec & other browsers?

[eseidel, 2014-04-09 17:30:04]

So before this change did style="background-repeat-x: norepeat" not work either?
 Presumably?

[Dmitry Zvorygin, 2014-04-09 17:39:39]

On 2014/04/09 17:30:37, eseidel wrote:

Additional details:

Before that change it didn't work (maybe that was a regression?).
Seems, it isn't in specs: googling for "site:w3c.org background-position-x"
doesn't find anything.

In our code we mention background-repeat-x(-y) in many times, so at least, seems
it should work.

The problem is that when we set background on node it expands to set of
"background-image, background-repeat-x(-y), background-position-x(-y),....", but
we can't set some of these properties back. So "style copying code" in DevTools
fails a little in one place.

I think that we should either support background-repeat-x(-y) work properly, or
hide it even as enumerateable property and expose it only via background-repeat,
to make behaviour more consistent.

Maybe I should forward it to blink-dev?

[eseidel, 2014-04-09 17:40:10]

Alan: you have more recent CSS context than I do, could you review and possibly
guide Dmitry towards coming up with a test?

The fix l-g-t-m, but we should test this to make sure it doesn't regress (and so
we can easily compare with other browsers).

[eseidel, 2014-04-09 17:43:18]

This does appear to be a webkit-only feature. :(

http://dev.w3.org/csswg/css-backgrounds/#the-background-position

[ojan, 2014-04-09 19:36:26]

On 2014/04/09 17:39:39, Dmitry Zvorygin wrote:
> I think that we should either support background-repeat-x(-y) work properly,
or
> hide it even as enumerateable property and expose it only via
background-repeat,
> to make behaviour more consistent.

Yes. We should definitely do one of these two. It's not clear to me which. If
the CSS working group has explicitly decided that these should not be exposed,
then we probably should fix them to not be enumerable. If not, then we should
probably send an email to www-style suggesting exposing them and then we expose
them.

If you don't want to bother with standards stuff, the safe thing is to make them
not enumerable. We can always expose them later if we decide it's useful.

Tab, do you know if this has been discussed on www-style? I couldn't find any
references to it, but maybe I'm searching for the wrong thing.

[TabAtkins, 2014-04-09 22:36:44]

On 2014/04/09 19:36:26, ojan wrote:
> On 2014/04/09 17:39:39, Dmitry Zvorygin wrote:
> > I think that we should either support background-repeat-x(-y) work properly,
> or
> > hide it even as enumerateable property and expose it only via
> background-repeat,
> > to make behaviour more consistent.
> 
> Yes. We should definitely do one of these two. It's not clear to me which. If
> the CSS working group has explicitly decided that these should not be exposed,
> then we probably should fix them to not be enumerable. If not, then we should
> probably send an email to www-style suggesting exposing them and then we
expose
> them.
> 
> If you don't want to bother with standards stuff, the safe thing is to make
them
> not enumerable. We can always expose them later if we decide it's useful.
> 
> Tab, do you know if this has been discussed on www-style? I couldn't find any
> references to it, but maybe I'm searching for the wrong thing.

The CSSWG definitely plans on shipping background-*position*-x/y.  There was
some angst over logical coordinates, but our current plan for handling that is
consistent with shipping -x/y first.

I see nothing wrong with background-repeat-x/y, then.  I suppose the grammar for
each is just "none | repeat", and background-repeat is just a magical syntax
that maps in prose?  We should at least email the CSSWG about it to give them a
heads-up.  I can do that.

Nobody's currently working on Backgrounds Level 4, which is why this hasn't
shown up in spec text yet.

[ojan, 2014-04-09 22:44:56]

Tab, if you could email www-style and post a link to this code review, then I'm
fine with moving forward with the patch once it gets a test.

[TabAtkins, 2014-04-10 00:41:38]

On 2014/04/09 22:44:56, ojan wrote:
> Tab, if you could email www-style and post a link to this code review, then
I'm
> fine with moving forward with the patch once it gets a test.

http://lists.w3.org/Archives/Public/www-style/2014Apr/0085.html

[alancutter (OOO until 2018), 2014-04-10 01:52:28]

We should support setting multiple repeat values for background-repeat-x/y eg.
"background-repeat-x: repeat, no-repeat".

Additionally we should support reading background-repeat-x/y off
getComputedStyle(), this is currently unsupported.

The test case should cover these use cases.

[eseidel, 2014-04-10 03:44:42]

Could one of you more advanced CSS hackers help Dmitry with some test case
pointers?  I fear we've collapsed on his simple CL.

[eseidel, 2014-04-10 03:45:36]

It sounds like the simplest fix is to make this non-enumerable and leave all the
standards bits to be worked out at a later time.

[Dmitry Zvorygin, 2014-04-10 10:32:28]

On 2014/04/10 03:45:36, eseidel wrote:
> It sounds like the simplest fix is to make this non-enumerable and leave all
the
> standards bits to be worked out at a later time.

Added tests.

This might not be the simplest fix, I suppose.

I've minified the test case, that made me to create this CL - you can try to run
this code in your inspector right now:

var div = document.createElement("div");
div.setAttribute("style", "background-repeat: no-repeat; font-size: 10px");

console.log("Original:");
for (var i = 0; i < div.style.length; i++ ) {
  console.log(div.style[i] + " = " + div.style[div.style[i]])
}

var copy = document.createElement("div");

for (var i = 0; i < div.style.length; i++ ) {
  copy.style[div.style[i]] = div.style[div.style[i]]
}

console.log("Copied:");

for (var i = 0; i < copy.style.length; i++ ) {
  console.log(copy.style[i] + " = " + copy.style[copy.style[i]])
}

// In firefox this snipped doesn't work at all

[Dmitry Zvorygin, 2014-04-14 09:40:30]

Ping?

[ojan, 2014-04-14 17:17:59]

Sorry to keep you waiting. LGTM. I think this is a minor enough change that it
doesn't need to go through the formal intent to ship process.

Please fix the comments on the test-case before committing.

https://codereview.chromium.org/231153003/diff/20001/LayoutTests/fast/backgro...
File LayoutTests/fast/backgrounds/background-repeat-computed-style.html (right):

https://codereview.chromium.org/231153003/diff/20001/LayoutTests/fast/backgro...
LayoutTests/fast/backgrounds/background-repeat-computed-style.html:3:
<title>Syntax for backgroundRepeat</title>
Typically we only include the html, head, title and body elements if the test
requires them for some reason, which this test doesn't.

Also, we default to testing in standards mode, so you'll need a <!DOCTYPE html>.

https://codereview.chromium.org/231153003/diff/20001/LayoutTests/fast/backgro...
LayoutTests/fast/backgrounds/background-repeat-computed-style.html:4: <style
type="text/css">
No need to include the type attribute.

https://codereview.chromium.org/231153003/diff/20001/LayoutTests/fast/backgro...
LayoutTests/fast/backgrounds/background-repeat-computed-style.html:39: <div
id="case12" class="testDiv" style="background-repeat-x:no-repeat;
background-repeat-y:no-repeat">This div should have no repeating background
(background-repeat-x:no-repeat; background-repeat-y:no-repeat)</div>
Can you also add a test-case for the values of background-repeat that are not
allowed in background-repeat-x/y, e.g. background-repeat-x: repeat-x?

https://codereview.chromium.org/231153003/diff/20001/LayoutTests/fast/backgro...
LayoutTests/fast/backgrounds/background-repeat-computed-style.html:41: <script
type="text/javascript">
ditto.

https://codereview.chromium.org/231153003/diff/20001/LayoutTests/fast/backgro...
LayoutTests/fast/backgrounds/background-repeat-computed-style.html:62: var
testCase = document.getElementById(caseId);
Indentation is off (should be 4 spaces)

[Dmitry Zvorygin, 2014-04-15 11:30:40]

https://codereview.chromium.org/231153003/diff/20001/LayoutTests/fast/backgro...
File LayoutTests/fast/backgrounds/background-repeat-computed-style.html (right):

https://codereview.chromium.org/231153003/diff/20001/LayoutTests/fast/backgro...
LayoutTests/fast/backgrounds/background-repeat-computed-style.html:3:
<title>Syntax for backgroundRepeat</title>
On 2014/04/14 17:17:59, ojan wrote:
> Typically we only include the html, head, title and body elements if the test
> requires them for some reason, which this test doesn't.
> 
> Also, we default to testing in standards mode, so you'll need a <!DOCTYPE
html>.

Done.

https://codereview.chromium.org/231153003/diff/20001/LayoutTests/fast/backgro...
LayoutTests/fast/backgrounds/background-repeat-computed-style.html:4: <style
type="text/css">
On 2014/04/14 17:17:59, ojan wrote:
> No need to include the type attribute.

Done.

https://codereview.chromium.org/231153003/diff/20001/LayoutTests/fast/backgro...
LayoutTests/fast/backgrounds/background-repeat-computed-style.html:39: <div
id="case12" class="testDiv" style="background-repeat-x:no-repeat;
background-repeat-y:no-repeat">This div should have no repeating background
(background-repeat-x:no-repeat; background-repeat-y:no-repeat)</div>
On 2014/04/14 17:17:59, ojan wrote:
> Can you also add a test-case for the values of background-repeat that are not
> allowed in background-repeat-x/y, e.g. background-repeat-x: repeat-x?

Done.

https://codereview.chromium.org/231153003/diff/20001/LayoutTests/fast/backgro...
LayoutTests/fast/backgrounds/background-repeat-computed-style.html:41: <script
type="text/javascript">
On 2014/04/14 17:17:59, ojan wrote:
> ditto.

Done.

https://codereview.chromium.org/231153003/diff/20001/LayoutTests/fast/backgro...
LayoutTests/fast/backgrounds/background-repeat-computed-style.html:62: var
testCase = document.getElementById(caseId);
On 2014/04/14 17:17:59, ojan wrote:
> Indentation is off (should be 4 spaces)

Done.

[Dmitry Zvorygin, 2014-04-15 11:30:51]

The CQ bit was checked by zvorygin@chromium.org

[commit-bot: I haz the power, 2014-04-15 11:31:17]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/zvorygin@chromium.org/231153003/40001

[commit-bot: I haz the power, 2014-04-15 12:37:05]

Change committed as 171568