Make WordPress Core

Opened 6 years ago

Closed 6 years ago

Last modified 5 years ago

#43715 closed enhancement (fixed)

Add Privacy Policy link to bundled theme footers

Reported by: xkon's profile xkon Owned by: xkon's profile xkon
Milestone: 4.9.6 Priority: normal
Severity: normal Version:
Component: Privacy Keywords: gdpr has-dev-note
Focuses: ui, rtl Cc:

Description

We are probably all using some kind of plugin that throws a little notification ( popup box / bar you name it ) in our front-end to inform visitors about having Cookies & a Privacy Policy page / Terms & Conditions and anything else you can probably have for users that is mandatory to read.

At least it's mandatory in the EU for quite some time now as well pre-GDPR.

One fine example is the https://wordpress.org/plugins/uk-cookie-consent/ ( I just use that but there are plenty others as well in the repo ).

I'd say it's time to bundle something like this with core just to be 100% sure that it will keep everybody on the 'safe' side:

Since we are helping the website owners create their Privacy Policy page and also help them include all information needed / possible about the cookies etc that they will be using, why not throw a notification as well in the front end that users could customize with some simple css ?

My thinking is this to be more precise:

If you have selected a Privacy Policy page from the new Tools that we are adding + that page gets a 'published' status, the popup is going to be automatically enabled in the front end for you as well pointing to that page. This way there's no way of 'forgetting' it and the users are automatically informed.

Of course there can be an option to disable it if the Admin wants to use any other plugin or create any custom notification etc.

What do you think, can we go for this as well since it won't be intrusive in any way if Admins can easily 'disable' it ? I'd personally prefer to have it built in corewise than depend on a plugin.

If you want a full scope of this it should be something like this (fairly simple):

Title: Privacy Policy
Text: Read about the privacy policy and the cookies we use.
Button: I Accept.

When clicking the button 1 extra cookie drops that the user has accepted so it won't popup again. We could also provide a UX in the admin so the site owner can select what the title/text/button would write ofc.

Attachments (11)

43715.diff (1.1 KB) - added by xkon 6 years ago.
PP shortcode
43715.2.diff (7.9 KB) - added by xkon 6 years ago.
footer link on bundled themes
43715.2.preview.jpg (39.8 KB) - added by xkon 6 years ago.
43715.3.jpg (42.4 KB) - added by xkon 6 years ago.
43715.3.diff (12.1 KB) - added by xkon 6 years ago.
43715.4.jpg (40.8 KB) - added by xkon 6 years ago.
43715.4.diff (9.1 KB) - added by xkon 6 years ago.
43715.5.diff (9.7 KB) - added by xkon 6 years ago.
43715.6.diff (10.1 KB) - added by iandunn 6 years ago.
Use CSS content for separator instead of <span> content
43715.7.diff (12.1 KB) - added by laurelfulford 6 years ago.
43715.8.diff (14.2 KB) - added by iandunn 6 years ago.
Add imprint class to "Proudly powered by WordPress" link

Download all attachments as: .zip

Change History (75)

#1 @xkon
6 years ago

  • Keywords gdpr added

#2 @allendav
6 years ago

+100 - i especially like the idea of automatically linking the privacy policy page if one exists - now that we have that as a dedicated option

This ticket was mentioned in Slack in #gdpr-compliance by xkon. View the logs.


6 years ago

#4 in reply to: ↑ description @nickwuk
6 years ago

Replying to xkon:

Great idea. However does the popup notification not need to give granular information about and control over the different classes of cookies like the OneTrust or CookieBot notifications? Also to provide a facility to revoke consent as well as accepting, and to record the user's id and date of acceptance for each class(and maybe even the Privacy Policy version)?

I'm NOT a lawyer, I'm just interested in following WP developments with regards GDPR.

#5 @azaozz
6 years ago

Yep, that's a good idea. The only thing I'm a bit worried about is that the regulations require consent before anything happens, cookies being set, data is send to third parties, etc.

