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

Case-fold named and system colors #52

Merged
merged 1 commit into from
Oct 15, 2020
Merged

Case-fold named and system colors #52

merged 1 commit into from
Oct 15, 2020

Conversation

rviscomi
Copy link
Collaborator

Prevent duplicate named colors like RED, red, Red, etc. Normalize all system colors to the casing in the source array.

Progress on #32

@rviscomi rviscomi requested a review from LeaVerou October 15, 2020 03:46
@LeaVerou LeaVerou merged commit 8015180 into master Oct 15, 2020
@LeaVerou LeaVerou deleted the rviscomi-patch-2 branch October 15, 2020 12:07
@LeaVerou
Copy link
Owner

That explains why Background was so high in the list. Can you give me a few URLs that had Background recognized as a named color so I can fix the bug?

@rviscomi
Copy link
Collaborator Author

rviscomi commented Oct 15, 2020

page url
https://allandomb.com/ inline
http://phongkhambonnela.com/ inline
http://beans-labo.com/ https://cdnjs.cloudflare.com/ajax/libs/Swiper/4.5.0/css/swiper.min.css
http://benjarong.kr/ http://benjarong.kr/css/swiper.min.css
http://web.asis.mcu.edu.tw/ http://web.asis.mcu.edu.tw/sites/all/themes/stability/vendor/mediaelement/mediaelementplayer.css?qevvnn
http://larrakia.com/ http://larrakia.com/wp-content/themes/larrakia/style.css?ver=e74ab323b7d6ba6a0ad02b214460ceed
http://bronxshoes.co.za/ http://bronxshoes.co.za/wp-content/themes/enfold/js/mediaelement/skin-1/mediaelementplayer.css?ver=1
https://vivipins.com/ inline
http://www.kloak-ekspressen.dk/ inline
http://cashflowminer.com/ https://cashflowminer.com/template/default/static/theme.css?v=1.3.1
http://nweb.oppein.com/ http://www.oppein.com/templates/specialty/style/mediaelementplayer.css
https://www.centre-endoscopie-rachis.fr/ inline
https://mfr-moirans.org/ inline
http://myt1.cc/ http://www.myt1.cc/template/default/style/t1/style.css
http://civilopedia.serenity.pw/ http://civilopedia.serenity.pw/civilopedia/styles.css
http://flymcw.com/ http://flymcw.com/wp-content/plugins/the-events-calendar/common/src/resources/css/common-skeleton.min.css?ver=4.12.0
http://suachuamaytinhtmtc.com/ http://suachuamaytinhtmtc.com/themes/banhang001/js/plugin/swipper-slide/asset/css/swiper.min.css
http://genia.ge/ http://genia.ge/wp-content/themes/Flow/style.css
http://www.radioclubedelages.com.br/ http://www.radioclubedelages.com.br/estilo.css

@LeaVerou
Copy link
Owner

Thank you! I've looked at a few of these, and it appears to be either a CSS syntax error (missing (semi)colons), which results in CSS properties being included in value tokens, or URLs containing background.

For the latter, I just added code that strips URLs out of the value before colors are matched. That seems to fix a lot of the false positives.

But not sure how to proceed with the syntax errors. Since the problem occurs only with Background, we could omit that, or match it only with an uppercase B. Or we can just run the new code and see if the change addresses enough false positives that the syntax errors don't matter anymore. What do you think?

@rviscomi
Copy link
Collaborator Author

The bug almost makes for an interesting side note in the analysis about the buggy/unpredictable ways CSS is written, which makes me inclined to leave it. Happy to rerun with your latest changes and take it from there.

@LeaVerou
Copy link
Owner

LeaVerou commented Nov 3, 2020

Yeah, I think we should not case fold system colors, and only match them when written with capitalization, which is how I've seen them used anyway.
This will prevent the erroneous Background showing up so high.
Are you able to push that change?

@rviscomi
Copy link
Collaborator Author

rviscomi commented Nov 3, 2020

I've updated the sheet with case sensitive system colors. Background usage drops to 0.00%.

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.

2 participants