Closed Bug 550426 Opened 13 years ago Closed 7 years ago

Add background-position-x and background-position-y (to make it easier to change one without the other)

Categories

(Core :: CSS Parsing and Computation, enhancement)

Product:
Core
Component:
CSS Parsing and Computation
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla49
Tracking Flags:
Tracking Status
firefox49 --- fixed

People

(Reporter: zefling, Assigned: mstange)

References

(
URL
)

Details

(Keywords: dev-doc-complete, Whiteboard: [parity-IE], [parity-webkit], [mozfr-community])

Attachments

(10 files, 1 obsolete file)

MozReview Request: Bug 550426 - Add support for {background,mask}-position-{x,y}, property_database.js changes. r?dbaron
58 bytes, text/x-review-board-request
dbaron
: review+
Details
MozReview Request: Bug 550426 - Add support for {background,mask}-position-{x,y}, most of the style system changes. r?dbaron
58 bytes, text/x-review-board-request
dbaron
: review+
Details
MozReview Request: Bug 550426 - Add support for {background,mask}-position-{x,y}, StyleAnimation changes. r?dbaron
58 bytes, text/x-review-board-request
dbaron
: review+
Details
MozReview Request: Bug 550426 - Add support for {background,mask}-position-{x,y}, computed style additions. r?dbaron
58 bytes, text/x-review-board-request
dbaron
: review+
Details
MozReview Request: Bug 550426 - In PropertySupportsVariant, add {background,mask}-position-{x,y} to the list of properties that are parsed by functions. r?dbaron
58 bytes, text/x-review-board-request
dbaron
: review+
Details
MozReview Request: Bug 550426 - Fix test_transitions_per_property.html. r?dbaron
58 bytes, text/x-review-board-request
dbaron
: review+
Details
MozReview Request: Bug 550426 - Fix browser_animation_animated_properties_delayed.js. r?dbaron
58 bytes, text/x-review-board-request
dbaron
: review+
Details
MozReview Request: Bug 550426 - Use background-position-x/y in ActiveLayerTracker. r?dbaron
58 bytes, text/x-review-board-request
dbaron
: review+
Details
MozReview Request: Bug 550426 - Use background-position-x/y when detecting scroll-linked effects. r?dbaron
58 bytes, text/x-review-board-request
dbaron
: review+
Details
MozReview Request: Bug 550426 - Add a few reftests for background-position-x/-y. r?dbaron
58 bytes, text/x-review-board-request
dbaron
: review+
Details 
User-Agent:       Mozilla/5.0 (Windows; U; Windows NT 6.0; fr; rv:1.9.2) Gecko/20100115 Firefox/3.6 FirePHP/0.4
Build Identifier: Mozilla/5.0 (Windows; U; Windows NT 6.0; fr; rv:1.9.2) Gecko/20100115 Firefox/3.6 FirePHP/0.4

Request support for background-position-x & background-position-y or an equivalent.

Reproducible: Always

Actual Results:  
background-position-x & background-position-y don't exist.
It's possible only in IE.
Summary: Change change only x or y is impossible. → background-position : Change change only x or y is impossible.
Component: General → Style System (CSS)
Product: Firefox → Core
QA Contact: general → style-system
Those properties don't exist in the CSS 3 border and background module.

and fwiw: http://www.w3.org/Style/CSS/Tracker/issues/9
Yep.  If the spec adds them we likely will too, but in the meantime it would break things that want to get computed background-position, no?
Stupid question: There would not be a state “no-change” ? This could be used a bit everywhere.

Example : 
background-position: no-change -50px;

With multiple background images, change only 1 image isn't possible too.
All of those sound like issues to raise on www-style@w3.org, not here.
Thank you for the address, I'll make a proposal.
http://snook.ca/archives/html_and_css/background-position-x-y

it's possible in both IE & Webkit (safari + chrome)...

It's a very nice feature to use if you have all icons in one image, inclusive it's various states, like normal, hover, disabled, active etc...

I don't see a reason why the w3c has denied this feature (according to the link I provided....)
A solution is used array. In multiple-background, for the first background :

background-position[1][x] : -50px;

Other exemple :
http://lists.w3.org/Archives/Public/www-style/2010Sep/0040.html
(In reply to comment #1)
> Those properties don't exist in the CSS 3 border and background module.
> 
> and fwiw: http://www.w3.org/Style/CSS/Tracker/issues/9

This is not a something new. This is already supported by IE (including IE6), Safari and Chrome (3 of 5 major browsers). (By the way, it makes sense to add "parity-ie" and "parity-webkit" keywords to this bug report.)

This is very useful when using sprites and hovering together and is just subject of Occam's razor.

The syntax is absolutely reasonable and is in full accordance with naming of existing standard values and properties that deal with two axes:
— background-repeat: repeat-x/-y,
— overflow-x/-y.

Mozilla would implement this just on parity basis so that Opera would left the only browser that does not implement this.

CSS spec authors then may just document implemented and used feature. (I personally am even not sure that this is subject to discuss with CSS WG. This is already implemented and just have to be documented.)

Thanks.

(In reply to comment #10)
Multiple backgrounds is somewhat offtopic in this report; separate bug should be reported for multiple backgrounds.
Please read comment 2.  If we did this, as things stand in the spec, we would have to start returning "" for background-position from getComputedStyle.  This would break pages.  So as the specs stand, we can't implement this without either breaking pages or breaking the spec.

Which is where the CSS WG comes in, of course.  And they have explicitly decided, so far, to not include these properties in the spec...
(In reply to comment #12)
I think it would be reasonable to consider {background-position-x: 5px} with no {background-position-y} specified as {background-position: 5px 0}. Implementing {overflow-x} far after {overflow} has not broken pages, isn't it?
I don't think background-position-x and background-position-y make sense for the intended use of background-position (positioning a background image within an element, not positioning a piece of the background image within the element).

Also, overflow-x and overflow-y are quite a bit of a mess; if one is 'visible' and the other is not, the one that is 'visible' is changed to 'hidden'.
(In reply to comment #14)
> I don't think background-position-x and background-position-y make sense for
> the intended use of background-position (positioning a background image within
> an element, not positioning a piece of the background image within the
> element).

Not sure I have correctly understood you. It does not matter what is positioned: whole image or its part. Specifying separate positioning offset for one of axis would be very useful when using sprites and hovering together with cascading. Hover effect is just most often used case where repeating ourselves is redundant. For example:
A {background: url(example.png) 100px 100px no-repeat; }
A:hover {background-position-x: 0; }

Currently (with overflow's shorthand version only), we forced to repeat ourselve by specifying y offset again:
A:hover {background-position: 0 100px; }

In IE (even in IE6), Safari and Chrome, we can do such things more intelligent. Why not in Firefox?

Can this be implemented at least with vendor (-moz-) prefix (so that we would use it in most of browsers except only Opera)?
> as {background-position: 5px 0}

What's the point?  The whole idea of the separate properties is to NOT change the other value, no?

> Implementing {overflow-x} far after {overflow}

Forced us to break the spec for getComputedStyle().overflow to keep working.  We'd rather not break the spec more than we have to.

> In IE (even in IE6), Safari and Chrome, we can do such things 

Yes, they have fewer problems with breaking the spec than we do, apparently?
> as {background-position: 5px 0}

> What's the point?  The whole idea of the separate properties is to NOT change
the other value, no?

i think the point was that if no previous position-y has been supplied then return it as 0, that way it doesn't break by returning background-position: 5px.

so if you have:

element {
background-position: 5px 10px;
}

element.class {
background-position-x: 10px;
}

you get:

element.class {
background-position: 10px 10px
}

and if you have no previously declared y position:

element {
background-position-x: 5px;
}

you just get 0 as the other value for the pair.

element {
background-position: 5px 0;
}

would this break the spec? as a developer, i'd be fine with this behavior, since it's clear that the use case is where you already have a pair of values and want to override just one.
(In reply to comment #16)
> > as {background-position: 5px 0}
> 
> What's the point?  The whole idea of the separate properties is to NOT change
> the other value, no?

The whole idea is to make possible to _override_ one of both previously defined parts of background-position property value. This is one of fundamental principles of CSS itself.

As for case when position is specified for one of axes only, then it's quite consistent to consider omitted value as equal to 0.

River Brandon's meaning of the idea is correct.
BTW, as per spec, {background-position: left} is equivalent to {background-position: left center}. So, default value for {background-position-x} in the absence of {background-position-y} (or vice versa) may be "center" too. This probably would be most spec-compliant solution.
Some clarification of my last phraze:

<<
So, default value for {background-position-x} in the absence of {background-position-y} (or vice versa) may be "center" too.
>>

should be read as:

<<
So, default value for {background-position-y} when {background-position-x} is specified in the absence of {background-position-y} (or vice versa) may be "center" too.
>>
You guys seem to not be following here.

Fact: The DOM spec for getComputedStyle requires that it return empty strings for all shorthand properties.

Fact: this bug is requesting that background-position be a shorthand that sets two new properties: background-position-x and background-position-y.

Fact: returning the empty strings for background-position from getComputedStyle will break sites.

Conclusion: if we make the change in this bug, we need to either break the spec or break sites.

That's quite apart from David's objections to the idea itself, which I'm not entirely sure I agree with, but maybe I just don't understand them.
boris,
thank you for that clarification. i'm not conversant in the spec, so i apologize for not understanding your earlier point. your explanation makes sense and i see the issue here.

i guess we can only hope that there's some chance this can be resolved at the level of the spec. unfortunately, that seems unlikely, as this has already been rejected, from what i can tell.
(In reply to comment #21)

OK, what about vendor-prefixed implementation?

-moz-background-position-x
-moz-background-position-y

AFAIK, vendor-prefixed properties are legitimate way to implement a draft or non-standard features while not breaking specs.

Expected behavior is following.

1. If {-moz-background-position-x: 5px} is specified _after_ previously specified {background-position: 20px 30px}, then (from DOM perspective) this is exact equivalent to direct respecifying background-position itself as {background-position: 5px 30px}.

2. If {-moz-background-position-x: 5px} is specified, but {background-position} has _not_ been previously specified, then this is:
	2.1. ignored (most simple solution);
	2.2. or equivalent to respecifying background-position as {background-position: 5px center} (consistent with CSS spec for background-position);
	2.3. or equivalent to respecifying background-position as {background-position: 5px 0} (consistent with existing implementations (IE, WebKit));

From practical perspective, implementation solution for the second case does not matter at all: web developers need just a way to _override_ background-position that specified before. But, in "parity" terms, it may make sense to be consistent with existing background-position-x/-y implementations (IE, WebKit).

Thus, vendor-prefixed implementation would be more than enough from practical perspective, while DOM spec would not be broken at all. Implementation itself is probably quite trivial: just automatically change background-position when -moz-background-position-x (or *-y) is altered.

Thanks.
> OK, what about vendor-prefixed implementation?

The problem is that we'd be breaking the spec for 'background-position', which is not vendor-prefixed...
Spec shortcoming is not a reason to stop progress. Is there really a problem in _practice_? What if just consider vendor-prefixed -moz-background-position-x/-y as non-related to background-position at all, so that latter is _not_ formally a shorthand version of prefixed x/y-subproperties (instead, background-position and -moz-background-position-x/-y would be just silently interlinked)? Old pages would be not broken anyway (they don't use -moz-background-position-x/-y).
To bug-report author (Célian VEYSSIÈRE) (or someone else responsible): please add "parity-ie" and "parity-webkit" keywords to the bug to make it more clear as for current status of the feature.
Whiteboard: parity-ie,parity-webkit
Whiteboard: parity-ie,parity-webkit → [parity-IE], [parity-webkit]
(In reply to comment #24)
> > OK, what about vendor-prefixed implementation?
> 
> The problem is that we'd be breaking the spec for 'background-position', which
> is not vendor-prefixed...

Then implement a non-standard background-position named '-moz-background-position' where we can split in x and y axis...

No offense but I only see avoidance to give us developers a workaround.
> Then implement a non-standard background-position

Then you have to figure out what to do if both are set.  Really, you do NOT want to go there as an author.
what to do if both are set: give -moz-background-position priority over standard background-position.

I know that saying it is not the same as working on it, and definitely not the same as justifying the rationale behind it to make the implementation, but the desired behaviour is working fine with other browsers and we, developers are asking for it because we also want to have a great experience when designing a cross-browser solution.

Imagine you want to make a card game with a 13x4 sprite, so you have

.card
{
background: url('cards-sprite.png') no-repeat 0 0;
}
.ace
{
background-position-x: 0;
}
.two
{
background-position-x: -48px;
}
/* 11 more rules for 3,4,5,6,7,8,9,10,J,Q,K */
.club
{
background-position-y: 0;
}
.diamond
{
background-position-y: -64px;
}
/* 2 more rules for heart and spade */

and you have the game working nicely in Trident and Webkit based browsers, but when you want to play it with Gecko based browsers it doesn't work because people from W3C "didn't find background-position-x/y useful enough to add it to the standard" and people from Mozilla "don't want implement it because it breaks the standard, but they are neither giving a workaround".

What you do?

Give the user a "Unfortunately you are using Firefox which doesn't support the technology used by the game, try with another browser please" message? NO!

You have to write background-position rules for every one of the 52 cards

.club.ace
{
background-position: 0 0;
}
.club.two
{
background-position: -48px 0;
}
/* 11 more rules for club cards */
.diamond.ace
{
background-position: 0 -64px;
}
.diamond.two
{
background-position: -48px -64px;
}
/* 11 more rules for diamond cards */
/* 13 more rules for heart cards */
/* 13 more rules for spade cards */

I'm not developing a card game (fortunately), but every time I have to work with sprites my first question is "where is the browser made to make the web a better place?" In a era with all the magic that CSS3 gives you, you still have to write code sprite by sprite?

All that story about the card game and my thoughts were not really needed, but I really hope to see in the near future that Mozilla's browsers are not more an obstacle trying to develop a cross-browser solution with the lesser effort as possible.
> give -moz-background-position priority over standard background-position

So if your site has these rules:

  * { -moz-background-position: center; }
  #mydiv { background-position: top left; }

the background for #mydiv should be centered?  That can't possibly be what you want.

> All that story about the card game and my thoughts were not really needed

Right.  It's clear what the use cases are.  The question is how to address them without violating web standards in the process.
(In reply to comment #30)
> So if your site has these rules:
> 
>   * { -moz-background-position: center; }
>   #mydiv { background-position: top left; }
> 
> the background for #mydiv should be centered?  That can't possibly be what you
> want.

id selector itself has higher priority than universal selector.

> It's clear what the use cases are.  The question is how to address them
> without violating web standards in the process.

Just implement this same way as it is already implemented in IE and WebKit. To avoid breaking CSS spec, just use vendor-prefixed properties. If it's so hard to make a decision what to do if regular properties and vendor-prefixed ones are specified simultaneously, then document that case as having undetermined behavior. That's all.
> id selector itself has higher priority than universal selector.

Yes, but these are _different_ properties, so they would both be set on the element once the cascade is done.  That's the whole point of the question in comment 28: what do you do when both are set after the cascade?

> That's all.

We don't consider that acceptable behavior.
(In reply to comment #32)
> what do you do when both are set after the cascade?

Apparently same what other browsers do. That's just matter of experiment.

> We don't consider that acceptable behavior.

Acceptability is abstraction. After all, which acceptability you consider when talking about _vendor-prefixed_ property? When web-developer is using vendor-prefixed property, then he takes responsibility for it. You don't have to provide for any use cases.
("any use cases" is to be read as "all/each use cases" in comment #33).
(In reply to comment #30)
> > give -moz-background-position priority over standard background-position
> 
> So if your site has these rules:
> 
>   * { -moz-background-position: center; }
>   #mydiv { background-position: top left; }
> 
> the background for #mydiv should be centered?  That can't possibly be what you
> want.
> 

Even when its undesired behaviour its acceptable if its documented, because the web developer should be careful enough to include both rules, the proprietary one and the standard one to avoid an unwanted behaviours:
* {
-moz-background-position: center;
background-position: center;
}
#mydiv {
-moz-background-position: top left;
background-position: top left;
}

On the other side, we just need -moz-background-position-x and -moz-background-position-y, -moz-background-position should be for internal use and not settable by user.
Yes, we don't need prefixed shorthand property (-moz-background-position) at all. -x/-y subproperties are only what we need.
I downloaded version 5 of Firefox and eagerly tried out my css animation... and then released that background-position x and y are not supported in Firefox. Ouch.
Which is why I have given it a vote... it really comes in handy when creating CSS animation...
This really is a useful feature for CSS sprites. It's not simply a matter of saving bytes, the functionality of maintain the x position while setting a new y position is crucially important.
(In reply to Boris Zbarsky (:bz) from comment #24)
> > OK, what about vendor-prefixed implementation?
> 
> The problem is that we'd be breaking the spec for 'background-position',
> which is not vendor-prefixed...

I'm not sure why this would have to break the spec for `background-position`, could you clarify?  My understanding is that we could define these properties like so:

    a.button { background-position: 0px 0px; }
    #a1      { /* no change */ }
    #a2      { -moz-background-position-y: -20px; }
    a.button:hover  { -moz-background-position-x: -50px; }
    a.button:active { -moz-background-position-x: -100px; }

Existence of `-moz-background-position` would be redundant and unnecessary, so only `-moz-background-position-x` and `-y` should be implemented.  This doesn't have to break the spec as `background-position` isn't technically a shorthand property.  `window.getComputedStyle(a2).backgroundPosition` should return "0px -20px" for a non-hovered, non-active state.

I don't see how it could hurt to implement this as a -moz prefix at all.  The use cases are all screaming for this - If I have a sprite that has 50 small buttons all with normal, hover and active states, my CSS has to be huge to support Firefox and Opera.  If I'm honest, I'd probably just leave it for those two browsers...
Summary: background-position : Change change only x or y is impossible (background-position-x or background-position-y). → Add background-position-x and background-position-y (to make it easier to change one without the other)
(In reply to Boris Zbarsky (:bz) from comment #21)
> You guys seem to not be following here.
> 
> Fact: The DOM spec for getComputedStyle requires that it return empty
> strings for all shorthand properties.

I think we should just ignore that bit for shorthands that used to be longhands (which I think we've already done in some cases).

So I don't think that's a real issue.


However, I think background-position a really bad way of doing spriting; see, e.g., http://blog.vlad1.com/2009/06/22/to-sprite-or-not-to-sprite/ (which doesn't even mention the issue of memory locality).  The underlying problem is that using background-position isn't a way of indicating that you only want part of the image, which means it's hard for the browser to optimize it correctly, in terms of memory use or performance.

The solution for sprites specified in http://dev.w3.org/csswg/css3-images/#image-fragments doesn't have this issue.
Hardware: x86 → All
(In reply to David Baron [:dbaron] from comment #47)
> However, I think background-position a really bad way of doing spriting;
> see, e.g., http://blog.vlad1.com/2009/06/22/to-sprite-or-not-to-sprite/
> (which doesn't even mention the issue of memory locality).  The underlying
> problem is that using background-position isn't a way of indicating that you
> only want part of the image, which means it's hard for the browser to
> optimize it correctly, in terms of memory use or performance.

Well, the point is not if sprites are good or bad. There are cases where CSS are good:

> the main example is combining a bunch of small similarily-sized images
> into a single one. For example, a bunch of 16×16 icons used as indicators
> for various things on your site, or a bunch of 32×32 icons used as 
> category headers or similar. 

Exactly this is the case that relates to this issue! You pack all your icons in a row with variations for lighter/darker background, active/inactive state, hover  etc. in different rows. Using a vertical offset you can switch all the icons (buttons, whatsoever) with just one directive. Your CSS is simpler, cleaner and easier to understand.

I guess this is the scenario that everyone have in mind, and this is what requires a solution.

image-fragment is not either a solution to this issue because it would require to replicate all the declarations for each variation.
(In reply to FlameStorm from comment #49)
> Not just IE, Safari and Chrome "can this". Opera supported
> background-position-x & background-position-y too.
> And one more time: have NO any ideas why FF so angrily ignored this issue ))
> It's amazing! )

Actually, Opera does not support this (neither stable 11.62 nor latest alpha build 1359).

Nevertheless two independent implementations for a feature (IE+WebKit in this case) is generally enough to standardize it.

BTW, there is www-style W3-mailing-list discussion that creates some probability that background-position-x/-y are finally on their way to be standardized in CSS4:

http://lists.w3.org/Archives/Public/www-style/2012Feb/0618.html

So probably it's really time to consider implementing this in Gecko.
(In reply to FlameStorm from comment #49)
> Let the most of our "singletoned" icons are visible. How many images loaded
> for icons? Just one! Where is "terms of memory use"?? Wall..

I don't understand what you're saying, but let me try to explain a little more clearly.  First, the amount of memory used isn't per image -- there's memory use per pixel when we uncompress the image, whether or not those pixels are going to be displayed at all.  So large transparent areas within a panel of icons can use *huge* amounts of memory.

Second, images are uncompressed (I think) into a row-primary format, so a pixel is adjacent in memory to the other pixels to its left and right, but not adjacent in memory to the other pixels above and below it.  Actual computers don't behave like an ideal Von Neumann machine; because of things like caches, access to memory near recently-accessed memory is faster than memory access to something you haven't touched in a while.  This means that if your sprite has a bunch of images next to each other, actually displaying those images will be a little bit slower.  (Maybe that part isn't such a big deal, though.)

Since background-position wasn't designed for sprites, it's not telling the browser which part of the image is to be used; it's saying where to put the image as a whole, and then relying on clipping rules that happen in a different layer to get the right effect.  So when authors use background-position as a sprite mechanism, we're relying on authors to understand the above issues and get things right.  On the other hand, a sprite mechanism in which authors declared which part of the image they want would give browsers more ability to optimize things above without having to teach every single Web author about the issues above.
@DavidBaron #47

> The solution for sprites specified in http://dev.w3.org/csswg/css3-images/#image-fragments doesn't have this issue.

While I agree that background-position isn't ideal for sprites, the unfortunate thing is that media fragments for images fails to address the problem that background-position-[x|y] solves (and the reason for this entire discussion).  This means we'd still have to write repeated CSS declarations for "state" sprites on individual elements, instead of being able to use a single class and then adjust the x/y position on state change.
Not splitting x and y means squaring the number of lines of code.
Although sprites might be done better, the extra overhead when you are animating x and y properties separately in both javascript and the css parser is unnecessary.

white-space is supposed to be shorthand property in css 3 as well... that's bound to break sites.  It wouldn't be helpful if browsers stuck to annoying parts of the spec, would it?  It's just not practical in this place.

Gecko stems from community work, so it would be greatly appreciated from web developers if it just eased off a bit of the work needed to write shims that would have been largely unnecessary and easily implemented on the Core side.
Guys, please VOTE for fixing the bug instead of spamming it with unhelpful comments.
(See "vote" link in "Importance" field under bug's summary.) Thanks.
CSS Variables should eliminate the need for background-position-[x|y] anyway, and work has already begun on implementing that feature.

https://bugzilla.mozilla.org/show_bug.cgi?id=773296

At this point, it doesn't really seem worth implementing -[x|y] if there's going to be a standards-compliant alternative way to do it.
By the way, new Opera 15 (based on Chromium/Blink) does support background-position-x/-y ([parity-opera] may now be added to the "Whiteboard" field), so Firefox is now the _only_ browser that does not support these properties.
(In reply to Andy Earnshaw from comment #72)
> CSS Variables should eliminate the need for background-position-[x|y]
> anyway, and work has already begun on implementing that feature.
> 
> https://bugzilla.mozilla.org/show_bug.cgi?id=773296
> 
> At this point, it doesn't really seem worth implementing -[x|y] if there's
> going to be a standards-compliant alternative way to do it.

'should eliminate'?  Sorry, I'm not following how - maybe your usecase is different to my usecase!
And how many years before all browsers support CSS Variables?

My use case: A row of 30 20px x 20px icons in a single image sprite with hover and active states on the 2nd and 3rd rows.
To support FF I'd need 60 extra css declarations that look like the following:
#result-list .expanded .section:hover .icons .sprite-icon.icon-1 { background-position: 0 -20px;}
#result-list .expanded .section:active .icons .sprite-icon.icon-1 { background-position: 0 -20px;}
... 58 others
#result-list .expanded .section:active .icons .sprite-icon.icon-8 { background-position: 140px -20px;}

Instead of the current 2:
#result-list .expanded .section:hover .icons .sprite-icon.icon-8 { background-position-y: -20px; }
#result-list .expanded .section:active .icons .sprite-icon.icon-8 { background-position-y: -40px;}

With the Image Fragments solution that David Baron mentions, I'd still need those extra 60 declarations as there is no way that I see to inherit the x/w portion of image('sprites.svg#xywh=40,0,20,20')

At the moment, I'm just going with the background-position-y solution that is supported by every other browser, and letting FF users do without the hover and active states.
(In reply to Eoghan Murray from comment #74)
> 
> 'should eliminate'?  Sorry, I'm not following how - maybe your usecase is
> different to my usecase!
> And how many years before all browsers support CSS Variables?

It doesn't matter when all browsers support CSS variables (for this use case), it only matters when Firefox supports it.  There's a WIP patch that will hopefully be finalized in the near future.
 
> My use case: A row of 30 20px x 20px icons in a single image sprite with
> hover and active states on the 2nd and 3rd rows.

...is the same as the use case I'm describing, solvable with CSS variables.  You would write the CSS like so:

a {
    background-position: 0px 0px;
    background-position: var(bgX) var(bgY);
}

a:hover, a:focus { background-position-x: -54px; var-bgX: -54px; }
a:active   { background-position-x: -108px; var-bgX: -108px; }
a.facebook { background-position-y: -20px; var-bgY: -20px;  }
a.gplus    { background-position-y: -40px; var-bgY: -40px;  }

This works just like regular CSS enhancements where older browsers ignore the newer rules/properties as they see them as invalid.

> With the Image Fragments solution that David Baron mentions, I'd still need
> those extra 60 declarations as there is no way that I see to inherit the x/w
> portion of image('sprites.svg#xywh=40,0,20,20')

I said the same thing in #53.

> At the moment, I'm just going with the background-position-y solution that
> is supported by every other browser, and letting FF users do without the
> hover and active states.

As am I.  The point of my comment was that, in over 3 years of this issue being active, nobody seems to want to implement the solution and at this point we may as well just wait for CSS variables (which is infinitely more useful) and forget about background-position-x.
(In reply to Andy Earnshaw from comment #75)

Thanks Andy, that shows how a CSS variable solution would work. And sorry for missing #53.

In case anyone is wondering, like I did, how the change in the bgX variable (by hovering on the gplus icon) doesn't also affect an adjacent facebook icon, check out Example 5 from http://www.w3.org/TR/css-variables/
Basically when a variable is redeclared, it doesn't globally update all values of that variable, but only affects uses of the variable according to the normal css cascading rules. Pretty cool!
See Also: → 733791
CSSWG resolution at:
http://lists.w3.org/Archives/Public/www-style/2014Apr/0188.html

(please don't take my commenting as an opportunity to add a bunch more comments)
Blocks: 1170542
FYI, for anyone on the thread who missed it, CSS Variables went live in Firefox 31.  You can safely achieve cros-browser background-position-[x|y] now even though Firefox doesn't support them. 

There's a usage example here: http://stackoverflow.com/a/29282573/94197.
Blocks: 1170774
Blocks: 1217868
Blocks: 1179386
Per CSSWG minutes at https://lists.w3.org/Archives/Public/www-style/2014Jun/0166.html I believe we should also do background-psoition-inline / background-position-block at the same time, although I'm not sure.
I'd say let's go for it, if it won't create a large delay in shipping background-position-x/y: (I don't know what it would take to implement those--first time I've heard of them).
Assignee: nobody → mstange
Status: NEW → ASSIGNED
Some notes:
 - These patches don't implement background-position-inline / background-position-block.
 - "mach mochitest layout/style" passes locally.
 - I don't really understand what the purpose of the properties passed in
   AppendToString(eCSSProperty_background_position,...) is. I've changed them to
   eCSSProperty_background_position_x/y and eCSSProperty_object_position somewhat
   arbitrarily.
 - The way CSSParserImpl::ParsePositionValueSeparateCoords piggybacks on
   ParsePositionValue is a little wasteful. Let me know if you want that to be
   optimized (by adding more code).
 - I just realized that I still need to fix the LayerActivity use of
   eCSSProperty_background_position. I'll add a patch that does that tomorrow.
 - I decided that we don't want to support the unitless numbers quirk on
   background-position-x/y.
 - For the parsing of background-position-x/y, I kept the requirement that the
   edge needs to come before the offset, even though it wouldn't be a problem to
   parse them in either order. I haven't checked what other browsers do.
Comment on attachment 8698693 [details]
MozReview Request: Bug 550426 - Add a layer property accessor template argument to SetBackgroundList(Pair) and FillBackgroundList. No functional changes. r?dbaron

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/28077/diff/1-2/
Comment on attachment 8698694 [details]
MozReview Request: Bug 550426 - Add support for {background,mask}-position-{x,y}, property_database.js changes. r?dbaron

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/28079/diff/1-2/
Comment on attachment 8698695 [details]
MozReview Request: Bug 550426 - Add support for {background,mask}-position-{x,y}, most of the style system changes. r?dbaron

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/28081/diff/1-2/
Comment on attachment 8698696 [details]
MozReview Request: Bug 550426 - Add support for {background,mask}-position-{x,y}, StyleAnimation changes. r?dbaron

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/28083/diff/1-2/
Comment on attachment 8698697 [details]
MozReview Request: Bug 550426 - Add support for {background,mask}-position-{x,y}, computed style additions. r?dbaron

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/28085/diff/1-2/
Comment on attachment 8698698 [details]
MozReview Request: Bug 550426 - In PropertySupportsVariant, add {background,mask}-position-{x,y} to the list of properties that are parsed by functions. r?dbaron

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/28087/diff/1-2/
Comment on attachment 8698699 [details]
MozReview Request: Bug 550426 - Fix test_transitions_per_property.html. r?dbaron

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/28089/diff/1-2/
I'm going to upload new patches tonight. I'm simplifying CSSParserImpl::ParsePositionCoord and I've rebased the computed style part on top of bug 1234707 and bug 1234676. I've also written a few reftests.
This bug and bug 686281 will probably have a lot of merge conflicts; not clear which will land first.
Comment on attachment 8698693 [details]
MozReview Request: Bug 550426 - Add a layer property accessor template argument to SetBackgroundList(Pair) and FillBackgroundList. No functional changes. r?dbaron

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/28077/diff/2-3/
Comment on attachment 8698694 [details]
MozReview Request: Bug 550426 - Add support for {background,mask}-position-{x,y}, property_database.js changes. r?dbaron

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/28079/diff/2-3/
Comment on attachment 8698695 [details]
MozReview Request: Bug 550426 - Add support for {background,mask}-position-{x,y}, most of the style system changes. r?dbaron

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/28081/diff/2-3/
Comment on attachment 8698696 [details]
MozReview Request: Bug 550426 - Add support for {background,mask}-position-{x,y}, StyleAnimation changes. r?dbaron

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/28083/diff/2-3/
Comment on attachment 8698697 [details]
MozReview Request: Bug 550426 - Add support for {background,mask}-position-{x,y}, computed style additions. r?dbaron

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/28085/diff/2-3/
Comment on attachment 8698698 [details]
MozReview Request: Bug 550426 - In PropertySupportsVariant, add {background,mask}-position-{x,y} to the list of properties that are parsed by functions. r?dbaron

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/28087/diff/2-3/
Comment on attachment 8698699 [details]
MozReview Request: Bug 550426 - Fix test_transitions_per_property.html. r?dbaron

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/28089/diff/2-3/
Comment on attachment 8698902 [details]
MozReview Request: Bug 550426 - Fix browser_animation_animated_properties_delayed.js. r?dbaron

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/28115/diff/1-2/
Comment on attachment 8698903 [details]
MozReview Request: Bug 550426 - Use background-position-x/y in ActiveLayerTracker. r?dbaron

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/28117/diff/1-2/
Comment on attachment 8698904 [details]
MozReview Request: Bug 550426 - Use background-position-x/y when detecting scroll-linked effects. r?dbaron

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/28119/diff/1-2/
Comment on attachment 8702608 [details]
MozReview Request: Bug 550426 - Add a few reftests for background-position-x/-y. r?dbaron

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/29141/diff/1-2/
I found another mistake: In the ActiveLayerTracker patch, I broke animation detection for inline style changes of the background-position property. So I'll update the patch queue one last time to fix that.
Comment on attachment 8698903 [details]
MozReview Request: Bug 550426 - Use background-position-x/y in ActiveLayerTracker. r?dbaron

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/28117/diff/2-3/
Comment on attachment 8698904 [details]
MozReview Request: Bug 550426 - Use background-position-x/y when detecting scroll-linked effects. r?dbaron

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/28119/diff/2-3/
Comment on attachment 8702608 [details]
MozReview Request: Bug 550426 - Add a few reftests for background-position-x/-y. r?dbaron

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/29141/diff/2-3/
Comment on attachment 8698693 [details]
MozReview Request: Bug 550426 - Add a layer property accessor template argument to SetBackgroundList(Pair) and FillBackgroundList. No functional changes. r?dbaron

https://reviewboard.mozilla.org/r/28077/#review26109

What effect does this have on code size?  I'm a little worried this will change us from having one copy of the generated code per type that it's used on to having one copy of the generated code per property.
Attachment #8698693 - Flags: review?(dbaron)
Comment on attachment 8698694 [details]
MozReview Request: Bug 550426 - Add support for {background,mask}-position-{x,y}, property_database.js changes. r?dbaron

https://reviewboard.mozilla.org/r/28079/#review26111

::: layout/style/test/property_database.js:2334
(Diff revision 3)
> +      "right -50px",
> +      "left -50px",
> +      "right 20px",
> +      "right 3em",
> +    ],
> +    invalid_values: [ "center 10px", "right 10% 50%", "left right", "left left",
> +                      "top", "bottom", "top, top", "top, bottom", "bottom, top", "top, 0%", "top, top, top, top, top",
> +                      "calc(0px + rubbish)"],

Could you add items to invalid_values for background-position-x that look like valid 2-part values for background-position-y, e.g., "top 20px", etc.

And the inverse for background-position-y, please.

It's a little odd to see this at this position in the patch sequence, but otherwise this looks good.

r=dbaron with that fixed
Attachment #8698694 - Flags: review?(dbaron) → review+
Comment on attachment 8698695 [details]
MozReview Request: Bug 550426 - Add support for {background,mask}-position-{x,y}, most of the style system changes. r?dbaron

https://reviewboard.mozilla.org/r/28081/#review26113

::: layout/style/Declaration.cpp:601
(Diff revision 3)
> +    case eCSSProperty_background_position: {
> +      // We know from above that all subproperties were specified.
> +      // However, we still can't represent that in the shorthand unless
> +      // they're all lists of the same length.  So if they're different
> +      // lengths, we need to bail out.

Maybe add a test for this to layout/style/test/test_shorthand_property_getters.html ?

::: layout/style/Declaration.cpp:613
(Diff revision 3)
> +        positionX->mValue.AppendToString(eCSSProperty_background_position_x,
> +                                         aValue, aSerialization);
> +        aValue.Append(char16_t(' '));
> +        positionY->mValue.AppendToString(eCSSProperty_background_position_y,
> +                                         aValue, aSerialization);

This, and the equivalent code in background serialization, isn't quite sufficient, I think.

The rules for 3-value and 4-value background-position are stricter than the rules for 1-value and 2-value.  In particular, note that the 3-value or 4-value syntax requires that the edges be specified for both values, while leaving the percentage/length optional.  This means that:
  background-position-x: 50%;
  background-position-y: top 10px;
is valid, but you serialize it to something:
  background-position: 50% top 10px;
that is invalid.  Instead, you need to serialize it to:
  background-position: left 50% top 10px;
  
In other words, you need to fill in a missing 'top' or 'left' when the other value has the long form.  But it's important that you *not* do this when it doesn't, since the short form is the longstanding syntax, and it's possible sites depend on serialization preferring the short form when it was what was used initially.

You should also add some tests for this, both to make sure you're getting valid values from serialization, and probably also specifically testing exactly what the expected values are.

::: layout/style/nsCSSParser.cpp:12082
(Diff revision 3)
> +  bool isHorizontal = (aPropID == eCSSProperty_background_position_x);

I'd prefer the conversion from nsCSSProperty -> bool happen inside ParseBackgroundPositionCoordItem.  It makes it a little more typesafe, and also a little more extensible to having a logical version as well.

::: layout/style/nsCSSParser.cpp:12462
(Diff revision 3)
> +  NS_ASSERTION((eCSSUnit_Enumerated == edge.GetUnit()  ||
> +                eCSSUnit_Null       == edge.GetUnit()) &&
> +                eCSSUnit_Enumerated != offset.GetUnit(),
> +                "Unexpected units");

please fix the indentation of the 3rd and 4th lines of this assertion

::: layout/style/nsComputedDOMStyle.cpp:2193
(Diff revision 3)
> -  for (uint32_t i = 0, i_end = bg->mPositionCount; i < i_end; ++i) {
> +  // If both background-position-x and background-position-y have the same
> +  // count, that's the count we'll use for the background-position shorthand.
> +  // Otherwise we use the image count.
> +  bool sameCount = (bg->mPositionXCount == bg->mPositionYCount);
> +  uint32_t i_end = sameCount ? bg->mPositionXCount : bg->mImageCount;

You should actually return null if the counts don't match.  (See DoGetOverflow, which does similar.  This matches https://drafts.csswg.org/cssom/#dom-cssstyledeclaration-getpropertyvalue linking to https://drafts.csswg.org/cssom/#serialize-a-css-value , although doesn't quite match the DOM Level 2 Style spec for getProperty... which we should really remove.

::: layout/style/nsRuleNode.cpp:6550
(Diff revision 3)
> +/* Helper function to convert a CSS <position-coord> specified value into its
> + * computed-style form. */
> +static void
> +ComputePositionCoordValue(nsStyleContext* aStyleContext,
> +                          const nsCSSValue& aValue,
> +                          nsStyleBackground::Position::PositionCoord& aComputedValue,
> +                          RuleNodeCacheConditions& aConditions)

Shouldn't this relpace ComputePositionValue rather than adding to it?

::: layout/style/nsRuleNode.cpp:6575
(Diff revision 3)
> +template <>
> +struct BackgroundItemComputer<nsCSSValueList, nsStyleBackground::Position::PositionCoord>

Shouldn't this replace the template specialization on nsStyleBackground::Position rather than adding to it?

::: layout/style/nsRuleNode.cpp:6753
(Diff revision 3)
> +      if (!(item->mValue.GetUnit() != eCSSUnit_Null &&
> +            item->mValue.GetUnit() != eCSSUnit_Inherit &&
> +            item->mValue.GetUnit() != eCSSUnit_Initial &&
> +            item->mValue.GetUnit() != eCSSUnit_Unset)) {
> +        printf("item->mValue.GetUnit(): %d\n", int(item->mValue.GetUnit()));
> +      }

please remove this debugging code

There are two things I feel I need to look at it a second review pass:
 (1) how you deal with the issue around serialization of invalid values
 (2) the diffs in the part of  nsRuleNode.cpp where I though the code should be changing rather than adding, and which I thus didn't review closely
Attachment #8698695 - Flags: review?(dbaron)
Comment on attachment 8698696 [details]
MozReview Request: Bug 550426 - Add support for {background,mask}-position-{x,y}, StyleAnimation changes. r?dbaron

https://reviewboard.mozilla.org/r/28083/#review26283
Attachment #8698696 - Flags: review?(dbaron) → review+
Comment on attachment 8698697 [details]
MozReview Request: Bug 550426 - Add support for {background,mask}-position-{x,y}, computed style additions. r?dbaron

https://reviewboard.mozilla.org/r/28085/#review26285

::: layout/style/nsComputedDOMStyle.cpp:2215
(Diff revision 3)
> +    RefPtr<nsDOMCSSValueList> itemList = GetROCSSValueList(false);
> +    RefPtr<nsROCSSPrimitiveValue> val = new nsROCSSPrimitiveValue;
> +    SetValueToPositionCoord(bg->mLayers[i].mPosition.mXPosition, val);
> +    itemList->AppendCSSValue(val.forget());
> +    valueList->AppendCSSValue(itemList.forget());

It's not clear to me why you need itemList here.  Why not just append val directly to valueList?  (For both x and y.)
Attachment #8698697 - Flags: review?(dbaron) → review+
Comment on attachment 8698698 [details]
MozReview Request: Bug 550426 - In PropertySupportsVariant, add {background,mask}-position-{x,y} to the list of properties that are parsed by functions. r?dbaron

https://reviewboard.mozilla.org/r/28087/#review26287
Attachment #8698698 - Flags: review?(dbaron) → review+
Comment on attachment 8698699 [details]
MozReview Request: Bug 550426 - Fix test_transitions_per_property.html. r?dbaron

https://reviewboard.mozilla.org/r/28089/#review26289
Attachment #8698699 - Flags: review?(dbaron) → review+
Comment on attachment 8698902 [details]
MozReview Request: Bug 550426 - Fix browser_animation_animated_properties_delayed.js. r?dbaron

https://reviewboard.mozilla.org/r/28115/#review26291
Attachment #8698902 - Flags: review?(dbaron) → review+
Comment on attachment 8698903 [details]
MozReview Request: Bug 550426 - Use background-position-x/y in ActiveLayerTracker. r?dbaron

https://reviewboard.mozilla.org/r/28117/#review26293

::: layout/base/ActiveLayerTracker.cpp:85
(Diff revision 3)
>      case eCSSProperty_background_position: return ACTIVITY_BACKGROUND_POSITION;

It's not clear to me whether the shorthand is still needed here.  Maybe it's not?

::: layout/style/nsDOMCSSAttrDeclaration.cpp:181
(Diff revision 3)
>    if (aPropID == eCSSProperty_opacity || aPropID == eCSSProperty_transform ||
>        aPropID == eCSSProperty_left || aPropID == eCSSProperty_top ||
>        aPropID == eCSSProperty_right || aPropID == eCSSProperty_bottom ||
>        aPropID == eCSSProperty_margin_left || aPropID == eCSSProperty_margin_top ||
>        aPropID == eCSSProperty_margin_right || aPropID == eCSSProperty_margin_bottom ||
> +      aPropID == eCSSProperty_background_position_x ||
> +      aPropID == eCSSProperty_background_position_y ||
>        aPropID == eCSSProperty_background_position) {

Given that it does look like the shorthand is needed here, maybe add a FIXME and file a bug about the absence of margin and margin-{block,inline}-{start,end} here.
Attachment #8698903 - Flags: review?(dbaron) → review+
Comment on attachment 8698904 [details]
MozReview Request: Bug 550426 - Use background-position-x/y when detecting scroll-linked effects. r?dbaron

https://reviewboard.mozilla.org/r/28119/#review26295

::: layout/style/nsDOMCSSDeclaration.cpp:82
(Diff revision 3)
> -    case eCSSProperty_background_position:
> +    case eCSSProperty_background_position_x:
> +    case eCSSProperty_background_position_y:

This needs both the shorthand and the longhand; you can't remove the shorthand here.

It also needs a FIXME for margin stuff that should also be fixed in the bug mentioned in my previous review.
Attachment #8698904 - Flags: review?(dbaron) → review+
Comment on attachment 8702608 [details]
MozReview Request: Bug 550426 - Add a few reftests for background-position-x/-y. r?dbaron

https://reviewboard.mozilla.org/r/29141/#review26297

::: layout/reftests/backgrounds/background-position-2c.html:4
(Diff revision 3)
> -  <title>background-position: center bottom 75%</title>
> +  <title>background-position: left 25% bottom 75%</title>

maybe fix the original here too?

::: layout/reftests/backgrounds/background-position-4d.html:4
(Diff revision 3)
> -  <title>background-position: left center</title>
> +  <title>background-position: left bottom 50%</title>

maybe the title should be background-position-y: bottom 50%?

::: layout/reftests/backgrounds/background-position-4e.html:4
(Diff revision 3)
>    <title>background-position: left</title>

This title also needs updating.
Attachment #8702608 - Flags: review?(dbaron) → review+
Now that bug 686281 landed, it would probably be good to merge and land this.
Flags: needinfo?(mstange)
Hey Markus, what's the state of this bug? Anything I can do to help?
Hello, comrads! It's interesting for me too, how much work is remaining? To begin and to end? ;)
(I guess one of the patches also needs re-review, in addition to the merging.)
I've picked this back up and am going to finish it. Sorry for the wait.
Flags: needinfo?(mstange)
Comment on attachment 8698694 [details]
MozReview Request: Bug 550426 - Add support for {background,mask}-position-{x,y}, property_database.js changes. r?dbaron

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/28079/diff/3-4/
Attachment #8698694 - Attachment description: MozReview Request: Bug 550426 - Add support for background-position-x/y, property_database.js changes. r?dbaron → MozReview Request: Bug 550426 - Add support for {background,mask}-position-{x,y}, property_database.js changes. r?dbaron
Attachment #8698695 - Attachment description: MozReview Request: Bug 550426 - Add support for background-position-x/y, most of the style system changes. r?dbaron → MozReview Request: Bug 550426 - Add support for {background,mask}-position-{x,y}, most of the style system changes. r?dbaron
Attachment #8698696 - Attachment description: MozReview Request: Bug 550426 - Add support for background-position-x/y, StyleAnimation changes. r?dbaron → MozReview Request: Bug 550426 - Add support for {background,mask}-position-{x,y}, StyleAnimation changes. r?dbaron
Attachment #8698697 - Attachment description: MozReview Request: Bug 550426 - Add support for background-position-x/y, computed style additions. r?dbaron → MozReview Request: Bug 550426 - Add support for {background,mask}-position-{x,y}, computed style additions. r?dbaron
Attachment #8698698 - Attachment description: MozReview Request: Bug 550426 - In PropertySupportsVariant, add background-position-x/y to the list of properties that are parsed by functions. r?dbaron → MozReview Request: Bug 550426 - In PropertySupportsVariant, add {background,mask}-position-{x,y} to the list of properties that are parsed by functions. r?dbaron
Attachment #8698695 - Flags: review?(dbaron)
Comment on attachment 8698695 [details]
MozReview Request: Bug 550426 - Add support for {background,mask}-position-{x,y}, most of the style system changes. r?dbaron

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/28081/diff/3-4/
Comment on attachment 8698696 [details]
MozReview Request: Bug 550426 - Add support for {background,mask}-position-{x,y}, StyleAnimation changes. r?dbaron

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/28083/diff/3-4/
Comment on attachment 8698697 [details]
MozReview Request: Bug 550426 - Add support for {background,mask}-position-{x,y}, computed style additions. r?dbaron

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/28085/diff/3-4/
Comment on attachment 8698698 [details]
MozReview Request: Bug 550426 - In PropertySupportsVariant, add {background,mask}-position-{x,y} to the list of properties that are parsed by functions. r?dbaron

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/28087/diff/3-4/
Comment on attachment 8698699 [details]
MozReview Request: Bug 550426 - Fix test_transitions_per_property.html. r?dbaron

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/28089/diff/3-4/
Comment on attachment 8698902 [details]
MozReview Request: Bug 550426 - Fix browser_animation_animated_properties_delayed.js. r?dbaron

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/28115/diff/2-3/
Comment on attachment 8698903 [details]
MozReview Request: Bug 550426 - Use background-position-x/y in ActiveLayerTracker. r?dbaron

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/28117/diff/3-4/
Comment on attachment 8698904 [details]
MozReview Request: Bug 550426 - Use background-position-x/y when detecting scroll-linked effects. r?dbaron

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/28119/diff/3-4/
Comment on attachment 8702608 [details]
MozReview Request: Bug 550426 - Add a few reftests for background-position-x/-y. r?dbaron

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/29141/diff/3-4/
Attachment #8698693 - Attachment is obsolete: true
https://reviewboard.mozilla.org/r/28081/#review26113

> Shouldn't this relpace ComputePositionValue rather than adding to it?

No, we still need ComputePositionValue for object-position.
(In reply to David Baron [:dbaron] ⌚️UTC-7 from comment #127)
> Comment on attachment 8698693 [details]
> MozReview Request: Bug 550426 - Add a layer property accessor template
> argument to SetBackgroundList(Pair) and FillBackgroundList. No functional
> changes. r?dbaron
> 
> https://reviewboard.mozilla.org/r/28077/#review26109
> 
> What effect does this have on code size?  I'm a little worried this will
> change us from having one copy of the generated code per type that it's used
> on to having one copy of the generated code per property.

Yes, this was the case. I've completely removed this patch from the patch series, but now we have some code duplication in the "most of the style system changes" part. I'm open to suggestions.
Attachment #8698695 - Flags: review?(dbaron) → review+
Comment on attachment 8698695 [details]
MozReview Request: Bug 550426 - Add support for {background,mask}-position-{x,y}, most of the style system changes. r?dbaron

https://reviewboard.mozilla.org/r/28081/#review46357

My previous review was in comment 129.

The patch reviewed in that review was:
https://reviewboard.mozilla.org/r/28081/diff/3/raw/

The current review is of:
https://reviewboard.mozilla.org/r/28081/diff/4/raw/

The change between these patches appears to be substantial enough that
I'm having trouble reading the interdiff and relating it to my comments.

Declaration.cpp:

Please fix these lines running past 80 by wrapping after an ||:
>+        if (repeat || positionX || positionY || clip || origin || size || attachment) {

>+        if (repeat || positionX || positionY || clip || origin || size || composite || mode) {

>+      if (!repeat || !positionX || !positionY || !clip || !origin || !size || !attachment) {



nsRuleNode.cpp:

>+/* Helper function to convert a CSS <position-coord> specified value into its
>+ * computed-style form. */

I don't see a CSS spec that defines <position-coord> as a term, since
the early draft at
https://drafts.csswg.org/css-backgrounds-4/#background-position-longhands
doesn't use the term.  Given that, I think you should describe this in
prose, as something like "the -x or -y part of a CSS <position>
specified value".


ComputePositionCoordValue should perhaps assert that positionArray->Count()
is 2.  (And, really, ComputePositionValue should assert that it's 4.)


It's unfortunate that you you needed to write SetImageLayerPositionCoordList
and FillBackgroundPositionCoordList instead of writing a BackgroundItemComputer
template specialization and continuing to use SetImageLayerList and
FillBackgroundList.  Was this because there are now two member variables for
the specified value but still just one member variable for the computed value?
Could you at least add a comment to those functions explaining that that's why
they're duplicated?  Or is there a way to avoid it?

I also don't see why the new functions need to be templates.  You could just
put the right type in.


Sorry for the delay on this.  And sorry for not really using MozReview,
but I find MozReview harmful for re-reviews.
https://reviewboard.mozilla.org/r/28081/#review46357

(In reply to David Baron [:dbaron] ⌚️UTC-7 (review requests must explain patch) from comment #156)
> It's unfortunate that you you needed to write SetImageLayerPositionCoordList
> and FillBackgroundPositionCoordList instead of writing a
> BackgroundItemComputer
> template specialization and continuing to use SetImageLayerList and
> FillBackgroundList.  Was this because there are now two member variables for
> the specified value but still just one member variable for the computed
> value?

Yes, that was the problem. This was why I had attachment 8698693 [details] in the last round of reviews - it allowed me to reuse the SetImageLayerList and FillBackgroundList code, at the cost of having the compiler generate more code.

> Could you at least add a comment to those functions explaining that that's
> why
> they're duplicated?

Sure.

> Or is there a way to avoid it?

If you're willing to eat the extra code size, I can go back to what I did originally.
Or I can try a hybrid approach, where we have a LayerPropAccessor that is shared between the one-field properties, and the pointer to the property is still passed as a function argument instead of as a template argument, with the background position coord accessor just ignoring that argument.

> I also don't see why the new functions need to be templates.  You could just
> put the right type in.

That's true.

> Sorry for the delay on this.  And sorry for not really using MozReview,
> but I find MozReview harmful for re-reviews.

Yeah, the underlying code really has changed quite a lot.
(In reply to Markus Stange [:mstange] from comment #157)
> > Or is there a way to avoid it?
> 
> If you're willing to eat the extra code size, I can go back to what I did
> originally.
> Or I can try a hybrid approach, where we have a LayerPropAccessor that is
> shared between the one-field properties, and the pointer to the property is
> still passed as a function argument instead of as a template argument, with
> the background position coord accessor just ignoring that argument.

I'd say just stick with the current approach (and add a comment explaining why).

> > Sorry for the delay on this.  And sorry for not really using MozReview,
> > but I find MozReview harmful for re-reviews.
> 
> Yeah, the underlying code really has changed quite a lot.

It's really because MozReview makes it harder to grab the raw diffs and diff them myself, and because many of my comments were written based on the interdiff (which I generated myself) rather than the diff shown by MozReview.
Comment on attachment 8698694 [details]
MozReview Request: Bug 550426 - Add support for {background,mask}-position-{x,y}, property_database.js changes. r?dbaron

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/28079/diff/4-5/
Comment on attachment 8698695 [details]
MozReview Request: Bug 550426 - Add support for {background,mask}-position-{x,y}, most of the style system changes. r?dbaron

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/28081/diff/4-5/
Comment on attachment 8698696 [details]
MozReview Request: Bug 550426 - Add support for {background,mask}-position-{x,y}, StyleAnimation changes. r?dbaron

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/28083/diff/4-5/
Comment on attachment 8698697 [details]
MozReview Request: Bug 550426 - Add support for {background,mask}-position-{x,y}, computed style additions. r?dbaron

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/28085/diff/4-5/
Comment on attachment 8698698 [details]
MozReview Request: Bug 550426 - In PropertySupportsVariant, add {background,mask}-position-{x,y} to the list of properties that are parsed by functions. r?dbaron

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/28087/diff/4-5/
Comment on attachment 8698699 [details]
MozReview Request: Bug 550426 - Fix test_transitions_per_property.html. r?dbaron

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/28089/diff/4-5/
Comment on attachment 8698902 [details]
MozReview Request: Bug 550426 - Fix browser_animation_animated_properties_delayed.js. r?dbaron

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/28115/diff/3-4/
Comment on attachment 8698903 [details]
MozReview Request: Bug 550426 - Use background-position-x/y in ActiveLayerTracker. r?dbaron

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/28117/diff/4-5/
Comment on attachment 8698904 [details]
MozReview Request: Bug 550426 - Use background-position-x/y when detecting scroll-linked effects. r?dbaron

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/28119/diff/4-5/
Comment on attachment 8702608 [details]
MozReview Request: Bug 550426 - Add a few reftests for background-position-x/-y. r?dbaron

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/29141/diff/4-5/
Oh no! :(
My try push hit this too, I don't know why I missed it.
Flags: needinfo?(mstange)
I'm going to push this again with the test fix.
I started https://developer.mozilla.org/en-US/docs/Web/CSS/background-position-x and https://developer.mozilla.org/en-US/docs/Web/CSS/background-position-y , and it looks like Sebastian Z fixed up some of the things that I didn't have permission to edit or didn't know how to edit.
(In reply to David Baron [:dbaron] ⌚️UTC-7 (review requests must explain patch) from comment #175)
> I started
> https://developer.mozilla.org/en-US/docs/Web/CSS/background-position-x and
> https://developer.mozilla.org/en-US/docs/Web/CSS/background-position-y , and
> it looks like Sebastian Z fixed up some of the things that I didn't have
> permission to edit or didn't know how to edit.

Yes, I added the parts that are stored in macros. What's still missing is the description of the different values (which is obviously also still missing from the spec.)

Sebastian
(In reply to Sebastian Zartner [:sebo] from comment #176)
> (In reply to David Baron [:dbaron] ⌚️UTC-7 (review requests must explain
> patch) from comment #175)
> > I started
> > https://developer.mozilla.org/en-US/docs/Web/CSS/background-position-x and
> > https://developer.mozilla.org/en-US/docs/Web/CSS/background-position-y , and
> > it looks like Sebastian Z fixed up some of the things that I didn't have
> > permission to edit or didn't know how to edit.
> 
> Yes, I added the parts that are stored in macros. What's still missing is
> the description of the different values (which is obviously also still
> missing from the spec.)
> 
> Sebastian

I've added the value descriptions now. Markus, please let me know if the property description are ok now.

While adding the value descriptions, I realized that Gecko's implementation differs from the currently specified syntaxes[1] insofar as it allows to define an offset without defining the related edge.
E.g. according to the spec. background-position-x: 20px; is invalid. David, was this already discussed with the W3C?

Also, is there a reason for restricting the syntax to first having to define the edge and then the offset?
I'd expect background-position-x: 20px right; to be valid, too (reading "20 pixels from the right"). Though that's currently interpreted as invalid.

Sebastian

[1] https://drafts.csswg.org/css-backgrounds-4/#background-position-longhands
Flags: needinfo?(mstange)
Flags: needinfo?(dbaron)
(In reply to Sebastian Zartner [:sebo] from comment #177)
> 
> While adding the value descriptions, I realized that Gecko's implementation
> differs from the currently specified syntaxes[1] insofar as it allows to
> define an offset without defining the related edge.
> E.g. according to the spec. background-position-x: 20px; is invalid. David,
> was this already discussed with the W3C?

This looks like an error. I'll fix it.

> Also, is there a reason for restricting the syntax to first having to define
> the edge and then the offset?
> I'd expect background-position-x: 20px right; to be valid, too (reading "20
> pixels from the right"). Though that's currently interpreted as invalid.

Yes, because in the background shorthand syntax you must put the edge first
and then the offset.
Flags: needinfo?(dbaron)
(In reply to Sebastian Zartner [:sebo] from comment #177)
> I've added the value descriptions now. Markus, please let me know if the
> property description are ok now.

Your description of the values on MDN look good to me, thank you.
Flags: needinfo?(mstange)
(In reply to Sebastian Zartner [:sebo] from comment #177)
> according to the spec. background-position-x: 20px; is invalid.

This is certainly a spec error. All existing implementations (IE, Edge, Chromium, WebKit) that the spec should be based on as for non-CSS3 syntax part, support declarations like `background-position-x: 20px`.

Actually, for now, as for non-CSS3 syntax part, it would probably be more reasonable to rely on existing implementations that work consistently for years than on an unstable draft.

I would even suppose that mixing implementation processes for two different parts of the syntax (old part supported for years by all browsers except for Firefox and the new CSS3 part invented rather recently by CSSWG) was probably a mistake making the implementation process just more confusing and slow.
I added an issue to the spec.
(In reply to Sebastian Zartner [:sebo] from comment #177)
> E.g. according to the spec. background-position-x: 20px; is invalid. David,
> was this already discussed with the W3C?

Actually, I think it's perfectly valid according to the spec.  I'll remove the issue.
(In reply to David Baron [:dbaron] ⌚️UTC-7 (review requests must explain patch) (busy May 9-13) from comment #183)
> (In reply to Sebastian Zartner [:sebo] from comment #177)
> > E.g. according to the spec. background-position-x: 20px; is invalid. David,
> > was this already discussed with the W3C?
> 
> Actually, I think it's perfectly valid according to the spec.  I'll remove
> the issue.

The syntax for background-position-x[1] is defined as follows:

[ center | [ left | right | x-start | x-end ] [ <percentage> | <length> ]? ]#

Which says that (if the value is not 'center') one of 'left', 'right', 'x-start', or 'x-end' must be defined according to the single bar separating them, right?
I think the syntax definition should instead look like this:

[ center | [ left | right | x-start | x-end ]? [ <percentage> | <length> ]? ]#

Sebastian

[1] https://drafts.csswg.org/css-backgrounds-4/#propdef-background-position-x
Whiteboard: [parity-IE], [parity-webkit] → [parity-IE], [parity-webkit], [mozfr-community]
You need to log in before you can comment on or make changes to this bug.