258080 - Convert background-position to using nsCSSValuePair

Status: RESOLVED FIXED

(Core :: CSS Parsing and Computation, defect, P1)

Description

[Boris Zbarsky [:bzbarsky]]

See bug 208729 comment 1 and bug 208729 comment 2.

Comment 1

[Patrice Levesque]

*** Bug 316981 has been marked as a duplicate of this bug. ***

Comment 2

[Boris Zbarsky [:bzbarsky]]

Another issue: unlike other value pairs, the "right" serialization of two identical values is NOT just the first value for background-position...

That, and the two parts of the background position use different keyword tables.

So maybe we should wontfix this and fix the dependent bugs in other ways.

Comment 3

[David Baron :dbaron:]

Attachment: patch

This is designed to fix bug 373881 and bug 375363.  It's still compiling, so I haven't had a chance to test those yet, or go through any of the other dependencies here.  But I think this is the right approach.

The enum-extension approach in nsCSSParser (so I can still use ParseChoice) is pretty ugly, though.  It even produces some compiler warnings.  But given the extra hunk of code in ParseBackground to fix up background-position, we probably shouldn't even be using ParseChoice there...

Comment 4

[David Baron :dbaron:]

Attachment: patch

In this one, I rewrote ParseBackground so that it doesn't use ParseChoice anymore.  We could probably switch back after fixing bug 376079.

This passes the mochitests in layout/style/test/ (which means it fixes bug 375363) and passes the test in bug 373881.

Comment 5

[David Baron :dbaron:]

Comment on attachment 260219 [details] [diff] [review]
patch

>+        // XXXldb Should we set (for inherit) the 3 props that we
>+        // normally reset to initial?

I proposed in http://lists.w3.org/Archives/Public/www-style/2007Mar/0110 that we should, so I'll change the patch to do so (add 3 lines).

Comment 6

[David Baron :dbaron:]

Attachment: patch

May as well do that, and restore the '-moz-initial' parsing code for the shorthand that I mistakenly deleted yesterday thinking it would be more compatible with the current code.  (Even though it doesn't work; that's true for most of the properties right now.)

Comment 7

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 260237 [details] [diff] [review]
patch

>diff --git a/layout/style/nsCSSParser.cpp b/layout/style/nsCSSParser.cpp

> PRBool CSSParserImpl::ParseBackground(nsresult& aErrorCode)

>+    if (tt == eCSSToken_Ident) {

You need to reget the token in this clause, no?  And add a test for that?

Also add a test to verify that we don't parse something like:

  background: left url(whatever) top;

or

  background: top url(whatever) left;


and do parse all of

  background: left top url(whatever);
  background: top left url(whatever);
  background: url(whatever) left top;
  background: url(whatever) top left;

and they give the same results?

r+sr=bzbarsky with that.

Comment 8

[David Baron :dbaron:]

Once I get my property database checked in I should automatically pick up a test that would have caught the mistake with initial/inherit, since I should be able to test initial/inherit on all shorthands.

Comment 9

[David Baron :dbaron:]

My tests also showed I was incorrectly accepting "50%" for background-position.

Comment 10

[David Baron :dbaron:]

Er, never mind, I need to fix the tests.  I forgot about:

"If only one percentage or length value is given, it sets the horizontal position only, and the vertical position will be 50%."

in http://www.w3.org/TR/CSS21/colors.html#propdef-background-position

Comment 11

[David Baron :dbaron:]

Checked in to trunk earlier today, although the "do parse all of" tests aren't hooked up yet.

Comment 12

[David Baron :dbaron:]

Attachment: fix regression in serialization of keyword values

So the previous patch broke serialization of keyword values.  This fixes it by changing the way we store them so that we can use a single keyword table for both halves.

I'm hoping to get reasonably quick turnaround on this since this is the cause of nearly 50 of the failures in layout/style/tests/test_value_storage.html (and a rather hard-to-categorize-and-thus-mark-as-todo set of them, since it causes dropped values, extra spaces, etc.).

Comment 13

[David Baron :dbaron:]

Never mind about blocking the test -- I can describe the failure without too much trouble.

Comment 14

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 261775 [details] [diff] [review]
fix regression in serialization of keyword values

>+      default:
>+        NS_NOTREACHED("unexpected value");

Toss in:

          // fall through

to this and the other switch?

r+sr=bzbarsky with that.

Comment 15

[David Baron :dbaron:]

Regression fix checked in to trunk.