Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Bug: auto-generated CSS class name clashing #978

Closed
GustavoSept opened this issue Nov 1, 2024 · 3 comments · Fixed by #996
Closed

Bug: auto-generated CSS class name clashing #978

GustavoSept opened this issue Nov 1, 2024 · 3 comments · Fixed by #996

Comments

@GustavoSept
Copy link
Contributor

Bug

Initial context

I'm building an app that involves rendering the periodic table. I'm new to templ (this is my first project on it).

To Reproduce

I've provided the code in this repo.
Simply run docker compose up -d --build and point a browser to localhost:4444.

Bug Description

The code

Each chemical element is a component. I'm rendering them using this code (notice how, for each sub-div, we're creating a new class):

package components

import (
    "webapp/components/ui"
    "webapp/utils/types"
    "webapp/utils/constants"
    "strconv"
)

css elementWrapperStyle(element types.Element){
    grid-column: { strconv.Itoa(element.ColNum) };
    grid-row: { strconv.Itoa(element.RowNum) };
}

templ Home() {
    @BaseBare("home") {
        <div class="periodicTableGrid">
            for _, element := range constants.PeriodicTable {
                <div class={ elementWrapperStyle(element) }>
                    @ui.ElementBlock(element)
                </div>
            }
        </div>
    }
}

I have a hard-coded Array of Literals containing the periodic table. It looks like this:

var PeriodicTable = []types.Element{
	{Name: "Hydrogen", Symbol: "H", AtomicNumber: 1, Group: 1, Period: 1, Category: "Nonmetal", Color: "#30c93d", ColNum: 1, RowNum: 1},
	{Name: "Helium", Symbol: "He", AtomicNumber: 2, Group: 18, Period: 1, Category: "Noble Gas", Color: "#06bbe8", ColNum: 18, RowNum: 1},
	{Name: "Lithium", Symbol: "Li", AtomicNumber: 3, Group: 1, Period: 2, Category: "Alkali Metal", Color: "#eda30e", ColNum: 1, RowNum: 2},
	
	// Skipping elements for conciseness
	
	// This one will be important later
	{Name: "Thallium", Symbol: "Tl", AtomicNumber: 81, Group: 13, Period: 6, Category: "Post-transition Metal", Color: "#2181bb", ColNum: 13, RowNum: 6},
}

Actual behavior

The issue is that this code consistently generates a single naming clash when templ generates the class for Hydrogen and Thallium specifically.

Here's a comparison of outputs:

Expected Table Actual Table
Expected Actual

And the generated HTML for the buggy table:

Notice how Hydrogen and Thallium have the same class name class="elementWrapperStyle_f781". It is correctly generated for Hydrogen, at position (1,1).

Things I tried

  • Removing Hydrogen: Hydrogen is excluded as expected, and Thallium moves back to its expected position!
  • Altering Hydrogen coordinates: Altering either Hydrogen's ColNum/RolNum (or both), to any coordinate (including 0,0), will move Thallium to its expected position.
    • Moving Hydrogen to 0,0 actually renders the Expected Table properly. But since it's unexpected behavior, I'm not using this solution.
  • Adding a new element to (1,1): Thallium keeps being rendered on (1,1).
  • Removing a random element other than Hydrogen: Thallium keeps being rendered on (1,1).
  • Moving Thallium to another coordinate: Thallium gets moved as expected to the location.
    • If it's a coordinate that already contains an element, they render one on top of the other (which is expected).
  • 🚨 Swapping a random element's coordinate with Thallium 🚨: Thallium gets placed where expected. The other element is rendered on top of Hydrogen.
    • This leads me to believe the problem internally has something to do with the coordinate(13,6) in specific.
    • The generated class name that is clashing both elements in this case is still the same elementWrapperStyle_f781 as the Hydrogen/Thallium case.

Hypothesis

I don't know exactly how templ works internally, as I'm new to it. I suppose this is a Hashing issue when generating the name, hence why it's so consistent on outputting the same result, given the same input.
Am i just unlucky that this particular combination of bytes is outputting the same Hash for two different rows, as long as they have the same coordinates?

