web: apply `classes` and `style` before all other attributes and properties (#1788)

* web: apply `classes` before all other attributes and properties

This mitigates "Flash of unstyled content"

* apply style {} before attr(...). Add more tests

* use val instead of var for classes

Co-authored-by: Oleksandr Karpovich <oleksandr.karpovich@jetbrains.com>

web/core/src/jsMain/kotlin/org/jetbrains/compose/web/attributes/AttrsScope.kt

@@ -47,6 +47,8 @@ interface AttrsScope<TElement : Element>: EventsListenerScope {
      *      style { maxWidth(5.px) }
      * })
      * ```
+     *
+     * `attr("style", ...)` overrides everything added in `style { }` blocks
      */
     fun style(builder: StyleScope.() -> Unit) {
         styleScope.apply(builder)
@@ -57,8 +59,10 @@ interface AttrsScope<TElement : Element>: EventsListenerScope {
      *  This method acts cumulatively, that is, each call adds values to the classList.
      *  In the ideology of Composable functions and their recomposition one just don't need to remove classes,
      *  since if your classList is, for instance, condition-dependent, you can always just call this method conditionally.
+     *
+     *  `attr("class", ...)` overrides everything added using `classes(...)` calls
      */
-    fun classes(vararg classes: String)= prop(setClassList, classes)
+    fun classes(vararg classes: String)
 
     fun id(value: String) = attr(ID, value)
     fun hidden() = attr(HIDDEN, true.toString())
@@ -144,6 +148,18 @@ open class AttrsScopeBuilder<TElement : Element> : AttrsScope<TElement>, EventsL
     override val propertyUpdates = mutableListOf<Pair<(Element, Any) -> Unit, Any>>()
     override var refEffect: (DisposableEffectScope.(TElement) -> DisposableEffectResult)? = null
 
+    internal val classes: MutableList<String> = mutableListOf()
+
+    /**
+     * [classes] adds all values passed as params to the element's classList.
+     *  This method acts cumulatively, that is, each call adds values to the classList.
+     *  In the ideology of Composable functions and their recomposition one just don't need to remove classes,
+     *  since if your classList is, for instance, condition-dependent, you can always just call this method conditionally.
+     */
+    override fun classes(vararg classes: String) {
+        this.classes.addAll(classes)
+    }
+
     /**
      * [ref] can be used to retrieve a reference to a html element.
      * The lambda that `ref` takes in is not Composable. It will be called only once when an element added into a composition.

web/core/src/jsMain/kotlin/org/jetbrains/compose/web/elements/Base.kt

@@ -54,8 +54,6 @@ private class DomElementWrapper(override val node: Element): DomNodeWrapper(node
     }
 
     fun updateProperties(applicators: List<Pair<(Element, Any) -> Unit, Any>>) {
-        node.removeAttribute("class")
-
         applicators.forEach { (applicator, item) ->
             applicator(node, item)
         }
@@ -81,14 +79,25 @@ private class DomElementWrapper(override val node: Element): DomNodeWrapper(node
 
     fun updateAttrs(attrs: Map<String, String>) {
         node.getAttributeNames().forEach { name ->
-            if (name == "style") return@forEach
-            node.removeAttribute(name)
+            when (name) {
+                "style", "class" -> {
+                    // skip style and class here, they're managed in corresponding methods
+                }
+                else -> node.removeAttribute(name)
+            }
         }
 
         attrs.forEach {
             node.setAttribute(it.key, it.value)
         }
     }
+
+    fun updateClasses(classes: List<String>) {
+        node.removeAttribute("class")
+        if (classes.isNotEmpty()) {
+            node.classList.add(*classes.toTypedArray())
+        }
+    }
 }
 
 
@@ -115,10 +124,11 @@ fun <TElement : Element> TagElement(
             refEffect = attrsScope.refEffect
 
             update {
+                set(attrsScope.classes, DomElementWrapper::updateClasses)
+                set(attrsScope.styleScope, DomElementWrapper::updateStyleDeclarations)
                 set(attrsScope.collect(), DomElementWrapper::updateAttrs)
                 set(attrsScope.collectListeners(), DomElementWrapper::updateEventListeners)
                 set(attrsScope.propertyUpdates, DomElementWrapper::updateProperties)
-                set(attrsScope.styleScope, DomElementWrapper::updateStyleDeclarations)
             }
         },
         elementScope = scope,

web/core/src/jsTest/kotlin/InlineStyleTests.kt

@@ -164,10 +164,12 @@ class InlineStyleTests {
             }) {}
         }
 
-        assertEquals(
-            expected = "<span id=\"container\" style=\"opacity: 0.4; padding: 40px;\"></span>",
-            actual = root.innerHTML
-        )
+        with(nextChild()) {
+            val attrsMap = getAttributeNames().associateWith { getAttribute(it) }
+            assertEquals(2, attrsMap.size)
+            assertEquals("container", attrsMap["id"])
+            assertEquals("opacity: 0.4; padding: 40px;", attrsMap["style"])
+        }
     }
 
     @Test
@@ -181,9 +183,11 @@ class InlineStyleTests {
             })
         }
 
-        assertEquals(
-            expected = "<span id=\"container\" style=\"height: auto;\"></span>",
-            actual = root.innerHTML
-        )
+        with(nextChild()) {
+            val attrsMap = getAttributeNames().associateWith { getAttribute(it) }
+            assertEquals(2, attrsMap.size)
+            assertEquals("container", attrsMap["id"])
+            assertEquals("height: auto;", attrsMap["style"])
+        }
     }
 }

web/core/src/jsTest/kotlin/elements/AttributesTests.kt

@@ -16,9 +16,109 @@ import kotlin.test.Test
 import kotlin.test.assertEquals
 import org.jetbrains.compose.web.testutils.*
 import org.w3c.dom.HTMLInputElement
+import kotlin.test.assertContains
+import kotlin.test.assertTrue
 
 class AttributesTests {
 
+    @Test
+    fun correctOrderOfApplyingClasses() = runTest {
+        composition {
+            Div(attrs = {
+                prop<HTMLDivElement, String>({ element, v ->
+                    assertEquals("c1 c2 c3 c4", element.classList.value)
+                    element.classList.add(v)
+                }, "classFromProperty1")
+
+                classes("c1", "c2")
+                classes("c3", "c4")
+            }) {
+                Text("test")
+            }
+        }
+
+        with(nextChild()) {
+            assertEquals("c1 c2 c3 c4 classFromProperty1", getAttribute("class"))
+        }
+    }
+
+    @Test
+    fun correctOrderOfApplyingClasses2() = runTest {
+        composition {
+            Div(attrs = {
+                // attr rewrites the content of 'class'
+                attr("class", "classSetFromAttr")
+
+                prop<HTMLDivElement, String>({ element, v ->
+                    assertEquals("classSetFromAttr", element.classList.value)
+                    element.classList.add(v)
+                }, "classFromProperty1")
+
+                classes("c1", "c2")
+                classes("c3", "c4")
+            }) {
+                Text("test")
+            }
+        }
+
+        with(nextChild()) {
+            assertEquals("classSetFromAttr classFromProperty1", getAttribute("class"))
+        }
+    }
+
+    @Test
+    fun attrClassOverridesClassesCall() = runTest {
+        composition {
+            Div(attrs = {
+                // attr rewrites the content of 'class'
+                attr("class", "classSetFromAttr")
+                classes("c1")
+            })
+        }
+
+        with(nextChild()) {
+            assertEquals("classSetFromAttr", getAttribute("class"))
+        }
+    }
+
+    @Test
+    fun attrStyleOverridesStyleCall() = runTest {
+        composition {
+            Div(attrs = {
+                // attr rewrites the content of 'style'
+                attr("style", "color: red;")
+                style {
+                    color(Color.green)
+                }
+            })
+        }
+
+        with(nextChild()) {
+            assertEquals("color: red;", getAttribute("style"))
+        }
+    }
+
+    @Test
+    fun propCanSeeAllAttrsSet() = runTest {
+        val attrsCollectedInProp = mutableMapOf<String, String>()
+
+        composition {
+            Div(attrs = {
+                attr("style", "color: red;")
+                attr("class", "c1")
+                prop<HTMLDivElement, Unit>({ e, _ ->
+                    attrsCollectedInProp.putAll(
+                        e.getAttributeNames().associateWith { e.getAttribute(it)!! }
+                    )
+                }, Unit)
+            })
+        }
+
+        assertEquals("color: red;", attrsCollectedInProp["style"])
+        assertEquals("c1", attrsCollectedInProp["class"])
+        assertEquals(2, attrsCollectedInProp.size)
+    }
+
     @Test
     fun copyFromStyleBuilderCopiesCorrectly() {
         val copyFromStyleBuilder = StyleScopeBuilder().apply {
@@ -387,18 +487,31 @@ class AttributesTests {
             }
         }
 
-        assertEquals(
-            expected = "<button class=\"a\" style=\"color: red;\">Button</button>",
-            actual = nextChild().outerHTML
-        )
+        with(nextChild()) {
+            val attrs = getAttributeNames().toList()
+            assertEquals(2, attrs.size)
+            assertTrue(attrs.containsAll(listOf("style", "class",)))
+
+            assertEquals("button", tagName.lowercase())
+            assertEquals("a", getAttribute("class"))
+            assertEquals("color: red;", getAttribute("style"))
+            assertEquals("Button", innerText)
+        }
 
         hasValue = true
         waitForRecompositionComplete()
 
-        assertEquals(
-            expected = "<button style=\"color: red;\" value=\"buttonValue\" class=\"a b\">Button</button>",
-            actual = currentChild().outerHTML
-        )
+        with(currentChild()) {
+            val attrs = getAttributeNames().toList()
+            assertEquals(3, attrs.size)
+            assertTrue(attrs.containsAll(listOf("style", "class", "value")))
+
+            assertEquals("button", tagName.lowercase())
+            assertEquals("a b", getAttribute("class"))
+            assertEquals("buttonValue", getAttribute("value"))
+            assertEquals("color: red;", getAttribute("style"))
+            assertEquals("Button", innerText)
+        }
     }
 
     @Test

web/svg/src/jsTest/kotlin/svg/SvgTests.kt

@@ -12,6 +12,8 @@ import org.jetbrains.compose.web.css.px
 import org.jetbrains.compose.web.svg.*
 import org.jetbrains.compose.web.testutils.*
 import org.w3c.dom.svg.SVGCircleElement
+import org.w3c.dom.svg.SVGElement
+import org.w3c.dom.svg.SVGTextElement
 import kotlin.test.Test
 import kotlin.test.assertEquals
 
@@ -132,16 +134,20 @@ class SvgTests {
     fun svgTextTest() = runTest {
         composition {
             Svg {
-                SvgText("some text", 20, 30, {
+                SvgText("some text", 20, 30) {
                     classes("small")
-                })
+                }
             }
         }
 
-        assertEquals(
-            "<svg><text x=\"20\" y=\"30\" class=\"small\">some text</text></svg>",
-            nextChild<SVGCircleElement>().outerHTML
-        )
+        with(nextChild<SVGElement>().firstChild!! as SVGTextElement) {
+            assertEquals("text", this.nodeName.lowercase())
+            assertEquals(3, this.attributes.length)
+            assertEquals("small", this.getAttribute("class"))
+            assertEquals("20", this.getAttribute("x"))
+            assertEquals("30", this.getAttribute("y"))
+            assertEquals("some text", this.innerHTML)
+        }
     }
 
     @Test
@@ -415,7 +421,7 @@ class SvgTests {
         }
 
         assertEquals(
-            "<svg><symbol id=\"myDot\" width=\"10\" height=\"10\" viewBox=\"0 0 2 2\"><circle cx=\"1px\" cy=\"1px\" r=\"1px\"></circle></symbol><use href=\"myDot\" x=\"5\" y=\"5\" style=\"opacity: 1;\"></use></svg>",
+            "<svg><symbol id=\"myDot\" width=\"10\" height=\"10\" viewBox=\"0 0 2 2\"><circle cx=\"1px\" cy=\"1px\" r=\"1px\"></circle></symbol><use style=\"opacity: 1;\" href=\"myDot\" x=\"5\" y=\"5\"></use></svg>",
             nextChild<SVGCircleElement>().outerHTML
         )
     }