feat: NS class selectors

[Confidenceman02]

fixes #134

Context

Svg and Html elements vary in the way css classes are applied. Elm sets svg classes by using the attribute "class" and other html elements by using the property "className". This follows the html/javascript specs and is a generally known quirk.

Let's see how Elm handles both the Svg.Html.class and Html.class functions.

Svg.Html.class

class : String -> Attribute msg
class =
  Elm.Kernel.VirtualDom.attribute "class"

Html.class

class : String -> Attribute msg
class =
  stringProperty "className"

Because of these differences, using class selectors in tests, specifically for svg elements, causes tests to fail. This is due to the class selectors triggering internal helpers that are looking for elements with the value "className".

classnames : Facts msg -> List String
classnames facts =
    Dict.get "className" facts.stringAttributes
        |> Maybe.withDefault ""
        |> String.split " "

Because "className" is not a valid property on svg elements we get the failing tests.

        test "find class on svg element" <|
            \() ->
                let
                    svgClass =
                        "some-NS-class"
                in
                Svg.svg
                    [ SvgAttribs.class svgClass ] []
                    |> Query.fromHtml
                    |> Query.has [ class svgClass ]
-- -> THIS FAILS, BUMMER!

Work completed

• Create NS class selectors
• Write tests for all NS selectors
• Write tests for attribute selectors with Svg elements
• Write tests for class selectors with Svg elements
• Create NS internal helpers
• Update elm package versions

NS specific selectors now map to internal helpers that know how to extract class information from svg elements.

        test "find class on svg element" <|
            \() ->
                let
                    svgClass =
                        "some-NS-class"
                in
                Svg.svg
                    [ SvgAttribs.class svgClass ] []
                    |> Query.fromHtml
                    |> Query.has [ classNS svgClass ]
-- -> THIS PASSES, WOW!

[Confidenceman02]

@rtfeldman fix as promised.

If I set my pedanto meter really high, I think there is a valid argument to move these selectors into their own Svg.Html.Selector namespace but perhaps it is not required.

I also noticed the MakeTestRegistry.hs elm package versions are not the latest and greatest. Would you like me to bump those?

Keen to get your feedback on this implementation.

[rtfeldman]

Can this lower bound be 1.0.0 instead of 1.0.1?

[Confidenceman02]

Done 👍

[rtfeldman]

I'd rather we not update this file if possible, just to make sure everything still works with the older versions - to minimize the chances that these changes cause regressions for anyone! 😄

[Confidenceman02]

Done 👍

[rtfeldman]

I think there is a valid argument to move these selectors into their own Svg.Html.Selector namespace

Given how much these selectors will overlap with the Html selectors (e.g. if I'm testing SVG things, I'll want to import the Html ones anyway) I think the current approach seems better.

@tesk9 What do you think? Also, any feedback on the PR in general?

[rtfeldman]

I also noticed the MakeTestRegistry.hs elm package versions are not the latest and greatest. Would you like me to bump those?

I'd prefer to keep this change as minimal as possible, so let's not!

[tesk9]

Initially on reading through, I felt a little unsure about the API. I wasn't sure if users would get that NS-means-SVG, especially since the module name is Test.Html.Selector. I wouldn't necessarily expect to have svg-specific helpers in a module with Html in the name, you know? So I did a little exploration of what info we currently have, to see if I could suggest an alternate approach to changing the Selectors.

I printed out what is produced when running tests with

Svg.svg [ SvgAttribs.class svgClass ] [  ]
    |> Query.fromHtml

and

Html.div [ Attr.class someClass ] []
    |> Query.fromHtml

I found the results to be mostly the same, expect for the attribute name:

SVG node entry:

NodeEntry {
    children = [],
    descendantsCount = 1,
    facts = {
        attributeNamespace = Nothing,
        boolAttributes = Dict.fromList [],
        events = Dict.fromList [],
        stringAttributes = Dict.fromList [("class","some-NS-class")],
        styles = Dict.fromList []
    },
    tag = "svg"
    }

HTML node entry:

NodeEntry {
    children = [],
    descendantsCount = 0,
    facts = {
        attributeNamespace = Nothing,
        boolAttributes = Dict.fromList [],
        events = Dict.fromList [],
        stringAttributes = Dict.fromList [("className","some-class")],
        styles = Dict.fromList []
    },
    tag = "div"
    }

I think what this means is that we can change the Query inernal code instead of the Selector API code!

I'm making a PR off of this one. Let me know what you think!

[tesk9]

I'm not sure that we should support exactClassName on SVGs. Might lead to more confusion with respect to bugs like https://ellie-app.com/gsphpMkJVN5a1 ?

[Confidenceman02]

Hmm.. I don't quite understand? Could you elaborate? exactClassNameNS simply queries the full class attribute on the svg which could consist of multiple classes, not sure how that pertains to the bug you linked.

[Confidenceman02]

I rambled here, then I realised you had a PR which addressed most of my ramblings so edited out most of said ramblings! 😆

Initially on reading through, I felt a little unsure about the API. I wasn't sure if users would get that NS-means-SVG, especially since the module name is Test.Html.Selector. I wouldn't necessarily expect to have svg-specific helpers in a module with Html in the name, you know?

