Closed Bug 567283 Opened 14 years ago Closed 8 years ago

Add support for 4 and 8 CSS hexcolor values (#RRGGBBAA and #RGBA)

Categories

(Core :: CSS Parsing and Computation, enhancement)

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

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla49
Tracking Flags:
Tracking Status
firefox49 --- fixed

People

(Reporter: dev, Assigned: dbaron)

References

Details

(Keywords: dev-doc-complete, site-compat, Whiteboard: [parity-webkit])

Attachments

(5 files)

Possible changes required to allow #RRGGBBAA and #RGBA in gecko
1.52 KB, text/plain
Details
MozReview Request: Bug 567283 patch 1 - Convert if enclosing most of function into early return. r?xidorn
58 bytes, text/x-review-board-request
xidorn
: review+
Details
MozReview Request: Bug 567283 patch 2 - Rename NS_HexToRGB to NS_HexToRGBA and add parameter saying whether 4 and 8 digit colors are allowed. r?xidorn
58 bytes, text/x-review-board-request
xidorn
: review+
Details
MozReview Request: Bug 567283 patch 3 - Make TestColorNames use the new alpha capabilities. r?xidorn
58 bytes, text/x-review-board-request
xidorn
: review+
Details
MozReview Request: Bug 567283 patch 4 - Support #rgba and #rrggbbaa colors in CSS. r?xidorn
58 bytes, text/x-review-board-request
xidorn
: review+
Details 
User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.2.5pre) Gecko/20100514 Ubuntu/9.10 (karmic) Firefox/3.6.3pre
Build Identifier: 

This feature is proposed for CSS3 Color, worst case will be in CSS4 Color.
It has been also reported in Webkit:
https://bugs.webkit.org/show_bug.cgi?id=39140
And filed accordingly in:
http://www.w3.org/Style/CSS/Tracker/issues/124
(related email discussion can be found in the previous link)

Reproducible: Always

Actual Results:  
unsupported


Some of the software using #rrggbbaa annotation:
   Google Charts API
   Inkscape
   Java Synth Look and Feel
   3DMLW (3D Markup Language)
   GML (Graph Modeling Language)
   iCal for Mac (in theme files)
   Maemo (Nokia)
   swfmill
   gbui for JME (Java Game engine), based in CSS
   wxPython
   Qt UI framework
   lcd4linux
   xine (OSD config)
   xmCHART for FileMaker Pro
   gwyddion
   skEdit (themes)
   Pixane
It is not exactly a patch, as my skills in C++ are limited, but I believe those are the required changes. I suggest to review it by perform the required modifications.
This has the same issues as the webkit patch; it changes the way HTML color attributes are handled in a way that doesn't obiously seem compatible to me...
The conservative thing would be to define a new NS_HexToRGBA() that allowed the four/eight digit syntax, and use that for #xxxxx colors in CSS but leave the old parsing for HTML color attributes.  We discourage use of HTML color attributes anyway, so if they don't get the new feature, no great loss.

