Tentative Workload: Embed TodoMVC-React in a big DOM with complex CSS.

[lpardosixtosMs]

As discussed here is the PR for the workload embedding TodoMVC-React in a big DOM with complex CSS.

Included in this PR:

• Complex UI generator. The generated html is a mail-like UI with a big DOM embedded in the sidebar, and styles using adobe's @spectrum-css library.
• Complex CSS generator, 200 matching rules and 200 not matching rules, with every rule triggered by the addition of a new todo element.
• A copy of the TodoMVC-React benchmark with a few modifications: Added class names to list items and view items to trigger the generated CSS, moved the info footer to inside the react component.

Before diving in the review of the code itself I would like us to agree on:

• The structure of the generated DOM (src/react-todomvc/public/index.html).
• The structure of the generated CSS (src/react-todomvc/public/matchingCss.css, src/react-todomvc/public/nonMatchingCss.css).
• The styling tool we are using for the UI (@spectrum-css).

The CLI is a bit hacky copying files from one place to another so it can be built from end-to-end with a single command. Since it's not clear to me what would be the final organization of the project, I didn't spend more time on it.

The hosted version can be found here.

Please look at the README in resources/tentative/complex-static-html for more information about the code and how it is organized.

Impact Analysis available in Embedding TodoMVC-React in a complex DOM (impact analysis) v2

[sulekhark]

Not for this PR, but the way I imagine this would ultimately be structured is:

resources/
  todomvc/
    big-dom-generator/
       ...
       generated.css
    ...

And then each workload would copy the generated HTML into their index.html, and reference to generated.css (possibly via an npm file reference in the package.json to the big-dom-generator folder similar to how we vendor in the common todomvc CSS).

@bgrins, we would be happy to take this up and try to complete it in time for the June code freeze.

[lpardosixtosMs]

The structure of the generated CSS (src/react-todomvc/public/matchingCss.css, src/react-todomvc/public/nonMatchingCss.css).

It's not a strong opinion, but I think I'd prefer to generate a single generated.css file which puts all matching and non matching in one file since that's more likely how this gets deployed in the wild (i.e. people are more like accidentally including non matching styles in a bundle with matching ones)

@bgrins I can do that. Is there any comment on the generated css rules themselves?

[bgrins]

The structure of the generated CSS (src/react-todomvc/public/matchingCss.css, src/react-todomvc/public/nonMatchingCss.css).

It's not a strong opinion, but I think I'd prefer to generate a single generated.css file which puts all matching and non matching in one file since that's more likely how this gets deployed in the wild (i.e. people are more like accidentally including non matching styles in a bundle with matching ones)

@bgrins I can do that. Is there any comment on the generated css rules themselves?

I spoke with some of our layout folks and one thing I think could be improved is having some variety in the CSS properties that get set in the matching rules. It currently only sets background-color which means only one of the properties actually gets applied after selector matching and iteration - in Gecko's case we'd hit this continue over and over.

I expect in practice there would be some more non-overlapping properties set by these rules, so perhaps we could find a way to simulate that. I made a quick trial patch to gen-css.js that iterates over a variety of properties and sets a couple for each rule. I don't see it obviously move scores in any browser, but I think something like this (though not necessarily this exactly implementation as there may be more interesting things to set) might be a more robust/realistic test.

--- a/resources/tentative/complex-static-html/src/big-dom-generator/gen-css.js
+++ b/resources/tentative/complex-static-html/src/big-dom-generator/gen-css.js
@@ -206,6 +206,18 @@ const buildNonMatchingSelector = (depth, index, oldCombinator, selLen, badSelect
     return buildNonMatchingSelector(nextDepth, nextIndex, combinator, selLen + 1, badSelector) + selector + oldCombinator;
 };
 
+const props = [
+    "accent-color",
+    "border-bottom-color",
+    "border-color",
+    "border-left-color",
+    "border-right-color",
+    "border-top-color",
+    "column-rule-color",
+    "outline-color",
+    "text-decoration-color",
+];
+
 // Returns a random 200 matching selectors and 200 non-matching selectors targeted at the todoMVC items.
 export const genCss = () => {
     const matchingSelectors = [];
@@ -220,12 +232,27 @@ export const genCss = () => {
     // Create random color styles. Same color, different opacity.
     // TODO: Choose a better color for the todoMVC theme.
     const matchingCssRules = [];
+    let currentPropIndex = 0;
     matchingSelectors.forEach((selector, i) => {
-        matchingCssRules.push(`${selector} { background-color: rgba(140,140,140,${i / 1000}) }`);
+        let propOne = props[currentPropIndex];
+        currentPropIndex = (currentPropIndex + 1) % props.length;
+        let propTwo = props[(currentPropIndex) % props.length];
+        currentPropIndex = (currentPropIndex + 1) % props.length;
+        matchingCssRules.push(`${selector} {
+            ${propOne}: rgba(140,140,140,${i / 1000});
+            ${propTwo}: rgba(140,140,140,${i / 1000});
+        }`);
     });
     const nonMatchingCssRules = [];
     nonMatchingSelectors.forEach((selector, i) => {
-        nonMatchingCssRules.push(`${selector} { background-color: rgba(140,140,140,${i / 1000}) }`);
+        let propOne = props[currentPropIndex];
+        currentPropIndex = (currentPropIndex + 1) % props.length;
+        let propTwo = props[(currentPropIndex) % props.length];
+        currentPropIndex = (currentPropIndex + 1) % props.length;
+        matchingCssRules.push(`${selector} {
+            ${propOne}: rgba(140,140,140,${i / 1000});
+            ${propTwo}: rgba(140,140,140,${i / 1000});
+        }`);
     });
     return { matchingCss: matchingCssRules.join("\n"), nonMatchingCss: nonMatchingCssRules.join("\n") };
 };

[bgrins]

Beyond that feedback and some UI nitpicks which I sent to you earlier, I'm pretty happy with what's being generated and would like to hear from the other reviewers.

[lpardosixtosMs]

Beyond that feedback and some UI nitpicks which I sent to you earlier, I'm pretty happy with what's being generated and would like to hear from the other reviewers.

Sounds good! @rniwa We are almost done adding the extra popups, do you have any thoughts on the generated CSS?

[lpardosixtosMs]

While the general approach seems sound, I'd like to see more realistic DOM structure. Namely, we should have more popup menus, etc... hidden in DOM tree. It's quite unrealistic for the entire DOM to be mostly visible as is currently the case.

@rniwa Added popovers to most of the buttons and hid them. You can see them in this hosted version

[bgrins]

@lpardosixtosMs one more request on top of #150 (comment) - can we not set the background-color property specifically (or if we do set it make it an alpha variation on the existing color)? The effect it makes with the slightly darker todo area (followed by additional darkness for nested nodes) doesn't match the otherwise realistic looking app.

[lpardosixtosMs]

@lpardosixtosMs one more request on top of #150 (comment) - can we not set the background-color property specifically (or if we do set it make it an alpha variation on the existing color)? The effect it makes with the slightly darker todo area (followed by additional darkness for nested nodes) doesn't match the otherwise realistic looking app.

@bgrins Sure, we'll make it match the existing colors, and we'll probably use the css properties that you suggested instead of bakground-color.

[bgrins]

Thanks for the follow up changes. Overall, I'm happy with this and would like to see it move forward. There are some questions about "how" we land this (i.e. temporarily in a tentative workload or land directly into one or more of the existing todo workloads) - let's discuss options in our upcoming meeting.

[flashdesignory]

general question: we usually have a "dev" script, would that make sense here as well?

[flashdesignory]

nit: something weird happening with the "all" navigation highlight.
Do the styles behave differently within this workload?

These css rules shouldn't interfere with the todo app:

[lpardosixtosMs]

nit: something weird happening with the "all" navigation highlight. Do the styles behave differently within this workload?

These css rules shouldn't interfere with the todo app:

I agree. I added a new class "targeted" to every todoMVC list item and their direct children. These are the only elements that should trigger the generated CSS rules, now every generated CSS rules ends with .targeted and they should not affect any other elements.

[flashdesignory]

@lpardosixtosMs - thanks for making the changes 🥳

Once the ci/linters succeed, this is good from my side.

[lpardosixtosMs]

@lpardosixtosMs - thanks for making the changes 🥳

Once the ci/linters succeed, this is good from my side.

@flashdesignory Thanks! Could you approve the CI workflow again? I had to change /resources/tentative to /resources/tentative/* in .eslintignore because the linter was not running on my machine, is it ok to commit that?

[flashdesignory]

LGTM

[bgrins]

Summarizing our discussion today:

• We should go ahead and merge this PR into tentative. Any final suggestions for content changes can be handled as follow ups.
• Perform some follow-up analysis on the performance impact it has in each browser compared with the existing React test (presumably building on what was done already before the PR was opened)
• There was some back and forth (without a decision) about (a) merging the big DOM into existing Todo workloads vs preserving the small workloads and adding new-new tests and (b) which specific Todo workloads the big DOM should be incorporated into.

[sulekhark]

Summarizing our discussion today:

• We should go ahead and merge this PR into tentative. Any final suggestions for content changes can be handled as follow ups.
• Perform some follow-up analysis on the performance impact it has in each browser compared with the existing React test (presumably building on what was done already before the PR was opened)
• There was some back and forth (without a decision) about (a) merging the big DOM into existing Todo workloads vs preserving the small workloads and adding new-new tests and (b) which specific Todo workloads the big DOM should be incorporated into.

Thanks for summarizing, @bgrins!

[rniwa]

Sigh... this payload is triggering "SecurityError: Attempt to use history.replaceState() more than 100 times per 30 seconds" error in WebKit again.