Page MenuHomePhabricator

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


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

related pages4.gif (678×389 px, 1002 KB)

Event Timeline

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 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

Squashed to

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

@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 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