522607 - [css3-background] Accept background-position values like "bottom 10px right 10px"

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Zack Weinberg (:zwol)]

The latest css3-background revision augments <bg-position> with a notation for specifying fixed offsets from somewhere other than the upper right-hand corner of the box.  We currently don't support this.

This nominally blocks 513395 as the new spec for gradients assumes the extended <bg-position> notation, but the majority of that bug could be dealt with ahead of this one.

Comment 2

[Melissa Newman]

Test Examples:

background-position: right 20px top 60px;
background-position: right 20px bottom 60px;
background-position: left 20px top 60px;
background-position: left 20px bottom 60px;
background-position: right -50px top -50px;
background-position: left -50px bottom -50px;
background-position: right 20px top -50px;
background-position: right -20px top 50px;
background-position: right 3em bottom 10px;
background-position: left 15px;
background-position: 10px top;
background-position: left top 15px;
background-position: left 10px top;

background-position: left 20%;
background-position: right 20%;
background-position: top 20%;
background-position: bottom 20%;

Comment 4

[William Chen [:wchen]]

Attachment: background-position patch rev 1

Patch to implement CSS3 background-position values.

Comment 5

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 549318 [details] [diff] [review]
background-position patch rev 1

Sorry for not getting to this sooner.  (I know we'd discussed this patch in person; I was expecting something different after those discussions, but I suspect that's my fault for not being clearer.)

I don't think this patch is quite the approach that we want.  Since when authors examine and manipulate specified styles via the object model they should be manipulating roughly what was specified, we should be storing the values in roughly their specified form in the parser (perhaps a pair of pairs), and then converting them to calc() form in the computation code in nsRuleNode.  The conversion to calc() form shouldn't be happening in the parser.

Comment 6

[Marat Tanalin]

Supported in IE9+ and Opera 11+.

Comment 7

[William Chen [:wchen]]

I'v

Comment 8

[William Chen [:wchen]]

I've unassigned myself because I have been very busy lately and haven't had time to work on the bug.

Comment 9

[Lazar Sumar [:nonsensickle]]

I'll take it. Looks like a good first bug.

Comment 10

[Lazar Sumar [:nonsensickle]]

I've gotten some advice from roc about the CSS system and we intend to use a four element CSSValue::Array in the parser to store the four values in the edge offset edge offset order. Also a change would be made to the background style struct which would convert the right and bottom values into calc().

Comment 11

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to Lazar Sumar from comment #10)
> I've gotten some advice from roc about the CSS system and we intend to use a
> four element CSSValue::Array in the parser to store the four values in the
> edge offset edge offset order.

How do you plan to express the full value space in "edge offset edge offset" format?  Are there cases where you'll leave either the edge or the offset null to indicate that it wasn't specified?  While it's sometimes ok to have slight differences in what gets serialized (e.g., with element.style.getPropertyValue()), you should definitely avoid cases where something specified as CSS2 syntax gets serialized as syntax that's only valid in css3-background.

Sounds good otherwise.

Comment 12

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to David Baron [:dbaron] from comment #11)
> How do you plan to express the full value space in "edge offset edge offset"
> format?  Are there cases where you'll leave either the edge or the offset
> null to indicate that it wasn't specified?  While it's sometimes ok to have
> slight differences in what gets serialized (e.g., with
> element.style.getPropertyValue()), you should definitely avoid cases where
> something specified as CSS2 syntax gets serialized as syntax that's only
> valid in css3-background.

Can we always use the 4-element Array for storage and ensure nsCSSValue::AppendToString() always returns CSS 2.1 syntax for values that can be expressed in CSS 2.1?

Comment 13

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

You might need to write some custom code in AppendToString to do that.

Comment 14

[Robert O'Callahan (:roc) (email my personal email if necessary)]

We definitely would, yes.

I guess if we just leave some elements null then AppendToString will work as-is. Then ParseBoxPositionValues can ensure that the non-null values in the array match what the author provided. OK, let's do it that way.

Comment 15

[Lazar Sumar [:nonsensickle]]

The background position use in the -moz-linear-gradient and -moz-radial-gradient will need to be modified also. This might affect related bugs.

Comment 16

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

This new syntax should be supported in those other uses of background-position.  (I'm not sure if that's what you were saying, or whether you were saying that the code would need to be modified so they'd be unchanged.)

However, transform-origin is more complicated, since 3-D transforms has a third argument that makes it incompatible with this notation.  I'm not quite sure what the right thing is there -- it may need to be unmodified by this patch.

Comment 17

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to David Baron [:dbaron] from comment #16)
> This new syntax should be supported in those other uses of
> background-position.

Latest draft for linear-gradient doesn't spec full background-position syntax. It does for radial.
http://dev.w3.org/csswg/css3-images/#linear-gradients

> (I'm not sure if that's what you were saying, or
> whether you were saying that the code would need to be modified so they'd be
> unchanged.)

I think he's saying what you said :-).

Comment 18

[Lazar Sumar [:nonsensickle]]

Attachment: background-position patch 1

This only imports William's test cases. His other modifications were manually added to patch 2.

Comment 19

[Lazar Sumar [:nonsensickle]]

Attachment: background-position patch 2 (incomplete)

This is an incomplete draft of the patch. So far I believe to have fixed the background-position parsing and resolved the issues with the '-moz-linear-gradient' and '-moz-radial-gradient' dependency on that code in line with what I discussed with dbaron on irc (transcript is given below):

---- Transcript of conversation about gradient dependency on background position  ----
<lsumar> dbaron: i'm working on a patch for https://bugzilla.mozilla.org/show_bug.cgi?id=522607, and I've got a question about gradients...
 <dbaron> lsumar, sure
 <lsumar> dbaron: the ParseGradient fn in nsCSSParser has a reference to the ParseBoxPositionValues which I've renamed and changed to support the new background-position
 <lsumar> dbaron: is it worth keeping a copy of the old function in order to preserve the functionality or try and patch the dependent code as well?
 <dbaron> lsumar, radial gradients explicitly cite 'background-position', so you're actually supposed to update them
 <dbaron> lsumar, however, transform-origin might be more of a problem
 <lsumar> dbaron: yup, i agree with the radial gradient, at the moment i'm asking for linear-gradient
 <lsumar> dbaron: there is a comment about <legacy-gradient-line> which roc feels refers to the old way -moz-linear-gradient() used to 
 <lsumar> dbaron: *used to work
 <dbaron> lsumar, I think it's fine to change -moz-linear-gradient behavior too
 <dbaron> lsumar, we'll eventually remove the code for the old gradient syntax (before we drop the -moz- prefix)
 <lsumar> dbaron: after I resolve gradients I will move to transform-origin (which looked more involved)
 <lsumar> dbaron: right, so it's ok to modify to be correct with respect to current draft?
 <dbaron> well, the current linear gradient draft doesn't seem to have a background-position-related syntax anymore
 <dbaron> so before we unprefix we'll update it
 <dbaron> but it's fine to change for now, since it's purely additive and won't break anything
 <lsumar> ok. so in the context of my patch I will try to leave the function the way it is now with a small update to the assertions. is there a bug for the linear-gradient that I should update?
 <dbaron> lsumar, not that I know of
--------------------------------- End Transcript -----------------------------

Comment 20

[Lazar Sumar [:nonsensickle]]

dbaron, could you please give feedback on patch2 ?

Comment 21

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 581526 [details] [diff] [review]
background-position patch 2 (incomplete)

Here are comments on up through the end of the nsCSSParser changes; I'm still going through the rest, but I figure I may as well get these to you now:

So the change to CSSParserImpl::ParseGradient seems wrong.  You can't
just remove the aAllowExplicitCenter argument, because:

  1. For background-position, 'left', 'center left' and 'left center'
     all mean the same thing.  See:
     http://www.w3.org/TR/CSS21/colors.html#propdef-background-position
     http://dev.w3.org/csswg/css3-background/#the-background-position

  2. For the gradient 'to' syntax, 'to left' is legal, but 'center left'
     and 'left center' are not legal.  See:
     http://dev.w3.org/csswg/css3-images/#linear-gradients

  3. The background-position parsing code reports the same thing for
     'left' and 'center left' and 'left center' (x=left and y=center).

I think the best way around this problem is either:

 1. to make the gradient syntax 'to' parsing code stop sharing the
    background-position parsing code, since all it has to parse is
      [ left | right ] || [ top | bottom ]

 or

 2. to make the |aAllowExplicitCenter| argument more explicitly an
    |aParseGradientLineTo| argument, and thus also exclude the other new
    features you're adding (I haven't yet looked into how hard that is)

You should do whichever is easier.


In ParseBackgroundItem, unless I'm misunderstanding, the GetArrayValue
(in the first chunk of changes in that function) you have isn't going to
work -- you need to *set* an array value.


In ParseBackgroundPosition, you're breaking the inherit case since you
put the value in |value| but then assign |valueList|.  You should
maintain the invariant that |value| is the toplevel value of the
property, and instead of introducing |valueList|, introduce something
like |itemValue| to replace |valuePair|.


Now to the main part, ParseBoxPositionValues:

>+  // [keword offset keyword offset]. If a keword or offset isn't 
>+  // parsed the value of the corresponding array element is set 
>+  // to a 0 integer of type eCSSUnit_Null.

s/keword/keyword/

Also, why did you choose a 0 integer?  We may actually want to
distinguish between an explicit 0 offset and no offset specified.  I'd
prefer using eCSSUnit_Null for unspecified for both.

>+  if (aAcceptsInherit) {
>+    // ParseVariant is not greedy so it is ok to simply try and see...
>+    if (ParseVariant(xEdge, VARIANT_INHERIT, nsnull)) {
>+      if (eCSSUnit_Inherit == xEdge.GetUnit() || 
>+          eCSSUnit_Initial == xEdge.GetUnit()) {
>+        yEdge = xEdge;
>+        return PR_TRUE;
>+      }
>+    }
>+  }

When properties store their values in an nsCSSValue (rather than a value
pair), inherit and initial should be stored directly in the value rather
than as items within an array.

Also, you don't need to check the result of GetUnit() -- the fact that
ParseVariant succeeded when passed VARIANT_INHERIT is sufficient.

>+    if (ParseVariant(value->Item(i), VARIANT_LP | VARIANT_CALC, nsnull)) {
>+      valueCount++;
>+    } else if (ParseEnum(value->Item(i), nsCSSProps::kBackgroundPositionKTable)) {
>+      valueCount++;
>+    } else {
>+      break;
>+    }

You can combine the ParseVariant and ParseEnum calls, since ParseVariant
will call ParseEnum if you pass VARIANT_KEYWORD (and pass the keyword
table as the last parameter).  So this could just be:

    if (!ParseVariant(value->Item(i), VARIANT_LPCALC | VARIANT_KEYWORD,
                      nsCSSProps::kBackgroundPositionKTable)) {
      break;
    }
    ++valueCount;

(also note VARIANT_LPCALC condensation and preference for prefix ++)


Throughout the patch you should change PR_TRUE to true and PR_FALSE
to false.

>+          yOffset.SetIntValue(0, eCSSUnit_Null);

eCSSUnit_Null isn't a valid int unit, so this will assert.  I think
you should use eCSSUnit_Null, so this should be yOffset.Reset().

(Same thing repeats many more times.)

>+        if (eCSSUnit_Enumerated != value->Item(1).GetUnit() ||
>+          BG_CENTER == value->Item(1).GetIntValue()) {
>+          return PR_FALSE;

local style is to line up the parts of the if, so the indentation
should have the middle line indented 2 more spaces:

        if (eCSSUnit_Enumerated != value->Item(1).GetUnit() ||
            BG_CENTER == value->Item(1).GetIntValue()) {
          return false;

>+        // Remaining value must be a keyword.
>+        if (eCSSUnit_Enumerated == value->Item(0).GetUnit()) {
>+          xEdge = value->Item(0);
>+          xOffset.SetIntValue(0, eCSSUnit_Null);
>+        } else {
>+          return PR_FALSE;
>+        }

I think it's best to leave all the moving for below the "Move the
values" comment, and just make this:

if (eCSSUnit_Enumerated != value->Item(0).GetUnit()) {
  return false;
}

I'd note that the xEdge = value->Item(0) and the SetIntValue both
duplicate the code you have just below (where I think they make more
sense -- it's best to have all the moving code together so it's easier
to check that you're not stomping over values in the wrong order).

(Likewise I think you could drop the "in their correct position"
comments from the clause above and just have the yOffset.Reset()
and a single comment about everything else being in the right position
after the last "return false" in the 4-value section.)

>+  // Keywords in first and second pairs can not both be vertical or
>+  // horizontal keywords. (eg. left right, bottom top). Additionally,
>+  // non-center keyword can not be duplicated (eg. left left).
>+  PRInt32 xEdgeField = xEdge.GetIntValue();
>+  PRInt32 yEdgeField = yEdge.GetIntValue();
>+  if ((xEdgeField | yEdgeField) == (BG_LEFT | BG_RIGHT) ||
>+      (xEdgeField | yEdgeField) == (BG_TOP | BG_BOTTOM) ||
>+      (xEdgeField & (yEdgeField & ~BG_CENTER))) {
>+    return PR_FALSE;
>+  }

You need to be more careful here since GetIntValue() is only legal
when there's an int unit.  You should do:
  PRInt32 xEdgeField =
    xEdge.GetUnit() == eCSSUnit_Enumerated ? xEdge.GetIntValue() : 0;
etc.  Also, (xEdgeField & (yEdgeField & ~BG_CENTER)) could just be
written with fewer parentheses as (xEdgeField & yEdgeField & ~BG_CENTER).

>+  if (xEdge.GetIntValue() & (BG_TOP | BG_BOTTOM) ||
>+      yEdge.GetIntValue() & (BG_LEFT | BG_RIGHT)) {

Need to reuse xEdgeField here, for the same reason.

(Though I'd actually change the name from Field -> Enum.)

Comment 22

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 581526 [details] [diff] [review]
background-position patch 2 (incomplete)

Here are comments on up to the start of nsRuleNode.cpp.

I'm inclined to not review the rest in detail right now -- I think it would probably be more useful if you added some tests and got them to pass (including making sure you haven't made debug builds assert a lot).  But I'm certainly open to answering questions about the code in nsRuleNode.  The parts I've gone through in detail look like a good start, but it also looks like you haven't gotten a chance to test it yet and there's a good bit that will get shaken out when you do.

You need to add tests to layout/style/test/property_database.js for both
background-position and gradients, and also some reftests.




nsCSSValue.cpp:

>+    // Renameing a few background-position values

I don't think this comment adds anything, and it's a little confusing
("rename"), so just cut it.

>     if (gradient->mIsToCorner) {
>       aResult.AppendLiteral("to");
>-      NS_ABORT_IF_FALSE(gradient->mBgPos.mXValue.GetUnit() == eCSSUnit_Enumerated &&
>-                        gradient->mBgPos.mYValue.GetUnit() == eCSSUnit_Enumerated,
>+      NS_ABORT_IF_FALSE(xEdge.GetUnit() == eCSSUnit_Enumerated &&
>+                        yEdge.GetUnit() == eCSSUnit_Enumerated,
>                         "unexpected unit");

You should also assert that xValue and yValue are null.

(Maybe they should be called xOffset and yOffset like in the parser
code, too?)

>-        gradient->mBgPos.mXValue.AppendToString(eCSSProperty_background_position,
>+        xEdge.AppendToString(eCSSProperty_background_position,
>                                                 aResult);

Fix the indentation of the second line -- probably by wrapping back
to just 1 line.

(Twice)

>-    } else if (gradient->mBgPos.mXValue.GetUnit() != eCSSUnit_None ||
>-        gradient->mBgPos.mYValue.GetUnit() != eCSSUnit_None ||
>-        gradient->mAngle.GetUnit() != eCSSUnit_None) {
>-      if (gradient->mBgPos.mXValue.GetUnit() != eCSSUnit_None) {
>-        gradient->mBgPos.mXValue.AppendToString(eCSSProperty_background_position,
>+    } else if (xEdge.GetUnit() != eCSSUnit_None ||
>+               yEdge.GetUnit() != eCSSUnit_None ||
>+               xValue.GetUnit() != eCSSUnit_None ||
>+               yValue.GetUnit() != eCSSUnit_None ||
>+               gradient->mAngle.GetUnit() != eCSSUnit_None) {
>+      if (xEdge.GetUnit() != eCSSUnit_None && 
>+          xEdge.GetUnit() != eCSSUnit_Null) {
>+        xEdge.AppendToString(eCSSProperty_background_position,
>                                                 aResult);
>         aResult.AppendLiteral(" ");
>       }
>-      if (gradient->mBgPos.mXValue.GetUnit() != eCSSUnit_None) {
>-        gradient->mBgPos.mYValue.AppendToString(eCSSProperty_background_position,
>+      if (xValue.GetUnit() != eCSSUnit_None && 
>+          xValue.GetUnit() != eCSSUnit_Null) {
>+        xValue.AppendToString(eCSSProperty_background_position,
>+                                                aResult);
>+        aResult.AppendLiteral(" ");
>+      }
>+      if (yEdge.GetUnit() != eCSSUnit_None && 
>+          yEdge.GetUnit() != eCSSUnit_Null) {
>+        yEdge.AppendToString(eCSSProperty_background_position,
>+                                                aResult);
>+        aResult.AppendLiteral(" ");
>+      }
>+      if (yValue.GetUnit() != eCSSUnit_None && 
>+          yValue.GetUnit() != eCSSUnit_Null) {
>+        yValue.AppendToString(eCSSProperty_background_position,
>                                                 aResult);
>

I think the reason the old code is checking for eCSSUnit_None is that
the constructor of nsCSSValueGradient initialized both members of the
pair to eCSSUnit_None.  But now that it's not a pair that doesn't make
sense.  You should figure out the right thing here -- but I don't think
this is it.  In particular, if the old code let it stay eCSSUnit_None,
then that means the new code probably, in those cases, still leaves it
none, which means it's not an array and you can't access the element
values.  I think right now you can either have the toplevel be an
array or none, and the items in the array can either have values or be
null.  That doesn't seem like the right thing -- and it's not what
this code handles.

Also, same comments on indentation of the hanging aResult lines as
before -- probably best to combine with the previous line.


nsCSSValue.h:

>+  nsCSSValue mBgPos; // background-posision property

s/posision/position/

Comment 23

[Lazar Sumar [:nonsensickle]]

Attachment: background-position patch 2 (incomplete) rev 2

(In reply to David Baron [:dbaron] from comment #21)

> So the change to CSSParserImpl::ParseGradient seems wrong.  You can't
> just remove the aAllowExplicitCenter argument, because:

Your proposed solution 1. (below) fixes the problem in a more sensible way and 
removes the need for that argument. The "to" syntax is not sufficiently 
close to background-position to warrant having that code in ParseBackgroundPosition. 

> I think the best way around this problem is either:
> 
>  1. to make the gradient syntax 'to' parsing code stop sharing the
>     background-position parsing code
>  ...

I believe 1 is easier and more sensible. I would also like to apply the same solution 
to the ParseMozTransformOrigin function.The ParseBackgroundPositionValues function 
now operates on an Array which is not a suitable data structure for the others to use. 
Trying to make it parse for the others will only make the code messy without making 
it better.

> In ParseBackgroundItem, unless I'm misunderstanding, the GetArrayValue
> (in the first chunk of changes in that function) you have isn't going to
> work -- you need to *set* an array value.

You are correct. It is done.

> ... You should maintain the invariant that |value| is the toplevel value of the
> property, and instead of introducing |valueList|, introduce something
> like |itemValue| to replace |valuePair| ...

Done.

> Also, why did you choose a 0 integer?  We may actually want to
> distinguish between an explicit 0 offset and no offset specified.  I'd
> prefer using eCSSUnit_Null for unspecified for both.

There might be some misunderstanding. I intended to invalidate the value to make 
the distinction clear. I noticed that using eCSSUnit_Null seemed to defacto 
invalidate values and thought that setting an integer with type eCSSUnit_Null 
would do it. This was the wrong approach as pointed out later in the comments and 
has been corrected. Now Reset() is used.

> When properties store their values in an nsCSSValue (rather than a value
> pair), inherit and initial should be stored directly in the value rather
> than as items within an array.

Noted and done.

> ... Also, you don't need to check the result of GetUnit() -- the fact that
> ParseVariant succeeded when passed VARIANT_INHERIT is sufficient ....

Done.

> You can combine the ParseVariant and ParseEnum calls, ...
> ...
> (also note VARIANT_LPCALC condensation and preference for prefix ++)

Noted and done.

> Throughout the patch you should change PR_TRUE to true and PR_FALSE
> to false.

Noted and done.

> >+          yOffset.SetIntValue(0, eCSSUnit_Null);
> 
> eCSSUnit_Null isn't a valid int unit, so this will assert.  I think
> you should use eCSSUnit_Null, so this should be yOffset.Reset().
> 
> (Same thing repeats many more times.)

That was negligent of me. Fixed for all occurrences. This also clears up the 
misunderstanding about the "0 integer" comment above.

> >+        if (eCSSUnit_Enumerated != value->Item(1).GetUnit() ||
> >+          BG_CENTER == value->Item(1).GetIntValue()) {
> >+          return PR_FALSE;
> 
> local style is to line up the parts of the if, so the indentation
> should have the middle line indented 2 more spaces:

Whoops, fixed.

> >+        // Remaining value must be a keyword.
> >+        if (eCSSUnit_Enumerated == value->Item(0).GetUnit()) {
> >+          xEdge = value->Item(0);
> >+          xOffset.SetIntValue(0, eCSSUnit_Null);
> >+        } else {
> >+          return PR_FALSE;
> >+        }
> 
> I think it's best to leave all the moving for below the "Move the
> values" comment, and just make this:
> 
> if (eCSSUnit_Enumerated != value->Item(0).GetUnit()) {
>   return false;
> }

Done.

> I'd note that the xEdge = value->Item(0) and the SetIntValue both
> duplicate the code you have just below (where I think they make more
> sense -- it's best to have all the moving code together so it's easier
> to check that you're not stomping over values in the wrong order).

I agree. xEdge = value->Item(0) shouldn't be there anyway. Fixed.

> (Likewise I think you could drop the "in their correct position"
> comments from the clause above and just have the yOffset.Reset()
> and a single comment about everything else being in the right position
> after the last "return false" in the 4-value section.)

Done.

> >+  // Keywords in first and second pairs can not both be vertical or
> >+  // horizontal keywords. (eg. left right, bottom top). Additionally,
> >+  // non-center keyword can not be duplicated (eg. left left).
> >+  PRInt32 xEdgeField = xEdge.GetIntValue();
> >+  PRInt32 yEdgeField = yEdge.GetIntValue();
> >+  if ((xEdgeField | yEdgeField) == (BG_LEFT | BG_RIGHT) ||
> >+      (xEdgeField | yEdgeField) == (BG_TOP | BG_BOTTOM) ||
> >+      (xEdgeField & (yEdgeField & ~BG_CENTER))) {
> >+    return PR_FALSE;
> >+  }
> 
> You need to be more careful here since GetIntValue() is only legal
> when there's an int unit.  You should do:
>   PRInt32 xEdgeField =
>     xEdge.GetUnit() == eCSSUnit_Enumerated ? xEdge.GetIntValue() : 0;
> etc.  Also, (xEdgeField & (yEdgeField & ~BG_CENTER)) could just be
> written with fewer parentheses as (xEdgeField & yEdgeField & ~BG_CENTER).
> 
> >+  if (xEdge.GetIntValue() & (BG_TOP | BG_BOTTOM) ||
> >+      yEdge.GetIntValue() & (BG_LEFT | BG_RIGHT)) {
> 
> Need to reuse xEdgeField here, for the same reason.
> 
> (Though I'd actually change the name from Field -> Enum.)

Again, that was negligent of me. Done.

Comment 24

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 581859 [details] [diff] [review]
background-position patch 2 (incomplete) rev 2

not sure if you saw comment 22 (since it came just before comment 23) -- but poking this flag in case you didn't (sorry, meant to do that earlier)

Comment 25

[Lazar Sumar [:nonsensickle]]

(In reply to David Baron [:dbaron] from comment #24)
> Comment on attachment 581859 [details] [diff] [review]
> background-position patch 2 (incomplete) rev 2
> 
> not sure if you saw comment 22 (since it came just before comment 23) -- but
> poking this flag in case you didn't (sorry, meant to do that earlier)

Yes, I've read it. I will get some reftests going after I complete the changes to ParseMozTransformOrigin.

Comment 26

[Lazar Sumar [:nonsensickle]]

Attachment: background-position patch 2 (incomplete) rev 3

Fails css-gradients reftests and transform reftests. Passes background reftests (with patch 1 applied). Work in progress.

Comment 27

[Lazar Sumar [:nonsensickle]]

Has the radial gradient changed since https://bugzilla.mozilla.org/show_bug.cgi?id=513395 had implemented it? 
There appear to be some differences with respect to the way the syntax is parsed in the code and the specification at http://dev.w3.org/csswg/css3-images/#radial-gradients 

In particular the example 19 "radial-gradient(ellipse at center, yellow 0%, green 100%);" does not render on the current trunk.

Similarly there is a difference between the transform and 3d-transform specification for the transform-origin property. 
http://dev.w3.org/csswg/css3-transforms/#transform-origin-property
http://dev.w3.org/csswg/css3-3d-transforms/#transform-origin-property

The perspective-origin property is parsed by the same function and follows the 3d-transform definition of the transform-origin property closely

http://dev.w3.org/csswg/css3-3d-transforms/#perspective-origin-property

At the same time the 2d transform-origin property is very similar to the background-position even though this is not explicitly stated like with the radial-gradient.

I could be reading the syntax wrong but the 2d and 3d specifications appear to differ, am I correct?

Comment 28

[Robert O'Callahan (:roc) (email my personal email if necessary)]

(In reply to Lazar Sumar [:lsumar] from comment #27)
> Has the radial gradient changed since
> https://bugzilla.mozilla.org/show_bug.cgi?id=513395 had implemented it? 

Yes.

> In particular the example 19 "radial-gradient(ellipse at center, yellow 0%,
> green 100%);" does not render on the current trunk.

Yeah we didn't implement that syntax originally, because it didn't exist when we implemented radial gradients.

> I could be reading the syntax wrong but the 2d and 3d specifications appear
> to differ, am I correct?

Yes. I'm not sure what to do. Neither is the CSS Working Group :-(.
https://www.w3.org/Bugs/Public/show_bug.cgi?id=14647
http://lists.w3.org/Archives/Public/www-style/2011Oct/0893.html

You might want to investigate what syntax Webkit accepts. Whatever we do probably needs to be compatible with that.

Comment 29

[Lazar Sumar [:nonsensickle]]

Attachment: background-position patch 2 rev 4

Patch completed. Passes local background, gradient, transform and transform-3d reftests.

linear-gradient
radial-gradient
transform-origin
perspective-origin

properties have been reverted back to old functionality.

Comment 30

[Lazar Sumar [:nonsensickle]]

Comment on attachment 583326 [details] [diff] [review]
background-position patch 2 rev 4

I still don't seem to have access to the try servers. As soon as I get it I will push this patch but in the meantime could you please review? It passes the local reftests listed in comment 29.

Comment 31

[Lazar Sumar [:nonsensickle]]

The passed reftests (comments 29 & 30) include those that were created by William in his patch (which were later moved to my patch 1).

Comment 32

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #28)
> (In reply to Lazar Sumar [:lsumar] from comment #27)
> > Has the radial gradient changed since
> > https://bugzilla.mozilla.org/show_bug.cgi?id=513395 had implemented it? 
> 
> Yes.
> 
> > In particular the example 19 "radial-gradient(ellipse at center, yellow 0%,
> > green 100%);" does not render on the current trunk.
> 
> Yeah we didn't implement that syntax originally, because it didn't exist
> when we implemented radial gradients.

Bug 627885 implements some of the new syntax.

Comment 33

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Try server push at: https://tbpl.mozilla.org/?tree=Try&rev=cadcd43729ac

(Sorry, not sure why I didn't do that last night.)

Comment 34

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

mochitest-4 (in layout/style/) are failing with:
###!!! ABORT: Shouldn't have mPercent!: 'pos.mYPosition.mPercent == 0.0f', file /builds/slave/try-osx64-dbg/build/layout/style/nsComputedDOMStyle.cpp, line 1679

crashtest and reftest are failing with:
###!!! ABORT: not a pairlist value: 'mUnit == eCSSUnit_PairListDep', file /builds/slave/try-osx64-dbg/build/layout/style/nsCSSValue.h, line 1001

Once you fix those you should also make sure that the layout/style mochitests pass locally (e.g., with "TEST_PATH=layout/style make mochitest-plain" in the root of your objdir, and then push to try again.

Comment 35

[Lazar Sumar [:nonsensickle]]

The mochitest-4 is failing because of the way I am using the mPercent value which was introduced in Boris' fix for Bug 647885.

One fix would be to just set the mPercent to be true when ever I assign a percentage but I'm concerned that this might reopen bug 647885.

I'm still looking into the eCSSUnit_PairListDep assertion failure.

Comment 36

[Lazar Sumar [:nonsensickle]]

Attachment: background-position patch 2 rev 5

The try server link is https://tbpl.mozilla.org/?tree=Try&rev=155335180109

mHasPosition variable in the ComputeBackgroundPositionCoord was implemented properly.

Declaration.cpp file was updated to use List instead of PairList.

Mochitest4 seems to be failing on the try server. 

Suspected cause: nsStyleAnimation::UncomputeValue function (nsStyleAnimation.cpp liine 2227) appears to be generating nsCSSValues from already computed Values and calling ComputeBackgroundData with a dependent pair list for background-position...

Comment 37

[Lazar Sumar [:nonsensickle]]

Attachment: 583722: background-position patch 2 rev 6 (incomplete)

This patch fails the local mochitest "layout/style/test/test_transitions_per_property.html" with 8 errors:

failed | property background-position: computed value before transition - got 20px 30px, expected 10px 40px, 50px 50px, 30px 20px

failed | property background-position: interpolation of lists of lengths - got 20px 30px, expected 20px 35px, 55px 50px, 30px 25px

failed | property background-position: computed value before transition - got 20px 30px, expected 10px 40%, 50% 50px, 30% 20%, 5px 10px

failed | property background-position: interpolation of lists of lengths and percents - got 20px 30px, expected 20px 35%, 55% 50px, 30% 25%, 10px 20px

failed | property background-position: computed value before transition - got 20px 30px, expected 20% 40%, 8px 12px

failed | property background-position: interpolation that computes to calc() - got 20px 30px, expected -moz-calc(3px + 15%) -moz-calc(5px + 30%), -moz-calc(6px + 10%) -moz-calc(9px + 4%)

failed | property background-position: computed value before transition - got 20px 30px, expected -moz-calc(40px + 20%) -moz-calc(40px + 40%), 8px 12%, -moz-calc(20px + 12%) -moz-calc(24px + 8%)

failed | property background-position: interpolation that computes to calc() - got 20px 30px, expected -moz-calc(33px + 15%) -moz-calc(30px + 35%), -moz-calc(6px + 5%) -moz-calc(4px + 24%), -moz-calc(17px + 14%) -moz-calc(28px + 10%)

There's probably a silly mistake somewhere and I hope to have it resolved soon.

Changes from last patch:
The nsStyleAnimation has been updated with the change to background-position (though not correctly as it seems).

Comment 38

[Lazar Sumar [:nonsensickle]]

Attachment: 587460: 583722: background-position patch 2 rev 7

Comment 39

[Lazar Sumar [:nonsensickle]]

Pushed to try.

Try server link https://tbpl.mozilla.org/?tree=Try&rev=3cb6ab5d9c33
Log for the try server http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-3cb6ab5d9c33

Comment 40

[Lazar Sumar [:nonsensickle]]

Comment on attachment 587860 [details] [diff] [review]
587460: 583722: background-position patch 2 rev 7

David, please review if it passes the tests on the try server.

Comment 41

[Lazar Sumar [:nonsensickle]]

Attachment: background-position patch 2 rev 7

Added user and date to the patch headers... No changes.

Try server tests have all passed (after restarting a few).

Try server link https://tbpl.mozilla.org/?tree=Try&rev=3cb6ab5d9c33

Waiting for David's review.

Comment 42

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 588281 [details] [diff] [review]
background-position patch 2 rev 7

>+  
>+  /* Lazar Sumar <[e-mail]>, 5th Jan 2012, bug 522607 
>+   * ParseBoxPositionValues function is deprecated and should not be used. 
>+   * Use ParseBackgroundPositionValues instead. 
>+   */

Don't put your name or the date in comments.  People can use "hg annotate"
and "hg log" to figure out where code came from when they need to.

That said, you should also point to the comment in the .cpp file
explaining why it's deprecated.

> #define BG_CTB    (BG_CENTER | BG_TOP | BG_BOTTOM)
>+#define BG_TB     (BG_TOP | BG_BOTTOM)
> #define BG_CLR    (BG_CENTER | BG_LEFT | BG_RIGHT)
>+#define BG_LR    (BG_LEFT | BG_RIGHT)

You're short one space of indentation on the last of these lines.

>         havePosition = true;
>-        nsCSSValuePair scratch;
>-        if (!ParseBoxPositionValues(scratch, false)) {
>+        nsCSSValue scratch;
>+        if (!ParseBackgroundPositionValues(scratch, false)) {
>           return false;
>         }
>-        aState.mPosition->mXValue = scratch.mXValue;
>-        aState.mPosition->mYValue = scratch.mYValue;
>+        aState.mPosition->mValue = scratch;

You don't need the |scratch| variable anymore.  (It was there because
nsCSSValuePairList doesn't actually contain an nsCSSValuePair; instead
it had 2 nsCSSValues, which was probably a mistake.)

Just pass aState.mPosition->mValue to ParseBackgroundPositionValues.

>-      nsCSSValuePair scratch;
>-      if (!ParseBoxPositionValues(scratch, false)) {
>+      nsCSSValue scratch;
>+      if (!ParseBackgroundPositionValues(scratch, false)) {
>         return false;
>       }
>-      aState.mPosition->mXValue = scratch.mXValue;
>-      aState.mPosition->mYValue = scratch.mYValue;
>+      aState.mPosition->mValue = scratch;

Same here.


In ParseBackgroundPosition, declare |itemValue| where |valuePair| used
to be declared, rather than at the start of the function.

Leave the BoxPositionMaskToCSSValue and ParseBoxPositionValues code
where it was located in the file rather than moving it (which breaks
annotations of blame).

>+bool CSSParserImpl::ParseBackgroundPositionValues(nsCSSValue& aOut,
>+                                           bool aAcceptsInherit)

Line up the "bool aAcceptsInherit" with "nsCSSValue& aOut".

>+  // CSS3 specification allows positions to be defined as offsets 

"CSS3 specification" -> "css3-background"

>+  // from an edge. There can be 2 kewords and 2 offsets given. These  

"kewords" -> "keywords"

>+  // four 'values' are stored in an array in the following order:
>+  // [keword offset keyword offset]. If a keword or offset isn't 

"keword" -> "keyword" (twice)

>+  // parsed the value of the corresponding array element is set 
>+  // to a type eCSSUnit_Null by a call to nsCSSValue::Reset().

"a type " -> ""

Also, don't have trailing spaces at the ends of all the wrapped lines.

>+  nsCSSValue &xEdge  = value->Item(0),
>+             &xOffset = value->Item(1),
>+             &yEdge  = value->Item(2),
>+             &yOffset = value->Item(3);

Either line up the = signs or put one space before each = sign.  Don't
average the results you'd get between those 2 options.

>+      // "If three or four values are given, then each <percentage> or<length>

space in "or<length>"

>+      } else if (eCSSUnit_Enumerated != value->Item(2).GetUnit()) { // keyword|offset keyword offset

This is actually "keyword keyword offset".  The code is correct, but the
comment is wrong.  Also, you shouldn't run past the 80th column.

>+        value->Item(2) = value->Item(1); // yEdge
>+        value->Item(1).Reset(); // xOffset
>+     } else {
>+        return false;
>+     }
>+     break;

At the end of case 3, the "} else {" line, the "}" line, and the
"break;" line are indented one space too few.  (Or did tabs sneak
in?)

You should restructure the if/else structure inside case 2: by using nesting, in particular, make it look like:
  if (0 has enumerated unit) {
    if (1 has enumerated unit) {
      // current clause 1
    } else {
      // current clause 2
    }
  } else {
    if (1 has enumerated unit) {
      // current clause 3
    } else {
      // current clause 4
    }
  }

>+/*****************************************************************************
>+ * The BoxPositionMaskToCSSValue and ParseBoxPositionValues functions 
>+ * are depricated. They are only present due to reliance of code
>+ * for linear-gradient, radial-gradient, perspective-origin and 
>+ * transform-origin properties on the functions. When this reliance is 
>+ * removed, these functions should also be removed.
>+ * 
>+ * Notes:
>+ * Radial and linear gradient syntax has changed and needs to be updated.
>+ * 
>+ * There is ambiguity between definitions on pages 
>+ * http://dev.w3.org/csswg/css3-transforms/#transform-origin-property
>+ * http://dev.w3.org/csswg/css3-3d-transforms/#transform-origin-property
>+ * http://dev.w3.org/csswg/css3-3d-transforms/#perspective-origin-property
>+ * The implementation of the -moz-transform-origin and -moz-perspective-origin 
>+ * properties will remain unchanged and rely on ParseBoxPositionValues
>+ * function specified below.
>+ * Lazar Sumar <[e-mail]>, 5th Jan 2012, bug 522607
>+ ***************** start of deprecated section ******************************/

Two nits, for a start:
  depricated -> deprecated
  remove the trailing spaces at the end of lines.

However, I think this comment is too big a warning.  I think all that's
needed would be something more like:
  // BoxPositionMaskToCSSValue and ParseBoxPositionValues are used
  // for parsing the CSS 2.1 background-position syntax (which has at
  // most two values).  (Compare to the css3-background syntax which
  // takes up to four values.)  Some current CSS specifications that
  // use background-position-like syntax still use this old syntax.
I don't think a comment that gradient syntax needs to be updated belongs
*here* -- whoever updates gradient syntax is unlikely to see that code
and remove it.  Comments on updating both transform-origin, perspective,
and gradient arguments probably belong in bugs on doing those things.

nsCSSValue.h:

  Don't edit this file just to add a single blank line.

nsRuleNode.cpp:

  Local style has the "{" that begins a function on its own line.

  Also, "static void" should be on its own line (so that the name
  of the function is at the start of a line and easy to search for).

  In ComputeBackgroundPositionCoord, call your |edge| and |offset|
  parameters |aEdge| and |aOffset|, per local style.  Call
  |outPositionCoord| |aResult|.

  In ComputeBackgroundPositionCoord, this line:
  >+  outPositionCoord->mHasPercent = false;
  could move into the |else| below it, since the then part sets
  it to true.

  >+    negateOffset = edge.GetIntValue() &           // length is subtracted from
  >+                  (NS_STYLE_BG_POSITION_BOTTOM | // percentage for bottom
  >+                   NS_STYLE_BG_POSITION_RIGHT);  // or right edges.

  You should indent the second and third lines here.

  I also think ComputeBackgroundPositionCoord is unnecessarily
  confusing.  I think you should:
   * initialize outPositionCoord->mPercent and mLength in the
     edge-null case
   * eliminate the edgePercent variable (just use
     outPositionCoord->mPercent)
   * change negateOffset to a variable that's either 1 or -1 so
     you can multiply by it

  You should also make the two tails of the if-else chains match
  (one uses NS_ASSERTION (probably preferable); the other uses a
  condition and NS_NOTREACHED).

  There's no need for the function ComputeBackgroundPositionCalcCoords
  -- just inline its contents at its single caller.  Then we don't need
  to worry about the awkward name.  Also fix the indentation.

nsStyleAnimation.cpp:

  >+#if DEBUG
  >+#include "nsPrintfCString.h"
  >+#endif

  Don't #ifdef includes -- just include unconditionally.  Doing
  otherwise is a recipe for somebody else breaking the build later.

  >+static bool
  >+BackgroundPositionToCalcValuePair(const nsCSSValue& aBackgroundPosition,
  >+                                  CalcValue* aXCalc, CalcValue* aYCalc) {

  Opening { on its own line.

  >+  NS_ASSERTION(aBackgroundPosition.GetUnit() == eCSSUnit_Array, 
  >+                                                       "Expected array unit");

  Weird indentation or tabs; the first " should line up with the first a.

  >+        /* It is exptected that only 'uncomputed' background position values
  >+         * are contained in the two lists. These allways have non-null values 
  >+         * for Item(1) and Item(3) and null values for Item(0) and Item(2).
  >+         * Lazar Sumar [e-mail], 12th Jan 2012 */

  You should check that with NS_ASSERTIONs.  Also, don't put your
  name in the comment.  And instead of "uncomputed" in quotes,
  maybe write it out as "background-position values from
  ExtractComputedValue" (or did you actually mean UncomputeValue?).

  In AddCSSValuePPC, call it PixelPercentCalc rather than PPC (PowerPC).
  Also, put the opening brace on its own line.

  And at its callers, don't put a space before the !.

  There's some weird indentation (probably tabs) in
  ExtractComputedValue.

  Doesn't ExtractComputedValue actually need
  to deal with the keywords (your XXX comment)?  You should be able
  to convert them to calc(), but you need to do so.  And you should
  add tests for that case to
  layout/style/test/test_transitions_per_property.html


You should add some tests to property_database.js that values like 'left
left' and 'top 20px bottom 20px' are rejected (put them in
invalid_values).

I'd like to look at ComputeBackgroundPositionCoord and the nsStyleAnamition
changes again once you revise the patch (so marking as review-), but
otherwise this looks good.  Sorry for taking so long to finish this review.

Comment 43

[Lazar Sumar [:nonsensickle]]

Comment on attachment 588281 [details] [diff] [review]
background-position patch 2 rev 7

> ... once you revise the patch (so marking as review-)

Marking as dbaron actually intended.

Comment 44

[Lazar Sumar [:nonsensickle]]

Attachment: background-position patch 2 rev 8

(In reply to David Baron [:dbaron] (busy through 12 Feb) from comment #42)
> Comment on attachment 588281 [details] [diff] [review]
> background-position patch 2 rev 7
 
> Don't put your name or the date in comments.  People can use "hg annotate"
> and "hg log" to figure out where code came from when they need to.

All names removed.

> That said, you should also point to the comment in the .cpp file
> explaining why it's deprecated.

Done.

> > #define BG_CTB    (BG_CENTER | BG_TOP | BG_BOTTOM)
> >+#define BG_TB     (BG_TOP | BG_BOTTOM)
> > #define BG_CLR    (BG_CENTER | BG_LEFT | BG_RIGHT)
> >+#define BG_LR    (BG_LEFT | BG_RIGHT)
> 
> You're short one space of indentation on the last of these lines.

Done.

> >         havePosition = true;
> >-        nsCSSValuePair scratch;
> >-        if (!ParseBoxPositionValues(scratch, false)) {
> >+        nsCSSValue scratch;
> >+        if (!ParseBackgroundPositionValues(scratch, false)) {
> >           return false;
> >         }
> >-        aState.mPosition->mXValue = scratch.mXValue;
> >-        aState.mPosition->mYValue = scratch.mYValue;
> >+        aState.mPosition->mValue = scratch;
> 
> You don't need the |scratch| variable anymore.  (It was there because
> nsCSSValuePairList doesn't actually contain an nsCSSValuePair; instead
> it had 2 nsCSSValues, which was probably a mistake.)

I mustn't have been paying attention. Done.

> Just pass aState.mPosition->mValue to ParseBackgroundPositionValues.
> 
> >-      nsCSSValuePair scratch;
> >-      if (!ParseBoxPositionValues(scratch, false)) {
> >+      nsCSSValue scratch;
> >+      if (!ParseBackgroundPositionValues(scratch, false)) {
> >         return false;
> >       }
> >-      aState.mPosition->mXValue = scratch.mXValue;
> >-      aState.mPosition->mYValue = scratch.mYValue;
> >+      aState.mPosition->mValue = scratch;
> 
> Same here.

Done.
 
> In ParseBackgroundPosition, declare |itemValue| where |valuePair| used
> to be declared, rather than at the start of the function.

Unnecessary construction removed.

> Leave the BoxPositionMaskToCSSValue and ParseBoxPositionValues code
> where it was located in the file rather than moving it (which breaks
> annotations of blame).

Done.

> >+bool CSSParserImpl::ParseBackgroundPositionValues(nsCSSValue& aOut,
> >+                                           bool aAcceptsInherit)
> 
> Line up the "bool aAcceptsInherit" with "nsCSSValue& aOut".

Done.

> >+  // CSS3 specification allows positions to be defined as offsets 
> 
> "CSS3 specification" -> "css3-background"
> 
> >+  // from an edge. There can be 2 kewords and 2 offsets given. These  
> 
> "kewords" -> "keywords"
> 
> >+  // four 'values' are stored in an array in the following order:
> >+  // [keword offset keyword offset]. If a keword or offset isn't 
> 
> "keword" -> "keyword" (twice)
> 
> >+  // parsed the value of the corresponding array element is set 
> >+  // to a type eCSSUnit_Null by a call to nsCSSValue::Reset().
> 
> "a type " -> ""
> 
> Also, don't have trailing spaces at the ends of all the wrapped lines.

Done.

> >+  nsCSSValue &xEdge  = value->Item(0),
> >+             &xOffset = value->Item(1),
> >+             &yEdge  = value->Item(2),
> >+             &yOffset = value->Item(3);
> 
> Either line up the = signs or put one space before each = sign.  Don't
> average the results you'd get between those 2 options.

Corrected.

> >+      // "If three or four values are given, then each <percentage> or<length>
> 
> space in "or<length>"

Done.

> >+      } else if (eCSSUnit_Enumerated != value->Item(2).GetUnit()) { // keyword|offset keyword offset
> 
> This is actually "keyword keyword offset".  The code is correct, but the
> comment is wrong.  Also, you shouldn't run past the 80th column.

At that point it could be either, the "offset keyword offset" is rejected lower. Nonetheless, corrected.

> >+        value->Item(2) = value->Item(1); // yEdge
> >+        value->Item(1).Reset(); // xOffset
> >+     } else {
> >+        return false;
> >+     }
> >+     break;
> 
> At the end of case 3, the "} else {" line, the "}" line, and the
> "break;" line are indented one space too few.  (Or did tabs sneak
> in?)

Indentation corrected.

> You should restructure the if/else structure inside case 2: by using
> nesting, in particular, make it look like:
>   if (0 has enumerated unit) {
>     if (1 has enumerated unit) {
>       // current clause 1
>     } else {
>       // current clause 2
>     }
>   } else {
>     if (1 has enumerated unit) {
>       // current clause 3
>     } else {
>       // current clause 4
>     }
>   }

Done.


>+/*****************************************************************************
> >+ * The BoxPositionMaskToCSSValue and ParseBoxPositionValues functions 
> >+ * are depricated. They are only present due to reliance of code
> >+ * for linear-gradient, radial-gradient, perspective-origin and 
> >+ * transform-origin properties on the functions. When this reliance is 
> >+ * removed, these functions should also be removed.
> >+ * 
> >+ * Notes:
> >+ * Radial and linear gradient syntax has changed and needs to be updated.
> >+ * 
> >+ * There is ambiguity between definitions on pages 
> >+ * http://dev.w3.org/csswg/css3-transforms/#transform-origin-property
> >+ * http://dev.w3.org/csswg/css3-3d-transforms/#transform-origin-property
> >+ * http://dev.w3.org/csswg/css3-3d-transforms/#perspective-origin-property
> >+ * The implementation of the -moz-transform-origin and -moz-perspective-origin 
> >+ * properties will remain unchanged and rely on ParseBoxPositionValues
> >+ * function specified below.
> >+ * Lazar Sumar <[e-mail]>, 5th Jan 2012, bug 522607
> >+ ***************** start of deprecated section ******************************/
> 
> Two nits, for a start:
>   depricated -> deprecated
>   remove the trailing spaces at the end of lines.

Typo. Corrected.

> However, I think this comment is too big a warning.  I think all that's
> needed would be something more like:
>   // BoxPositionMaskToCSSValue and ParseBoxPositionValues are used
>   // for parsing the CSS 2.1 background-position syntax (which has at
>   // most two values).  (Compare to the css3-background syntax which
>   // takes up to four values.)  Some current CSS specifications that
>   // use background-position-like syntax still use this old syntax.
> I don't think a comment that gradient syntax needs to be updated belongs
> *here* -- whoever updates gradient syntax is unlikely to see that code
> and remove it.  Comments on updating both transform-origin, perspective,
> and gradient arguments probably belong in bugs on doing those things.

Agreed. I've copied your comment word for word into the "rev 8" patch.

> nsCSSValue.h:
> 
>   Don't edit this file just to add a single blank line.

Removed.

> nsRuleNode.cpp:
> 
>   Local style has the "{" that begins a function on its own line.

Corrected.

>   Also, "static void" should be on its own line (so that the name
>   of the function is at the start of a line and easy to search for).

This too was corrected.

>   In ComputeBackgroundPositionCoord, call your |edge| and |offset|
>   parameters |aEdge| and |aOffset|, per local style.  Call
>   |outPositionCoord| |aResult|.

Renamed.

>   In ComputeBackgroundPositionCoord, this line:
>   >+  outPositionCoord->mHasPercent = false;
>   could move into the |else| below it, since the then part sets
>   it to true.

This logic was reworked according to advice from comments below.

>   >+    negateOffset = edge.GetIntValue() &           // length is
> subtracted from
>   >+                  (NS_STYLE_BG_POSITION_BOTTOM | // percentage for bottom
>   >+                   NS_STYLE_BG_POSITION_RIGHT);  // or right edges.
> 
>   You should indent the second and third lines here.

done.

>   I also think ComputeBackgroundPositionCoord is unnecessarily
>   confusing.  I think you should:
>    * initialize outPositionCoord->mPercent and mLength in the
>      edge-null case
>    * eliminate the edgePercent variable (just use
>      outPositionCoord->mPercent)
>    * change negateOffset to a variable that's either 1 or -1 so
>      you can multiply by it

I've restructured it to make it simpler.

>   You should also make the two tails of the if-else chains match
>   (one uses NS_ASSERTION (probably preferable); the other uses a
>   condition and NS_NOTREACHED).

Both use NS_ASSERTION in "rev 8".

>   There's no need for the function ComputeBackgroundPositionCalcCoords
>   -- just inline its contents at its single caller.  Then we don't need
>   to worry about the awkward name.  Also fix the indentation.

Inlined. Indented.

> nsStyleAnimation.cpp:
> 
>   >+#if DEBUG
>   >+#include "nsPrintfCString.h"
>   >+#endif
> 
>   Don't #ifdef includes -- just include unconditionally.  Doing
>   otherwise is a recipe for somebody else breaking the build later.

Done.

>   >+static bool
>   >+BackgroundPositionToCalcValuePair(const nsCSSValue& aBackgroundPosition,
>   >+                                  CalcValue* aXCalc, CalcValue* aYCalc) {
> 
>   Opening { on its own line.

Fixed.

>   >+  NS_ASSERTION(aBackgroundPosition.GetUnit() == eCSSUnit_Array, 
>   >+                                                       "Expected array
> unit");
> 
>   Weird indentation or tabs; the first " should line up with the first a.

Weird indentation, not tabs, fixed.

>   >+        /* It is exptected that only 'uncomputed' background position
> values
>   >+         * are contained in the two lists. These allways have non-null
> values 
>   >+         * for Item(1) and Item(3) and null values for Item(0) and
> Item(2).
>   >+         * Lazar Sumar [e-mail], 12th Jan 2012 */
> 
>   You should check that with NS_ASSERTIONs.  Also, don't put your
>   name in the comment.  And instead of "uncomputed" in quotes,
>   maybe write it out as "background-position values from
>   ExtractComputedValue" (or did you actually mean UncomputeValue?).

The comment was moved into the called function. NS_ASSERTION added.

>   In AddCSSValuePPC, call it PixelPercentCalc rather than PPC (PowerPC).
>   Also, put the opening brace on its own line.

Name and brace corrected.

>   And at its callers, don't put a space before the !.

Removed.

>   There's some weird indentation (probably tabs) in
>   ExtractComputedValue.

Tabs removed.

>   Doesn't ExtractComputedValue actually need
>   to deal with the keywords (your XXX comment)?  You should be able
>   to convert them to calc(), but you need to do so.  And you should
>   add tests for that case to
>   layout/style/test/test_transitions_per_property.html

No. My comment regards the fact that the specified value cannot be completely and correctly determined from the computed value. This comment adds no new information to the code and was removed.

> I'd like to look at ComputeBackgroundPositionCoord and the nsStyleAnamition
> changes again once you revise the patch (so marking as review-), but
> otherwise this looks good.  Sorry for taking so long to finish this review.

Revised patch is attached.

Comment 45

[Lazar Sumar [:nonsensickle]]

Attachment: background-position patch 1 rev 2

> You should add some tests to property_database.js that values like 'left
> left' and 'top 20px bottom 20px' are rejected (put them in
> invalid_values).

There were numerous test like this added in William's patch. I've now included the suggested ones in the list.

Comment 46

[Lazar Sumar [:nonsensickle]]

Try server link for patch 2 rev 8 and patch 1 rev 2 queue

https://tbpl.mozilla.org/?tree=Try&rev=4c0775deef05

Comment 47

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 595151 [details] [diff] [review]
background-position patch 1 rev 2

># HG changeset patch
># User Lazar Sumar  <[e-mail]>

Since this is primarily William's patch, it should be attributed to him.

># Date 1328646756 -46800
># Node ID 761299026c506d2ef17cfb34c90ce44b5186502f
># Parent e0d9c8ddd5bd31cfc5481d47945ffe6e62009704
>css3 background-position patch.

The commit message should say something more like:

Tests for new css3-background background-position syntax.  (Bug 522607)  r=dbaron



r=dbaron, though it might be good to also have a few more reftests for the various 3-value combinations.

Comment 48

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Comment on attachment 595150 [details] [diff] [review]
background-position patch 2 rev 8

>+  
>+  /* ParseBoxPositionValues function is deprecated and should not be used. 
>+   * Use ParseBackgroundPositionValues instead. See comment at definition for 
>+   * details.
>+   */
>   bool ParseBoxPositionValues(nsCSSValuePair& aOut, bool aAcceptsInherit,

How about just
  // Parses the CSS 2.1 background-position syntax, which is still used
  // by some properties.  See ParseBackgroundPositionValues for the
  // css3-background syntax.

There are still a bunch of trailing spaces at the end of the
comment in CSSParserImpl::ParseBackgroundPositionValues.

nsRuleNode.cpp:

>+    nsRefPtr<nsCSSValue::Array> bgPositionArray =
>+                                     aSpecifiedValue->mValue.GetArrayValue();

funny indentation -- just make "aSpecifiedValue" begin 2 chars in
from the line above it.

nsStyleAnimation.cpp:

>+   * ExtractComputedValue are contained in the two lists. These allways 

allways -> always

(Also, another point where you're introducing spaces at the end of lines.
You should check for that throughout the patch.)


Also, in nsStyleAnimation, instead of having
BackgroundPositionToCalcValuePair return a result that's only
checked in an NS_ABORT_IF_FALSE, just put the NS_ABORT_IF_FALSE
conditions inside the function at the places you currently have it
return false, and make the function return void.

That said, I don't think you really even need
BackgroundPositionToCalcValuePair -- why not just make the code that
calls it (in ComputeDistance) do a loop over indices 1 and 3 of the
value just like you do in AddWeighted.


r=dbaron with that fixed

Sorry for the delay getting to this

Comment 49

[Lazar Sumar [:nonsensickle]]

Attachment: background-position patch 1 rev 3

(In reply to David Baron [:dbaron] from comment #47)
> Comment on attachment 595151 [details] [diff] [review]
> background-position patch 1 rev 2
> 
> ># HG changeset patch
> ># User Lazar Sumar  <[e-mail]>
> 
> Since this is primarily William's patch, it should be attributed to him.

Apologies. My mercurial setup automatically appends my name on every qref. Fixed.

> ># Date 1328646756 -46800
> ># Node ID 761299026c506d2ef17cfb34c90ce44b5186502f
> ># Parent e0d9c8ddd5bd31cfc5481d47945ffe6e62009704
> >css3 background-position patch.
> 
> The commit message should say something more like:
> 
> Tests for new css3-background background-position syntax.  (Bug 522607) 
> r=dbaron

Done.

> r=dbaron, though it might be good to also have a few more reftests for the
> various 3-value combinations.

Added tests 7 and 8.

Comment 50

[Lazar Sumar [:nonsensickle]]

Attachment: background-position patch 2 rev 9

(In reply to David Baron [:dbaron] from comment #48)
> Comment on attachment 595150 [details] [diff] [review]
> background-position patch 2 rev 8
> >+  /* ParseBoxPositionValues function is deprecated and should not be used. 
> >+   * Use ParseBackgroundPositionValues instead. See comment at definition for 
> >+   * details.
> >+   */
> >   bool ParseBoxPositionValues(nsCSSValuePair& aOut, bool aAcceptsInherit,
> 
> How about just
>   // Parses the CSS 2.1 background-position syntax, which is still used
>   // by some properties.  See ParseBackgroundPositionValues for the
>   // css3-background syntax.

Done.

> There are still a bunch of trailing spaces at the end of the
> comment in CSSParserImpl::ParseBackgroundPositionValues.

I've removed all trailing spaces in all the files I've modified.

> nsRuleNode.cpp:
> 
> >+    nsRefPtr<nsCSSValue::Array> bgPositionArray =
> >+                                     aSpecifiedValue->mValue.GetArrayValue();
> 
> funny indentation -- just make "aSpecifiedValue" begin 2 chars in
> from the line above it.

Done.

> nsStyleAnimation.cpp:
> 
> >+   * ExtractComputedValue are contained in the two lists. These allways 
> 
> allways -> always

Fixed.

> (Also, another point where you're introducing spaces at the end of lines.
> You should check for that throughout the patch.)

Done.

> Also, in nsStyleAnimation, instead of having
> BackgroundPositionToCalcValuePair ...
> ... That said, I don't think you really even need
> BackgroundPositionToCalcValuePair -- why not just make the code that
> calls it (in ComputeDistance) do a loop over indices 1 and 3 of the
> value just like you do in AddWeighted.

Inlined.

> r=dbaron with that fixed

Done.

Comment 51

[Lazar Sumar [:nonsensickle]]

Latest tryserver push https://tbpl.mozilla.org/?tree=Try&rev=7cd9456d39ff

Comment 52

[Lazar Sumar [:nonsensickle]]

Attachment: background-position patch 2 rev 9

Comment 53

[Lazar Sumar [:nonsensickle]]

Latest tryserver https://tbpl.mozilla.org/?tree=Try&rev=83bee6f099d0

Comment 54

[Lazar Sumar [:nonsensickle]]

The Mac OS X opt build failure is already in the tree and is unrelated.

Parent tryserver link with failure https://tbpl.mozilla.org/?rev=24f2c7e26fbd

Ready to land.

Comment 55

[Lazar Sumar [:nonsensickle]]

Attachment: background-position patch 2 rev 10

commit message updated

Comment 56

[Lazar Sumar [:nonsensickle]]

Attachment: background-position patch 1 rev 4

commit message updated

Comment 57

[Lazar Sumar [:nonsensickle]]

Patch 1 must land at the same time as patch 2.

Comment 58

[Timothy Nikkel (:tnikkel)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f25d65f080f8
https://hg.mozilla.org/integration/mozilla-inbound/rev/1e6b5a9bbdcc

Comment 59

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/f25d65f080f8
https://hg.mozilla.org/mozilla-central/rev/1e6b5a9bbdcc

Comment 60

[Eric Shepherd [:sheppy]]

https://developer.mozilla.org/en/CSS/background has been updated, but background-position has not, yet.

Comment 61

[Jean-Yves Perrier [:teoli]]

Doc updated:

I created : https://developer-new.mozilla.org/en-US/docs/CSS/position_value
and
https://developer-new.mozilla.org/en-US/docs/CSS/background-position points now to it
(as well as the WIP https://developer-new.mozilla.org/en-US/docs/CSS/radial-gradient )

Firefox 13 for developers was already up-to-date.