Make x-text work on SVG elements

[philippbosch]

The x-text directive sets innerText on the element. The problem is that innerText is only available on elements based on HTMLElement but not SVGElement which is the base for e.g. the <text> SVG element.

So instead of blindly setting el.innerText we now first check if it's actually an HTML element and otherwise fall back to setting el.textContent which is part of the Node interface that all elements are based on.

Based on the discussion in #734, this PR swaps innerText out for textContent because they behave identically (at least when setting the value) and the latter is part of the Node interface that all elements – HTML and SVG – are based on.

Fixes #734

[HugoDF]

There's some other failing tests that were checking innerText, a search and replace of innerText to textContent should fix them.

[philippbosch]

@HugoDF I'm on it. There's much more to it.

[philippbosch]

So, I changed all the tests to work on textContent instead of innerText, and they all pass now.

There are three kinds of changes to the tests:

1. Substitute innerText with textContent

   - await wait(() => { expect(document.querySelector('span').innerText).toEqual('bar') })
   + await wait(() => { expect(document.querySelector('span').textContent).toEqual('bar') })

   This one is straighforward and a no-brainer.

2. Additional change of type from int to string

   - expect(document.querySelector('span').innerText).toEqual(0)
   + expect(document.querySelector('span').textContent).toEqual('0')

   This kinda makes sense that because the thing is called textContent what you get from it is text, or a string.

3. Array to string conversion

   - expect(document.querySelector('span').innerText).toEqual(['foo', 'bar'])
   + expect(document.querySelector('span').textContent).toEqual('foo,bar')

   This is also understandable, because ['foo', 'bar'].toString() === 'foo,bar' but it has the potential to throw people off when reading the test code.

I am torn about whether this is what we want. Especially no. 3 is really confusing I find.

One idea I had about that was that instead of testing for equality (expect(el.textContent).toEqual('bar')) we could have a custom Jest matcher specifically for testing textContent (expect(el).toHaveATextContentOf(['bar'])). This way we could hide the fact that the value is converted to a string and still allow people to pass in non-string values.

What do you think? @HugoDF @ryangjchandler @SimoTod

[ryangjchandler]

@philippbosch - I personally think that having a custom matcher is a bit overkill. The tests are pretty clear in what they do, since we are actually making assertions against a string so it's all regular JavaScript behaviour, I can't see any need for the custom matcher. See what the other guys think though.

[SimoTod]

My 2 pence: I think the current tests are not real because jest doesn't support innerText. If you use it in a real browser x-text="['foo','bar']will print `foo,bar' not the js array.
Ideally we should use your tests because they are better.
textContent is also more performant but it only affect us marginally since we don't use it to read values.
That being said, the amount of changes to tests may require a longer review. Maybe ask Caleb directly and if he doesn't want to change those tests, you can re-PR the initial version.

[philippbosch]

@calebporzio Here's the tl;dr:

I want to make x-text work on SVG elements which is currently broken because SVGElements don't have an innerText property. But there is also the textContent property which all elements have and which behaves identically and is actually standardized. So my approach was s/innerText/textContent/g. But after that tons of tests were broken. Turns out that jsdom does not implement innerText at all (jsdom/jsdom#1245) so the existing tests that work on innerText are not realistic because if they ran in a real browser they would break – mainly because real innerText (as well as textContent) always returns a string. For example, this one would break because the value would be "0" instead of the expected 0:

alpine/test/if.spec.js

Line 92 in 12b2da3

expect(document.querySelector('span').innerText).toEqual(0)

I propose to adjust all the tests and swap out innerText for textContent and make the necessary changes to the expected values like I have done in e72737f. It's a lot of changes but I'm conviced it's the right thing to do. What do you think?

[calebporzio]

Totally agree. Good PR.

.textContent ftw!

[HugoDF]

found 2-3 instances where the test didn't need to be updated, I think it's fine to update them to be fair, just thought I would point them out.

[philippbosch]

@HugoDF yeah, I saw these but thought it made sense to also change them for the sake of consistency

[HugoDF]

@HugoDF yeah, I saw these but thought it made sense to also change them for the sake of consistency

Makes sense