Page MenuHomePhabricator

[bug] related articles /sometimes/ appear in wrong place on articles
Closed, ResolvedPublic3 Estimated Story Points


Steps to reproduce:

related article block appears between Last edited and last section instead of being on the base layer. it looks broken as well.

IMG_1225.PNG (2×1 px, 656 KB)

It's been almost a year we deployed related pages, this was thought to be a caching issue but I still see this behaviour every now and then. anecdotally, once in three.

Feel free to close it if this is still a caching issue. but any explanation in why such issues are not seen with any other UI changes we do, will be helpful.

Developer notes

Update resources/ext.relatedArticles.readMore.bootstrap/init.js so that it only adds the read more widget to the page when the document is ready.

Testing criteria

Go through 30-40 articles and note their related pages section. Related pages should appear at the bottom of the page under "last edited".

Event Timeline

What was the URL? I've also seen this issue. If I recall correctly I noticed it was related to unclosed div's in the article HTML.

Jdlrobson renamed this task from [bug] related articles appear in wrong place on articles to [bug] related articles /sometimes/ appear in wrong place on articles.Feb 15 2018, 6:02 PM
Jdlrobson moved this task from Incoming to Needs Prioritization on the Web-Team-Backlog board.

If I recall correctly I noticed it was related to unclosed div's in the article HTML.

this issue gets fixed if you refresh the page. so it's probably not because of article structure

url is

Have you seen this in any browser other than Chrome?

yes, happens on same article on safari as well

ovasileva triaged this task as Medium priority.Feb 15 2018, 6:22 PM

So this is how the content gets added to the page

if ( $( '.footer-content' ).length ) {
        $( '<div class="read-more-container" />' ).prependTo( '.footer-content' );
} else {
        $( '<div class="read-more-container post-content" />' )
                .insertAfter( '#content' );

So what's happening (for reasons unknown is $( '.footer-content' ).length is being evaluated as 0)
This suggests something weird is happening in the Minerva skin. This element should always exist.. unless the DOM hasn't been completed parsed when the code is run.

This is hopefully a simple case of wrapping this in a document ready.

Let's try that as a starting point?

Niedzielski moved this task from To Do to Doing on the Readers-Web-Kanbanana-Board-Old board.

@ovasileva pulling this into the sprint from sprint+1 because all other tasks are claimed

Change 413193 had a related patch set uploaded (by Niedzielski; owner: Stephen Niedzielski):
[mediawiki/extensions/RelatedArticles@master] Fix: wait to show related articles until ready

This is not easy to replicate so I'm not sure if it's possible to easily QA this.

Change 413193 merged by jenkins-bot:
[mediawiki/extensions/RelatedArticles@master] Fix: wait to show related articles until ready

a weird QA method would be going to random articles and seeing the footer. the current frequency is very high. you would at least find 1/5 articles with broken footers. with this change, finding even one in 10-20 would mean we didn't fix it. all we need is one and it's easy to find. unless there are multiple issues causing this and we fixed one of them which I would find very fascinating

This can be tested on the beta cluster now. If we need production to do this, this will need to stay open until next week when the change goes live.

Let's wait until it's live since we'll have a larger sample to test on.

ovasileva added a subscriber: ABorbaWMF.

over to you @ABorbaWMF, this will be ready to test Thursday, March 1st. Testing criteria in the description

Hey @ABorbaWMF - I meant this will be ready to test on production Thursday, March 1st. Do you think you could go through a number of articles (maybe 20-30?) on production to confirm? This bug is a bit odd to catch, hence the need for more test articles.

Looking good on production as well. Tested on a variety of browsers/OSs.

Thanks @ABorbaWMF! Looks like we're all done here.