DOM Element descriptor interface for test helpers

[bendemboski]

rendered

[bendemboski]

For reference, here is a PR I opened in qunit-dom. I've since realized that this would ideally apply to @ember/test-helpers as well, and should therefore be a more generalized and carefully thought out interface.

[rwjblue]

Thanks for working on this!

[rwjblue]

Should link to #726

[rwjblue]

Can you rename the file to 0726-....md ?

[rwjblue]

We discussed this generally in discord chat, and I think I'd like to go a slightly different direction:

function retrieveElementDescriptor<T extends object>(obj: T): IElementDescriptor;
function associateElementDescriptor<T extend object>(obj: T, retriever: retrieveElementDescriptor<T>);

This allows us to use (essentially) a single system and fits in with the other APIs that we have (e.g. associateDestroyableChild).

[buschtoens]

We could also make use of this in Ember's Application class rootElement property.

Some questions that popped up for me, not sure, if in scope though:

What if a descriptor matches multiple elements, but the function it is passed to expects a single element?

How would a "live" descriptor work, that defers looking up the element(s) until asked to? By implementing element / elements as getters?

In the case of selector strings, could it be useful to retain the selector string in the descriptor, e.g for debugging purposes or re-evaluation?

[bendemboski]

We could also make use of this in Ember's Application class rootElement property.

Good point, I hadn't thought of that!

What if a descriptor matches multiple elements, but the function it is passed to expects a single element?

Element descriptors have roughly the same mental model as selectors -- they don't inherently choose between describing one matched element or all matched elements and, like selectors today, it's up to the semantics of the API method to decide what to do with them -- pass a selector to querySelector() or querySelectorAll() and act on the result, or access the element or elements property of an element descriptor and act on the result. A great illustration of this is qunit-dom's assert.dom() that accepts a selector, but allows you to call .hasClass(), in which case it acts on the first matched element, or .exists({ count: 3 }), in which case it acts on all matched elements.

So descriptors just expose element and elements and consumers choose which to access based on the context.

How would a "live" descriptor work, that defers looking up the element(s) until asked to? By implementing element / elements as getters?

Yes, exactly. Like selectors, the primary use case I have in mind is "live" descriptors that can describe something that may or may not be rendered at the time the descriptor is constructed. But making the interface use properties instead of methods makes it very simple to implement static/ad-hoc descriptors as well, e.g. { element, elements: [element] }.

In the case of selector strings, could it be useful to retain the selector string in the descriptor, e.g for debugging purposes or re-evaluation?

Yes, that's the intent behind the description field in IElementDescriptor and my assumption is that most of the time element descriptors will have a selector as their description because it's by far the most common ways to refer to DOM elements. But fractal-page-object, for example, supports list indexing, so the descriptions could end up looking like ui li[1] .checkbox. But I do expect that we'll end up with some kind of function makeSelectorDescriptor(selector: string) that will use the selector as the description.

[ro0gr]

I'm super excited to see this RFC! Thanks for doing it!

I've left some comments from the ember-cli-page-object maintainer's standpoint.

[ro0gr]

