Closed Bug 774122 Opened 12 years ago Closed 12 years ago

limit CSS parser hashless-color and unitless-length quirks to only the properties that need them

Categories

(Core :: CSS Parsing and Computation, defect)

Product:
Core
Component:
CSS Parsing and Computation
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla17

People

(Reporter: dbaron, Assigned: dbaron)

References

(Depends on 1 open bug, Blocks 1 open bug)

Details

(Keywords: dev-doc-needed)

Attachments

(3 files, 1 obsolete file)

Limit the hashless color quirk to the properties where it's needed, per http://simon.html5.org/specs/quirks-mode#the-hashless-hex-color-quirk . (, patch 1)
38.01 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review
Limit the unitless length quirk to the properties where it's needed, per http://simon.html5.org/specs/quirks-mode#the-unitless-length-quirk . (, patch 2)
61.45 KB, patch
Details | Diff | Splinter Review
Disable the unitless length quirk inside of calc(). (, patch 3)
1.80 KB, patch
bzbarsky
: review+
Details | Diff | Splinter Review 
The CSS parser has two quirks, for unitless lengths and for hashless colors, that we currently apply when the property parser doesn't note that it's a shorthand.  That is, we apply them except when properties opt out.

We should instead flip this around so that we only apply them to properties that opt in, and we should use the list of properties that need these quirks from http://dvcs.w3.org/hg/quirks-mode/raw-file/tip/Overview.html#css
Comment on attachment 642431 [details] [diff] [review]
Limit the hashless color quirk to the properties where it's needed, per http://simon.html5.org/specs/quirks-mode#the-hashless-hex-color-quirk .  (, patch 1)

Does the first assertion in ParseVariant need updating?

The "NOTE" about the next free bit in nsCSSProps.h needs to be updated.

r=me with those two nits.
Attachment #642431 - Flags: review?(bzbarsky) → review+
Comment on attachment 642432 [details] [diff] [review]
Limit the unitless length quirk to the properties where it's needed, per http://simon.html5.org/specs/quirks-mode#the-unitless-length-quirk .  (, patch 2)

Same comments here as for the other patch.

r=me
Attachment #642432 - Flags: review?(bzbarsky) → review+
Comment on attachment 642436 [details] [diff] [review]
Limit the unitless length quirk to the properties where it's needed, per http://simon.html5.org/specs/quirks-mode#the-unitless-length-quirk .  (, patch 2)

