124193 - 'center right' and 'center left' 'background-position' doesn't parse within shorthand

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect)

Description

[Eric A. Meyer (dead account)]

You can see the problem in action on http://www.meyerweb.com/ by selecting the
presentation choice "natural" and allowing the cookie to be set.  (If you want
to delete it after testing, the name is 'meyerweb-style'.)  The site's
"masthead" doesn't take on the green background and butterfly image that it
should.  The problem seems to center around styling the same element multiple
times using different stylesheets.  I will attach a reduced testcase
demonstrating that this is (apparently) confined to background styles and not
others.

Comment 1

[Eric A. Meyer (dead account)]

Attachment: Reduced testcase (based on meyerweb.com)

Note that the font-family and color are set by the second stylesheet in the
source, just as they should be, but the background styles are ignored.	This
happens on meyerweb.com with LINKed external stylesheets, and here the
stylesheets are embedded in the HEAD element.  The behavior is the same either
way.

Comment 2

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

I looked at the code and I didn't see anything obviously wrong with
'background-color' except that we don't do 'background-color: inherit' properly
-- we treat it tas transparent.

Is this only a problem with 'background-color', or other things?  (I can't look
at the testcase right now...)

Comment 3

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

This is a parsing problem.  We're ignoring that 'background' line for some
reason.  If I take out the 'center right' it works.

Comment 4

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

I filed bug 124219 on the other issue I noticed.

Comment 5

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

The problem seems to be that 'center right' and 'center left' values for
'background-position' don't seem to parse within the shorthand.  'center top'
and 'center bottom' seem to work OK.  I didn't try very much else.

Comment 6

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

Attachment: simpler testcase

Comment 7

[Eric A. Meyer (dead account)]

Verifying that 'center top' and 'center bottom' are fine.  I also tried 'left
center' and 'right center' and they were okay.  It would seem that any time
'center' comes before the either one of the keywords 'right' or 'left', Mozilla
gets confused.

Comment 8

[Eric A. Meyer (dead account)]

Attachment: Stepping through background-position keyword combinations

Comment 9

[Eric A. Meyer (dead account)]

Attachment: Stepping through background-position keyword combinations

This testcase shows all of the various combinations for position keywords. 
Green backgrounds showing an image are okay.  Red backgrounds with no image
fail.

Comment 10

[Boris Zbarsky [:bzbarsky]]

We have eCSSProperty_background_x_position coming before
eCSSProperty_background_y_position in the kBackgroundIDs list in ParseBackground.

So when parsing this shorthand we try the x-position parser on "center" and it
works.  So we say that x-position is center.  Then we try to parse "right" and
parse y-position as "right".

Now we go back to ParseBackground and start validating the values.  x-position
is "really an X value", so we check the y value which is _not_ really a y-value,
and then we fail.

The validation logic needs to handle this case.

Comment 11

[Boris Zbarsky [:bzbarsky]]

Attachment: Proposed patch

Comment 12

[Pierre Saslawsky]

Comment on attachment 68377 [details] [diff] [review]
Proposed patch

r=pierre

Comment 13

[Marc Attinasi]

Comment on attachment 68377 [details] [diff] [review]
Proposed patch

sr=attinasi

Comment 14

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

a=roc+moz for 0.9.9

Comment 15

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

->bzbarsky, so he doesn't forget to land it :-)

Comment 16

[Eric A. Meyer (dead account)]

I assume the patch will also fix this problem in 'background-position' , 
which suffer the same problem?  The patch appears to do so but I wanted 
to make double-sure.

Comment 17

[Boris Zbarsky [:bzbarsky]]

Checked in on trunk (pre-branch).

Comment 18

[Boris Zbarsky [:bzbarsky]]

erm, actually I'm pretty sure this does not fix any problems there may be with
background-position.  Eric, could you file a bug on me on that?  (if you have a
testcase that would be great too).

Comment 19

[Boris Zbarsky [:bzbarsky]]

Just looked at the code for background-position.  It's correct.  I also modified
the testcase in this bug to use background-position and we passed.  So no new
bug needed that I can see.  :)