Closed Bug 1573206 Opened 1 year ago Closed 1 year ago

DiscoveryStream lazy load card html

Categories

(Firefox :: New Tab Page, enhancement, P1)

enhancement

Tracking

()

VERIFIED FIXED
Firefox 70
Iteration:
70.3 - Aug 5 - 18
Tracking Status
firefox69 - verified
firefox70 --- verified

People

(Reporter: thecount, Assigned: thecount)

References

(Blocks 1 open bug)

Details

(Keywords: github-merged)

Attachments

(2 files)

If we use intersection Observer on some of the html cards, the below the fold cards don't need to render until the user scrolls.

I saw some benefits from doing this in the profiler, and also did some screen recordings and got pretty good results: https://docs.google.com/document/d/1VsvhJ3eLcRhpOYLb7AR9S58QIjD7jK3ZvpPOH5nOzoQ/edit

[Tracking Requested - why for this release]: I think the code is simple enough and the wins big enough to consider this for uplift.

Assignee: nobody → sdowne
Priority: -- → P1
Summary: DiscoveryStream lazy load some card html → DiscoveryStream lazy load card html

Doesn't sound like this is significant enough to warrant tracking for 69, but I'm still happy to consider an uplift request when the time comes if the patch isn't risky.

Comment on attachment 9085298 [details]
Uplift 1573206 - A lazy render change

Beta/Release Uplift Approval Request

  • User impact if declined: Performance increase on newtab.
  • Is this code covered by automated tests?: Yes
  • Has the fix been verified in Nightly?: No
  • Needs manual test from QE?: Yes
  • If yes, steps to reproduce: To test:
  1. Go to newtab.
  2. Inspect a card, and look for these: <div class="ds-card">
  3. Scroll down a bit.

Expected: Initially, any cards above the fold should be <div class="ds-card"> and any below the fold should be <div class="ds-card placeholder"> and be empty (so they render fast) . As you scroll, they should update with their content.

  • List of other uplifts needed: None
  • Risk to taking this patch: Low
  • Why is the change risky/not risky? (and alternatives if risky): Once verified in nightly, it's low risk.

The patch is pretty small, has tests, and the method used to do this is similar to another method we use before rendering images and has been working great.

  • String changes made/needed: none
Attachment #9085298 - Flags: approval-mozilla-beta?
Flags: qe-verify+
Blocks: 1574334
Status: NEW → RESOLVED
Iteration: --- → 70.3 - Aug 5 - 18
Closed: 1 year ago
Keywords: github-merged
Resolution: --- → FIXED
Target Milestone: --- → Firefox 70

Comment on attachment 9085298 [details]
Uplift 1573206 - A lazy render change

New Tab Page perf improvement. Approved for 69.0b15.

Attachment #9085298 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
QA Whiteboard: [qa-triaged]

Confirmed lack of patch with 69.0b14.
Verified enhancement with 70.0a1 (2019-08-18) on Windows 10, macOS 10.13, Ubutnu 16.04.

Leaving qe+ flag up for the the beta15 build.

Status: RESOLVED → VERIFIED

Looking at 69.0b15 the structure is with lists ( <li class="card-outer"> [...] </li> ) instead of the ds-card + div(s) one.

Flags: needinfo?(sdowne)

Marking as verified after changing the browser.newtabpage.activity-stream.discoverystream.enabled pref to true for beta.
The placeholder class and structure are in order.

Flags: qe-verify+
Flags: needinfo?(sdowne)
You need to log in before you can comment on or make changes to this bug.