News Site - Navigation workload by flashdesignory · Pull Request #167 · WebKit/Speedometer

flashdesignory · 2023-05-19T17:28:43Z

News site built with next.js, using its static export feature.

To visualize the test - stepping through breakpoints:

flashdesignory · 2023-05-19T19:44:30Z

@rniwa - could you try one more time for me?
I am waiting now for that close button, before running the test.

rniwa · 2023-05-19T22:22:19Z

Hm... I'm still hitting this error in Safari midway through the suite:

TypeError: null is not an object (evaluating 'page.querySelector("#close-message-link").click')

flashdesignory · 2023-05-19T22:26:19Z

Hm... I'm still hitting this error in Safari midway through the suite:
TypeError: null is not an object (evaluating 'page.querySelector("#close-message-link").click')

ah - I can't repro on my machine.

bgrins · 2023-05-19T23:13:27Z

+    },
+    tests: [
+        new BenchmarkTestStep("NavigateToUS", (page) => {
+            page.querySelector("#close-toast-link").click();


We discussed this a bit before, but I have been considering if we should to force a layout in between each of these actions within a step. I believe this would create a more realistic workload for this type of test. While opening and immediately closing the navbar will create React and DOM work there won't be any layout required, while in practice a user discreetly clicking the toggle button twice would cause multiple layouts.

An alternative would be to make each step tiny (with a single action in sync time, then taking the forced layout in async time) but I understand there's a practical problem with noise when steps are so short. So I wonder if having a few big steps that run through a bunch of user actions would be a good compromise that allows them to be heavy enough to be more measurable but also not skipping work that would be happening if they were individual steps.

page.querySelector("#close-toast-link").click(); page.layout(); page.querySelector("#navbar-dropdown-toggle").click(); page.layout(); // and so on, possibly containing actions from multiple of the current BenchmarkTestSteps if there is a logical grouping

I'm curious for thoughts from others on the idea (cc @rniwa @camillobruni @smaug----)

Forcing layouts after each click seems like a reasonable compromise to me since splitting work into multiple pieces can also lead to CPU getting throttled down.

@bgrins - just to ensure I am making the right change - instead of multiple test steps, we'd have just one and calling the page.layout() function in between each action?

added page.layout() function, but kept the individual steps for now.

Yes, something like that is what I was thinking

bgrins · 2023-05-22T22:42:11Z

I like the shape of this test and think it adds some good coverage for the benchmark, thanks @flashdesignory! Will give @julienw a chance to make any comments on the code or the generated site itself (https://news-thor-next--speedometer-preview.netlify.app/resources/tentative/newssite/news-next/dist/), although I know there are a number of other items from the repo on his plate so we can discuss as a group on Wednesday how to sequence this if he doesn't have a chance to in the meantime (i.e. if we should just land in tentative and then take any issues as follow ups, or keep it as a PR until then).

julienw

This looks pretty good to me. I just left a few comments, but it's not clear to me what was generated and what was written by you, so feel free to ignore them..

Also I profiled locally and it doesn't seem to me that we're losing any work in the user timing marks. To be honest I'm a bit surprised because this uses React 18, I was expecting things to be more asynchronous :-)

julienw · 2023-05-23T12:29:09Z

+            <div className="article-content">
+                <ul className={`article-list vertical ${display ?? ""}`}>
+                    {content.map((item, index) =>
+                        <li key={`article-list-item-${index}`} className="article-list-item">


Normally we shouldn't use the index as the key, rather we should use something that represents this article. Maybe its URL?
Otherwise React will update everything when the order changes for any reason. But maybe that's not a big deal in this context, because the content doesn't change actually? You know better than me.

(ditto in all other similar cases below)

julienw · 2023-05-23T12:29:59Z

+                        {item.url ? <a href={item.url}>
+                            <ArticleText className="article-content truncate-multiline truncate-multiline-3" text={item.text} type="h3"/></a>


Indentation looks a bit weird here. I think that prettier+eslint ignores this code currently, it would be good to enable it right now.

I'll take a look - thanks for flagging!

julienw · 2023-05-23T12:36:34Z

+        return null;
+
+    const Tag = type;
+    return <Tag className={`${className ?? ""}`}>{text}</Tag>;


Suggested change

return <Tag className={`${className ?? ""}`}>{text}</Tag>;

return <Tag className={className ?? ""}>{text}</Tag>;

or maybe even

Suggested change

return <Tag className={`${className ?? ""}`}>{text}</Tag>;

return <Tag className={className}>{text}</Tag>;

(as when it's undefined or null, then I believe that the attribute just isn't added)

julienw · 2023-05-23T12:44:13Z

+        // Since the navbar is supposed to appear below the menu, we can't use position: fixed, height: 100%.
+        // Therefore we are using 100vh for the height. This function fixes the challenge on mobile, where
+        // 100vh might include the addressbar, ect.


I'm not following completely, but would one of the newer units (such as dvh) be useful here? The support looks good enough, and I'd be interested in seeing this in the benchmark. What do you think?

Yeah, there are newer units, but I think this still represents a larger amount of websites using this approach.
The only downside is that it's using JavaScript to set the custom property in CSS, but that's really common.

It's working and I think it's ok to keep and maybe follow up at a later point with a newer approach.

I'm fine with keeping the current code as well

julienw · 2023-05-23T12:47:50Z

+    const [navItems, dropdownItems] = keys.reduce(
+        (result, key) => {
+            // priority 0 does not show up in nav
+            if (content[key].priority === 0)
+                return result;
+
+            // priority 1 shows up in nav list
+            // priority 2 shows up in nav dropdown
+            result[content[key].priority === 1 ? 0 : 1].push(key);
+            return result;
+        },
+        [[], []]
+    );


optional nit: I think this would be clearer using an object with explicit properties, rather than a tuple.

I could do something like this:

const navItems = []; const dropdownItems = []; keys.forEach(key => { if (content[key].priority === 1) navItems.push(key); else if (content[key].priority === 2) dropdownItems.push(key); });

pushed above snippet for review

julienw · 2023-05-23T13:14:07Z

+    useEffect(() => {
+        setIsSelected(checked);
+    }, [checked]);


It's not clear to me why we need a local state instead of using directly the prop checked.

The toggle component uses a "checked" prop, which will let us assign the initial state, or change the toggle from a parent component.

julienw · 2023-05-23T13:17:28Z

+function App({ Component, pageProps }) {
+    const [render, setRender] = useState(false);
+    useEffect(() => setRender(true), []);
+    return render ? <Component {...pageProps} /> : null;
+}


It's not clear to me what the intent of this is. Have you written this or is that generated?

ah, yes, this tripped me off too.
This next.js app uses static files, and this piece of code ensures that next.js doesn't try to render anything "on the server". With the useEffect, we ensure this code can only run client-side.

julienw · 2023-05-23T13:21:17Z

+    async prepare(page) {
+        await page.waitForElement("#close-toast-link");
+    },


Can you look whether we can have the initial rendering (including rehydration!) in the tests step instead of in the prepare phase? This would be really good for the benchmark IMO.

it's static files, no server-side phase is present in this build.
We should treat it the same as other static apps.

camillobruni · 2023-05-23T17:55:44Z

Looking good from my side, module sync-step length.
We've talked separately about this, and I think it will be good enough to just add a for-loop with some 4 iterations to bump the sync step time closer to 10ms.

flashdesignory · 2023-05-23T18:15:13Z

Looking good from my side, module sync-step length.
We've talked separately about this, and I think it will be good enough to just add a for-loop with some 4 iterations to bump the sync step time closer to 10ms.

Done in the last commit - thanks @camillobruni

Co-authored-by: Kara <kara@users.noreply.github.com>

This reverts commit 372bd7a.

flashdesignory added 30 commits May 4, 2023 19:10

initial build

49e8472

add dist build

9a20fe6

reorg

a9cad4f

add one section for each page

6b81ab5

grid article

51ba22f

paid content

ceccf50

wrapping columns

6c3a34c

fix class

1682879

id for nav list

159e269

add simple test

223b494

fix a11y image

b0477f7

cleanup

012b904

testing scrollIntoView

b0afd98

use native img element

7be9c3c

replace all Image usage

fd9e717

use ArticleImage

01f9f15

scrollTo

de0953a

remove unused helper

a0c89a9

skip link

248466b

use text component

da6f67d

navbar

fa532b6

cleanup

726fd35

remove focus

46db792

fix test name

9144300

center mobile nav

d5f2571

add dropdown

64be523

visual cleanup

09ecf4f

preview article

57f26a4

dropdown collapse

fdfc6ac

add active label to mobile nav

af262f8

wait for close button

3b957a0

flashdesignory added 2 commits May 19, 2023 14:17

remove console log

eb16e2c

Merge branch 'main' into news-thor-next

f48e44a

rniwa reviewed May 19, 2023

View reviewed changes

Comment thread resources/tests.mjs Outdated

remove message click

4a76ac0

rniwa approved these changes May 19, 2023

View reviewed changes

bgrins reviewed May 19, 2023

View reviewed changes

flashdesignory added 2 commits May 19, 2023 16:44

Merge branch 'main' into news-thor-next

4a18564

add page.layout()

dbc4cd0

julienw reviewed May 23, 2023

View reviewed changes

pr feedback

daa4823

bgrins approved these changes May 23, 2023

View reviewed changes

add loop to test

ed6c373

flashdesignory added 3 commits May 23, 2023 12:08

Merge branch 'main' into news-thor-next

31c9ed6

npm format before including news site

012ecbb

update readme

62c4037

kara reviewed May 23, 2023

View reviewed changes

Comment thread resources/tentative/newssite/news-next/README.md Outdated

Update resources/tentative/newssite/news-next/README.md

be5de90

Co-authored-by: Kara <kara@users.noreply.github.com>

flashdesignory force-pushed the news-thor-next branch from 0dc6ee9 to be5de90 Compare May 23, 2023 21:56

update readme

733b976

flashdesignory merged commit 372bd7a into WebKit:main May 23, 2023

flashdesignory deleted the news-thor-next branch May 23, 2023 22:03

bgrins added a commit to mozilla/Speedometer that referenced this pull request May 23, 2023

Revert "News Site - Navigation workload (WebKit#167)"

3f5036a

This reverts commit 372bd7a.

bgrins mentioned this pull request May 24, 2023

CSS Rich Browsing Proposal #175

Open

		{item.url ? <a href={item.url}>
		<ArticleText className="article-content truncate-multiline truncate-multiline-3" text={item.text} type="h3"/></a>

	return <Tag className={`${className ?? ""}`}>{text}</Tag>;
	return <Tag className={className ?? ""}>{text}</Tag>;

	return <Tag className={`${className ?? ""}`}>{text}</Tag>;
	return <Tag className={className}>{text}</Tag>;

Conversation

flashdesignory commented May 19, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

flashdesignory commented May 19, 2023

Uh oh!

rniwa commented May 19, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

flashdesignory commented May 19, 2023

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

flashdesignory May 20, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bgrins commented May 22, 2023

Uh oh!

julienw left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

flashdesignory May 23, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

camillobruni commented May 23, 2023

Uh oh!

flashdesignory commented May 23, 2023

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

flashdesignory commented May 19, 2023 •

edited

Loading

rniwa commented May 19, 2023 •

edited

Loading

flashdesignory May 20, 2023 •

edited

Loading

flashdesignory May 23, 2023 •

edited

Loading