I had the same intuition and this is very easily fixed by treating these selectors as Svg specific. i.e. Test.Svg.Html.Selector.
I know @rtfeldman wasn't totally keen on this. Considering Svg's in elm are treated similarly, consumers of this package don't have to do too many mental gymnastics to accept if you are importing and testing Svg.Html or Svg.Html.Attrbute then logically you would find those selectors as Test.Svg.Html.Selector. This would also mean Svg selectors are discoverable when viewing the package documentation on package.elm-lang.

I think this is analogous to the Html packages and how people import and use them and outweighs the convenience of having them all live in Test.Html.Selector.

However, it would likely be just the class selector that is problematic so I can see the argument against this.

I found the results to be mostly the same, expect for the attribute name:

I think what this means is that we can change the Query internal code instead of the Selector API code!

I 100% encourage exploration of this idea.

• Changing the internal query without exposing svg specifics selectors means that we would be asserting that a Html class selector works for both svg and html, which is not actually how Html works in elm. So we would be hiding the limitation in a way. Maybe this is good? As we know, setting a class on an svg element with a Html.Attribute.class throws an exception, and thus the question is, should it fail in Test also? Perhaps more verbosely than it currently does? I appreciate that the Test selectors are an abstraction to actual attributes so this could all be ramblings of a mad man.

If we reflected Elm's special treatment of svg classes, might that make testing clearer?

[tesk9]

I'm not opposed to API changes in general, but I would want people who worked on the dom portion of the package to be pulled in before anything major changes.

I also think that the key problem here doesn't necessarily require API changes to fix.

My thinking is the key problem is that there are 2 ways to set the class, and the elm test will only find classes set one these 2 ways. (I assert that it's a separate problem that there's an error if you try to set a className on an SVG node.)

I think a solution that doesn't fix these tests:

suite : Test
suite =
    describe "class attribute assertions"
        [ describe "Using Html.Attribute.class" <|
            classAssertions <|
                Html.div [ Attr.class "some-custom-class" ] []
        , describe "Using Html.Attribute.property" <|
            classAssertions <|
                Html.div [ Attr.property "className" (Json.Encode.string "some-custom-class") ] []
        , describe "Using Html.Attribute.attribute" <|
            classAssertions <|
                Html.div [ Attr.attribute "class" "some-custom-class" ] []
        ]


classAssertions : Html.Html msg -> List Test
classAssertions html =
    [ test "Test.Html.Selector.class finds the class" <|
        \_ ->
            html
                |> Query.fromHtml
                |> Query.has [ Selector.class "some-custom-class" ]
    , test "Test.Html.Selector.classes finds the class" <|
        \_ ->
            html
                |> Query.fromHtml
                |> Query.has [ Selector.classes [ "some-custom-class" ] ]
    ]

isn't solving the problem of the Query module losing track of HTML classes.

(It's not clear to me whether exactClassName has className in the name because it's supposed to be about the className property. Depending on what the intent is, it might make sense for it to be in the classAssertions list too. In my PR, I treated exactClassName as being about the method of setting the className. I could see an argument either way.)

I agree that SVGs are related to the problem (in that you can only set classes on them in the way that elm-test can't yet test for unless you want errors in the browser), but again I think the core problem is the attribute/property distinction.

I'll add these tests explicitly to my PR.

[Confidenceman02]

It's a good point, I was viewing exactClassName as "the class name" not "the value of className property" even though weirdly, both are correct. I came to this thinking because the selectors relating to classes are Selector.class and Selector.classes respectively, not Selector.className or Selector.classNames.

Furthermore, if we leave out exactClassName, we essentially lose the ability to assert the entire class value i.e. "class1 class2 class3 class4" but only for svg which doesn't make intuitive sense in my opinion. My understanding is exactClassName works the same as the other class selectors, the only difference being it doesn't String.split the value.

Ultimately, I would rather leave the current working implementations of Html selectors alone and instead mimic those behaviours for NS elements specifically. I think this provides the least chance peoples tests will break due to something we don't know, and directly addresses issue #134 and nothing more.

I'm not opposed to API changes in general, but I would want people who worked on the dom portion of the package to be pulled in before anything major changes.

The API isn't really changing, it's more so being added to, and adding these changes shouldn't cause a major change, it's a minor change because no publicly exposed function is changing?

[Confidenceman02]

@rtfeldman Any chance we could get some direction here? Would be incredibly sad for this to go stale.

[rtfeldman]

Sorry, had a lot of other stuff on my plate recently - I'll circle back to this over the weekend!

[rtfeldman]

if we leave out exactClassName, we essentially lose the ability to assert the entire class value i.e. "class1 class2 class3 class4" but only for svg

🤔 What if we changed it to pass if either the attribute or the property is present and an exact match?

I can think of a downside to having them separate: getting confused when your query says it found no matches even though the property (or attribute) is clearly set, the problem is just that you were querying for the other one...but I'm having trouble thinking of what bugs would be caught by having them separate.

All the scenarios I can come up with seem very unrealistic - e.g. someone sets both class and className on the same element, but they set them both to different strings, and then use the wrong type of query, which happens to cause them to look more closely at the attributes and realize they've mistakenly set both. It's hard for me to think of something that narrow as being a significant downside.

Maybe there's a scenario I'm not thinking of, though!

[dinopascale-prima]

Hey @Confidenceman02 @rtfeldman! Thank you for your work with this PR. Is there a reason why it was not merged?

[Janiczek]

@dinopascale-prima The package has changed maintainers a few times over the past few years. I'll try and take a look at merging this sometime soon, thanks for the ping!