Sounds like it will have to be a blocking modal that stops the site from loading? Or perhaps a whole landing page (for no-js?) :(

Of course we can keep it a "simple" message linking to the privacy policy that sets a cookie when accepted (so it goes away). Will also be interesting to see how some big sites handle that.

#6 @nickwuk
6 years ago

I've just called the UK's ICO helpline to clarify the situation on cookies. They say that this area is under consultation and will be part of the ePrivacy Regultions in 2019, and until that time the existing PECR applies. I was informed therefore that granular cookie control and saving consent with id and date is not required at this time.

Also I found this aricle interesting about not requiring explicit opt-in for standard Google Analytics https://www.peakdemand.co.uk/blog/the-impact-of-gdpr-on-google-analytics/

#7 @iandunn
6 years ago

The article that @nickwuk linked to includes this interesting quote:

in January 2017, the European Commission clarified the high-level privacy rules as follows: The so called “cookie provision”, which has resulted in an overload of consent requests for internet users, will be streamlined. New rules will allow users to be more in control of their settings, providing an easy way to accept or refuse the tracking of cookies and other identifiers in case of privacy risks.

The proposal clarifies that no consent is needed for non-privacy intrusive cookies improving internet experience (e.g. to remember shopping cart history). Cookies set by a visited website counting the number of visitors to that website will no longer require consent.” [emphasis mine]

I think that's something that should be carefully considered before adding a cookie modal to Core. I personally feel like those modals have a very negative impact on user experience while simultaneously failing to provide any kind of meaningful improvement to a user's privacy.

#8 @xkon
6 years ago

Note for clarification of the ticket as well: My intention on this was to connect the default front-end more with our Privacy Policy page tools as everything will be explained in there, not create an actual 'consent popup' - a consent per cookie / data handling thing etc popup needs a lot of discussion and it will pretty much be 99% intrusive / UI breaking for most of the themes afaic so we shouldn't go for it without getting feedback from theme authors first as well.

That being said an extra thought I had instead of going the 'intrusive' popup way since any actual 'modals' would again mess with how Themes look etc and we don't want to mess peoples sites up UI wise is to just display a Privacy Policy page link in the default footers if the Privacy Policy page exists.

This idea comes down to simply updating all bundled themes footers with something like Proudly powered by WordPress. - Privacy Policy. Or an iteration of that.

So again everybody else could simply use any plugin he likes or any preferred/custom way to link to his PP page + create his consent modals until we implement something to cover everything.

Extra note about the consents in general: In the company I work they decided that we won't be asking for consent for each cookie / data gather individually. Instead our Privacy Policy / Terms & Conditions of using any given website will state all aspects of data collection and everything GDPR needs of course but there will be 1 single notification that you simply accept 'Everything stated in this page' if you want to use the website ( that's why we will be tracking who 'opted in for the whole website experience basically'.

Now I'm not even sure if that's legally bulletproof but it basically strips pretty much all individual consents / opt-ins and merges them into 1 big thing.

To sum this up: There have been some mockups all over the internet showing popup boxes with loads of multi-consent forms so the user has the ability to 'accept' whatever he likes etc.

We ended up to go the way of 'if you want to see this website, well you have to accept everything' and that's it.

#9 @iandunn
6 years ago

This idea comes down to simply updating all bundled themes footers with something like Proudly powered by WordPress. - Privacy Policy. Or an iteration of that.

I like that a lot better, since it's not intrusive to the user, and fits in elegantly with the existing design and markup.

#10 @allendav
6 years ago

If we do just a footer link, doesn't that preclude capturing the user's consent? E.g. how can I avoid serving tracking tech to them before they've consented? I assume many if not most sites will want to embrace the guidance of the IAPP regarding obtaining clear consent from their users: https://iapp.org/news/a/top-10-operational-impacts-of-the-gdpr-part-3-consent/

This ticket was mentioned in Slack in #gdpr-compliance by allendav. View the logs.


6 years ago

#12 @iandunn
6 years ago

Core doesn't do any tracking, though, right? At least where "tracking" is defined with respect to comment:7.

So is the goal of this ticket to just provide a standardized modal for plugins to use, even though it'd be off by default on a fresh install? Or do you think that even something as benign as wordpress_sec_{hash} requires explicit consent? Even if it did, couldn't we collect that consent during the log-in process, similar to how it's collected for commenting?

#13 @xkon
6 years ago

Okay before we get lost in translation again I think it's a good time to make a division as we did on what is personal data between what a site has to offer in the front-end.

To my knowledge as some stuff already exist today a website 'should' have:

  • Privacy Policy ( what we are helping with as a tool )
  • Terms of Use ( of the website and I don't know if that could be the same with the PP )
  • Cookie information ( It might as well be bundled with the PP )
  • Copyright notice ( that's admin/website dependent so it doesn't have to do with us at all )
  • Cookie & general 'consents'

---

As far as the Privacy Policy that we are helping to be authored via our Tools, I suggest a simple link on the bundled themes footer as I said above, that could/should include all the 'Cookie' information as well in my opinion whether there's an extra 'Consent' popup or not. That information should be accessible always to the user.

I've had some chats with the #themereview team and even the small change we made on the comment form with the extra label is going to cause a bit of trouble even though it's a really small footprint.

I can't imagine at the moment what a complete popup would do to themes. So my 'original' point of view when I opened this ticket changed pretty fast between chats :D

Having a built-in shortcode for example [privacy-policy-page] that generates a link to the created PP page etc would be a good thing as well and will probably help people even more to add it on various places if they want ( widgets, custom areas, custom code etc ).

This link has nothing to do with the actual Consent process that has to take place and needs a LOT of talk of how it should/could be implemented as there is nothing final afaic for the time being.

I'll change my tone a bit but: The web works with cookies whether the GDPR likes it or not. If the GDPR needs consents before dropping cookies ( entering a website ) well they will hit a really big wall as websites will probably go into a lockdown as we normally see with Age verification sites. A full blank page with just 1 small box stating the cookies to first accept them and then enter the website. That will seriously kill websites and I don't think that this will be eventually done as it's counter-productive and really really not user-friendly. I don't yet see any cookie consents when entering google / amazon or any other major player out there either even though everybody updated their Terms/ Privacy policies.

--

As I said in the chat at some point the company that I work is dealing with the whole cookie/consent matter as a single 'consent / statement' in the meaning of 'We need and gather this information and use these cookies. If you don't accept this we will redirect you to google.' so if you hit no you are just booted off the website, and that's with lawyers behind this idea.

--

My final proposal on this is (at least after my long extra chats with various slack channels and from proposals that I gathered):

1] An automated link to Privacy Policy page on bundled themes footer if it exists
2] Core shortcode of maybe [privacy-policy-page] so people can easily use it anywhere as it will again check if the page exists and output / refresh the link automatically.

