com-lihaoyi / scalatags

ScalaTags is a small XML/HTML construction library for Scala.
https://com-lihaoyi.github.io/scalatags/
MIT License
757 stars 117 forks source link

Merging classes still broken when using JsDom. #200

Closed bwbecker closed 4 years ago

bwbecker commented 4 years ago

Issue #139 is fixed in in scalatags.Text but not scalatags.JsDom. The following code passes all three tests when the import is import scalatags.Text.all._ but fails all three when the import is import scalatags.JsDom.all._.

import scalatags.JsDom.all._

import utest._

object ScalaTagsTests extends TestSuite {

  val tests = Tests {
    "scalatags" - {
      "should not concatenate classes" - {
        val d = div(cls := "red", cls := "yellow")("body")

        d.toString ==> """<div class="red yellow">body</div>"""
      }
    }
    "should not concatenate classes even when separated by another attribute" - {
      val d = div(cls := "red", id := "testing", cls := "yellow")("body")
      d.toString ==> """<div class="red yellow" id="testing">body</div>"""
    }
    "should not concatenate classes even when one is an option" - {
      val d = div(cls := "red", id := "testing", Some(cls := "yellow"))("body")
      d.toString ==> """<div class="red yellow" id="testing">body</div>"""
    }
  }
}

The fix that I supplied in pull request 152 (back in January, 2017) fixes this case. I'm not familiar enough with the scalatags code base to know whether this is the optimal solution, but it works in the situations I've encountered. Unfortunately, the pull request that was actually applied (182) does not.

Briefly, the solution I proposed originally was to apply the following change to the apply method at line 148 in JsDom:

-          if (!a.raw) t.setAttribute(a.name, v.toString)
+          if (!a.raw) {
+            a.name match {
+              case "class" if t.getAttribute("class") != null =>
+                t.setAttribute("class", t.getAttribute("class") + " " + v.toString)
+              case _                                          => t.setAttribute(a.name, v.toString)
+            }

I'd submit another pull request, but haven't figured out how to build the current version.