Last Comment Bug 124193 - 'center right' and 'center left' 'background-position' doesn't parse within shorthand
: 'center right' and 'center left' 'background-position' doesn't parse within s...
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: CSS Parsing and Computation (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla0.9.9
Assigned To: Boris Zbarsky [:bz]
: Hixie (not reading bugmail)
Mentors:
http://www.meyerweb.com/
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2002-02-07 10:20 PST by Eric A. Meyer (dead account)
Modified: 2002-02-21 18:03 PST (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Reduced testcase (based on meyerweb.com) (525 bytes, text/html)
2002-02-07 10:21 PST, Eric A. Meyer (dead account)
no flags Details
simpler testcase (362 bytes, text/html)
2002-02-07 12:04 PST, David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch)
no flags Details
Stepping through background-position keyword combinations (1.80 KB, text/html)
2002-02-07 12:42 PST, Eric A. Meyer (dead account)
no flags Details
Stepping through background-position keyword combinations (1.80 KB, text/html)
2002-02-07 12:43 PST, Eric A. Meyer (dead account)
no flags Details
Proposed patch (1.58 KB, patch)
2002-02-07 13:13 PST, Boris Zbarsky [:bz]
pierre: review+
attinasi: superreview+
Details | Diff | Review

Description Eric A. Meyer (dead account) 2002-02-07 10:20:06 PST
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) 2002-02-07 10:21:47 PST
Created attachment 68339 [details]
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-7 (review requests must explain patch) 2002-02-07 11:44:18 PST
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-7 (review requests must explain patch) 2002-02-07 11:50:55 PST
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-7 (review requests must explain patch) 2002-02-07 11:52:24 PST
I filed bug 124219 on the other issue I noticed.
Comment 5 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2002-02-07 12:02:47 PST
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-7 (review requests must explain patch) 2002-02-07 12:04:14 PST
Created attachment 68359 [details]
simpler testcase
Comment 7 Eric A. Meyer (dead account) 2002-02-07 12:34:40 PST
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) 2002-02-07 12:42:56 PST
Created attachment 68369 [details]
Stepping through background-position keyword combinations
Comment 9 Eric A. Meyer (dead account) 2002-02-07 12:43:53 PST
Created attachment 68371 [details]
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 [:bz] 2002-02-07 12:49:11 PST
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 [:bz] 2002-02-07 13:13:12 PST
Created attachment 68377 [details] [diff] [review]
Proposed patch
Comment 12 Pierre Saslawsky 2002-02-19 17:35:45 PST
Comment on attachment 68377 [details] [diff] [review]
Proposed patch

r=pierre
Comment 13 Marc Attinasi 2002-02-20 11:03:53 PST
Comment on attachment 68377 [details] [diff] [review]
Proposed patch

sr=attinasi
Comment 14 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2002-02-21 09:07:07 PST
a=roc+moz for 0.9.9
Comment 15 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2002-02-21 13:07:08 PST
->bzbarsky, so he doesn't forget to land it :-)
Comment 16 Eric A. Meyer (dead account) 2002-02-21 15:23:11 PST
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 [:bz] 2002-02-21 16:32:19 PST
Checked in on trunk (pre-branch).
Comment 18 Boris Zbarsky [:bz] 2002-02-21 16:44:52 PST
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 [:bz] 2002-02-21 18:03:13 PST
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.  :)

Note You need to log in before you can comment on or make changes to this bug.