Add QElement dom node constructor

[greyepoxy]

This is a work in progress towards this suggested enhancement https://github.com/jamesshore/quixote/issues/55

So far it just demonstrates how to remove QFrame from QElement. I believe it works as expected, I am on windows so only was able to run the tests in chrome, firefox, and edge. At least those three browsers reported success.

I also want to test using this in cypress to make sure it works as expected over there. Will try and get to that in the next couple of days.

[jamesshore]

This is a good start. The way QFrame was being passed around to so many places feels like a hack, and this cleans it up.

Part of the design philosophy of Quixote—for better or worse—is that we abstract the DOM. To that end, I wonder if we need a class to encapsulate contentDocument and contentWindow.

One thing that bugs me is the duplicate implementation of getRawScrollPosition() in in ElementEdge. That could go on QElement (although I don't love it there), or possibly on whatever abstraction encapsulates contentWindow.

[greyepoxy]

Okay thanks for the feedback, yeah I agree I think we are missing an abstraction. Will see if I can add one in the next couple of days.

I tested using these changes in cypress today and here are two examples of using it (for the below examples I created a top level elementFromDomElement function, not sure if you had something else in mind)

First Example

  it('When no text content should be the maximum width of the viewport', () => {
    cy.document()
      .then(testDocument => {
        const editor = quixote.elementFromDomElement(
          testDocument.querySelector(`[data-testid="${editorTestId}"]`)!
        );
        const body = quixote.elementFromDomElement(testDocument.querySelector('body')!);

        return [editor, body];
      })
      .should(([editor, body]) => {
        editor.assert({
          width: body.width.plus(12)
        });
      });
  });

Here is the failure message displayed in this case This approach is pretty similar to how quixote works today, when I look at this I see the missing abstraction we were talking about as being a "window" that is constructed from a document or window and exposes many of the methods QFrame does today (get being the main one). All of the functions that actually require the iFrame (like resizing) would not be present.

Second Example

A second possibly more "cypress" way of doing the same thing

  it('When no text content should be the maximum width of the viewport2', () => {
    cy.getDataTestId(editorTestId).then($editor => {
      cy.get('body').should($body => {
        const qBody = quixote.elementFromDomElement($body.get(0));
        const qEditor = $editor.get(0);
        quixote.elementFromDomElement(qEditor).assert({
          width: qBody.width.plus(12)
        });
      });
    });
  });

Here is the failure message in this case This uses cypress's get to do the query selection, it annoyingly returns a JQuery selection result type (which is why there is the extra .get(0) calls). Cypress also does a lot of async-ish type execution (but doesn't use promises), and unfortunately there is no way to merge a bunch of cy.get calls into a single then (like one would with Promise.all). So I had to put the second get inside of the first. Works completely fine from an execution perspective, just kind of annoying to read.

A distinct advantage of the second case over the first is that cyrpess will actually do the expected render waiting. In the first case you would have to add an artificial cy.get call to force it to wait until the element renders and then do the quixote asserting. With the second that is already built in.

Next Steps thoughts

So all of this to say, I am leaning towards the following,

1. Add a new "window"/"document" abstraction which is constructed from a window, or dom Element and then move many of the methods in QFrame to that abstraction (viewport, page, body, add, get, getAll, scroll, and getRawScrollPosition). QFrame will then use the abstraction, as well as update ElementEdge/ViewportEdge to use it as well.
2. Expose an instance creation function for this new QWindow type in the top level quixote module.
3. Expose an instance creation function for the QElement type from a dom node in the top level quixote module.

What do you think?

[jamesshore]

This all looks good. You didn't suggest a name for the abstraction, so I'll propose QContainer, but I'm open to other names. (QDocument perhaps? QWindow? QContent? I like QContent because it lines up with DOM names, but it's kind of unclear to people who don't know the DOM well.)

Regarding the name of the new top-level method, I'm leaning toward quixote.elementFromDom() to keep it a bit simpler.

In terms of a top-level method for the container, do we really need it? Or can we just expose it as a method/property on QElement instead of QFrame? In other words, instead of having element.frame, we'd have element.container or element.getContainer().

[greyepoxy]

Okay I went with QContent but would be happy to rename it later if needed. I like it in some of the places but not in others (element.parentContent or frame.toContent both seem weird to me, I want to add "Host" to the end of those names).

I have run out of time to work on this today (will pick it up next week) so unfortunately did not get completely done with everything we talked about.

Here is what is left

• [x] Move methods from QFrame to QContent (and have QFrame existing methods just call through to QContent), in order to make window and document private properties on QContent
• [x] Create tests for QContent
• [x] Add documentation for QContent and its public methods
• [x] Add documentation for QElement method parentContent
• [x] Add documentation for QFrame method toContent

Anything else I missed?

[jamesshore]

I like QContentHost better. QContent is fairly vague, despite the link to DOM names.

Your checklist looks good. My schedule's pretty full, so I haven't been able to study your commits, but it sounds right.

[greyepoxy]

Rename is done and I extracted the methods from QFrame to QContentHost. There are still a couple of places the raw contentHost HTML document is accessed that I am unsure what to do about.

1. QFrame loaded()
2. QFrame loadStylesheets()
3. QFrame loadRawCSS()
4. QFrame.reset()
5. PageEdge.value()
6. ViewportEdge.value()

As you described above do not want to expose raw dom externally so not sure of the best way to abstract these cases (or if we even need to). Curious what you think?

[jamesshore]

Accessing the DOM directly means our design isn't quite right. I'd guess we're missing abstractions in QContentHost. I'm pretty tired today, though, and not sure what they should be. Any ideas?

[greyepoxy]

Apologies for the delay, hmmm...

Is there a way to have "internal" public functions? Doesn't really solve the problem but would allow for the document to be exposed (or DOM specific functions for the different uses cases) without exposing them externally.

If not some ideas, For PageEdge and ViewportEdge we could move the pageSize and viewportSize functions onto QContentHost, I don't really want to do that as I kind of like how they pull that responsibility out. Alternatively since one would get the PageEdge and ViewportEdge via QPage and QViewport which both come from QContentHost now. We could just update the PageEdge, ViewportEdge, QPage, and QViewportconstructors to take the html document directly (since the constructors are not externally exposed I think this would be okay). That is the type they want anyway so seems better from the perspective of "pass in supplies not the supplier."

For the QFrame functions, I believe we are going to have to expose some more methods publicly if we want to keep this new abstraction. Looks like there are two main things QFrame needs to do.

looking first at QFrame loaded(), and QFrame.reset(), it needs to save the body html so it can be reset later. To make that work we could expose the following,

• QContentHost.getRawBodyContent() -> for getting the body's html content in QFrame loaded()
• QContentHost.setRawBodyContent(bodyHTML) -> for resetting the body's html content in QFrame.reset()

or alternatively move the responsibility of resetting into QContentHost

• QContentHost.captureContentSnapshot() -> for use in QFrame loaded() to tell the content host to capture a snapshot of its body
• QContentHost.resetToLastContentSnapshot() -> for use in QFrame.reset() to reset the body HTML content.

With this second approach since its part of the public API there is some weirdness that users of quixote could muck with the reset behavior. Maybe that is good? IDK I don't know of a scenario where someone would want to do that so could make the public api needlessly confusing.

Looking now at the second responsibility, QFrame needs to load stylesheets, we could just add these as public apis to QContentHost.

• QContentHost.addStylesheetLink(url, onStylesheetLoad) -> used instead of QFrame addLinkTag by QFrame loadStylesheets
• QContentHost.addRawStylesheet(rawCSS) -> replaces QFrame loadRawCSS

Seems generally okay with me from an abstraction perspective but would result in people being able to write tests where they could load more CSS during the test run, not sure if that is a good thing to expose or not.

[greyepoxy]

ahh so as usual something works in my head and then I try and do it and it turns out to be more difficult,

ViewportEdge uses QContentHost.getRawScrollPosition() so changing its constructor to require a HTMLDocument instead results in the same problem we were trying to avoid by introducing QContentHost.

Another option for PageEdge and ViewportEdge is that we could move the viewportSize and pageSize functions into their own files and then expose those same methods also on QContentHost. PageEdge and ViewportEdge would then just call the methods on QContentHost.

[jamesshore]

Thanks for continuing to think about it. I'll take a look this weekend and see if I can come up with anything clever. At the moment it seems like the cleanest option would be to put the sizing functions on QContentHost, but I don't love it.

Any method that isn't documented is considered "internal," so we have leeway in adding methods and classes that aren't part of the official API.

[greyepoxy]

I had a bit of time today so went ahead and put in place one of the ideas we talked about and added tests.

For ViewportEdge and PageEdge I re-exposed QContentHost.document, they are the only uses of this property so while not ideal not as bad as I think reaching through toDomElement

For QFrame body "reset" functions I updated them to use QContentHost.body().toDomElement().innerHTML, I think its about the same as what we discussed above about creating getRawBodyContent and setRawBodyContent functions.

For QFrame header stylesheet functions I created the methods I mentioned above QContentHost.addStylesheetLink and QContentHost.setRawBodyContent.

Also I added tests for QContentHost, mostly I moved the existing QFrame tests over and removed them from QFrame (since they just call through to QContentHost now. Decided not to add tests for the "internal" functions of QContentHost since they are already being tested by their users.

Let me know what you think

[greyepoxy]

Hey James hope you and your family are safe during this COVID-19 crisis. No pressure on following up on this, it can wait. Wanted to document the current status of this. I believe its ready to go, I updated the documentation to include QContentHost api information as well as documented the QFrame.toContentHost(), and QElement.host() methods. At this point I don't have any further work planned, although happy to make any updates or changes. Still not completely pleased with the QContentHost api but cannot think of anything better at the moment.

[jamesshore]

Thanks for your hard work on this, Justin. I'm on vacation this week but do plan to take a closer look next week.

[aultuser]

This looks like a great improvement, can't wait to see it land in master!

[jamesshore]

I started working on it yesterday :-)

[jamesshore]

I've taken a closer look at this and it's definitely going in the right direction. I'd rather have a lower impact on the API, though. I'm going to look at options for doing that. No action required on your part, just wanted to keep you informed.

[jamesshore]

I've pulled this in and it's working great. I was able to resolve my concerns about API complexity just by removing QContentHost from the API documentation.

I think I will rename it, though. After researching a bit more, the formal name for what QContentHost is doing is "browsing context."