Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Share code with a11y-outline #303

Open
xi opened this issue Mar 23, 2019 · 14 comments
Open

Share code with a11y-outline #303

xi opened this issue Mar 23, 2019 · 14 comments

Comments

@xi
Copy link

xi commented Mar 23, 2019

Hi,

I maintain a very similar extension called a11y-outline. I split the underlying code into a separate library called aria-api. As these projects are very similar in scope, I think we could share some code or at least learn from each other. If you think this is a good idea, my proposal would be to use aria-api in both extensions and work on that together.

@matatk
Copy link
Owner

matatk commented Mar 25, 2019

Good to virtually meet you, and thanks for your suggestion. a11y-outline is the inspiration for Landmarks’ issue #156 :-). I had a quick look at aria-api; abstracting over the source of an element’s semantics is very neat.

I’ve been thinking of factoring out the main tests for Landmarks so that they could be used as a general test suite for any code that finds landmarks (and, later, headings). This could be extensions like ours, or other assistive technologies such as screen-readers. Could also incorporate some of Landmarks’ benchmarking code too. (I hadn’t yet factored out the tests because I’m mindful of the need to change the test case data structure to a tree, from the current list format, in order to accommodate headings.)

Perhaps it would be a good idea for me to separate out that test suite, then we could use it to help us combine the core code?

There is another architectural matter I’ve known about for some time, but not had the time to explore. Since Landmarks started using mutation observers to track changes to dynamic pages, I have wondered if it would be more efficient to support scanning parts of pages for landmarks, rather than the whole page, on each mutation. This would require completely reworking the data structure for found landmarks (and in future headings). But, it seems like it could bring significant performance benefits. I’m not completely certain on this, because it would require matching the tree of found landmarks/headings to the part of the DOM that changed, which would involve potentially many containment tests, as well as slightly increased memory requirements. Scans actually tend to be very quick already (and avoided where possible), but for very complex pages, and ones that change a lot, this might be more appropriate. Seems like something for the future, though :-).

@xi
Copy link
Author

xi commented Mar 25, 2019

Nice to hear you know of a11y-outline. I get very little feedback, so it's great to hear you even took inspiration from it!

So far I mostly focused on tests for the accessible name calculation. So getting more tests for the landmark part would be great. Anyway, that sounds like a reasonable first step. Let me know how I can help.

Concerning the mutation observers, I don't have too much experience with that. Performance has not been a big concern for me yet. But it sounds like an interesting idea. I would be interested to help with that, too.

@matatk
Copy link
Owner

matatk commented Mar 28, 2019

I understand what you mean regarding feedback; sometimes I am curious about how people are getting on with Landmarks. However, I have been lucky and got some helpful feedback digitally and in person - some reviews, which is great, and several constructive issue reports, which is ace. I hope you get more feedback. If you’ve not seen it, you might be interested in reading an article that covers both a11y-outline and Landmarks

I’ve been exploring some ideas relating to testing, and I think I’ve got a reasonable starting point for a test suite that would cover aria-api and Landmarks, based on Landmarks’ current tests; I’ll try to get an initial version published within a week. Then we could perhaps refine it, and later I would love to extend it to other assistive technologies.

In the short term, I think the test suite will help us check that both our codebases are spec-compliant (assuming I’ve got the tests right—having your input on that would be great too), and if there are any performance improvements we could make.

aria-api seems really nice (especially now I’ve actually been using it) and sharing code is a good idea, so I’d like to use it. There are some upcoming design decisions for Landmarks that we might need to discuss first, but working together sounds like a great idea. I’ll post some further thoughts on the design questions after I’ve got an initial release of the test suite to show you :-).

@matatk
Copy link
Owner

matatk commented Apr 5, 2019

I’ve not met my intended deadline of a week to factor out the test suite, but it’s not far off :-).

I’m writing the more general test harness in a test-first manner and, whilst it will speed up future development loads, I had to do a bit of research into test libraries and if/how I might have some tests that read the file system (managed to avoid that for all but, I think, a couple of tests).

I’m AFK this weekend, but hope to have something to show you earlyish next week.

@xi
Copy link
Author

xi commented Apr 6, 2019

Is there a branch or something I could look at? I could try to include those tests in aria-api. That way I might be able to give feedback.

matatk added a commit to matatk/page-structural-semantics-scanner-tests that referenced this issue Apr 9, 2019
This is a basic version of the test suite from Landmarks that has been designed to work with other tools, such as a11y-outline/aria-api. The fixtures and expectations come from the Landmarks browser extension, but the code was written from scratch using TDD.

There are lots more features to come, though this initial version is used as a drop-in replacement for the Landmarks extension test suite.