Last edited 6 years ago by xkon (previous) (diff)

#14 @iandunn
6 years ago

That makes sense, thanks for the clarification!

@xkon
6 years ago

PP shortcode

#15 @xkon
6 years ago

43715.diff is a first pass to create the [privacy-policy-page] shortcode.

It checks if a Privacy Policy page is set in the Privacy tool & if it is Published to return the page link.

The shortcode accepts 2 parameters at the moment:

  • text = The links text so a user can easily alter it
  • divider = a divider in front of the link just in case they want to use it in a sentence etc

Example:

[privacy-policy-page text="Read our Privacy Policy" divider=" - "]

would output

- <a href="http://dev.oo/src/privacy-policy/">Read our Privacy Policy</a>

I have some questions though to finalize it:

1] I've put this into wp-includes/shortcodes.php as I don't know if there's a better place atm to add this since we can't create new files etc as well.

2] Should we sanitize the params somehow? I couldn't figure out a good way as there could be different use cases since some people might want to put html etc in it as well for example a &bull; divider.

3] Should we better add 2 dividers? For the use of 'in-between' text? For example left-divider=" - " right-divider=" | " to give us - Privacy Policy |

--

When the shortcode is finalized I can easily make a pass into the bundled themes and add the link along the footer.

#16 @azaozz
6 years ago

Frankly I'm not sure we need a shortcode for that link. Shortcodes are generally a "bad way to do stuff" :) They may make some sense as placeholders in posts, but should be avoided if at all possible.

The privacy policy page is a standard WP page. It can easily be added to menus, the URL to it can easily be copied and pasted... anywhere. Then we already have get_privacy_policy_url() for themes. A shortcode that is not intended to be used in posts seems... perhaps not needed?

This ticket was mentioned in Slack in #gdpr-compliance by allendav. View the logs.


6 years ago

#18 @xkon
6 years ago

After todays meeting ( adding this as a note ): we're cancelling the shortcode idea and will update the bundled themes after the release to add the link in the footer.

#19 @desrosj
6 years ago

  • Milestone changed from Awaiting Review to 4.9.6
  • Owner set to xkon
  • Status changed from new to assigned

Moving to the 4.9.6 milestone after consensus was reached in the most recent GDPR chat (https://wordpress.slack.com/archives/C9695RJBW/p1524063200000304).

#20 @xkon
6 years ago

@allendav I have not yet seen anybody requiring consents before the website loads. So basically all cookies are already set at start either you consent or not. The difference is that they are removed if you choose not to consent and not droped again. Not even the regulating authorities themselved do it ( if that makes a point :D ).

But, If we are to follow this concept should we create something like wp_add_privacy_policy_content() that gathers all cookie info into a nice list / modal like the one that cookiebot.com is producing ?

Although I'm saying again a straight up modal from core will be very intrusive to some themes etc. So we have to figure out a way or add an option somewhere to enable/disable it at least and provide al the necessary CSS and stuff so everybody can theme it if they like.

That's why I'm still not really sure about a full modal and prefer to add footer links for bundled themes that are core for starters and maybe we can review it in the future again.

#21 @iandunn
6 years ago

  • Summary changed from Popup notification for Privacy Policy page to Add Privacy Policy link to bundled theme footers

@xkon
6 years ago

footer link on bundled themes

@xkon
6 years ago

#22 follow-up: @xkon
6 years ago

43715.2.diff makes a first pass in all bundled themes to add a footer link to Privacy Policy page by using the newly added get_privacy_policy_url().

All links are enclosed in a <span class="privacy-policy-page-link"> for the purpose of easily hiding them with simple css if needed by the users/admins.

I tried to match each themes ui feeling without it being intrusive & without it looking out of place.

43715.2.preview.jpg shows in order from Twenty Ten to Twenty Seventeen.

#23 follow-up: @birgire
6 years ago

@xkon these theme screenshots are very informative, thanks

do you think it would make sense here to have a function like:

/**
 * Determines whether the privacy policy page is set.
 *
 * @since 4.9.6
 *
 * @return bool If the privacy policy page is set.
 */
function has_privacy_policy() {
	return (bool) get_option( 'wp_page_for_privacy_policy' );
}

to simplify theme codes like:

if ( has_privacy_policy() ) {
	printf(
		'<span class="privacy-policy-page-link"><a href="%s">%s</a></span>',
		esc_url( $url ),
		esc_html__( 'Privacy Policy', 'twentyten' ),
	);
}

Not far away from existing functions like:

  • has_header_image()
  • has_header_video()
  • has_custom_header()
  • has_site_icon()
  • has_custom_logo()
  • has_nav_menu()
Last edited 6 years ago by birgire (previous) (diff)

#24 in reply to: ↑ 22 @iandunn
6 years ago

Replying to xkon:

43715.2.diff makes a first pass in all bundled themes to add a footer link to Privacy Policy page

That looks good to me at a quick glance. Here's a few suggestions:

  • There's a coding standard guideline about not setting variables inside if ( ) blocks. It seems cleaner to set it at the top of the file:
<?php
/**
 * The template for displaying the footer
 *
 * Contains the closing of the #content div and all content after.
 *
 * @link https://developer.wordpress.org/themes/basics/template-files/#template-partials
 *
 * @package WordPress
 * @subpackage Twenty_Seventeen
 * @since 1.0
 * @version 1.2
 */

$privacy_policy_url = get_privacy_policy_url();

?>

I'm not sure if that's a common convention in Core, though, so maybe be more appropriate to define itjust above the if ( ) block.

  • Rather than echoing HTML, it seems more readable to me to interpolate <?php tags:
<?php if ( $privacy_policy_url ) : ?>
	<a class="privacy-policy-page-link" href="<?php echo esc_url( $privacy_policy_url ); ?>" title="<?php echo esc_attr( 'Privacy Policy', 'twentyseventeen' ); ?>">
		<?php _e( 'Privacy Policy', 'twentyseventeen' ); ?>
	</a>
<?php endif; ?>

That may also not fit with existing Core conventions, though.

  • I think esc_url() is better for the URL than esc_attr(), because it's a more targeted context.
  • Similarly, I think esc_attr() is better for the title than esc_html().
  • Is the span necessary? I wonder if it'd be more semantic, and more consistent with the existing links in the footers, to just have an a element with the class?
  • Do we need the <br> for back-compat? If not, then it seems more semantic to use something like .privacy-policy-page-link { display: block }.
  • If we keep the <br> should we use <br /> in some of the themes, depending on their doctype?

#25 @birgire
6 years ago

ps: the link title attributes are no longer recommended: #24766

#26 in reply to: ↑ 23 ; follow-up: @iandunn
6 years ago

Replying to birgire:

do you think it would make sense here to have a function like [has_privacy_policy()?]

If we do add that, I think it should use get_privacy_policy_url() rather than fetching the option directly. That would allow for filtering the URL, like in ticket:43620#comment:30.

ps: the link title attributes are no longer recommended: #24766

Good catch!

#27 in reply to: ↑ 26 @birgire
6 years ago

Replying to iandunn:

If we do add that, I think it should use get_privacy_policy_url() rather than fetching the option directly. That would allow for filtering the URL, like in ticket:43620#comment:30.

yes, I agree - one could e.g. return the non-empty check.

I like that filter suggestion +1

@xkon
6 years ago

@xkon
6 years ago

#28 follow-up: @xkon
6 years ago

@iandunn / @birgire thank you both for the notices :)

In 43715.3.diff fixed the CS errors and added the appropriate CSS in all themes.

@iandunn I'm using a <span> as a wrapper for the link so I can place the dividers / where necessary by doing :after or :before in css. This way users can easily change it to something else or hide it etc. Imho it's a bit more accessible this way just in case. It's the same principle as used in TwentySixteen by design.

@birgire I replace the title with aria-label instead. I think we're ok with this correct? :)

I'll be updating #43721 in the same way as here to be consistent with this new addition everywhere.

#29 in reply to: ↑ 28 @iandunn
6 years ago

Replying to xkon:

In 43715.3.diff fixed the CS errors and added the appropriate CSS in all themes.

I think that's looking good :)

I'd keep ?> on the same line as the <?php if(), to make it cleaner to read, and consistent with other places in Core.

Should we use ::before instead of :before? I think the latter is CSS2 syntax, and all the browsers we support can handle the CSS3 syntax.

@iandunn I'm using a <span> as a wrapper for the link so I can place the dividers / where necessary by doing :after or :before in css. This way users can easily change it to something else or hide it etc. Imho it's a bit more accessible this way just in case. It's the same principle as used in TwentySixteen by design.

Huh, I'm not sure I understand. I tested out adding adding the dividers on the a instead of the span, and some styles like text-decoration need the psuedo element to be display: inline-block, but other than that it worked fine.

What specific problem are you running in to?

@birgire I replace the title with aria-label instead. I think we're ok with this correct? :)

Is that necessary in this context? I thought that that was used in cases where the anchor text was not descriptive, for example:

<button aria-label="Close" onclick="myDialog.close()">X</button>

It seems like in this case, the anchor text might be all that's needed?

#30 follow-up: @xkon
6 years ago

@iandunn Everything thta concerns getting the link moved into https://core.trac.wordpress.org/ticket/43620#comment:35 just informing so we won't go back and forth in tickets and get lost again.

I'll reply here though for these points as they are theme related mostly.

I tried to be as consistent as possible with whatever code exists in themes already so :

<span>: If we ad a divider with a:after it goes within the <a> tag so it will inherit all on :hover and other styles of the a as well and that means even more CSS styles to make it visually independent from the link without a good reason, so I thought of following the same way it's already done in Twenty Sixteen's footer that adds a divider with .site-title:after

<span class="site-title"><a href="http://develop.oo/src/" rel="home">My Website</a></span>

This way if you ad a divider with :after, it doesn't get highlighted as well as it's out of the <a> and the themes handle it as plain text for visual purposes if that makes sense now.

::after / :after /: Again I just went with whatever I saw in the .css files already being done.

As for the aria-label I think you're correct, I'm not that well versed with accessibility and I'm always getting confused, I thought the more labels the better (for some reason). But I wasn't 100% sure. We can update the .diff in the other ticket with whatever is needed for this :) .

Last edited 6 years ago by xkon (previous) (diff)

#31 @birgire
6 years ago

@birgire I replace the title with aria-label instead. I think we're ok with this correct? :)

@xkon I think the link looks good without it, just as mentioned by @iandunn

This is a great resource:

https://make.wordpress.org/accessibility/handbook/

that I just discovered recently ;-)

There's an interesting ticket #40428 regarding how CSS generated content could affect screen-readers:

When CSS generated content is not intended to be available for speech output, then it should always
be wrapped by an element (for example a <span>) with an aria-hidden="true" attribute. At the moment,
this is the only reliable way to hide CSS generated to assistive technologies.

So I wonder if content: "\002f"; should be replaced with an aria hidden span tag instead?

Last edited 6 years ago by birgire (previous) (diff)

#32 in reply to: ↑ 30 @iandunn
6 years ago

Replying to xkon:

<span>: If we ad a divider with a:after it goes within the <a> tag so it will inherit all on :hover and other styles of the a as well and that means even more CSS styles to make it visually independent from the link
::after / :after /: Again I just went with whatever I saw in the .css files already being done.

Ah, ok, those are good points :)

#33 @iandunn
6 years ago

  • Keywords needs-patch added

r43002 adds the necessary template tags, so I think this is ready for a new patch.

#34 @xkon
6 years ago

I'll have everything updated by tonight. Thank you @iandunn .

@xkon
6 years ago

@xkon
6 years ago

#35 @xkon
6 years ago

  • Keywords has-patch needs-testing added; needs-patch removed

In 43715.4.diff based on previous patches made the necessary changes as seen on 43715.4.jpg by utilizing the newly added the_privacy_policy_link().

Note: I thought of keeping everything in 1 line this time to not add any extra footer 'height' as it was done in the previous patches.

#36 @iandunn
6 years ago

  • Keywords needs-dev-note added

Awesome :)

It looks like for all of the themes, the separator psuedo-element is added after the link text, except for TwentyTen, where it's before. I'm assuming that was intentional, but just wanted to double check.

@xkon, can you add a link to the discussion you had with #themereview? I couldn't find it, but I think it may be helpful.


@laurelfulford, @davidakennedy: do you have time to give some feedback about the impact this will have on child themes that are based on the bundled themes?

The new link markup won't be added default, because the link isn't added unless the user has created and set a Privacy Policy (#43435). The styles will still apply, though, and could conflict with some child theme elements/styles.

And then once the user creates the privacy page, the additional markup will be added, which may also cause some conflicts.

Once any issues there are mitigated, we should probably post a dev note on make/Core to let theme authors know they should check for how this impacts their child themes.

#37 @xkon
6 years ago

That's correct on the separator yes, it's the only theme that the policy link is after the website's name in the footer so the separator needs to be on the left instead.

There was a discussion on #themereview but it was about the cookie consent on the comments form, not the privacy policy links in the bundled themes ( as these shouldn't mess much with peoples theme-ing or at least I haven't informed the channel yet for this ).

---

I'll put the starting thread for the cookies, the feedback was to inform the theme authors with a Meta Post in advance for any theme changes -> https://wordpress.slack.com/archives/C02RP4Y3K/p1522605683000010 and we had 2 comments on https://core.trac.wordpress.org/ticket/43436#comment:27 / https://core.trac.wordpress.org/ticket/43436#comment:29 .

In general the only additions to the themes for the time being are the links from this ticket + the opt-in on comments as seen in #43436 that needs a review as well before committing :)

@xkon
6 years ago

#38 @xkon
6 years ago

43715.5.diff adds an if ( function_exists( 'the_privacy_policy_link' ) ) {} to avoid errors.

#39 @iandunn
6 years ago

Would it be good to add the / via CSS content, instead of actual HTML content? I'm thinking that would allow child themes to customize it without having to override footer.php.

#40 @laurelfulford
6 years ago

@laurelfulford, @davidakennedy: do you have time to give some feedback about the impact this will have on child themes that are based on the bundled themes?

@iandunn I should be able to take a closer look at this tomorrow!

This ticket was mentioned in Slack in #core by jeffpaul. View the logs.


6 years ago

#42 @xkon
6 years ago

@iandunn sorry I just saw the reply for the CSS. Yes I'll get an update asap, good thinking.

@iandunn
6 years ago

Use CSS content for separator instead of <span> content

#43 @iandunn
6 years ago

Thanks @laurelfulford :)

43715.6.diff makes the change to use CSS content. It looks identical to 43715.4.jpg.

#44 follow-ups: @laurelfulford
6 years ago

I discussed possible child theme issues with @davidakennedy. We could think of scenarios where the update could cause issues, but we both think that they would be minor:

For example, if someone modified the footer via CSS to add content (like a copyright), spacing could be off. Or if they’ve absolutely positioned elements in the footer, the new text could result in overlaps or some minor visual weirdness once a Privacy Policy page was set.

As mentioned, there’s also a chance of CSS class name clashes. The styles that have been included are well-scoped, though, which should lessen interference with other elements in a child theme.

Any child themes that have very custom footers likely have their own footer.php, which won’t include this code until those theme authors add it.

I can’t think of anything else we could do proactively to prevent some issues from cropping up, since we can’t really predict what’s been done to the themes. But I think the fact the link has to be enabled before appearing will help a lot — it won’t suddenly spring up on a bunch of sites — and the placement of the link is a really good choice, since it's piggybacking off text that's already there :)


I also checked over each theme with the latest patch. Overall things are looking really good! :) I included some feedback below that is 90% nitpicky spacing stuff -- I'm happy to update the patch if these seem like reasonable updates:

In General

We’re using a CSS entity in Twenty Sixteen for the other / used in the footer -- content:'\002F' rather than content: '/' -— what do you all think about using that throughout for consistency?

Twenty Ten

The spacing seems a little off around the separator (with more on the right than left):

https://cldup.com/Jc-L8YsYYU-3000x3000.png

The CSS is already adjusting a bit for the space (which is coming from a line break in the HTML) but I think pushing it a bit further would help. Here it is with padding: 0 0.5em 0 0.3em

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

Twenty Ten uses “|” as a separator in other spots in the theme, like the post meta. Does it makes sense to use that here as well? It’d look like:

https://cldup.com/61Og3jzqr0-3000x3000.png

Twenty Eleven

Similar notes about the padding — right now it’s:

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

Updating to padding: 0 0.25em 0 0.5em turns it into:

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

Twenty Eleven also uses “|” in the post meta; if it makes sense to use it in the footer as well, it’d look like:

https://cldup.com/Ldb_n-gAca-3000x3000.png

Twenty Twelve

Similar notes about the padding — right now it’s:

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

And with padding: 0 0.3em 0 0.6em I think it feels a bit more even:

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

Twenty Thirteen

Similar notes about the padding — right now it’s:

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

With padding: 0 0.25em 0 0.5em , it looks like:

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

Twenty Fourteen

Similar notes about the padding — right now it’s:

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

With padding: 0 0.25em 0 0.5em, it’d look like:

https://cldup.com/HPDMu-F6ut-3000x3000.png

Twenty Fifteen

Similar notes about the padding — right now it’s:
https://cldup.com/tjdm_ad7xX-3000x3000.png

With padding: 0 0.25em 0 0.5em, it’d look like:

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

Twenty Sixteen

The RTL styles for this theme look a bit off. LTR is perfectly spaced:

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

The “/“ with the Privacy link includes the font family in the styles, and it’s not overwritten in the RTL styles. The original “/“ is actually the one that looks off to me in this case (smaller, and misaligned), but they should be made to look the same:

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

Twenty Seventeen

Again, just a small spacing thing - right now it’s:

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

With padding: 0 0.2em 0 0.4em, it would look like:

https://cldup.com/5FtL4hYUCm-3000x3000.png

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

#45 @davidakennedy
6 years ago

Thanks for all the work on this everyone! One thought I had, if we wanted to keep potential stylistic issues to a minimum in child themes because of this change: Only make the changes work on newer default themes. Like Twenty Fifteen, Twenty Sixteen and Twenty Seventeen.

Nothing says new features need to work in every default theme. And I'm of the mindset that we should put more effort into the newer ones anyway, and change the older ones as little as possible. That cuts our potential for possible bugs way down.

Not to ride in and suggest big changes at the last minute, or diminish the work @xkon and others have done. But it's an option, and I would be for it.

#46 in reply to: ↑ 44 @iandunn
6 years ago

Replying to laurelfulford:

I included some feedback below that is 90% nitpicky spacing stuff -- I'm happy to update the patch if these seem like reasonable updates:

Those all sound good to me, thanks for the thorough review and eye for detail!

Out of curiosity, what's the reason we use entities in CSS instead of the "regular" character? I'm sure there's a reason, but I searched around but couldn't find anything.

Replying to davidakennedy:

Nothing says new features need to work in every default theme. And I'm of the mindset that we should put more effort into the newer ones anyway, and change the older ones as little as possible. That cuts our potential for possible bugs way down.

I think that's a really good guideline in general, but it seems like this case might be an exception, since the reason behind it is legal compliance with the GDPR, rather than improving the design or offering a new feature. My impression is that we pretty much have to do this in order for Core to be compliant, and that there will be lots of sites using the themes that will want to be compliant as well, so a standardized solution will be very beneficial for users.

#47 @laurelfulford
6 years ago

Out of curiosity, what's the reason we use entities in CSS instead of the "regular" character? I'm sure there's a reason, but I searched around but couldn't find anything.

You know, I don't know! :) I spent a bit of time in Google, too, but to no avail. I had made the suggestion to keep things consistent; my best guess is they're easier to use for less-frequently typed characters. / definitely doesn't quality there, but a lot of icon-font content: values would.

This is pure speculation, but I wonder if Twenty Sixteen does it to keep all content: values a consistent format?

#48 in reply to: ↑ 44 @xkon
6 years ago

Replying to davidakennedy:

Not to ride in and suggest big changes at the last minute, or diminish the work @xkon and others have done. But it's an option, and I would be for it.

We're all here to help no matter how big or small the changes are, aren't we? :D . Don't even worry about that part :) . But as @iandunn mentioned since this update is about helping people with GDPR so it's tied to legal things imho we should update everything and provide as much as possible by default.

--

@laurelfulford : this + the cookie opt-in are 'unfortunately' something that needs to be implemented I've been discussing these changes with #themereview as well and we'll be doing posts on Meta also when they are finalized to help either users & theme authors to get notified in time etc. There will be some stress added especially for the cookie opt-in but it's something that is needed 100% unfortunately.

As for the patch if you want to update it that would be a lot of help atm so thanks in advance. Sorry that I didn't check for the different types of separators elsewhere in themes to be consistent - I thought that since there where none in the footers already we wouldn't mind having the same across themes :D oh well !

#49 @laurelfulford
6 years ago

I added the above spacing adjustments in 43715.7.diff, and updated Twenty Sixteen's styles so the RTL footer now looks like:

https://cldup.com/h0-hq1yCwr-3000x3000.png

@xkon Looks like we're concurrently commenting on this one! :D

As for the patch if you want to update it that would be a lot of help atm so thanks in advance. Sorry that I didn't check for the different types of separators elsewhere in themes to be consistent - I thought that since there where none in the footers already we wouldn't mind having the same across themes :D oh well !

No worries! This is the kind of stuff I find having more than one set of eyes on helps a lot! We all spot different things :) Thank you for digging in and doing the bulk of the work on this one!

Speaking of seeing different things, if anyone sees any weirdness with the above patch, please comment!

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

This ticket was mentioned in Slack in #gdpr-compliance by desrosj. View the logs.


6 years ago

#51 follow-up: @iandunn
6 years ago

I tested 43715.7.diff on some WordCamp sites, to get some real-world data. WordCamp.org doesn't allow customizing the markup, though, only the CSS, so this wouldn't reveal any issues related to modified markup in child themes.

Of the 89 sites I checked, 27 had some kind of problem (30%). My initial feeling is that none of them are blockers, but it'd be really nice if we can find a way to mitigate the worst of them.

Common Problems

  1. By far the most common root problem was custom CSS targeting .site-info a. Previously the proudly powered... link was the only thing there, so this didn't have any side-effects. With the privacy link and span, though, it causes various issues.

    We could possible move the privacy link outside of site-info, but that might be worse semantically, and would probably cause some other issues, although they might be less bad.

    Regardless, it might be helpful to add a class to the proudly-powered links for all themes, so that it can be targeted specifically, rather than relying on .site-info a.

  2. The second most common problem was that the .site-info span[role="separator"]::before rules weren't applied on some sites, so the / was missing and the span had a width of 0. This is an odd one, and I haven't determined the cause yet. It's not that the rules were applied, and then overwritten by something else, they just don't exist in the DOM inspector. I'll need to dig into it some more.

  3. The third common problem was that #colophon had a dark background color set with custom CSS, and TwentyThirteen's default text color is also dark. A corresponding color wasn't set, because previously only links were inside the element, which have their own color properties. I don't have any ideas on improving the situation here.

Details

I've linked to the sites below for reference, but the problems don't show up on production right now, since WordCamp.org is running the 4.9 branch. To reproduce, you'll need to manually insert the markup & CSS with dev tools, or wait until 4.9.6-beta1 is tested and deployed to production (assuming there aren't any bugs that prevent that).

TwentyEleven

Tested 9 sites, no issues found.

TwentyTwelve

Tested 20 sites,

TwentyThirteen

Tested 20 sites.

TwentyFourteen

Tested 10 sites

TwentyFifteen

Tested 10 sites

TwentySixteen

Tested 10 sites

TwentySeventeen

Tested 10 sites.

#52 in reply to: ↑ 51 ; follow-up: @iandunn
6 years ago

  • Component changed from General to Bundled Theme
  • Focuses rtl added

Replying to iandunn:

  1. The second most common problem was that the .site-info span[role="separator"]::before rules weren't applied on some sites

It turns out that's because Jetpack's Custom CSS module and WordCamp.org's Remote CSS plugin both support a "replace" mode, where the theme's stylesheet is completely removed, so that only the custom CSS exists.

So, missing those styles is actually the expected behavior, and I don't think there are any reasonable mitigations for that from Core's perspective.

#53 in reply to: ↑ 52 ; follow-up: @xkon
6 years ago

Replying to iandunn:

It turns out that's because Jetpack's Custom CSS module and WordCamp.org's Remote CSS plugin both support a "replace" mode, where the theme's stylesheet is completely removed, so that only the custom CSS exists.