Er, never mind, not worth asking for re-review for the tiny tweak (to the column-rule-color values in property_database.js).
Attachment #642436 - Flags: review?(bzbarsky)
(In reply to Boris Zbarsky (:bz) from comment #3)
> Does the first assertion in ParseVariant need updating?

My current inclination is to replace the single assertion with:
  NS_ASSERTION(!(mHashlessColorQuirk && (aVariantMask & VARIANT_COLOR) ||
               !(aVariantMask & VARIANT_NUMBER),
               "can't distinguish colors from numbers");
  NS_ASSERTION(!(mHashlessColorQuirk && (aVariantMask & VARIANT_COLOR) ||
               !(mUnitlessLengthQuirk && (aVariantMask & VARIANT_LENGTH)),
               "can't distinguish colors from lengths");
  NS_ASSERTION(!(mUnitlessLengthQuirk && (aVariantMask & VARIANT_LENGTH)),
               !(aVariantMask & VARIANT_NUMBER),
               "can't distinguish lengths from numbers");

> The "NOTE" about the next free bit in nsCSSProps.h needs to be updated.

It should just be removed; it was there because the previous bit was used by a 2-bit construct.
Actually, compilers are rather more happy with:
  NS_ASSERTION(!(mHashlessColorQuirk && (aVariantMask & VARIANT_COLOR)) ||
               !(aVariantMask & VARIANT_NUMBER),
               "can't distinguish colors from numbers");
  NS_ASSERTION(!(mHashlessColorQuirk && (aVariantMask & VARIANT_COLOR)) ||
               !(mUnitlessLengthQuirk && (aVariantMask & VARIANT_LENGTH)),
               "can't distinguish colors from lengths");
  NS_ASSERTION(!(mUnitlessLengthQuirk && (aVariantMask & VARIANT_LENGTH)) ||
               !(aVariantMask & VARIANT_NUMBER),
               "can't distinguish lengths from numbers");
(In reply to David Baron [:dbaron] from comment #8)
> Actually, compilers are rather more happy with:
>   NS_ASSERTION(!(mHashlessColorQuirk && (aVariantMask & VARIANT_COLOR)) ||
>                !(aVariantMask & VARIANT_NUMBER),
>                "can't distinguish colors from numbers");
>   NS_ASSERTION(!(mHashlessColorQuirk && (aVariantMask & VARIANT_COLOR)) ||
>                !(mUnitlessLengthQuirk && (aVariantMask & VARIANT_LENGTH)),
>                "can't distinguish colors from lengths");
>   NS_ASSERTION(!(mUnitlessLengthQuirk && (aVariantMask & VARIANT_LENGTH)) ||
>                !(aVariantMask & VARIANT_NUMBER),
>                "can't distinguish lengths from numbers");

The third of which actually catches an interesting bug in quirks mode:  I think I need to temporarily disable these inside of calc().
Comment on attachment 642444 [details] [diff] [review]
Disable the unitless length quirk inside of calc().  (, patch 3)

r=me, but the spec should say so too.
Attachment #642444 - Flags: review?(bzbarsky) → review+
I honestly have no idea why the spec for these quirks is written the way it is; it has no relationship to the way we actually implement them and seems highly unlikely to match.  (I thought it used to be different, though.)
Though, actually, I think does happen to be what the spec says, since the spec applies a function nesting level test.  I'm still skeptical that we won't find other cases where the spec doesn't match, though.
In particular, the spec describes the quirk in terms of token-preprocessing when it's actually implemented at the parser level rather than the tokenizer.  While the difference isn't detectable now, it will be once we implement CSS variables.
I backed these patches out of inbound because they caused reftest bustage:

REFTEST TEST-UNEXPECTED-FAIL | file:///c:/talos-slave/test/build/reftest/tests/layout/reftests/transform-3d/translatez-1a.html | image comparison (==)
REFTEST TEST-UNEXPECTED-FAIL | file:///c:/talos-slave/test/build/reftest/tests/layout/reftests/transform-3d/translate3d-1a.html | image comparison (==) 

https://hg.mozilla.org/integration/mozilla-inbound/rev/33ebe0407469
https://hg.mozilla.org/integration/mozilla-inbound/rev/588ea3ebb1ef
https://hg.mozilla.org/integration/mozilla-inbound/rev/e85863a0d897

(I also backed out your no-bug changeset, 0e6039a2c90a)
So the problem here is that there are two reftests depending on -moz-transform-origin's z component accepting unitless lengths in quirks mode.  Is it supposed to accept unitless lengths?
Anyway, I don't see any sign in the spec that these transform-origin is supposed to accept lengths, so I'm just going to fix the tests.
Didn't make it to mozilla-central before the uplift (merge was blocked on bug 774259). Adjusting milestone accordingly.
Target Milestone: --- → mozilla17
Keywords: dev-doc-needed
Depends on: 776591
(In reply to David Baron [:dbaron] from comment #14)
> In particular, the spec describes the quirk in terms of token-preprocessing
> when it's actually implemented at the parser level rather than the
> tokenizer.  While the difference isn't detectable now, it will be once we
> implement CSS variables.

So with CSS variables, per spec the quirk shouldn't apply, right? Isn't that a good thing? Would it be hard to implement assuming you keep it at parser level?
I don't think it's worth switching to a much more complicated implementation of the quirk in order to make it not apply to CSS variables used in these properties.  (And I expect the implementation is similar in other browsers.)
These quirks are now defined at parser-level in css3-syntax.
Note that at least for text-indent and background-position this might have broken some sites.  See http://stackoverflow.com/questions/14173821/firefox-17-css-issue-text-indent-ignored
Depends on: 844649
@Boris: That's right, this "unquirking the quirks" has broken some sites.

Even more, I suggest that Firefox from now on implements the quirks again, or it doesn't show anything anymore without a "px".

This will be very interesting to watch, because the Browser user will realise how many sites rely on those "quirks", and change the browser or not.

However, you should be aware that since changing above "quirks", many user stopped the use of Firefox. It's clearly visible in the statistics: Firefox is strongly loosing user since the introduction of 17.0.1.

Well, I use to say: "Never change a running machine". Otherwise the machine won't run anymore.

More comments on this issue: https://bugzilla.mozilla.org/show_bug.cgi?id=844641 ("Browser must support more than the standard. Otherwise user change the browser.")
Depends on: 875153
Depends on: 1103000
For documentation: This is partly reverted, see bug 1103000
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: