1552596 - Revert dynamic line clamping in favor of performant static line clamping

Status: VERIFIED FIXED

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

Description

[Ed Lee :Mardak]

Attachment: example refresh where scrollHeight = 19

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 (??)

Comment 1

[Ed Lee :Mardak]

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 ?

Comment 2

[GitHub Bugzilla PR Linker]

Attachment: Link to GitHub pull-request: https://github.com/mozilla/activity-stream/pull/5044

Comment 3

[Ed Lee :Mardak]

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.

Comment 4

[Ed Lee :Mardak]

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?

Comment 5

[Andrei Oprea [:andreio]]

Attachment: clampLinesScreenshot.png

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.

Comment 6

[Jessilyn Davis]

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

Comment 7

[Ed Lee :Mardak]

Attachment: 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?

Comment 8

[Tawanda Kanhema]

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

Comment 9

[Ed Lee :Mardak]

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

Comment 10

[Mike Conley (:mconley) (:⚙️)]

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.

Comment 11

[Ed Lee :Mardak]

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.

Comment 12

[Ed Lee :Mardak]

https://hg.mozilla.org/mozilla-central/rev/6dc17248fa2c

Comment 13

[Marius Coman [:mcoman], Ecosystem QA]

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.

Comment 14

[Ed Lee :Mardak]

Attachment: Bug 1552596 - Revert dynamic line clamping in favor of performant static line clamping

Comment 15

[GitHub Bugzilla PR Linker]

Attachment: Link to GitHub pull-request: https://github.com/mozilla/activity-stream/pull/5093

Comment 16

[Ritu Kothari (:ritu) (Inactive, please n-i to RyanVM, jcristau, or pascal)]

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

Comment 17

[Ed Lee :Mardak]

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

Comment 18

[Julien Cristau [:jcristau] (back April 22)]

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

Comment 19

[Julien Cristau [:jcristau] (back April 22)]

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?

Comment 20

[Julien Cristau [:jcristau] (back April 22)]

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

approved for 68.0b11

Comment 21

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/releases/mozilla-beta/rev/23e2a740c60f

Comment 22

[Marius Coman [:mcoman], Ecosystem QA]

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.