Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[css-color-4] Add new set of CSS system colors (addresses #3804) #4091

Merged
merged 1 commit into from
Aug 8, 2019

Conversation

melanierichards
Copy link

Preserves language around system colors from css-color-3; moves undeprecated values from the appendix, leaving behind those which are still deprecated.

css-color-4/Overview.bs Show resolved Hide resolved
css-color-4/Overview.bs Outdated Show resolved Hide resolved
@@ -2332,6 +2332,56 @@ Converting Between Uncalibrated CMYK and RGB-Based Colors</h3>
<li>fallback color must be set to the input color
</ul>


<h2 id="css-system-colors">
CSS System Colors</h2>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As I mentioned above, I think it would likely be better if something in this section included the deprecated system colors in the <system-color> term so that other pieces of spec wouldn't miss half of them.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Besides removing the definition of <deprecated-system-color>, would you mind clarifying what you'd like changed here? Move the deprecated list out of the appendix and into this section (though of course still noting they are deprecated)?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dbaron it looks like this PR is awaiting your clarification of what changes you requested.

Copy link
Contributor

@AmeliaBR AmeliaBR left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Additional comments:

The security & privacy section needs updating, as we are re-introducing some of the fingerprinting potential of system colors. The introductory section on system colors should probably also explicitly call out this risk.

It would be helpful if new paragraph text could be re-structured to use semantic line breaks.

Also, Melanie, you might need to add yourself to the acknowledgements section!

css-color-4/Overview.bs Outdated Show resolved Hide resolved
css-color-4/Overview.bs Outdated Show resolved Hide resolved
css-color-4/Overview.bs Outdated Show resolved Hide resolved
css-color-4/Overview.bs Show resolved Hide resolved
@melanierichards
Copy link
Author

melanierichards commented Jul 5, 2019

@dbaron @AmeliaBR one thing I wanted to ask after I take the PR out of draft mode was whether it makes sense to have this new section on system colors as its own section of the spec, or if it would make more sense as a subsection under named colors. Do you all have thoughts/opinions on that?

@AmeliaBR
Copy link
Contributor

AmeliaBR commented Jul 5, 2019

whether it makes sense to have this new section on system colors as its own section of the spec, or if it would make more sense as a subsection under named colors.

A subsection makes sense. Or maybe the top-level section should be renamed "Color keywords", and then have four subsections below it:

  • named colors
  • system colors
  • transparent
  • currentcolor

@melanierichards melanierichards force-pushed the new-system-colors branch 6 times, most recently from 22df596 to 0eda720 Compare July 5, 2019 22:38
@melanierichards
Copy link
Author

Commit has been updated to include the following changes:

  • Moved system colors section under "named colors" section
  • Addressed inline feedback, with the exception of clarification on how folks want deprecated system colors integrated into the definition (if other changes made don't suffice)
  • Updated language in security & privacy, and referenced from system colors section
  • Moved from length-based line breaks to semantic line breaks

@AmeliaBR
Copy link
Contributor

AmeliaBR commented Jul 5, 2019

Thanks for the fast updates, Melanie!

On security & privacy, the issue is not just about spoofing, it's also about fingerprinting. See also #3873.

@melanierichards
Copy link
Author

@AmeliaBR The security & privacy section of the spec already has a mention for the fingerprinting risk, do we feel it needs more detail? Seems like #3873 might be out of scope for this particular PR, or at least we might need to discuss first as a group?

@Crissov @svgeesus was your upvote on Amelia's comment in favor of renaming the "Named colors" section to "Color keywords"? For now I had just moved system colors within the "Named colors" section.

Thanks all for the feedback!

@AmeliaBR
Copy link
Contributor

I think your current text re security & privacy is accurate. Any discussion of stricter requirements on UAs can happen in the other issue.

@Crissov
Copy link
Contributor

Crissov commented Jul 13, 2019

Iʼm I in the favor of renaming the section and adding sub-headings as suggested.

@svgeesus svgeesus marked this pull request as ready for review August 5, 2019 11:50
@svgeesus
Copy link
Contributor

svgeesus commented Aug 5, 2019

@svgeesus was your upvote on Amelia's comment in favor of renaming the "Named colors" section to "Color keywords"?

Yes, it was.

@melanierichards
Copy link
Author

Just renamed "Named colors" section => "Color keywords" as requested (and sheepishly added myself to acknowledgements per @AmeliaBR's comment).

Apologies for the intermittent delay on these last changes!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants