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
Description
Description
Details
Details
| Status | Subtype | Assigned | Task | ||
|---|---|---|---|---|---|
| Resolved | None | T122260 [GOAL] Promote related pages to production (on for everyone) on all wikis | |||
| Resolved | ovasileva | T135030 [GOAL] Roll related pages from beta on mobile web for most wikis | |||
| Resolved | ovasileva | T135044 [EPIC] There should not be significant lag before related pages appear on mobile web | |||
| Resolved | • Nirzar | T135607 Trigger loading related pages using isElementCloseToViewport to avoid visible lag |
Event Timeline
Comment Actions
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?
Comment Actions
@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.
Comment Actions
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.
Comment Actions
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.
Comment Actions
@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.
Comment Actions
@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.
Comment Actions
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?
Comment Actions
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?
Comment Actions
@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?
Comment Actions
Change 307547 had a related patch set uploaded (by Jhobs):
Use mw.viewport to check when to load related pages
Comment Actions
This is scope creep. We estimated changing this without touching the threshold. Please respect the description of this task and change the threshold separately.
Comment Actions
@jhobs do you need any help with this? I see the task has been sitting here for about a day now.
Comment Actions
Change 307961 had a related patch set uploaded (by Jhobs):
Alternate approach to I82dfa78e93f4bb7a8a28d038470265e7fd30c423
Comment Actions
Change 307961 abandoned by Bmansurov:
Alternate approach to I82dfa78e93f4bb7a8a28d038470265e7fd30c423
Reason:
Squashed to https://gerrit.wikimedia.org/r/#/c/307547
Comment Actions
Please review https://gerrit.wikimedia.org/r/#/c/307547. I've squashed the alternative approach.
Comment Actions
Change 307547 merged by jenkins-bot:
Use mw.viewport to check when to load related pages
Comment Actions
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.
Comment Actions
Related pages don't load on safari anymore. Works on chrome. But i think trigger isnt working on ios safari. Can someone verify?
Comment Actions
@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?
Comment Actions
i checked again and it worked this time. i was checking with other pages too but they might not have any related pages