To test this hypothesis, I've added a third random element to the generated classes:

css elementWrapperStyle(element types.Element) {
    grid-column: { strconv.Itoa(element.ColNum) };
    grid-row: { strconv.Itoa(element.RowNum) };
    background-color: { element.Color };
}

And this also solves the issue! All elements are rendered where they should (and have a terrible look to them. But this was just to test the hypothesis).
Uggly Table

More info

templ info output
Run templ info and include the output:

(✓) os [ goos=linux goarch=amd64 ]
(✓) go [ location=/home/gustsept/.asdf/shims/go version=go version go1.23.2 linux/amd64 ]
(✓) gopls [ location=/home/gustsept/.asdf/installs/golang/1.23.2/packages/bin/gopls version=golang.org/x/tools/gopls v0.16.2 ]
(✓) templ [ location=/home/gustsept/.asdf/installs/golang/1.23.2/packages/bin/templ version=v0.2.793 ]

Desktop (please complete the following information):

  • OS: Linux (Bookworm Debian 12.7)
  • templ CLI version (templ version): v0.2.793
  • Go version (go version): go version go1.23.2 linux/amd64
  • gopls version (gopls version): golang.org/x/tools/gopls v0.16.2

Observation

I've fixed my issue, by implementing the coordinates directly in the div's style as an attribute, like this:

func elementWrapperStyle(element types.Element) templ.Attributes {
    return templ.Attributes{
        "style": "grid-column: " + strconv.Itoa(element.ColNum) + "; grid-row: " + strconv.Itoa(element.RowNum) + ";",
    }
}

templ Home() {
    @BaseBare("home") {
        <div class="periodicTableGrid">
            for _, element := range constants.PeriodicTable {
                <div { elementWrapperStyle(element)... }>
                    @ui.ElementBlock(element)
                </div>
            }
        </div>
    }
}

This solves my problem for now. But what if I actually needed dozens or hundreds of different CSS class names?

@a-h
Copy link
Owner

a-h commented Nov 9, 2024

I think you hit the limits of the function that's used to calculate the CSS class names.

templ/runtime.go

Lines 257 to 264 in 4f2ce16

func CSSID(name string, css string) string {
sum := sha256.Sum256([]byte(css))
hp := hex.EncodeToString(sum[:])[0:4]
// Benchmarking showed this was fastest, and with fewest allocations (1).
// Using strings.Builder (2 allocs).
// Using fmt.Sprintf (3 allocs).
return name + "_" + hp
}

As you can see, it only uses the 4 chars of the hex-encoded SHA256 hash of the CSS output once any values have been interpolated. Since it's a cryptographic hash, the output value should be evenly distributed regardless of input value, but since it only uses 4 hex chars there's only 65,536 random suffixes, so if a class name has a lot of usage, it's possible to get a clash.

There's a tradeoff between generated HTML output size, and the chance of collision. I think 65,536 is too low. Using 8 instead chars would be a 32 bit uint max value of 4,294,967,295.

IIRC, I originally picked 4 hex values because I looked up random prefixes that were used in Next.js or some other framework.

So, I think the fix for this... is to increase the number 4 to 8, and to add some tests to cover potential clashes to prevent regression.

Happy to take a PR for this, and happy to take a steer on a larger number than 8 from the author of the PR.

@a-h
Copy link
Owner

a-h commented Nov 9, 2024

Cool project by the way!

@GustavoSept
Copy link
Contributor Author

So, essentially I was just unlucky then.

  • 65k (4 hex) naming possibilities means that any two class instance (from the same css block) has 0.0015% chance of conflicting like this.
  • 1M (5 hex) naming possibilities drops that to 0.000095367% chance.
  • 16M (6 hex) naming possibilities drops that to 0.00000596% chance.
  • 268M (7 hex) naming possibilities drops that to 0.000000373% chance.
  • 4.3B (8 hex) naming possibilities drops that to 0.000000023% chance.

Technically, the problem will always be there though.
I've thought about it, and any solution to make it go to 0% would require either a degraded performance (control conflicts on a per request basis), or would be too much of a hassle to implement.

I'll make a PR to bump it to 8 as soon as i can.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants