Make WordPress Core

Opened 6 years ago

Closed 3 months ago

Last modified 3 months ago

#45944 closed defect (bug) (fixed)

Twenty Nineteen: Added URL in :after styles for print.css is unreliable

Reported by: kjellr's profile kjellr Owned by: karmatosed's profile karmatosed
Milestone: 6.6 Priority: low
Severity: normal Version: 5.0.3
Component: Bundled Theme Keywords: has-patch needs-testing commit
Focuses: css Cc:

Description

The print styles for Twenty Nineteen include a little snippet meant to append the URL to links so it's visible when printed:

a[href^=http]:after {
    content: " < " attr(href) "> ";
}

Via: https://themes.trac.wordpress.org/browser/twentynineteen/1.2/print.css#L85

https://cldup.com/iUgRDksax1-3000x3000.png

This snippet is based on recommendations in this article:

https://www.jotform.com/blog/css-perfect-print-stylesheet-98272/

In practice, this appears to be fairly unreliable. In testing, @Joyously and I only see those URLs sporadically, and haven't been able to pin down why.

More details: https://github.com/WordPress/twentynineteen/issues/609#issuecomment-439489841

If these rules aren't working correctly, I suggest we remove them from the theme.

Attachments (2)

45944.patch (1.8 KB) - added by kjellr 5 years ago.
45944-2.diff (912 bytes) - added by poena 3 months ago.
Refreshed patch using tabs instead of spaces, .scss only. The .css file needs to be built.

Download all attachments as: .zip

Change History (15)

#1 @laurelfulford
5 years ago

  • Milestone changed from Awaiting Review to Future Release

Thanks for moving this over, @kjellr!

I did a bit of investigation because, honestly, this was totally bugging me!

It looks like these styles are the culprit:

/* Remove image filters from featured image */
  .image-filters-enabled *:after {
    display: none !important;
}

... which explains why it was only working sometimes -- if the image filter wasn't enabled, that class wouldn't be present.

I think this should be fixable by making the selector more specific (assuming the !important has to stay):

/* Remove image filters from featured image */
  .image-filters-enabled .site-featured-image:after {
    display: none !important;
}

... but just let me know if I'm missing something here!

--

Edited to correct my bad CSS suggestion :D

Last edited 5 years ago by laurelfulford (previous) (diff)

#2 @laurelfulford
5 years ago

  • Keywords needs-patch added; needs-testing removed

#3 @kjellr
5 years ago

🎉 Great sleuthing, @laurelfulford. That was exactly correct. I've attached a patch that applies some more specific styles down in that portion of the file. (I also snuck in a tiny fix for the featured image overlay in post lists while I was in there).

Thanks!

@kjellr
5 years ago

#4 @sabernhardt
2 years ago

  • Focuses css added
  • Keywords has-patch added; needs-patch removed

#5 @sabernhardt
2 years ago

  • Keywords needs-refresh added

#6 @karmatosed
4 months ago

  • Keywords needs-testing added
  • Milestone changed from Future Release to 6.6

#7 @karmatosed
3 months ago

@sabernhardt just noting you added 'needs-refresh' is that confirmation you tested and found the patch had issues? I wanted to check.

#8 @sabernhardt
3 months ago

The patch applies today, so I think my complaint might have only been the use of spaces instead of tabs in the Sass file. I should have explained the needs-refresh keyword either way, but noticing any problem with the CSS would have required a comment and/or screenshots.

Version 0, edited 3 months ago by sabernhardt (next)

@poena
3 months ago

Refreshed patch using tabs instead of spaces, .scss only. The .css file needs to be built.

#9 @shailu25
3 months ago

  • Keywords needs-refresh removed

#10 @karmatosed
3 months ago

  • Keywords commit added
  • Owner set to karmatosed
  • Status changed from new to assigned

Thanks for refresh, will move to commit now.

#11 @karmatosed
3 months ago

  • Resolution set to fixed
  • Status changed from assigned to closed

In 58100:

Twenty Nineteen: Adds URL in for print css.

The print styles is meant to append the URL to links. These rules weren't working correctly and this refreshed patch resolves that.

Props kjellr, laurelfulford, sabernhardt, poena, shailu25.
Fixes #45944.

#12 @karmatosed
3 months ago

In 58101:

Twenty Nineteen: Attempt to fix resolve failures cause by [58100].

This aligns print css to be the same as the scss file. This was suggested by failing tests.

Props swisspidy.
See #45944

#13 @karmatosed
3 months ago

In 58102:

Twenty Nineteen: Test failure fix for theme [58101].

Patching print file to get to pass tests after diff file fix. Newline patch.

Props swisspidy.
See #45944

Note: See TracTickets for help on using tickets.