For more information, consult matatk/landmarks#303
@matatk
Copy link
Owner

matatk commented Apr 10, 2019

I have an update :-).

I've factored out the Landmarks extension's test suite into a separate project, and made it so that the Landmarks extension's code now uses that project to run the landmarks-finding tests. If you'd like to run the test suite, try this:

  • Clone the Landmarks repo, or pull the latest changes.
  • npm install
  • npm test or, more specifically, npm run test:landmarks

Internally the new test suite works by creating a node-tap test function for each test fixture. The fixtures are turned into DOMs using JSDOM. The test function runs the landmarks-scanning code against that fixture, and checks the expectation to see if it matches. It uses node-tap to output the results, which includes a diff if the test is unsuccessful.

I'm working on making the new project either depend on or somehow include Landmarks, a11y-outline and any other tools so that they can be tested "out-of-the-box", but for now the tests must be run from a checked-out Landmarks repo. (I think that using git submodules to ensure that the tests are being run on a known version of Landmarks or a11y-outline might be a nice idea.)

The test fixtures for the new project aim to be comprehensive and use a reasonable general data structure (i.e. tree). Currently they only cover landmarks, but I plan to add headings and articles soon.

Landmarks doesn't use a tree internally (it uses an array, and each found landmark is given a "depth"), whereas a11y-outline uses a tree, but each object that represents a landmark has slightly different property names, and some properties are different (it doesn't need to compute a selector, but it does have the "href" field). Therefore I created several "converter" functions that change the "standard" test expectation data into the formats recognised by Landmarks and (if I've done it correctly) a11y-outline.

In order to test a11y-outline, I had to re-structure the code a bit so that the tree-creating code is in a separate module, so that the tree can be returned either to the main a11y-outline extension code or a new test script that simply runs a11y-outline's landmarks-scanning code against the test fixtures in the new project. Unfortunately I haven't fully completed this work, but wanted to send you what I have so far as I know you'd like to try it.

I have got quite far, but I'm a bit stuck at the moment, because both a11y-outline and aria-api's code assumes the existence of the globals window and document, which are not present in a Node context, which is where the scripts are being run as part of the test runner. I fixed this for a11y-outline by creating a new file called "src/buildTreeData.js" and moved your functions createItem(), insertItem() and a new one I called buildTreeData(), which is just the first part of buildTree() in it. I set module.exports to be buildTreeData and changed the function to accept a win (window) and doc (document) as arguments - they are provided by the new test runner. This allowed me to call the tree-building code from both the test runner in Node and as part of the a11y-outline extension as usual in the browser. However, the aria-api module also assumes a global window object too. The solution is fairly easy I think but, as above, I thought it was about time to show you what I have so far.

There seemed to be two possible solutions to this, and then I found another one...

  1. Support running code in a real browser. This will be necessary in some for or other in future, when I hope to support other tools such as screen-readers, as an external program would need to be used to somehow get the results, but I was hoping to avoid the need for it just yet.
  2. Parameterise aria-api so that it can be given a window object. I already did this to the a11y-outline code that builds the tree and it works, but then it requires aria-api and that is where I got stuck for now.
  3. We could use browserify to create a combined script that includes just the tree-building code and aria-api and then run that in the context of the DOM itself with JSDOM - that would give it access to the global window and document objects. I'm looking into this possibility now.

There are some other things that the new project can do, or will be able to shortly...

  • There's a script in there that will run the tests and create an HTML file that shows a table of the test results. I thought this would be nice in future so we can produce comparisons of any tools that scan for landmarks. Currently it only works with the Landmarks extension, and doesn't produce real results (though will be able to as soon as I've wired up the latest Landmarks code, possibly via a git submodule).
  • I am in the process of porting over Landmarks' profiling code so we can use it to compare the speed of both Landmarks and a11y-outline.
  • It seems like in future it would be helpful to know why a test fixture was included, and to provide some metadata about it. Therefore I expect there may be some metadata added such as links to a W3C spec requirement and/or Landmarks bug report perhaps. This information could be used to enhance the HTML output.
  • I need to set up Travis CI, which will be happening shortly.

The documentation needs a lot of work and I'd like to set up a GitHub Pages site on which the HTML results could be stored for different tools.

@xi
Copy link
Author

xi commented Apr 10, 2019

Wow, that sounds like a lot of work. Thanks a lot already!

I did not have time to look into it properly, but it sounds like the main issue is with providing a single test runner that works for both implementations. Wouldn't it be much simpler to just share the fixtures and expectations and leave the execution to each project? That would also have the benefit that each project would just need to execute one test scripts instead of two (its own and page-structural-semantics-scanner-tests). That's the approach I took with accname-test.

Can you give a bit of context on why you chose to use JSDOM? So far I use mocha-headless-chrome. I always shyed away from re-implementations of the DOM because I had a vague feeling that something could be missing, so I have no real experience with it. What are the pros and cons from your perspective?

@matatk
Copy link
Owner

matatk commented Apr 10, 2019

Thanks for your very quick reply and constructive feedback; it's given me some good stuff to think about.

The main reason for providing a test runner was that I thought (read: assumed :-)) that, given the code that scans for landmarks (or headings, or articles) is going to be relatively simple, and similar across projects, it would be possible to have a simple test-running API, and that would bring some benefits: the test code could be pooled into one project; projects using it would automatically get reporting of test results with diffs when the returned results were not expected, and the generation of HTML results could be automated (so it'd be easy to compare scanners). I had seen accname-test but not how it worked—oops :-). It's an elegant approach.

You've helped me realise that an alternative might be to provide some smaller utilities that projects could optionally use and still have their choice of test/assertion library and test environment. There's an iterator() function that is like forEach() for the tests; very simple, but tested, and maybe of use (details below). The converter functions that change the "standard" expectations into the formats required by Landmarks and (again, if I've got it right) a11y-outline are exported and should be helpful. Of course, the code could be skipped entirely and the fixtures and expectations loaded directly, in much the same way as accname-test. As below I'm planning to add some metadata soon to give the tests some context.

I thought there'd be a couple of advantages for running the code outside of a browser, at least initially: it'd take less time, with fewer dependencies, to get to the stage of getting test output from both Landmarks and a11y-outline. As I'm hoping to experiment with testing screen-readers (somehow :-)) in future, browser support would be needed at some point. I didn't spot the difference in globals, though funnily enough I had to abstract away from global window and document when I factored out the landmarks-scanning code from the GUI code in the Landmarks extension, when creating the test suite. It's easy but, in the long run, I wouldn't want to require you (or anyone) to do it.

With respect to JSDOM specifically: I know what you mean about it not being a mainstream browser, though I've been using it as part of Landmarks' test suite for a couple of years and it is solid, as well as quick and light on dependencies. Incidentally, I noticed we both use window.getComputedStyle() (which makes sense) and it works fine with stuff like that (and far more advanced things); it's very comprehensive.

I've also used Karma and Puppeteer on other projects, and imagine they may be good for browser testing (I'd like to make it work on Firefox as well as Chrome).

What's next? I would like you to be able to use the tests as easily as possible, so I am wondering:

  • How about exporting the existing node-tap-based test-runner for projects that want it (e.g. if they don't already have tests), and also exporting the far simpler "for each test" iterator for projects that either already have a test suite, or have a preferred set of testing tools?

  • Currently the iterator() function uses some string replacement to convert the fixture file name into a test name (just to get some basic human-readable output going), but I intend to remove that in favour of metadata for each test soon. Then, if you were to use it, you'd get a callback that, for each test, is passed:

    • a metadata object, containing:

      • test name,
      • description,
      • any relevant links, e.g. to W3C specs or scanner bug reports;
    • the HTML fixture string, and

    • the expectation object.

    You could then use your chosen test runner/library to do the actual testing bit. Does that sound more useful?

  • Perhaps there could be a helper function, intended to be called after each test, that records each test result (including diff if available) in a JSON file, and the HTML producer could be a separate process that takes one or more of these JSON files and produces a table comparing test results.

  • Is there anything that should be provided in order to make it easier for you to adopt the tests?

  • Is there anything that should not be provided?

  • I will look into running the tests in-browser, or at least offering that as a choice, too.

P.S. No rush to reply to this :-).

@xi
Copy link
Author

xi commented Apr 16, 2019

Today I finally found time to look at this again. I tried to integrate the tests with my existing setup and found some issues:

I run the tests in a browser, so node APIs are not available. Instead I use browserify with brfs to bundle everything into a single file. This is not possible if fs.readFileSync() is called with a variable. So I had to rewrite the complete javascript code.

Then I ran into another issue: The tests are executed inside of a host document. My existing tests only provide a HTML fragement that is injected into that document. Your test fixtures on the other hand are complete documents themselves. This is especially important for the "landmark-role-on-body" test.

I am not sure what to make of that. I was already able to find some bugs, both in my code and in the tests (I opened separate issues for those). So the shared tests are definitely useful. However, I am not sure yet how to integrate the test cases into my setup. I will think about it some more.

@matatk
Copy link
Owner

matatk commented Apr 18, 2019

Thanks very much for your continued feedback and trying this out. I’ll respond to your bug reports as soon as I’m able.

I’ve been working on several things (they’re in the page-structural-semantics-scanner-tests repo but not yet released):

  • Providing metadata to go with each test. Currently this is just an object containing the test’s name, but I am thinking of adding a section for links to important things, such as the spec(s) or bug reports, to explain why a test was added. There’s a schema for the metadata.
  • Exporting a function that gives you the metadata, fixture (string) and expectation (object) for each test, but allows you to use your own test environment. (Still in Node - I’d not fully understood your point about running in the browser environment, but I think I do now. I expect this wouldn’t work with brfs.)
  • Providing clearer documentation in general, with code samples.
  • Thinking about how to support running the tests in a browser. I imagined a headless browser controlled by Node/Karma would load each fixture page and then run the landmark-finding code on it and collect the results.
  • Continuing to think about how to collect and collate the results of the tests, probably as JSON files, that can then be turned into HTML tables that can be used to compare scanners.

I hadn’t considered the tests running the other way around - with the test suite running inside a single page in a browser. This makes a lot of things make sense now :-).

The brfs documentation doesn’t mention fs.esistsSync(), however I guess in theory the test hardness could be re-written to not use it, and aa ‘built’ version could be provided that includes hardcoded paths. Though I was thinking if there’s another way…

Apart from the two fixtures that have roles set on the <body> element ("application-alone-on-body-is-ignored" and "landmark-role-on-body"), a script could automatically combine all the others into one large fixture file. The expectations could be combined too, into an object you could require() in a Browserify-compatible way.

I've written such a script and put the results in the "try-combining-all-fixtures-expectations" branch. You can find the combined files in the "combined/" directory in the repo, and run npm build to update them (that command also puts the example code in the README).

The output isn't so neat at the moment, but (apart from the two tests it can't support) I'm wondering if this approach may be of help?

@xi
Copy link
Author

xi commented Apr 18, 2019

That is exacelt what I need. So now the question is: Should the combination be in the page-structural-semantics-scanner-tests repository or in mine? I lean towards having a separate repository with only the fixtures and expectations and separate ones that provide the test code. But I will leave that to you.

Thanks for all the work!

@matatk
Copy link
Owner

matatk commented Apr 23, 2019

Glad this helps :-).

I think you’re right; I can foresee some code moving back to the Landmarks repo—though I am working on some smaller-scale code to help with collecting and collating results into JSON and then HTML. Maybe later I’ll try doing some benchmarking/profiling, but that really depends on where the scanner is run, so will see about that. It is a good idea to keep dependencies as light as possible.

I have a few questions for you about the combined fixtures and expectations files:

  1. Should the combined HTML be a fully-formed HTML document (currently it’s just a fragment)?
  2. Would it be helpful for me to provide a CLI tool, or function, to create a fully-formed HTML document that contains the test fixtures and imports any CSS/JS that you need to run your tests? This would mean you could re-generate an up-to-date test harness with just one call to this tool/function. Simply opening that page would run your tests.
  3. I’m thinking I could make the expectations a JS file, rather than JSON, and then reference them via a <script> tag from the fixtures file, so that the expectations are a global variable for your test runner to use—would that help?

I’ve been thinking about the issues you reported and will get back to you as soon as I’m able.

@xi
Copy link
Author

xi commented Apr 23, 2019

the combined HTML be a fully-formed HTML document (currently it’s just a fragment)?

I think a fragment is fine.

Would it be helpful for me to provide a CLI tool

I would leave that to the authors.

I’m thinking I could make the expectations a JS file, rather than JSON

In my specific case that would make things easier. However, It would also mean that the expectations can only be used from JavaScript. If for example someone wanted to implement the same in C or Javaor whatever, they would not be able to reuse the test cases. So I would recommend against that.

@matatk
Copy link
Owner

matatk commented Jun 5, 2019

You may've noticed that my speed on this has slowed a bit (though we have closed some issues in the tests repo: no. 3, no. 1 and no. 2)—I've got a lot on at the moment (and will for the next month or two), but am very keen to continue working on this and applying the tests to other tools.

I still agree that the tests repo should be simplified as much as possible, though I also like that it can be used as an out-of-the-box testing solution if that's what people would like. When I've implemented what I felt was the "minimum viable product" it should be apparent what can be moved back out and into the tools that make use of it.

The test repo already seems useful for landmark tests, and thanks to your help it's fixed a bug in Landmarks that I'd missed :-). I'd like to add headings soon too (and maybe articles, as they are semantically separate).

I'm working on some simple code that should allow results from different tools to be collated and compared—if I can figure out how to interface with those other tools :-).

This thread started due to your suggestion of us sharing code, and I am still looking forward to us doing that, too :-).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants