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

DocumentHead: Fix #24977

Closed
ockham opened this issue May 21, 2018 · 1 comment · Fixed by #25501
Closed

DocumentHead: Fix #24977

ockham opened this issue May 21, 2018 · 1 comment · Fixed by #25501
Assignees
Labels
Framework [Type] Bug When a feature is broken and / or not performing as intended [Type] Task

Comments

@ockham
Copy link
Contributor

ockham commented May 21, 2018

There are currently at least two issues with DocumentHead (or rather, the Redux state associated with it).

  1. (per DocumentHead: reseting title on tab view changes #23924)

To reproduce:

  1. Navigate to /stats/day. Note the Stats - WordPress.com title.
  2. Click on Weeks. The title is reset to just WordPress.com.

Similar thing happens on comments/all/{site} when switching comment status, and /pages when switching page status.

  1. (per DocumentHead: Prevent resetting the page title at every route change #23961 (comment))

Try the following on master (harder to reproduce on production since you don't have control over when the cache is primed there):

  • Restart Calypso
  • Open a new incognito window
  • Enter view-source:https://wordpress.com/theme/mood (or use curl 😄)
  • Cmd+F canonical
  • Notice there's an occurrence for canonical (<link rel="canoncial" .../>)
  • Reload
  • Cmd+F canonical
  • Notice the (<link rel="canoncial" .../>) is gone.

That's because of the reset of documet-head's link subreducer that ROUTE_SET triggers.


Any fix to these issues also needs to keep the original motivation for #8224 in mind:

This pull request seeks to resolve an issue where document head state is not properly reset when navigating between screens. For example, if while viewing the Reader an unread count is assigned into the title, if I then navigate away from the Reader, that count will become stuck in the title.


I think our strategy for a fix should be roughly as follows:

  1. Remove the dedicated unreadCount handling from the DocumentHead component and state. Arguably, DocumentHead should reflect the contents of the HTML head. unreadCount is simply reflected in the <title />, and it's not a very generic or widely used thing -- I think only the Reader uses it. Hence, I think it's fair to put the burden of managing unreadCount state on the consuming component rather than DocumentHead.
  2. This will allow us to think differently about resetting all DocumentHead props on each ROUTE_SET change -- more specifically, I think it'll allow us to drop that resetting. The title (now including the unreadCount) is guaranteed to be updated by any <DocumentHead /> component or setTitle() call found on whatever route the user navigates to, meaning the unreadCount isn't going to linger (thus preserving the intention of Framework: Reset title (document head) state after navigating #8224). Removing the resets on ROUTE_SET should fix DocumentHead: reseting title on tab view changes #23924.
  3. Removing the resets for link and meta should fix DocumentHead: Prevent resetting the page title at every route change #23961 (comment). It shouldn't matter much if they linger on the client side upon route switch.

/cc @Copons @gwwar @sirreal @a8dar

@ockham ockham added [Type] Bug When a feature is broken and / or not performing as intended Framework [Type] Task labels May 21, 2018
@ockham ockham self-assigned this May 21, 2018
@ockham
Copy link
Contributor Author

ockham commented May 21, 2018

I'll file a PR that removes setUnreadCount for starters.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Framework [Type] Bug When a feature is broken and / or not performing as intended [Type] Task
Projects
None yet
1 participant