Last Comment Bug 774122 - limit CSS parser hashless-color and unitless-length quirks to only the properties that need them
: limit CSS parser hashless-color and unitless-length quirks to only the proper...
Status: RESOLVED FIXED
: dev-doc-needed
Product: Core
Classification: Components
Component: CSS Parsing and Computation (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla17
Assigned To: David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch)
:
Mentors:
Depends on: 875153 776591 844641 844649 1103000
Blocks: quirks-mode-spec
  Show dependency treegraph
 
Reported: 2012-07-15 14:37 PDT by David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch)
Modified: 2015-01-09 02:27 PST (History)
7 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
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)
2012-07-15 14:39 PDT, David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch)
bzbarsky: review+
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) (61.36 KB, patch)
2012-07-15 14:40 PDT, David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch)
bzbarsky: review+
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) (61.45 KB, patch)
2012-07-15 15:19 PDT, David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch)
no flags Details | Diff | Review
Disable the unitless length quirk inside of calc(). (, patch 3) (1.80 KB, patch)
2012-07-15 16:15 PDT, David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch)
bzbarsky: review+
Details | Diff | Review

Description David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2012-07-15 14:37:42 PDT
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 1 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2012-07-15 14:39:54 PDT
Created 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)
Comment 2 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2012-07-15 14:40:37 PDT
Created 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)
Comment 3 Boris Zbarsky [:bz] 2012-07-15 15:04:05 PDT
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.
Comment 4 Boris Zbarsky [:bz] 2012-07-15 15:08:14 PDT
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
Comment 5 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2012-07-15 15:19:21 PDT
Created 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)
Comment 6 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2012-07-15 15:21:33 PDT
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).
Comment 7 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2012-07-15 15:32:41 PDT
(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.
Comment 8 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2012-07-15 15:53:10 PDT
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");
Comment 9 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2012-07-15 15:58:30 PDT
(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 10 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2012-07-15 16:15:53 PDT
Created attachment 642444 [details] [diff] [review]
Disable the unitless length quirk inside of calc().  (, patch 3)
Comment 11 Boris Zbarsky [:bz] 2012-07-15 21:02:11 PDT
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.
Comment 12 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2012-07-15 21:09:13 PDT
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.)
Comment 13 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2012-07-15 21:13:08 PDT
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.
Comment 14 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2012-07-15 21:14:37 PDT
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.
Comment 16 :Gavin Sharp [email: gavin@gavinsharp.com] 2012-07-16 01:07:26 PDT
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)
Comment 17 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2012-07-16 06:02:17 PDT
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?
Comment 18 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2012-07-16 06:04:32 PDT
(Actually, one of them has x, y, and z in quirks mode.)

http://hg.mozilla.org/mozilla-central/file/4cffe2b37d0c/layout/reftests/transform-3d/translate3d-1-ref.html
http://hg.mozilla.org/mozilla-central/file/4cffe2b37d0c/layout/reftests/transform-3d/translatez-1-ref.html
Comment 19 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2012-07-16 06:08:07 PDT
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.
Comment 21 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2012-07-16 06:32:40 PDT
er, make that the last 2 of those plus
https://hg.mozilla.org/integration/mozilla-inbound/rev/3350e8b1618c
Comment 22 Ed Morley [:emorley] 2012-07-16 11:15:56 PDT
Didn't make it to mozilla-central before the uplift (merge was blocked on bug 774259). Adjusting milestone accordingly.
Comment 24 Simon Pieters 2012-08-16 01:57:04 PDT
(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?
Comment 25 David Baron :dbaron: ⌚️UTC-7 (review requests must explain patch) 2012-08-16 10:37:50 PDT
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.)
Comment 26 Simon Pieters 2012-08-22 03:52:29 PDT
These quirks are now defined at parser-level in css3-syntax.
Comment 27 Boris Zbarsky [:bz] 2013-01-05 20:31:44 PST
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
Comment 28 Marcus 2013-02-24 12:45:41 PST
@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.")
Comment 29 j.j. 2015-01-09 02:27:04 PST
For documentation: This is partly reverted, see bug 1103000

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