Closed Bug 1552596 Opened 2 years ago Closed 2 years ago

Revert dynamic line clamping in favor of performant static line clamping

Categories

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

defect

Tracking

()

VERIFIED FIXED
Firefox 69
Iteration:
69.1 - May 13 - 26
Tracking Status
firefox-esr60 --- unaffected
firefox67 --- unaffected
firefox68 + verified
firefox69 --- verified

People

(Reporter: Mardak, Assigned: Mardak)

References

(Regression)

Details

(Keywords: github-merged, regression)

Attachments

(6 files)

The line clamping code from bug 1551359 checks scrollHeight to determine how many lines the normal text would want, but some reason with separatePrivilegedContentProcess=true (default on Nightly), sometimes scrollHeight returns just 1-line of height.

E.g., adding this ref to the header.title for DSCard_DSCard in the bundle.js:

diff --git a/browser/components/newtab/data/content/activity-stream.bundle.js b/browser/components/newtab/data/content/activity-stream.bundle.js
--- a/browser/components/newtab/data/content/activity-stream.bundle.js
+++ b/browser/components/newtab/data/content/activity-stream.bundle.js
@@ -8029,2 +8029,3 @@ class DSCard_DSCard extends external_React_default.a.PureComponent {
     }, this.props.source), external_React_default.a.createElement("header", {
+      ref: h => (h && (h.s = h.scrollHeight) && setTimeout(() => console.log(h.s, h.scrollHeight))),
       className: "title clamp",

sometimes prints 19 48

It looks like the 19 is the unstyled height which then makes some sense in that I see the same 19 if I remove the stylesheet from prerendered/locales/en-US/activity-stream-noscripts.html:

<link rel="stylesheet" href="resource://activity-stream/css/activity-stream.css" />

So… ? First guess is somehow the scripts get loaded too soon (??)

To be clear, I don't see this with separatePrivilegedContentProcess=false which results in https://searchfox.org/mozilla-central/source/browser/components/newtab/prerendered/locales/en-US/activity-stream.html getting loaded. That file has

<link stylesheet>
…
<script bundle.js>

So I guess that ensures the scripts are loaded after styles ?

In the PR, I added a couple nested requestAnimationFrames to avoid forcing many synchronous reflows triggered when each card gets mounted then checking sizing with scrollHeight to then immediately invalidate by setting a line clamp -- repeat reflow for every single child.

The PR reduces on my development machine clamping time from 55.3ms/load to 4.7ms/load. But it does introduce some jank because the clamping is applied on a later frame but that actually happens to avoid the separatePrivilegedContentProcess=true issue of scripts running before styles. Most users won't see the jank anyway as new tabs are preloaded and probably aren't reloading the page.

Assignee: nobody → edilee
Priority: -- → P1
No longer blocks: pocket-newtab-68

andreio, did performance regress significantly with the last nightly 68 export containing bug 1551359? I definitely see profiler CPU time differences, but if reference machine is more disk bottlenecked than CPU, maybe it wouldn't be as critical to get this fix into 68 beta?

Flags: needinfo?(andrei.br92)

Visually the experience is not affected: on the reference hardware all sections are painted at the same time, there is no gradual loading so the user does not observe any jank. The running time of the function is significant though.

Flags: needinfo?(andrei.br92)

Ed to put screen recordings here for Tawanda to review and decide if it's important to uplift into 68.

Question about: is this only impacting not-cached (first load)?

Iteration: --- → 69.1 - May 13 - 26
Flags: needinfo?(edilee)
Attached video 1 frame of long excerpt

kanhema, here's an example of the potential performance fix that uses 10x less CPU (less than 1% regression vs 10% regression) but allows the page to briefly show longer text than desired. For most users, this would only happen on opening a new window which would show about:home as it renders whereas in most cases, a regular new tab will be preloaded and there will be no changing of card size.

Is this an acceptable tradeoff to avoid the performance regression?

Flags: needinfo?(edilee) → needinfo?(tkanhema)

Ed, I think this is an acceptable trade-off considering the gains on performance. The longer text only shows for a split second.

Flags: needinfo?(tkanhema)

[Tracking Requested - why for this release]: There was a ~10% performance regression from bug 1551359 at the end of 68 nightly

Hey Mardak,

Here's where the privileged content process differs from normal content processes:

https://searchfox.org/mozilla-central/rev/6c9f60f8cc064a1005cd8141ecd526578ae9da7a/browser/components/newtab/AboutNewTabService.jsm#148-167

jlim's project from last summer made it so that we pull those scripts from the ScriptPreloader in the event that we're using the privileged content process. It waits until DOMContentLoaded before doing this - and I guess that means that's before the CSS resources have finished loading.

Looks like bug 1551359 dynamic line clamping uncovered some other bugs in addition to the performance issues, so we'll just revert it and get back to static line clamping for 69 and 68.

Summary: Only 1 line shown sometimes when refreshing with separatePrivilegedContentProcess=true → Revert dynamic line clamping in favor of performant static line clamping
Blocks: 1554322
Status: NEW → RESOLVED
Closed: 2 years ago
Keywords: github-merged
Resolution: --- → FIXED
Target Milestone: --- → Firefox 69

I have verified that this issue is no longer reproducible with the latest Firefox Nightly (69.0a1 Build ID - 20190530214830) installed, on Windows 10 x64, Arch Linux and Mac 10.14.5. Now, the lines clamps are displayed after three rows.

Status: RESOLVED → VERIFIED

Hi Ed, please request uplift for Beta68 if you think it's worth shipping this fix in 68.

Flags: needinfo?(edilee)

Comment on attachment 9070330 [details]
Bug 1552596 - Revert dynamic line clamping in favor of performant static line clamping

Beta/Release Uplift Approval Request

  • User impact if declined: Slower loading of Firefox Home content
  • Is this code covered by automated tests?: No
  • Has the fix been verified in Nightly?: Yes
  • Needs manual test from QE?: Yes
  • If yes, steps to reproduce: Open new tab with discovery stream content and make sure card title and description have at most 3 lines each instead of dynamically filling up 6 lines total
  • List of other uplifts needed: None
  • Risk to taking this patch: Low
  • Why is the change risky/not risky? (and alternatives if risky): Reverting a change that landed late in 68 nightly to a previously verified state
  • String changes made/needed: none
Flags: needinfo?(edilee)
Attachment #9070330 - Flags: approval-mozilla-beta?
Flags: qe-verify+

Comment on attachment 9070330 [details]
Bug 1552596 - Revert dynamic line clamping in favor of performant static line clamping

regression fix, verified in nightly, approved for 68.0b10

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

Comment on attachment 9070330 [details]
Bug 1552596 - Revert dynamic line clamping in favor of performant static line clamping

Actually looks like the cherry-pick is waiting on review?

Attachment #9070330 - Flags: approval-mozilla-beta+ → approval-mozilla-beta?
Flags: needinfo?(sdowne)

Comment on attachment 9070330 [details]
Bug 1552596 - Revert dynamic line clamping in favor of performant static line clamping

approved for 68.0b11

Flags: needinfo?(sdowne)
Attachment #9070330 - Flags: approval-mozilla-beta? → approval-mozilla-beta+

I have verified this issue with the latest Firefox Beta (68.0b11 Build ID - 20190617172838) installed, on Windows 10 x64, Arch Linux and Mac 10.14.5. Now, the card title and description have at most 3 lines each.

Flags: qe-verify+
Component: Activity Streams: Newtab → New Tab Page
You need to log in before you can comment on or make changes to this bug.