Problem/Motivation

Files:

  • core/themes/claro/css/base/print.css
  • core/themes/claro/css/base/print.pcss.css
  • core/themes/seven/css/base/print.css

contain links to h5bp[dot]com which is suspicious to say the least (see screenshot).

These links are also visible in the documentation too (eg: https://api.drupal.org/api/drupal/core%21themes%21seven%21css%21base%21p...)

Steps to reproduce

Open any of the above files or visit api.drupal.org for any of the above files. A reference to the website is in the file.

Following is a screenshot of the page where one lands after visiting the URL from Drupal source:
suspicious website

Proposed resolution

Remove the comment with the reference

Remaining tasks

Review

User interface changes

None

API changes

None

Data model changes

None

Release notes snippet

None

CommentFileSizeAuthor
#22 3264911-22-d10.patch2.19 KBlauriii
#2 remove-reference-suspicious-site-3264911-2.patch1.88 KBbserem
suspicious_referenced_website.png43.24 KBbserem

Issue fork drupal-3264911

Command icon Show commands

Start within a Git clone of the project using the version control instructions.

Or, if you do not have SSH keys set up on git.drupalcode.org:

About issue forks

Comments

bserem created an issue. See original summary.

bserem’s picture

bserem commented
StatusFileSize
new remove-reference-suspicious-site-3264911-2.patch1.88 KB
bserem’s picture

bserem commented
Component: Stark theme » CSS
Status: Active » Needs review
bserem’s picture

bserem commented
Issue summary: View changes
vensires’s picture

vensires
Location Greece
commented

It seems the files were based on https://github.com/h5bp/main.css/blob/main/dist/_print.css. The original comment for example (eg. /* Black prints faster */) from that repo was inline and at some point transferred to the new line: https://github.com/h5bp/main.css/commit/15d950488b5f94a7fcfc4907b03ab060.... The link was missing though. Actually, these links might have been added as an attribution from Drupal to HTML5BOILERPLATE. Thus, I don't know if they should be removed or just replaced with a proper comment at the top of the file maybe.

bserem’s picture

bserem commented

@vensires valid concern! I tried (a lot actually) to find a working or an archived (archive.org) reference towards h5bp but couldn't find any.
I also couldn't find any valid references that black actually prints faster, since it is a per-printer thing. That is why I opted to remove the "bad" link rather than replacing it.

As it is know, the currently referenced domain could contain anything in the near future, good or bad.

vensires’s picture

vensires
Location Greece
commented

The website is not .com but .org actually: https://h5bp.org. At least, nowadays.

bserem’s picture

bserem commented

The .org is not a website with content, but rather a landing page with all the repositories of h5bp.

arismag’s picture

arismag commented
Status: Needs review » Reviewed & tested by the community

The patch looks good to me, IMHO although the reference could change to https://github.com/h5bp/main.css/blob/main/dist/_print.css I don't think it adds any value in the current status, it would be cleaner if we remove it.

lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
commented
Status: Reviewed & tested by the community » Needs work

We could add a file documentation that this is based on https://github.com/h5bp/main.css/blob/main/dist/_print.css. IMO that would be helpful since I wouldn't have recalled this is taken from there if it wasn't mentioned there.

bserem opened merge request !1953

bserem’s picture

bserem commented
Status: Needs work » Needs review

Reference to source on github added.
Removed the patch file, switched to a Merge Request.

Please review.

arismag’s picture

arismag commented
Status: Needs review » Reviewed & tested by the community

Seems fine!

xjm’s picture

xjm
she/her
Primary language English
commented

What does "Black prints faster" mean?

xjm’s picture

xjm
she/her
Primary language English
commented

At first I was worried that this had been injected by the PostCSS build, but the fact that it's also present in Seven makes that less likely.

It looks like this was introduced in Seven in #1892006: Include a print styling for Seven., which is already in the related issues. Comment #14 on that issue has the first patch to include the change. The comment reads:

Here is the new version. It"s use many things from hml5boilerplate css:
https://github.com/h5bp/html5-boilerplate/blob/master/css/main.css

The github repo mentioned still exists at https://github.com/h5bp/html5-boilerplate so maybe they just stopped paying for the other domain, and it got scooped up by whatever sketchy owner?

nod_’s picture

nod_
Primary language French
Location Lille
commented

don't have the exact url it's pointing to but we can see that the domain used to be legit: https://web.archive.org/web/20141217091001/h5bp.com/ url shortner don't work well over long period of time

lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
commented

This is a print specific CSS file and it overrides text color everywhere to black so that printers would be able to print the text faster using K (black) instead of mix of CMY (cyan, magenta and yellow).

For what I'm concerned the patch is ready to be committed. I just wanted to move the comment before the line that is being commented.

xjm’s picture

xjm
she/her
Primary language English
commented
Status: Reviewed & tested by the community » Needs work

Thanks @nod_ and @lauriii.

The current docs lines with the new URL are over 80 chars so those need to be rewrapped. Other than that, this makes sense with the above info. Thanks!

bserem’s picture

bserem commented
Status: Needs work » Needs review

Thanks @xjm. Pushed a fix and resolved the threads in gitlab.
Many thanks to @lauriii for the explanation about print cartridges.

Up for review

arismag’s picture

arismag commented
Status: Needs review » Reviewed & tested by the community

Over 80 chars issue was fixed. If no other issues, I think we could move forward :)

P.S @lauriii nice explanation on print cartridges.

yogeshmpawar made their first commit to this issue’s fork.

lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
commented
StatusFileSize
new 3264911-22-d10.patch2.19 KB

  • lauriii committed 7064f9f on 10.0.x 
    Issue #3264911 by bserem, lauriii, arismag, xjm, vensires: Core CSS...

  • lauriii committed f7e571c on 9.4.x 
    Issue #3264911 by bserem, lauriii, arismag, xjm, vensires: Core CSS...

lauriii closed merge request !1953

lauriii’s picture

lauriii
he/him
Primary language Finnish
Location Finland
commented
Status: Reviewed & tested by the community » Fixed

Committed 7064f9f and pushed to 10.0.x. Also committed to 9.4.x. Thanks!

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.