Page MenuHomePhabricator

Last modified bar css regression
Closed, ResolvedPublic

Description


It seems a change [1] to the last modified HTML was made in stable without proper care for caching.

git checkout f221ccc666795a0fb724a3e845b1dbf26da8fcd0
git checkout -b ohnobutyolo
./dev-scripts/cachedpage.sh 6d588b23ddab1a2cb1514d97018a8213063153ad San_Francisco

(Where San Francisco is an existing article and the commit is the one before this change)
Open the page and you'll see that the last modified bar is unstyled.

I noticed this whilst anonymous on stable. This may be riding the train I'm not sure.

[1] https://gerrit.wikimedia.org/r/#/c/210841

Event Timeline

Jdlrobson raised the priority of this task from to High.
Jdlrobson updated the task description. (Show Details)
Jdlrobson moved this task to Incoming on the Readers-Web-Backlog board.
Jdlrobson added a subscriber: Jdlrobson.
Restricted Application added a subscriber: Aklapper. · View Herald TranscriptMay 26 2015, 3:08 PM

@phuedx @JKatzWMF more drama caused by this new main menu redesign :-/.... better get some eyes on it asap

Jdlrobson updated the task description. (Show Details)May 26 2015, 3:09 PM
Jdlrobson set Security to None.

I cannot reproduce this. The page has been recently edited (so the cache is cleared), but other pages which have not been recently edited don't have this problem. For example: [1]. Screenshot attached.

[1] http://en.m.wikipedia.org/wiki/Jondor?mobileaction=stable

Jdlrobson added a comment.EditedMay 26 2015, 9:07 PM

The description has steps to reproduce it. Basically generate the static page and then fix the stylesheet to support it.

You cannot reproduce it easily on beta labs as you are viewing pages that are not cached.

Note that appending mobileaction query string parameter will also bypass the cache as will being logged in. Best to produce locally via the script.

I've also followed the steps you have written; but no luck. Everything looks alright. Also why are you not looking at the master branch? Maybe the issue you mention has been fixed since the commit that introduced the change.

Jdlrobson added a comment.EditedMay 27 2015, 7:43 AM

I'm still seeing this problem on http://en.m.wikipedia.beta.wmflabs.org/wiki/Headings and locally whilst anonymous with JS enabled (JS disabled is fine).

I'm worried you are not understanding the core problem here and that is because I'm explaining it badly so let me try again just in case (apologies if you know all this but I want to make sure we are able to avoid these issues in future and fix them when issues arise).

HTML is cached for 30 days on the wiki cluster. So if you can change the HTML the resulting effect can be a page is served using old HTML with up to date JavaScript and CSS. It's a really important thing to think about when developing on our site, so please if you don't understand anything ask me questions.

  1. Simulate old HTML

You can run this script

./dev-scripts/cachedpage.sh 6d588b23ddab1a2cb1514d97018a8213063153ad San_Francisco

It basically fetches the San Francisco article for the commit 6d588b23ddab1a2cb1514d97018a8213063153ad which is before the breaking change.

It's the same as doing

git checkout 6d588b23ddab1a2cb1514d97018a8213063153ad
in your browser navigating to view-source:http://127.0.0.1:8888/w/index.php/Headings?mobileaction=stable
pasting the HTML markup in a file that you can serve on your local machine

We store it in a static file as we don't have caching on local machines and the HTML generated will always be new.

  1. Viewing the breakage

Now if you revert back to master, you will have up to date JavaScript and CSS
You can visit your older HTML in the cached page on the same local host and the URLs will now point to newer resources. You'll be able to see any breaking visual changes very straightforwardly.

  1. Fixing it

Taking a look at the HTML, this is the HTML markup that is in the page and it is unstyled:

<a id="mw-mf-last-modified" data-timestamp="1432669192" href="/w/index.php/Special:History/Headings" data-user-name="Admin" data-user-gender="unknown" class="truncated-text">Last modified on 26 May 2015, at 19:39</a>

Is this clear?
To save you time here is the cached HTML I have generated that we need to make our styles compatible with:
https://gist.github.com/jdlrobson/478fb05eeff6a2c51870

Thanks @Jdlrobson for writing this up. As you stated, it works fine when JS is disabled, and in the HTML gist you gave JS is disabled, but then you say that HTML doesn't look right. This contradiction makes things confusing.

Also the headings link in labs that you gave has recently been updated which cleared the cache and I can't see what you saw earlier anymore. I tried looking at random pages as an anonymous user in labs that haven't been updated recently. For example [1] (which was updated 2 years ago) and it looks fine. I would really appreciate it if you can show me some pages in labs that I can see the issue on. Thanks.

[1] http://en.m.wikipedia.beta.wmflabs.org/wiki/WeightedLink1379310060_2#/random

Change 214113 had a related patch set uploaded (by Bmansurov):
Fix the last modified bar regression for cached pages

https://gerrit.wikimedia.org/r/214113

Jdlrobson added a comment.EditedMay 29 2015, 7:03 AM

Okay I looked again at your change. It's a complicated one, which is difficult to read and grok the head round. Now I've looked close I see that you are fixing two issues here - one being the truncated text not working and the caching issue.

Forgive the breadth of this comment - a lot of it is for me, but also to explain my review rationale in case someone like @phuedx wants to review it while I'm out today.

So from what I can see if A was the patch that introduced the regression and C is your patch this is the markup we are playing with:

Pre (A):

		<a id="mw-mf-last-modified"
			data-timestamp="" class="last-modified-bar truncated-text">

Post (B):

		<div class="last-modified-bar">
			<div id="mw-mf-last-modified">
				 <a class="truncated-text" data-timestamp=""></a>
			</div>
		</div>

Your current patch (C):

		<div class="last-modified-bar">
			<div id="mw-mf-last-modified" class="truncated-text">
				 <a data-timestamp></a>
			</div>
		</div>

So basically we need to turn both A and B into C.
It seems that you're code block if ( isPageCached ) { turns A into C.
Confusingly for B isCachedPage is false (even though it's cached) and the line $lastModified.addClass( 'truncated-text' ); runs on both B and C (on C not changing the DOM at all to correct this)

So I think your patch works but it needs a little clean up. Please see my latest comments.

Change 214113 merged by jenkins-bot:
Fix the last modified bar regression for cached pages

https://gerrit.wikimedia.org/r/214113

phuedx closed this task as Resolved.Jun 2 2015, 9:36 AM

Change 215983 had a related patch set uploaded (by Kaldari):
Fix the last modified bar regression for cached pages

https://gerrit.wikimedia.org/r/215983

Change 215983 merged by jenkins-bot:
Fix the last modified bar regression for cached pages

https://gerrit.wikimedia.org/r/215983