Closed Bug 258080 Opened 16 years ago Closed 13 years ago

Convert background-position to using nsCSSValuePair

Categories

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

defect

Tracking

()

RESOLVED FIXED
mozilla1.8alpha4

People

(Reporter: bzbarsky, Assigned: dbaron)

References

(Blocks 3 open bugs)

Details

(Whiteboard: [patch])

Attachments

(2 files, 2 obsolete files)

Priority: -- → P1
Target Milestone: --- → mozilla1.8alpha4
Blocks: 253222
Blocks: 299960
*** Bug 316981 has been marked as a duplicate of this bug. ***
Blocks: 364432
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.
Blocks: 373881
Attached patch patch (obsolete) — Splinter Review
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...
Assignee: bzbarsky → dbaron
Status: NEW → ASSIGNED
Attached patch patch (obsolete) — Splinter Review
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.
Attachment #260213 - Attachment is obsolete: true
Attachment #260219 - Flags: superreview?(bzbarsky)
Attachment #260219 - Flags: review?(bzbarsky)
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).
Attached patch patchSplinter Review
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.)
Attachment #260219 - Attachment is obsolete: true
Attachment #260237 - Flags: superreview?(bzbarsky)
Attachment #260237 - Flags: review?(bzbarsky)
Attachment #260219 - Flags: superreview?(bzbarsky)
Attachment #260219 - Flags: review?(bzbarsky)
Whiteboard: [patch]
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.
Attachment #260237 - Flags: superreview?(bzbarsky)
Attachment #260237 - Flags: superreview+
Attachment #260237 - Flags: review?(bzbarsky)
Attachment #260237 - Flags: review+
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.
My tests also showed I was incorrectly accepting "50%" for background-position.
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
Checked in to trunk earlier today, although the "do parse all of" tests aren't hooked up yet.
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Flags: in-testsuite+
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.).
Attachment #261775 - Flags: superreview?(bzbarsky)
Attachment #261775 - Flags: review?(bzbarsky)
Never mind about blocking the test -- I can describe the failure without too much trouble.
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.
Attachment #261775 - Flags: superreview?(bzbarsky)
Attachment #261775 - Flags: superreview+
Attachment #261775 - Flags: review?(bzbarsky)
Attachment #261775 - Flags: review+
Regression fix checked in to trunk.
You need to log in before you can comment on or make changes to this bug.