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
Jhernandez | |
May 18 2016, 11:22 AM |
F3996911: related pages4.gif | |
May 18 2016, 11:25 AM |
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
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 |
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.
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.
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
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.
Change 307961 had a related patch set uploaded (by Jhobs):
Alternate approach to I82dfa78e93f4bb7a8a28d038470265e7fd30c423
Change 307961 abandoned by Bmansurov:
Alternate approach to I82dfa78e93f4bb7a8a28d038470265e7fd30c423
Reason:
Squashed to https://gerrit.wikimedia.org/r/#/c/307547
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
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.
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