From af2c354f72c75302faf1696f8f71e884debb8490 Mon Sep 17 00:00:00 2001 From: Oleksandr Karpovich Date: Mon, 7 Feb 2022 11:43:38 +0100 Subject: [PATCH] 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 --- .../compose/web/attributes/AttrsScope.kt | 18 ++- .../jetbrains/compose/web/elements/Base.kt | 20 ++- .../src/jsTest/kotlin/InlineStyleTests.kt | 20 ++- .../jsTest/kotlin/elements/AttributesTests.kt | 141 ++++++++++++++++-- web/svg/src/jsTest/kotlin/svg/SvgTests.kt | 20 ++- 5 files changed, 184 insertions(+), 35 deletions(-) diff --git a/web/core/src/jsMain/kotlin/org/jetbrains/compose/web/attributes/AttrsScope.kt b/web/core/src/jsMain/kotlin/org/jetbrains/compose/web/attributes/AttrsScope.kt index e4f35bcde2..81d6fd0f67 100644 --- a/web/core/src/jsMain/kotlin/org/jetbrains/compose/web/attributes/AttrsScope.kt +++ b/web/core/src/jsMain/kotlin/org/jetbrains/compose/web/attributes/AttrsScope.kt @@ -47,6 +47,8 @@ interface AttrsScope: 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: 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 : AttrsScope, EventsL override val propertyUpdates = mutableListOf Unit, Any>>() override var refEffect: (DisposableEffectScope.(TElement) -> DisposableEffectResult)? = null + internal val classes: MutableList = 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. diff --git a/web/core/src/jsMain/kotlin/org/jetbrains/compose/web/elements/Base.kt b/web/core/src/jsMain/kotlin/org/jetbrains/compose/web/elements/Base.kt index 5b708c32cb..99c2fd96d5 100644 --- a/web/core/src/jsMain/kotlin/org/jetbrains/compose/web/elements/Base.kt +++ b/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 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) { 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) { + node.removeAttribute("class") + if (classes.isNotEmpty()) { + node.classList.add(*classes.toTypedArray()) + } + } } @@ -115,10 +124,11 @@ fun 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, diff --git a/web/core/src/jsTest/kotlin/InlineStyleTests.kt b/web/core/src/jsTest/kotlin/InlineStyleTests.kt index 7a283bf4bb..4dd01831c5 100644 --- a/web/core/src/jsTest/kotlin/InlineStyleTests.kt +++ b/web/core/src/jsTest/kotlin/InlineStyleTests.kt @@ -164,10 +164,12 @@ class InlineStyleTests { }) {} } - assertEquals( - expected = "", - 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 = "", - actual = root.innerHTML - ) + with(nextChild()) { + val attrsMap = getAttributeNames().associateWith { getAttribute(it) } + assertEquals(2, attrsMap.size) + assertEquals("container", attrsMap["id"]) + assertEquals("height: auto;", attrsMap["style"]) + } } } diff --git a/web/core/src/jsTest/kotlin/elements/AttributesTests.kt b/web/core/src/jsTest/kotlin/elements/AttributesTests.kt index cf2f32e7be..cf848f8eaa 100644 --- a/web/core/src/jsTest/kotlin/elements/AttributesTests.kt +++ b/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({ 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({ 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() + + composition { + Div(attrs = { + attr("style", "color: red;") + attr("class", "c1") + prop({ 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 { @@ -43,17 +143,17 @@ class AttributesTests { classes("a b c") attr("title", "customTitle") - prop({_, _ ->}, "Value") + prop({ _, _ -> }, "Value") - ref { onDispose { } } + ref { onDispose { } } style { width(500.px) backgroundColor(Color.red) } - onClick { } - onFocusIn { } - onMouseEnter { } + onClick { } + onFocusIn { } + onMouseEnter { } } val copyToAttrsScope = AttrsScopeBuilder().apply { @@ -75,7 +175,7 @@ class AttributesTests { val copyToAttrsScope = AttrsScopeBuilder().apply { id("id1") - onClick { } + onClick { } style { width(100.px) } @@ -387,18 +487,31 @@ class AttributesTests { } } - assertEquals( - expected = "", - 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 = "", - 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 diff --git a/web/svg/src/jsTest/kotlin/svg/SvgTests.kt b/web/svg/src/jsTest/kotlin/svg/SvgTests.kt index 7b5c5420c2..9ba368e4a0 100644 --- a/web/svg/src/jsTest/kotlin/svg/SvgTests.kt +++ b/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( - "some text", - nextChild().outerHTML - ) + with(nextChild().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( - "", + "", nextChild().outerHTML ) }