I've just checked the "qunit-dom" API, and seems there are only 2 methods that support many items, if I'm not missing something, these are exists( and isVisible(.

This alignes well with my personal testing experience. Most of the time you are interested to interact or assert against exactly 1 element. If test methods forgives you an unintended multiple selection, but do only select the first element for assertion, it could lead to some mistakes in tests. This is the reason, why in the ember-cli-page-object the "find-exactly-one-element-or-fail" approach is leveraged actively.

That being said, I think exposing elements at this point, is not necessarily a good thing to do. Maybe we could reduce the scope of the RFC and implementation for now, by sacrificing the elements, and focus on a minimal but the most common flow for a single element with the description?

[bendemboski]

A couple of thoughts:

1. While there are only 2 methods that support multi-element matching, in my experience it's a pretty common usage. I personally do some variant of assert.dom('.list-item').exists({ count: 3 }) quite often. When using page object libraries it may be less common because you would just do assert.equal(page.listItems.length, 3), but that results in less informative assertion messaging (because assert only knows it was given two Numbers, and nothing about the selector used to match the elements). I think that's not a big problem, but one of the goals of this RFC is to enable further convergence on qunit-dom, with all its well thought out semantics and messaging, even when using page objects.
2. I'm reluctant to scope this down to only single-element scenarios, because then the integration with qunit-dom gets a lot more difficult to think about. At the point that you call assert.dom(...) we don't know if you're planning to do a single-element access, a multi-element access, or both! For example, assert.dom('.thing').exists({ count: 1 }).hasClass('foo'). So if we were passing objects to assert.dom() that could only provide single elements, I'm just not sure how we'd make that work. I guess qunit-dom would have to throw an error at the time that the user calls .exists({ count }), and that seems pretty far from ideal.

I am very interested in making sure this proposal would support your "find-exactly-one-element-or-fail" and I think it does? If I understand it, you do that enforcement when you are doing the element lookup, right? So I believe you could implement your page objects to produce element data whose element getter verifies that the page object's selector only matches one element, and whose elements getter unconditionally throws an error (assuming multiple: true wasn't set).

[ro0gr]

IMO the multiple elements topic can be tricky sometimes. It might even have some extra design. We've deprecated the multiple in the ember-cli-page-object just because it overcomplicated internals, w/o any value (cause everything is covered with collections). Unfortunately, I don't have a single good answer for you, so let me drop some raw thoughts here.

1. Let's consider your example modified to have count: 3:

 `assert.dom('.thing').exists({ count: 3 }).hasClass('foo')`

I'd expect it to evaluate hasClass over each of 3 elements(is it just me? 😄). But documentation says:

assertion will return true if any of the element's CSS classes match.

However, (here, I have to admit I'm not an active qunit-dom user at the moment so I may miss smth) if I read the hasClass implementation correct, it just checks only for the first element, and this is unexpected to me. Here I just want to highlight a non-obvious hidden complexity of the MANY thing, so even in this small use case, I am not sure, as a user, what to expect when it comes to a mutliple thing, and we have 3 possible scenarios to handle it.

2. The other question I have is, whether each elements item needs it's own description. I'm not currently sure, how would I use it, but I can imagine, I would want to specify an order for each element in the list, so the final error message would have an exact index per each failed element(let's imagine for a moment hasClass iterates over each element).

3. Also it just feels a little bit undermenistic to me to put both single element and many elements in the same descriptor. For instance, in terms of the ember-cli-page-object, I think I'd prefer to return an element for a node and elements for collection. I mean, we may want to distinct between the single VS multiple descriptiors. This one is not a big deal to me, but the need for a mixed approach is not clear to me.

I am very interested in making sure this proposal would support your "find-exactly-one-element-or-fail" and I think it does? If I understand it, you do that enforcement when you are doing the element lookup, right?

Yes, you are right. This RFC doesn't block this behaviour in any way. I've mentioned it to tell that the element semantics is much more obvious, and it's simpler to reason about, at least for me.

---

That being said, I'm not strongly opposed to the elements in general. I'm just uncertain if the current design is good enough. And unfortunatelly for now, we have too few use cases for it, so it seems tricky to reason about the real requirements. Maybe we can invent a syntetic test helper for demonstrational purpose(RFC only), which will do something more than just exists. Something like hasClass iterating over all the elements, with error/success messages? I think it may help us to design the thing with more considerations in mind.

[bendemboski]

Yeah, I definitely agree with you that multi-element semantics are kinda complex and often not completely clear, and your count: 3 example is a good illustration of that.

The documentation is a little confusing, but I think accurate as long as you're a bit of a grammatical pedant 😆 any of the element's CSS classes refers to any of the CSS classes of the (singular) element. any of the elements' CSS classes would refer to any of the CSS classes of the (plural) elements. So the documentation already assumes that there's only one element we're talking about, which I think of/refer to as a single element context.

But yes, this is a good illustration of the ambiguity and confusing-ness of single vs. multiple elements. But that has its root in a very broad ecosystem-wide pattern, which is CSS selectors. We use selectors all over the place to reference the DOM, but they don't encode any information about whether they reference a single element or multiple elements -- it depends on whether you pass them to querySelector() or querySelectorAll().

So any DOM helper that accepts a selector string as an argument embraces this ambiguity, and either solves it with implicit behavior (@ember/test-helpers and most of qunit-dom implicitly look for the first match), explicit behavior (those two qunit-dom methods accept an argument that tells them if they are in single-element mode or multi-argument mode), or via enforcement (e.g. ember-cli-page-object interprets them as single-element references and enforces the validity of that with an assertion).

This RFC is not seeking to address that, because that would involve breaking changes to qunit-dom, but also because I think it's fundamentally un-addressable because this RFC is seeking to "extend" CSS selectors, and CSS selectors are fundamentally ambiguous as far as referencing one element or multiple elements.

I think this also hides the ambiguity from most end users, because they will never see the element and elements properties. They'll probably use page objects that automatically integrate with DOM helpers, and that they will use in the same way they use CSS selectors today, or else they'll use the factory helpers described in this RFC and will probably 95% of the time pass in a single element to the factory function without needing to know about multi-element support.

[bendemboski]

So that's a bunch of reasons, but the bottom line is that I tried to mirror how CSS selectors work and are used, in large part because we have such a limited set of use cases, so I prioritized parity over re-imagining anything. Specifically:

• a descriptor is equivalent to a CSS selector
• accessing .element on the descriptor's data is equivalent to calling document.querySelector() on a CSS selector
• accessing .elements on the descriptor's data is equivalent to calling document.querySelectorAll() on a CSS selector
• passing a descriptor to a DOM helper is equivalent to passing a CSS selector to a DOM helper, with exactly the same ambiguities

Maybe that also helps to answer The other question I have is, whether each elements item needs it's own description. To make that work, I'd expect something outside of this RFC. For example, in fractal-page-object, if you were to do the odd assert.dom(page.listItems).hasClass('foo') the description would be something like .list-item because that's the selector corresponding to the page.listItems node. If you did assert.dom(page.listItems[2]).hasClass('foo') the description would be something like .list-item[2] because that's the a decent selector-ish representation of the list item and index 2. So I'm not currently thinking of multi-element scenarios as operating on the contents of the list of Elements, but as operating on the list of Elements itself (e.g. .exists({ count }) is asking how long the list is, not anything about the elements in the list).

So, rather than using a single descriptor to reference multiple elements and give each of them their own description, I'd expect a consumer to construct a descriptor, with description, for each element -- the difference between "talking about" a list and talking about the contents of the list.

[bendemboski]

That's my thinking here. I do want to pause and give you a big thanks for putting time and thought into this! I'm still feeling pretty confident that descriptors need to embrace the single/multi element ambiguity so they are fundamentally at parity with CSS selectors, but I'm only medium confident that we've got the right design for that in this RFC, so appreciate challenges/ideas/etc.

[ro0gr]

Let's say we have a page object collection with the ul > li selector for items.

Now coming back to the hypothetical hasClass example, when passing the whole collection:

  assert.dom(MyCoolList.items).exist({ count: 2 }).hasClass('some-class');

if the second item fails to satisfy the condition, It'd be nice to see as pricese context as possible in the error message, like:

does not have a class "some-class" for:

Page Object: MyCoolList.items[2]
Selector "ul > li[2]"

(ya! this is a bit overwhelmed with context 😅 )

I think if we don't have a description per element, we can't have such a level of control in the error messages, and we can only have, smth like:

does not have a class "some-class" for "ul > li"

Through there is not such a thing like multiple hasClass yet, I hope my example makes sense for you.

[bendemboski]

Yeah, that makes sense. Although in that case it would be qunit-dom that would be implementing the iteration over the elements, so I would think that it would be its job to generate a useful message based on the description of the whole collection plus its knowledge of the indices of the elements, e.g.

class 'some-class' not present on 'ul > li' collection at index 2

where it gets ul > li from the descriptor and constructs the rest of the message itself. That way it could also do

class 'some-class' not present on 'ul > li' collection at index 2, 4, and 5

whereas if the descriptor provides a description for each element including the index, all qunit-dom could do is

class 'some-class' not present on 'ul > li[2]', 'ul > li[4]', and 'ul > li[5]'

which could quickly get very hard to read if the selector was something like .page .content-section .accordion-frame[2] ul > li 😅 . If one description is provided for the whole collection, then since the DOM helper that's using the collection knows the index of each item as it iterates, it could construct either of the messages above, whereas if we make descriptors provide a per-element description, then the DOM helpers have less flexibility in their messaging.

We could certainly do both -- have descriptors provide a description for the collection, and optionally provide a parallel array of descriptions or a function that is given an element and returns a description. But this could also be added later once we have gather more usage data to determine if it would be helpful. I think either way we'd want a whole-collection description so exists({ count }) can output expected 2 elements in 'ul > li' collection but found 3 or whatever, so I guess I'm proposing that we defer the per-element description question until we have more data indicating a need for it.

[ro0gr]

Yes, I agree about the qunit-dom responsibility here. Thanks for pointing me to it!

[NullVoxPopuli]

what's the status of this? like a tldr, what's left / remaining / etc?

[bendemboski]

@NullVoxPopuli I'm not super familiar with the RFC process, so not sure what needs to be done there. I have been working on an implementation of the RFC, an @ember/test-helpers branch adding support for descriptors, an equivalent qunit-dom branch, and a fractal-page-object branch making PageObjects IDOMElementDescriptors.

I've run into a few issues that may or may not require/motivate adjustments to this RFC:

Element array vs. iterable

The RFC currently types DescriptorData#elements as Element[], and I think it should change to Iterable<Element> since it follows a querySelectorAll()-like pattern (and seems likely to be a wrapper around some form of querySelectorAll() much of the time).

The resolveDOMElements() helper should still flatten to an array for nice ergonomics, but the descriptor interface itself should deal in iterators to require less data transformation by descriptor implementations, and leave open the option to use iterators in performance-sensitive scenarios, etc.

IDOMElementDescriptor type safety

I'm finding myself a little un-satisfied with IDOMElementDescriptor being an empty interface. Since it's an empty interface, any object can be implicitly cast to it, which seriously limits the help the compiler/typechecker can provide. Here's a contrived example:

class Page extends PageObject {
  date = selector('.date');
  listItems = selector('li');

  // some data parsed out of the list items
  get data(): Record<string, string> {
    let data: Record<string, string> = {};
    for (let li of this.listItems) {
      let [key, value] = li.element.textContent.split(' ');
      data[key] = value;
    }
    return data;
  }
}
let page = new Page();

// Good, fine
assert.dom(page.date).exists();

// Whoops, typo, but the compiler can't tell me that there's a problem because it can
// implicitly cast `Record<string, string>` to `IDOMElementDescriptor`, so, I only discover
// my typo at runtime even though this is exactly the kind of thing the compiler is meant
// to help with
assert.dom(page.data).exists();

I don't know if there's any typescript wizardry to fix this, but I don't think so because of how interfaces are meant to work. So I'm kind of inclined to improve these end-developer-facing ergonomics by putting a tiny bit more burden on implementors of IDOMElementDescriptor. My only idea for how to do this would be to export a symbol from dom-element-descriptors and include that symbol in IDOMElementDescriptor:

export const IS_DESCRIPTOR = Symbol('is descriptor');

export interface IDOMElementDescriptor {
  [IS_DESCRIPTOR]: any;
}

so descriptors would have to

import { IDOMElementDescriptor, IS_DESCRIPTOR } from 'dom-element-descriptors';

export default class DescriptorThingy implements IDOMElementDescriptor {
  [IS_DESCRIPTOR] = true;
  // or, to keep it in the prototype
  // [IS_DESCRIPTOR]() {}
}

But I'm definitely open to ideas here.

Library/dependency structure

My naive attempt at hooking this all together is not working out. Currently I have dom-element-descriptors as a dependency of @ember/test-helpers, qunit-dom, and fractal-page-object. Because dom-element-descriptors stores the descriptor data in a module-scoped weak map, if the different versions drift such that an application doesn't end up with just a single hoisted copy of dom-element-descriptors, then different libraries might be using different copies, e.g. fractal-page-object might register its descriptors in one weak map, while qunit-dom tries to look them up in another and fails to find them.

This seems like a nasty footgun waiting to happen. So I'm wondering if we can figure out some better dependency structure than "everyone that needs dom-element-descriptors specifies your own dependency." I hate peer dependencies, but I wonder if this is the right time for them. Another also-not-super-great option would be to make the weak map a global rather than module scoping it. I'm not sure what the right path is here, and I'm also not 100% sure this needs to be specified in the RFC vs. being an "implementation detail," but I do think we need to figure something out.

Conclusion

I think my current front-runners are to:

1. change DescriptorData#elements to Iterable<Element>
2. add the symbol to IDOMElementDescriptor
3. make the weak map a global, at least for the time being (note that this won't affect any of the APIs, just how various libraries need to consume dom-element-descriptors)

If I don't hear otherwise, in a few days I'll update the RFC to specify (1) and (2), and continue assuming that (3) doesn't need to be specified in this RFC.

[wagenet]

@bendemboski are you still interested in getting this moved forward? If so, I'll do what I can to facilitate.

[bendemboski]

@wagenet I definitely am! I have an implementation of the RFC put together as well as (fairly old and in need of rebasing) forks of fractal-page-object, @ember/test-helpers, and qunit-dom that provide descriptor functionality. I'd love to get this moving again!

[wagenet]

@bendemboski anything Core can do to help?

[bendemboski]

@wagenet I just made the updates to the RFC that I described in #726 (comment), so I think now it's really just about getting the RFC merged, and then afterwards helping to get the @ember/test-helpers PR (that I haven't opened yet) merged.

I think I've been in kinda a holding pattern here for some time, because Rob (the PR champion) hasn't had bandwidth for this, and I haven't had the time/focus to be a squeaky enough wheel 😄

[bendemboski]

@wagenet or anyone else -- any advice on how to get this moving forward again?

[MrChocolatine]

Suggested change

	return Array.from(data.elements)[0] || null;
	return Array.from(data.elements)[0] ?? null;

[wagenet]

After discussion in the RFC Review Meeting we decided to move this to Final Comment Period.