Bug 155874 - Align quirky number parsing with other browsers
Summary: Align quirky number parsing with other browsers
Alias: None
Product: WebKit
Classification: Unclassified
Component: CSS (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Ali Juma
Keywords: InRadar, WebExposed
Depends on:
Blocks: 155870
  Show dependency treegraph
Reported: 2016-03-24 22:55 PDT by Simon Fraser (smfr)
Modified: 2017-07-18 16:54 PDT (History)
9 users (show)

See Also:

Patch (4.65 KB, patch)
2017-07-18 16:20 PDT, Ali Juma
no flags Details | Formatted Diff | Diff
Patch (4.65 KB, patch)
2017-07-18 16:26 PDT, Ali Juma
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Simon Fraser (smfr) 2016-03-24 22:55:57 PDT
We currently parse unit-less, non-zero numbers as valid time values, which breaks things like animation shorthand parsing (bug 155870).

We should align with other browsers.
Comment 1 Radar WebKit Bug Importer 2016-03-24 22:56:37 PDT
Comment 2 Simon Fraser (smfr) 2016-03-24 22:58:48 PDT
Here's the current code:

inline bool CSSParser::shouldAcceptUnitLessValues(CSSParserValue& value, Units unitFlags, CSSParserMode cssParserMode)
    // Qirks mode and svg presentation attributes accept unit less values.
    return (unitFlags & (FLength | FAngle | FTime)) && (!value.fValue || cssParserMode == CSSQuirksMode || cssParserMode == SVGAttributeMode);
Comment 3 Simon Fraser (smfr) 2016-04-09 11:00:01 PDT
Comment 4 Ali Juma 2017-07-18 13:52:01 PDT
Currently, the only properties this affects are transition-duration, animation-duration, and -webkit-marquee-speed.

-webkit-marquee-speed is a bit of a special case though, since the marquee element's scrolldelay attribute (which is unit-less) gets converted to -webkit-marquee-speed using the CSS parser (in quirks mode, even when the document itself is not in quirks mode). It's also not supported by any other browser (Blink removed this property a while ago), so there's no concern about aligning with other browsers.

So I'd be inclined to fix transition-duration and animation-duration, and leave -webkit-marquee-speed alone.
Comment 5 Simon Fraser (smfr) 2017-07-18 14:21:34 PDT
I think it's fine to treat -webkit-marquee-speed as not web-exposed, and just fix them all if it's easier.
Comment 6 Ali Juma 2017-07-18 16:20:14 PDT
Created attachment 315853 [details]
Comment 7 Ali Juma 2017-07-18 16:23:44 PDT
(In reply to Simon Fraser (smfr) from comment #5)
> I think it's fine to treat -webkit-marquee-speed as not web-exposed, and
> just fix them all if it's easier.

Leaving -webkit-marquee-speed as-is turned out to be easier, since fixing that would have required making HTMLMarqueeElement::collectStyleForPresentationAttribute append a unit to the value of the scrolldelay attribute before it gets converted to a -webkit-marquee-speed value.
Comment 8 Ali Juma 2017-07-18 16:26:42 PDT
Created attachment 315855 [details]
Comment 9 WebKit Commit Bot 2017-07-18 16:54:48 PDT
Comment on attachment 315855 [details]

Clearing flags on attachment: 315855

Committed r219642: <http://trac.webkit.org/changeset/219642>
Comment 10 WebKit Commit Bot 2017-07-18 16:54:49 PDT
All reviewed patches have been landed.  Closing bug.