Make WordPress Core

Opened 9 years ago

Closed 9 years ago

Last modified 9 years ago

#34946 closed defect (bug) (fixed)

new comment redirects break anchors in Safari

Reported by: chuckmoulton's profile chuckmoulton Owned by: boonebgorges's profile boonebgorges
Milestone: 4.4.1 Priority: normal
Severity: normal Version: 4.4
Component: Comments Keywords: fixed-major
Focuses: Cc:

Description

WordPress has the following setting:
[x] Break comments into pages with [ 50 ] top level comments per page and the [last] page displayed by default

In previous versions of WordPress, changing that setting resulted in recent comment links appended with "/comment-page-1/" between the end of the post URL and the # anchor tag to the specific comment. This triggers a server redirect to the post URL, stripping "/comment-page-1/" off. Most browsers handle it fine... Safari across all platforms chokes. Safari redirects to the post without the anchor tag, meaning the user is at the top of the page rather than scrolled down to the particular comment.

Original recent comments link:
example.com/2015/12/example-post/comment-page-1/#comment-1234567

Redirects (Firefox, Chrome, IE, every sensible browser):
example.com/2015/12/example-post/#comment-1234567

Redirects (Safari):
example.com/2015/12/example-post/

The logical solution was to uncheck the setting:
[_] Break comments into pages with [ 50 ] top level comments per page and the [last] page displayed by default

Unfortunately, the WordPress 4.4 update changed this. Now the recent comments redirect happens whether or not that setting is checked. This breaks recent comments anchor tags for Safari. Sites with many comments now leave iPhone users forced to scroll to the bottom of the post in order to find the linked to comment.

Obviously, the real problem is Safari, which has been broken for a decade. But the reality is mobile users are a growing part of the WordPress audience... sites can't jettison their iPhone users just because Safari has a bug. Safari isn't likely to fix the bug on their end anytime soon.

Please provide an option for disabling the "/comment-page-1/" redirects.

Attachments (1)

34946.diff (1.2 KB) - added by boonebgorges 9 years ago.

Download all attachments as: .zip

Change History (14)

#1 @ocean90
9 years ago

  • Focuses accessibility removed

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


9 years ago

#3 @boonebgorges
9 years ago

@chuckmoulton Welcome to Trac! Thanks for the detailed bug report.

It's not good that 4.4 resulted in a regression for your specific use case. However, it sounds to me like your use case was a total hack - you shouldn't have to turn off comment pagination just to placate Safari/WP.

I'd like to take a step back and try to solve this in a more sane way. If the problem has to do with *redirection*, then let's try to eliminate the problematic redirections. Can you please give more detail on the user flow that results in the page redirect? What link are you clicking - the permalink as sent in an email notification, in a recent comments widget, or somewhere else? What is the URL of the link? And what is the page that it redirects to? You have sketched it above, but I'm still not 100% clear on the difference between 4.3 and 4.4 in this regard.

#4 @chuckmoulton
9 years ago

@boonebgorges

The recent comments widget puts a sidebar on a WordPress blog with the 20 most recent comments. Clicking on any of those 20 comments results in a different link depending on the configuration.

Configurations:

1) If pre- WordPress 4.4 and "break comments into pages" is unchecked, then this link:
example.com/2015/12/example-post/#comment-1234567

2) If pre- WordPress 4.4 and "break comments into pages" is checked, then this link:
example.com/2015/12/example-post/comment-page-1/#comment-1234567

3) If WordPress 4.4 -- regardless of the "break comments into pages" setting --, then this link:
example.com/2015/12/example-post/comment-page-1/#comment-1234567

When "comment-page-1" is in the link, the WordPress server redirects. Safari drops anchor tags from any server redirect.

Redirect behavior:

A) If (1), then no redirect.

B) If [ (2) or (3) ] and most browsers (Chrome, Firefox, IE, etc.), then this redirect:
example.com/2015/12/example-post/#comment-1234567

C) If [ (2) or (3) ] and Safari, then this redirect:
example.com/2015/12/example-post/

I am trying to avoid (C) because many readers and active participants of the blog use iPhones. Before WordPress 4.4 came out, I could avoid (C) by choosing configuration (1) instead of (2). After the WordPress 4.4 update, I am forced to use configuration (3), which means I get behavior (C).

@boonebgorges
9 years ago

#5 @boonebgorges
9 years ago

  • Keywords needs-testing added
  • Milestone changed from Awaiting Review to 4.4.1

Thanks, @chuckmoulton. These details are very helpful.

There are two things going on here. First, there is a regression in 4.4 that causes get_comment_link() to use the 'cpage=1' query var (or 'comment-page-1', when pretty permalinks are enabled) when comment pagination is disabled. This should absolutely be fixed. Could you please test 34946.diff to be sure that it's resolving the problem as you see it?

The other problem is a broader one. If the canonical comment-page-1 resolution is causing problems, then we should try to avoid using comment-page-1 in permalinks when possible, even when comment pagination is enabled. When the newest comments are shown first, we cannot eliminate comment-page-1; it's a critical piece of the URL. But when *oldest* comments are shown on the first page, it should be possible to eliminate comment-page-1. I tried doing this in 4.4, but ran into some related issues. Maybe it's something to try again in 4.5.

#6 @chuckmoulton
9 years ago

@boonebgorges

I'm sorry, I am not currently in a position to test the change. I don't have a testing server, I'm not the lone decision maker in upgrades/downgrades, and my site has chosen to downgrade to 4.3 for this as well as several other bugs. I reported this particular bug because it affected me the most and I encouraged others to report the other bugs*, but I don't know if they did. Until all the bugs seem acknowledged and resolved, we will be sticking with 4.3.

However, your description above seems quite sensible and based on looking at the code you linked (without running it) I think you're right. If anyone tries your code on a server and links it here, I'd be happy to test it as a user with an iPhone / Safari.

Sorry I can't be of more help. I greatly appreciate your looking into this.

  • Note: The other bugs concern out of order / non-chronological comments from comment replies in the admin console when threaded (nested) comments are not enabled.

#7 @peterwilsoncc
9 years ago

@boonebgorges double checking per comment:5 request, 34946.diff is working for me against trunk.

  • comment pagination enabled: always includes cpage or comment-page-# in link
  • comment pagination disabled: never includes cpage or comment-page-# in link

Tested from the latest comments widgets.

@chuckmoulton which version of Safari are you using? I'm unable to replicate the redirect error in Safari 9.0.2/OSX 10.11.2.

#8 @boonebgorges
9 years ago

  • Keywords needs-testing removed

@peterwilsoncc Thanks so much for testing. It's interesting that you can't reproduce this issue in recent versions of Safari. But the behavior is a regression in the behavior of get_comment_link() in 4.4, regardless of whether the Safari bug exists.

#9 @boonebgorges
9 years ago

  • Owner set to boonebgorges
  • Resolution set to fixed
  • Status changed from new to closed

In 35933:

Omit cpage query var in comment link if comment pagination is disabled.

WP 4.4 changed the way comment pagination is calculated. See #8071. In the
context of get_comment_link(), these changes introduced a regression that
causes cpage (or its pretty-permalink correlate comment-page-x) to appear
in comment links when comment pagination is disabled. The current changeset
fixes the regression.

Fixes #34946.

#10 @boonebgorges
9 years ago

  • Keywords fixed-major added
  • Resolution fixed deleted
  • Status changed from closed to reopened

Reopening for 4.4.1.

#11 @boonebgorges
9 years ago

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

In 35934:

Omit cpage query var in comment link if comment pagination is disabled.

WP 4.4 changed the way comment pagination is calculated. See #8071. In the
context of get_comment_link(), these changes introduced a regression that
causes cpage (or its pretty-permalink correlate comment-page-x) to appear
in comment links when comment pagination is disabled. The current changeset
fixes the regression.

Merges [35933] to the 4.4 branch.

Fixes #34946.

#12 @boonebgorges
9 years ago

In 35936:

After [35934], ensure get_comment_link() test works without shared fixtures.

get_comment_link() test fixtures are shared in trunk as of [35857]. This
change was not backported to the 4.4 branch, so the 4.4 test should not
expect shared fixtures.

See #34946.

#13 @swissspidy
9 years ago

(Using 4.4 trunk, no comment pagination)

I still have to check, but it looks like after submitting a comment, WordPress redirects to .../comment-page-1/#comment-123, which redirects to .../#comment-123. Not a big deal to me, but Safari strips off #comment-123` in the second redirect, so the user isn't taken directly to their comment.

comment-page-1 is not part of the comment links anywhere else, so I'm not sure where this comes from.

EDIT: Looks like my files were outdated, oops.

Last edited 9 years ago by swissspidy (previous) (diff)
Note: See TracTickets for help on using tickets.