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

RESOLVED FIXED in mozilla17

Status

()

Core
CSS Parsing and Computation
RESOLVED FIXED
5 years ago
2 years ago

People

(Reporter: dbaron, Assigned: dbaron)

Tracking

(Depends on: 1 bug, Blocks: 1 bug, {dev-doc-needed})

Trunk
mozilla17
dev-doc-needed
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(3 attachments, 1 obsolete attachment)

(Assignee)

Description

5 years ago
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
(Assignee)

Comment 1

5 years ago
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)
Attachment #642431 - Flags: review?(bzbarsky)
(Assignee)

Comment 2

5 years ago
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)
Attachment #642432 - Flags: review?(bzbarsky)
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+
(Assignee)

Comment 5

5 years ago
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)
Attachment #642436 - Flags: review?(bzbarsky)
(Assignee)

Updated

5 years ago
Attachment #642432 - Attachment is obsolete: true
(Assignee)

Comment 6

5 years ago
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)
(Assignee)

Comment 7

5 years ago
(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.
(Assignee)

Comment 8

5 years ago
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");
(Assignee)

Comment 9

5 years ago
(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().
(Assignee)

Comment 10

5 years ago
Created attachment 642444 [details] [diff] [review]
Disable the unitless length quirk inside of calc().  (, patch 3)
Attachment #642444 - Flags: review?(bzbarsky)
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+
(Assignee)

Comment 12

5 years ago
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.)
(Assignee)

Comment 13

5 years ago
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.
(Assignee)

Comment 14

5 years ago
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.
(Assignee)

Comment 15

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/ac282e15dc02
https://hg.mozilla.org/integration/mozilla-inbound/rev/e3007422b989
https://hg.mozilla.org/integration/mozilla-inbound/rev/64ff8c2d37f9
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)
(Assignee)

Comment 17

5 years ago
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?
(Assignee)

Comment 18

5 years ago
(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
(Assignee)

Comment 19

5 years ago
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.
(Assignee)

Comment 20

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/9cdb8f9ec426
https://hg.mozilla.org/integration/mozilla-inbound/rev/b1ffd5406ad9
https://hg.mozilla.org/integration/mozilla-inbound/rev/473df589abf1
(Assignee)

Comment 21

5 years ago
er, make that the last 2 of those plus
https://hg.mozilla.org/integration/mozilla-inbound/rev/3350e8b1618c
Didn't make it to mozilla-central before the uplift (merge was blocked on bug 774259). Adjusting milestone accordingly.
Target Milestone: --- → mozilla17

Updated

5 years ago
Keywords: dev-doc-needed
https://hg.mozilla.org/mozilla-central/rev/b1ffd5406ad9
https://hg.mozilla.org/mozilla-central/rev/473df589abf1
https://hg.mozilla.org/mozilla-central/rev/3350e8b1618c
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Depends on: 776591

Comment 24

5 years ago
(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?
(Assignee)

Comment 25

5 years ago
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

5 years ago
These quirks are now defined at parser-level in css3-syntax.
(Assignee)

Updated

5 years ago
Blocks: 747485
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
(Assignee)

Updated

4 years ago
Depends on: 844641

Updated

4 years ago
Depends on: 844649

Comment 28

4 years ago
@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.")

Updated

4 years ago
Depends on: 875153
Depends on: 1103000

Updated

3 years ago

Comment 29

2 years ago
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.