So, missing those styles is actually the expected behavior, and I don't think there are any reasonable mitigations for that from Core's perspective.

Ah nope! I think you're correct, any 'custom' things should be handled with well extra custom things as well :D . That would be the case as well for any live site out there that might end up having fully customized footers etc.

But to be honest, that is something that could happen with any theme update that introduced new things, I'd still wouldn't worry much about 1 extra link that is not by default enabled as well.

#54 in reply to: ↑ 53 @iandunn
6 years ago

Replying to xkon:

But to be honest, that is something that could happen with any theme update that introduced new things, I'd still wouldn't worry much about 1 extra link that is not by default enabled as well.

I get what you're saying, but I feel like we should try our hardest to not cause any problems. If it weren't for GDPR, I'm not sure that a change like this would ever be added to an existing default theme. It helps a lot that nothing happens until the user creates a privacy page, and the link is in the footer, which isn't seen as often as other parts of the page, but it still bothers me that some users will experience problems, and that lots of theme devs will have to do extra work.

I think backwards compatibility is one of the best things about WP -- maybe one of the top 5 reasons for it's success. Making sure that updates -- especially minor ones that are installed automatically -- don't cause any problems is very important to maintaining user's trust in the update process, which has broader effects for things like security.

@iandunn
6 years ago

Add imprint class to "Proudly powered by WordPress" link

#55 @iandunn
6 years ago

Privacy Link Placement

I played around w/ moving the privacy link outside of site-info, but that was worse than the current placement. I don't have any other ideas of how to mitigate the effects of custom CSS targeting .site-info a under the assumption that there's only 1 link, and no other elements.

43715.8.diff adds class="imprint" to all the Proudly powered by WordPress links, though, so that at least going forward people will be able to target both links individually.

i'm not sure that "imprint" is the best term, but I spent an embarrassing amount of time trying to find a fancy, semantic publishing term that would fit, and that's the closest I came up with. I'm very open to any other suggestions.

To kickstart brainstorming, here's some others I considered:

  • platform
  • generator
  • powered-by
  • powered-by-wordpress
  • shout-out

"Powered by..." formatting

I also broke the "powered by..." links into multiple lines, because they were pretty long -- one was 266 characters! -- and I find that very difficult to work with. I think that's kosher according to the guidelines and precendent, but let me know if anyone sees a reason to leave them like they were.

Next steps

I think this is pretty close to being ready for commit. We can iterate some more on the above If anyone has any feedback or ideas, but if not then it might be at the point where we can commit it and see what comes up during beta testing.

This ticket was mentioned in Slack in #gdpr-compliance by desrosj. View the logs.


6 years ago

This ticket was mentioned in Slack in #core-committers by iandunn. View the logs.


6 years ago

#58 @iandunn
6 years ago

In 43051:

Bundled Themes: Add link to privacy policy page in footer.

If a privacy policy has been set, then a link to it will automatically be shown in the footer.

The element containing the "Proudly powered by WordPress" link was chosen for the new policy link, in order to minimize visual conflicts with custom CSS that was written before the new link existed. Unfortunately, some minor conflicts are expected and unavoidable. Adding this link is required as part of GDPR compliance, and the benefits outweigh the downsides.

To further mitigate the conflicts, a new imprint class was added to the "Proudly powered..." link, in order to facilitate targeting each link invididually with custom styles.

Props xkon, laurelfulford, birgire, azaozz, iandunn.
See #43715.

#59 @iandunn
6 years ago

  • Keywords has-patch needs-testing removed
  • Resolution set to fixed
  • Status changed from assigned to closed

When committing r43051, I thought this would need to stay open for a bit longer, but now I don't think there's anything left to do here. I created #43915 for releasing new theme versions alongside 4.9.6.

This ticket was mentioned in Slack in #gdpr-compliance by allendav. View the logs.


6 years ago

#61 @desrosj
6 years ago

  • Component changed from Bundled Theme to Privacy

Moving to the new Privacy component.

This ticket was mentioned in Slack in #core by iandunn. View the logs.


6 years ago

#63 @iandunn
6 years ago

In 43294:

Bundled Themes: Add link to privacy policy page in footer.

If a privacy policy has been set, then a link to it will automatically be shown in the footer.

The element containing the "Proudly powered by WordPress" link was chosen for the new policy link, in order to minimize visual conflicts with custom CSS that was written before the new link existed. Unfortunately, some minor conflicts are expected and unavoidable. Adding this link is required as part of GDPR compliance, and the benefits outweigh the downsides.

To further mitigate the conflicts, a new imprint class was added to the "Proudly powered..." link, in order to facilitate targeting each link invididually with custom styles.

This was accidentally not backported to the 4.9 branch before the beta/RC phase, but there was a consensus that it is safe to do that this late in the release cycle.
See https://wordpress.slack.com/archives/C02RQBWTW/p1526577643000132.
See https://wordpress.slack.com/archives/C02RQBWTW/p1526580781000240.

Props xkon, laurelfulford, birgire, azaozz, iandunn.
Merges [43051] to the 4.9 branch.
See #43715.

Note: See TracTickets for help on using tickets.