Page MenuHomePhabricator

Trigger loading related pages using isElementCloseToViewport to avoid visible lag
Closed, ResolvedPublic2 Story Points

Description

Use upstreamed method isElementCloseToViewport instead of custom code when detecting when to load Related Articles. Do not modify the trigger threshold.

See blocked task for example of the problem T135044

Details

Related Gerrit Patches:
mediawiki/extensions/RelatedArticles : masterUse mw.viewport to check when to load related pages
mediawiki/extensions/RelatedArticles : masterAlternate approach to I82dfa78e93f4bb7a8a28d038470265e7fd30c423

Event Timeline

Restricted Application added subscribers: Zppix, Aklapper. · View Herald TranscriptMay 18 2016, 11:22 AM
Jhernandez updated the task description. (Show Details)May 18 2016, 11:25 AM

I think the threshold should be 60% of the height of the device and not fixed amount. for an average 4" phone, it would be around 600px-700px

Also, what is the threshold we use for lazy loading images?

@Nirzar There's a task open for making this same improvement to lazy loading images because there's lag that can show too.

Regarding height, I could agree with you, but device height can change, and polling the window height triggers layout thrashing (see here), so if we do it we have to be smart about sharing somewhere the window dimensions to avoid performance issues.

Related task T133565 about doing the same with the images.

hmm.. so to have better performance, we will have to hardcode this value? if that's the case lets go ahead with the current description of this task.

Jdlrobson triaged this task as Medium priority.May 19 2016, 5:08 PM
Jdlrobson added a project: Readers-Web-Backlog.

For this rather mobile-centric case, a pragmatic one time calculation (e.g., at page load) seems a simple way to deal with the common case. It's unclear to me if listening in on https://api.jquerymobile.com/orientationchange/ could allow a live update to the threshold without thrashing.

@dr0ptp4kt we don't use jquery mobile, but normal jquery. We can listen to debounced resize events if necessary, I'd like to avoid it though. A fixed number can give us the improvement without adding performance overhead.

@Jhernandez, got it. I knew that we don't use jquery mobile, but somehow failed to notice the page that I lined to was jquery mobile! Yeah, a fixed number sounds like a fine workaround.

Jdlrobson updated the task description. (Show Details)Aug 22 2016, 4:36 PM
Nirzar updated the task description. (Show Details)Aug 22 2016, 4:45 PM
MBinder_WMF set the point value for this task to 2.Aug 22 2016, 4:48 PM
jhobs updated the task description. (Show Details)Aug 22 2016, 4:52 PM
jhobs moved this task from To Do to Doing on the Reading-Web-Sprint-80-V-for-Vandalism board.

For the record, the current implementation puts the scroll threshold at $( document ).height() / 2 - $window.height(). As the task is written, we should leave the threshold at what it is, but the comments suggest we should use a hardcoded value for now instead to prevent layout thrashing. I'm inclined to go with the latter. Thoughts?

I was going to suggest to change it to simply

$( document ).height()

time to load those cards are higher even on faster connections, we need more threshold.

pardon my ignorance but what is layout thrashing? guessing it has to do something with calculating value on the fly. can we use height of an average mobile device then?

@Nirzar Layout thrashing. TL/DR: Constantly recalculating layout unnecessarily. I would agree using a hardcoded height of an average mobile device is probably the way to go. What's your value for that, 600px?

Change 307547 had a related patch set uploaded (by Jhobs):
Use mw.viewport to check when to load related pages

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

This is scope creep. We estimated changing this without touching the threshold. Please respect the description of this task and change the threshold separately.

@jhobs do you need any help with this? I see the task has been sitting here for about a day now.

I suggest we talk about this post standup tomorrow.

Change 307961 had a related patch set uploaded (by Jhobs):
Alternate approach to I82dfa78e93f4bb7a8a28d038470265e7fd30c423

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

bmansurov claimed this task.Sep 2 2016, 5:45 PM
bmansurov added a subscriber: jhobs.

Change 307961 abandoned by Bmansurov:
Alternate approach to I82dfa78e93f4bb7a8a28d038470265e7fd30c423

Reason:
Squashed to https://gerrit.wikimedia.org/r/#/c/307547

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

Please review https://gerrit.wikimedia.org/r/#/c/307547. I've squashed the alternative approach.

Change 307547 merged by jenkins-bot:
Use mw.viewport to check when to load related pages

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

bmansurov removed bmansurov as the assignee of this task.Sep 2 2016, 7:06 PM
phuedx added a subscriber: phuedx.Sep 5 2016, 4:50 PM

The change has been deployed to the Beta Cluster. Y'all can test it on any page, e.g. https://en.m.wikipedia.beta.wmflabs.org/wiki/Black-backed_jackal.

phuedx assigned this task to Nirzar.EditedSep 5 2016, 4:50 PM

@Nirzar: Give it a spin and let us know what you think.

Related pages don't load on safari anymore. Works on chrome. But i think trigger isnt working on ios safari. Can someone verify?

@Nirzar I've tested it in https://en.m.wikipedia.beta.wmflabs.org/wiki/Black-backed_jackal with desktop Safari and (simulator)iphone 5 ios 9.3 Safari and it seems to load just fine.

Do you have any more details?

i checked again and it worked this time. i was checking with other pages too but they might not have any related pages