On the other hand, if we *can* allow the new notation in HTML color attributes without compatibility issues, it seems pointless not to (and then we wouldn't need two parsers).

ccing hixie for HTML5 perspective.
As I just commented in the webkit bug, if this syntax is supported then:

  color="000000ff"

goes from blue in quirks mode and ignored in standards mode to black.

  color="0000"

goes from black in quirks mode and ignored in standards mode to transparent.

I really don't think we can start supporting this syntax in HTML attributes without at least some breakage.

Doing it just in CSS might be easier, since we don't do NS_LooseHexToRGB there.
<html>
<head>
</head>
<body>
1: <font color="#005F">This is blue</font><br>
2: <font color="#000055FF">This is blue</font><br>
3: <font style="color: #005F">This is blue</font><br>
4: <font style="color: #000055FF">This is blue</font><br>
</body>
</html>

Firefox (quirk mode):
 (1) green
 (2) blue
 (3,4) black.

Firefox (standard mode):
 (all) black
> (3,4) black.
> (all) black

Not black.  Whatever the color would have been without the attempt to change it.

But yes, that's what comment 4 said.
Status: UNCONFIRMED → NEW
Ever confirmed: true
Agreed that the parsing of HTML colors may run into compat concerns.

I've done some compat testing on CSS colors, though, using the 450k pages in the DotBot index.  Only about 150 pages showed up as using 4/8 hexit colors in color or *-color properties, from about 100 unique sites.  Most of the uses would cause minor breakage when they start being valid, though.
I also agree that this should only be added into CSS colors and not in HTML properties. As Zack commented, people should not be using the HTML color attribute anyway.
We should just do whatever the specs say. Not sure what that is for CSS (is there a proposal to support this in CSS3? If so, and if it's not yet in CR, then the thing to do is to support it as -moz-#... rather than just #.... In HTML, the attributes are to be parsed as described here:

   http://www.whatwg.org/specs/web-apps/current-work/complete/common-microsyntaxes.html#rules-for-parsing-a-legacy-color-value

I wouldn't recommend changing that.
This has been implemented in WebKit in this bug: https://bugs.webkit.org/show_bug.cgi?id=150853
See Also: → https://bugs.webkit.org/show_bug.cgi?id=150853
Whiteboard: [parity-webkit]
This is in CSS Color Level 4 drafts https://drafts.csswg.org/css-color/#hex-notation
(In reply to Xidorn Quan [:xidorn] (UTC+10) from comment #10)
> This has been implemented in WebKit in this bug: https://bugs.webkit.org/show_bug.cgi?id=150853

... and https://bugs.webkit.org/show_bug.cgi?id=157402
Does anybody know whether what is being implemented in Chromium and WebKit affects only colors specified in CSS, or whether it also affects colors in HTML attributes?
Assignee: nobody → dbaron
Status: NEW → ASSIGNED
(In reply to David Baron [:dbaron] ⌚️UTC-7 (review requests must explain patch) (busy May 9-13) from comment #14)
> Does anybody know whether what is being implemented in Chromium and WebKit
> affects only colors specified in CSS, or whether it also affects colors in
> HTML attributes?

Even if they did (which I hope they didn't), it seems like a bad idea per the discussion above, so I'll just implement this for CSS.
Pach 3, the comment that ends (usually multiplier, different behavior that percent)

I think that should be replaced by than
This conversion just reindents most of the function, but it then avoids
having indentation changes in patch 2.

Review commit: https://reviewboard.mozilla.org/r/51291/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/51291/
Attachment #8750091 - Flags: review?(bugzilla)
Attachment #8750092 - Flags: review?(bugzilla)
Attachment #8750093 - Flags: review?(bugzilla)
Attachment #8750094 - Flags: review?(bugzilla)
This patch tells all callers to use the existing behavior, so it is
intended not to change behavior.  Callers that will be modified in later
patches are marked with "FIXME" comments that will be removed in those
later patches (patches 3 and 4).

Review commit: https://reviewboard.mozilla.org/r/51293/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/51293/
This adds support for #rgba and #rrggbbaa colors to CSS.  This feature
is specified in https://drafts.csswg.org/css-color-4/#hex-notation .

This adds new types to nsCSSValue so that we can serialize the syntax
that was specified, as we do for other distinctions in how colors are
specified.

It does not change the behavior of the hashless color quirk, which
continues to support only 3 and 6 digit colors as specified in
https://quirks.spec.whatwg.org/#the-hashless-hex-color-quirk (step 4).

This changes property_database.js to remove various uses of 4 and 8
digit colors as invalid values.  It then adds them in slightly fewer
places as valid values, but more thoroughly testing both initial and
non-initial values on 'color'.

It marks two canvas tests explicitly testing this feature as no longer
known to fail by removing their .ini files.

Finally, it adjusts the web platform test testing the hashless color
quirk to no longer treat 4 and 8 digit colors as invalid values.
Removing the relevant test items seems like the right thing since
they're in a section where 3 and 6 digit colors were skipped but other
lengths were tested.  Modifying this imported test is OK since:
  <jgraham> dbaron: Commit the change you want to m-c, it is
    (semi-)automatically upstreamed every so often (typically
    about once a week)

Review commit: https://reviewboard.mozilla.org/r/51297/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/51297/
Attachment #8750091 - Flags: review?(bugzilla) → review+
Comment on attachment 8750092 [details]
MozReview Request: Bug 567283 patch 2 - Rename NS_HexToRGB to NS_HexToRGBA and add parameter saying whether 4 and 8 digit colors are allowed.  r?xidorn

https://reviewboard.mozilla.org/r/51293/#review47945

::: gfx/src/nsColor.cpp:78
(Diff revision 1)
> -bool NS_HexToRGB(const nsAString& aColorSpec, nscolor* aResult)
> +bool NS_HexToRGBA(const nsAString& aColorSpec, nsHexColorType aType,
> +                  nscolor* aResult)

As far as you are here, probably consider giving the return type it's own line like what you did for the declaration.
Attachment #8750092 - Flags: review?(bugzilla) → review+
Comment on attachment 8750093 [details]
MozReview Request: Bug 567283 patch 3 - Make TestColorNames use the new alpha capabilities.  r?xidorn

https://reviewboard.mozilla.org/r/51295/#review47951
Attachment #8750093 - Flags: review?(bugzilla) → review+
Comment on attachment 8750094 [details]
MozReview Request: Bug 567283 patch 4 - Support #rgba and #rrggbbaa colors in CSS.  r?xidorn

https://reviewboard.mozilla.org/r/51297/#review47959

> Finally, it adjusts the web platform test testing the hashless color
> quirk to no longer treat 4 and 8 digit colors as invalid values.

When read this sentence, I wondered why, because you said earlier that we do not change the behavior of hashless color quirk in terms of 4 and 8 digit colors. But later I realized that you mean no longer treat 4 and 8 digit (not hashless) colors as invalid values.

::: layout/style/nsCSSParser.cpp:6614
(Diff revision 1)
>    nsCSSToken* tk = &mToken;
>    nscolor rgba;
>    switch (tk->mType) {
>      case eCSSToken_ID:
>      case eCSSToken_Hash:
>        // #xxyyzz

This comment could probably be updated accordingly?
Attachment #8750094 - Flags: review?(bugzilla) → review+
(In reply to David Baron [:dbaron] ⌚️UTC-7 (review requests must explain patch) (busy May 9-13) from comment #14)
> Does anybody know whether what is being implemented in Chromium and WebKit
> affects only colors specified in CSS, or whether it also affects colors in
> HTML attributes?

Chrome's patch only affects CSS.  WebKit accidentally did affect HTML, but Dean just fixed it when we pointed it out to him.
https://hg.mozilla.org/integration/mozilla-inbound/rev/0e34b5aaeec01d4c832d1dd8f63fc45b53aa5a9e
Bug 567283 patch 1 - Convert if enclosing most of function into early return.  r=xidorn

https://hg.mozilla.org/integration/mozilla-inbound/rev/c913af2812365dfc8f63e27c15e8763a00db4bae
Bug 567283 patch 2 - Rename NS_HexToRGB to NS_HexToRGBA and add parameter saying whether 4 and 8 digit colors are allowed.  r=xidorn

https://hg.mozilla.org/integration/mozilla-inbound/rev/a9205982d844c80e8d2af5e84994984e631053f7
Bug 567283 patch 3 - Make TestColorNames use the new alpha capabilities.  r=xidorn

https://hg.mozilla.org/integration/mozilla-inbound/rev/cf57eb3d5080fa6d95bc956c6d870366d6f3840a
Bug 567283 patch 4 - Support #rgba and #rrggbbaa colors in CSS.  r=xidorn
Blocks: 1271191
This is now described at https://developer.mozilla.org/en-US/docs/Web/CSS/color_value#rgb%28%29 and https://developer.mozilla.org/en-US/Firefox/Releases/49#CSS.

Chris, is that enough or should this be mentioned more broadly? And if so, where?

Sebastian
Flags: needinfo?(cmills)
Slight error in this part:

> "#", followed by eight hexadecimal characters (0-9, A-F), where the first digit represents the red part, the second the green part, the third two the blue part and the last two the transparency.

That should say "first two digits represent" and "the second two"
(In reply to Sebastian Zartner [:sebo] from comment #29)
> This is now described at
> https://developer.mozilla.org/en-US/docs/Web/CSS/color_value#rgb%28%29 and
> https://developer.mozilla.org/en-US/Firefox/Releases/49#CSS.
> 
> Chris, is that enough or should this be mentioned more broadly? And if so,
> where?
> 
> Sebastian

Thanks Sebastian! This mostly looked fine; I've moved the descriptions down to here:

https://developer.mozilla.org/en-US/docs/Web/CSS/color_value#rgba()

As they are really ways to represent RGBA colors, not RGB. I also fixed the issue pointed out by Ian.
Flags: needinfo?(cmills)
I've also updated the support info, as Chrome appears to support it, but Safari doesn't.
(In reply to Chris Mills (Mozilla, MDN editor) [:cmills] from comment #31)
> Thanks Sebastian! This mostly looked fine; I've moved the descriptions down
> to here:
> 
> https://developer.mozilla.org/en-US/docs/Web/CSS/color_value#rgba()
> 
> As they are really ways to represent RGBA colors, not RGB. I also fixed the
> issue pointed out by Ian.

Thank you for fixing it!

(In reply to Chris Mills (Mozilla, MDN editor) [:cmills] from comment #32)
> I've also updated the support info, as Chrome appears to support it, but
> Safari doesn't.

I don't know how Google's release strategy looks like, though while it works in Canary, the related bug[1] is not marked as fixed yet and the Chromium platform status[2] also still claims 'In development'. That's why I previously added no compatibility for it. But I agree that it's better to reflect the support in Canary. So, given that, I've updated the status and added a compatibility note.

WebKit, on the other side, claims that it already got implemented[3] in November last year in contrast to the bug status of the original report[4]. Though I don't have OS X, so I couldn't try it out.

Anyway, with those changes in place, I mark this now as ddc.

Sebastian

[1] https://bugs.chromium.org/p/chromium/issues/detail?id=76362
[2] https://www.chromestatus.com/feature/5685348285808640
[3] https://bugs.webkit.org/show_bug.cgi?id=150853
[4] https://bugs.webkit.org/show_bug.cgi?id=39140
Keywords: dev-doc-needed → dev-doc-complete
(In reply to Sebastian Zartner [:sebo] from comment #33)

> I don't know how Google's release strategy looks like, though while it works
> in Canary, the related bug[1] is not marked as fixed yet and the Chromium
> platform status[2] also still claims 'In development'.

I will close the chromium bug [1] when developer tools are done.  I've updated chrome status [1] to state that the feature will ship in Chrome 52.

> [1] [1] https://bugs.chromium.org/p/chromium/issues/detail?id=76362
> [2] https://www.chromestatus.com/feature/5685348285808640
ahem ... chrome status _[2]_ to state ...
(In reply to noel gordon from comment #34)
> (In reply to Sebastian Zartner [:sebo] from comment #33)
> 
> > I don't know how Google's release strategy looks like, though while it works
> > in Canary, the related bug[1] is not marked as fixed yet and the Chromium
> > platform status[2] also still claims 'In development'.
> 
> I will close the chromium bug [1] when developer tools are done.  I've
> updated chrome status [1] to state that the feature will ship in Chrome 52.
> 
> > [1] [1] https://bugs.chromium.org/p/chromium/issues/detail?id=76362
> > [2] https://www.chromestatus.com/feature/5685348285808640

Thank you for the info! I've updated the compatibility information accordingly.

Sebastian
Adding site-compat keyword, there can be some unintended side-effects: https://github.com/webcompat/web-bugs/issues/2628.
Keywords: site-compat
(Thanks Kohei!)
Depends on: 1304810
(In reply to Chris Mills (Mozilla, MDN editor) [:cmills] from comment #32)
> I've also updated the support info, as Chrome appears to support it, but
> Safari doesn't.

This is behind a Flag in Chrome and not enabled by default
https://www.chromestatus.com/feature/5685348285808640

So https://developer.mozilla.org/en-US/docs/Web/CSS/color_value#Browser_compatibility is wrong.
(In reply to j.j. from comment #40)
> (In reply to Chris Mills (Mozilla, MDN editor) [:cmills] from comment #32)
> > I've also updated the support info, as Chrome appears to support it, but
> > Safari doesn't.
> 
> This is behind a Flag in Chrome and not enabled by default
> https://www.chromestatus.com/feature/5685348285808640
> 
> So
> https://developer.mozilla.org/en-US/docs/Web/CSS/
> color_value#Browser_compatibility is wrong.

Thank you for the hint! I've fixed the compatibility info (also for Opera) accordingly.

Please post such issues to the MDN mailing list in the future or create a separate bug under the "Developer Documentation" product, so it can be tracked separately.

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

Attachment

General

Creator:
Created:
Updated:
Size: