| Summary: | AX: [PATCH] procedures for automated atk text and caret tests | ||||||
|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Jarek Czekalski <jarekczek> | ||||
| Component: | Accessibility | Assignee: | Nobody <webkit-unassigned> | ||||
| Status: | RESOLVED WONTFIX | ||||||
| Severity: | Normal | CC: | apinheiro, jdiggs, mario, webkit-bug-importer | ||||
| Priority: | P2 | Keywords: | InRadar | ||||
| Version: | 528+ (Nightly build) | ||||||
| Hardware: | All | ||||||
| OS: | All | ||||||
| Attachments: |
|
||||||
|
Description
Jarek Czekalski
2014-04-29 12:50:44 PDT
Hi Jarek, First of all, thanks a lot for your efforts to help improve the accessibility support in WebKitGTK, is very much appreciated indeed. I've been following your bug reports closely, but unfortunately did not have much time to reply due to other things that keep me busy these days, however, I think I do need to make a comment now, before you spend perhaps too much time in the wrong path: testatk.c is part of the unit tests suite used in the WebKit1 port, which is now in the process of being deprecated[1][2] and so has actually been removed already now from trunk (still kept in a branch, though). You can see in [3][4] the relevant commits removing that code. So, that means that testatk.c no longer exist, meaning that any accessibility related test you want to add has to be written as a Layout test using the API provided by the AccessibilityController and AccessibilityUIElement classes in WebKitTestRunner. That said, it might be that we would need to extend those APIs to be able to properly test some things, as we did in the past, but that's definitely the way to go, so I'm letting you know asap so you can better refocus your efforts from now on. Of course, I'm more than happy to help with anything you might need (not only reviews) and also will try helping with code if possible as soon as I get rid of the extra things I have on top of myself these days (not sure when, though). Thank you very much! [1] https://lists.webkit.org/pipermail/webkit-gtk/2014-March/001821.html [2] https://lists.webkit.org/pipermail/webkit-gtk/2014-April/001862.html [3] http://trac.webkit.org/changeset/166979 [4] http://trac.webkit.org/changeset/166977 Hi Mario Thank you for the warning :) Now I won't spend too much time on a bug, that will look difficult. But one of the bugs (bug #13941) is already fixed and just for the sake of this bug we could try to patch testatk.c with my auto test method. It would help to keep stability of webkit1 in the maintanance period. Another advantage of my patch: it helps much in diagnosing other atk bugs, because new tests can be added so quickly. If you think it would be too much work, then yes, I can prepare an old fashioned test for that. By the time we make the decision, I will try to gather information about Debian and their plans regarding webkit2. Jarek (In reply to comment #3) > Hi Mario > > Thank you for the warning :) > Now I won't spend too much time on a bug, that will look difficult. But one of > the bugs (bug #13941) is already fixed and just for the sake of this bug we > could try to patch testatk.c with my auto test method. It would help to keep > stability of webkit1 in the maintanance period. I'm not sure about this, to be honest. It would mean putting effort on testing something in a way that we know will go away, while keeping the main trunk untested, which is where I think the effort should be put instead. > Another advantage of my patch: it helps much in diagnosing other atk bugs, > because new tests can be added so quickly. Yes, testatk.c is easier to handle than layout tests for this (too ATK specific) kind of tests, but again that's code that will go away and that is no longer in trunk, so I see little point on that approach. > If you think it would be too much work, then yes, I can prepare an old > fashioned test for that. > By old-fashioned test you mean a layout test? If so, then yes I do think that would be better :) > By the time we make the decision, I will try to gather information about > Debian and their plans regarding webkit2. Cool thanks! > > Jarek Ok, I forget about this patch. I wanted to achieve a goal in a way that turned out to be unacceptable. Mario, could you help me do it in an acceptable way? I would like to have a function that would receive 2 arguments and perform 3 tests. Details as described in the description of this bug report. If possible, actually only 1 argument could be given, the html source, because perhaps webkit could give us the text using non-accessibility functions. Is extending AccessibilityController a way to go? Would you suggest a signature of a function or functions? Sorry if I make a conceptual mistake again, but... When we had webkit1, testatk.c did all atk (and spi2 in other words) tests, including caret-moved tests. It served whole webkit, including webkit2, by testing accessibility scenarios. Now that we don't have testatk.c, what takes its role of testing atk related issues? Shouldn't we simply port testatk.c to webkit2 and use it then as before? Wording it differently: testatk.c (and other files from this directory) covered a set of tests, that were not covered by other tests in webkit. Do we want to drop this set? If I don't make a mistake, it may be better to answer this on webkit-gtk. (In reply to comment #5) > Ok, I forget about this patch. I wanted to achieve a goal in a way that turned out to be unacceptable. > > Mario, could you help me do it in an acceptable way? I would like to have a function that would receive 2 arguments and perform 3 tests. Details as described in the description of this bug report. If possible, actually only 1 argument could be given, the html source, because perhaps webkit could give us the text using non-accessibility functions. > > Is extending AccessibilityController a way to go? Would you suggest a signature of a function or functions? I think there's a bit of confusion here, which I'm not surprised about since everything related to a11y in WebKitGTK is quite confusing anyway :)... As per the description of this bug: > testatk.c lacks a convenient machinery to quickly add more test. I have an idea how to improve it. I think of a test that would get 2 things on input: > > 1. html source > 2. corresponding text > > The test procedure would verify 3 things: > > A. Whether all text atk components summed up give the expected text. > B. Whether caret may be inserted into all text components at all offsets. > C. Whether caret movements cause text-caret-moved events. > I think you might not be understanding how we test things in WebKit and, more specifically, accessibility related features. Let me try to summarize some points: We do have 2 kind of tests of interest: * Unit tests: used to test APIs, both public (e.g. WebKit2GTK+ API) or private (between internal components). * Layout tests: used to test the features of the engine by feeding HTML/CSS/JavaScript content to a WebView and then performing some checks over that using some helper tools (DumpRenderTree for the WebKit1, WebKitTestRunner for WebKit2). So, testatk.c belongs to the old set of unit tests from WebKit1, because it's just a C file explicitly checking ATK functions which is indeed an API consumed outside of webkit (by the ATSPI-ATK bridge). So, even though it was not a "true WebKit API" as for instance is the case of the WebKit2GTK+ API, it was more or less fine for WebKitGTK+ when based in WebKit1 for the following reasons: * It gave complete control on how to test ATK specific functions and signals without bothering other ports * It was possible to do it because we only had one process, so we could access the ATK world from the unit tests even if those actually lived in WebCore * It was easy and quick :) However, the testatk.c posed a big number of problems too, which rendered into a not feasible approach when WebKit2 arrived. Some reasons: * It does not scale well: not easy to add more cases in an organized way (as you noticed) * It forced other ports using ATK for a11y (EFLWebKit) to either not have a11y unit tests or code their own. Duplication is bad * In WebKit2GTK+ is simply not possible to use the ATK API since the unit tests are in the UI Process and the ATK world lives in the Web Process. We could use AT-SPI instead, but then we would be getting into an additional layer of trouble, would not be testing exactly the same things and still would have the other problems * Probably something else I'm missing now... So, what we decided to do was to convert the unit tests in testatk.c into regular layout tests that would end up testing exactly the same ATK calls under the hood, by means of the helper tools in place: DumpRenderTree (for WebKit1) and WebkitTestRunner (for WebKit2). That's precisely what Joanmarie was doing during the WebKitGTK+ hackfest in december, as you can see here: http://trac.webkit.org/changeset/160316 http://trac.webkit.org/changeset/160365 As you can see there, she converted a bunch of unit tests in testatk.c into layout tests, both by (1) writing new HTML files that would check the same things AND (2) adding the needed support -if not present already- in the ATK versions of AccessibilityUIElement in DumpRenderTree and WebKitTestRunner to make sure we could test that by using regular Javascript calls. So, if you really want to help with this (which is extremely appreciated, since I barely would have time for that these days), I believe a good point to start from would be to convert into layout tests the remaining tests that were left in testatk.c after Joanie's patches, adding support when needed in the accessibility related classes (AccessibilityUIElement, AccessibilityController) for ATK in WebKitTestRunner only (as DRT is gone too from trunk these days). Those files live in Tools/WebKitTestRunner/InjectedBundle/atk, btw. How does that sound? (In reply to comment #6) > Sorry if I make a conceptual mistake again, but... > > When we had webkit1, testatk.c did all atk (and spi2 in other words) tests, > including caret-moved tests. It served whole webkit, including webkit2, by > testing accessibility scenarios. Now that we don't have testatk.c, what > takes its role of testing atk related issues? Shouldn't we simply port > testatk.c to webkit2 and use it then as before? See my previous comment for further details but, trying to reply shortly here: - testatk.c only tested things using ATK, no AT-SPI2 was involved at all - testatk.c only served the WebKit1 version of WebKitGTK+. Webkit2GTK+ or EFL, for instance, were not covered by it - Porting testatk.c to WebKit2 would be possible, but then we would need to call it testatspi2.c instead, because we could no longer use ATK API to test things, but AT-SPI2 instead, since the unit tests and the ATK world would be living in different processes. - Converting those tests into layout tests is a much better approach (see my previous comment) I recommend you to read a post I wrote last year (and not because of self-promotion purposes), I guess you might find it handy to understand some of the messy concepts here :). Here's the link: http://mariospr.org/2013/02/03/accessibility-in-webkitgtk/ > Wording it differently: testatk.c (and other files from this directory) > covered a set of tests, that were not covered by other tests in webkit. That is right but, as mentioned in my previous comment, the best idea would be to continue Joanie's work on converting those leftovers in testatk.c into layout tests, so we can keep testing those cases in upstream ports based in WebKit2, such as WebKit2GTK+ and EFLWebKit. > Do we want to drop this set? > Of course not, and I would have gladly worked on this myself if I could, but has not been possible so far :( And that's part of the reason why I'm so happy to help you with this by giving you the right pointers, because if you could help with that that would be really great (and I'm sure Joanie would agree with that too). > If I don't make a mistake, it may be better to answer this on webkit-gtk. I'm fine if you want to move the discussion to webkit-gtk, yet I'm fine as well with keeping discussing here too. Joanie is on CC in this bug anyway and I'm adding apinheiro (ATK maintainer) now, to keep him in the loop Mario, thank you for your exhaustive answers. All makes sense after these explanations. Actually I missed the LayoutTests/platform/gtk directory so this part was especially helpful: >So, what we decided to do was to convert the unit tests in testatk.c into regular layout tests that would end up testing exactly the same ATK calls under the hood, by means of the helper tools in place: DumpRenderTree (for WebKit1) and WebkitTestRunner (for WebKit2). That's precisely what Joanmarie was doing during the WebKitGTK+ hackfest in december, as you can see here: >http://trac.webkit.org/changeset/160316 >http://trac.webkit.org/changeset/160365 Further: >I recommend you to read a post I wrote last year (and not because of self-promotion purposes), I guess you might find it handy to understand some of the messy concepts here . Here's the link: http://mariospr.org/2013/02/03/accessibility-in-webkitgtk/ Never too much promotion about this spot on article. The things explained there fill the gap, because they are nowhere expressed so clearly and completely. It is a must for anyone willing to discover accessibility framework on Linux. I read it thoroughly 2 weeks ago and also bookmarked it permanently. I wanted to say thanks at the bottom, because the last "thank you" was in 2013, but the page did not allow. So now expressing my gratitude directly :) From the first glance at caret layout tests I see no additional api is needed. But a javascript function may probably be added to perform the same things I implemented in testatk.c. I think now I can get back to work and return with something for platform/gtk/accessibility directory. >the best idea would be to continue Joanie's work on converting those leftovers in testatk.c into layout tests Actually I don't intend to become a full-time webkit contributor, so I don't plan to rewrite many tests from testatk.c. But the few things I plan to do, I would like to do them completely and in a good style. In the meantime I'll gather some knowledge about webkit, and probably will be coming back when another bug trace leads from orca to webkit. I like the webkit code, its documentation, its webpage. I like the order and clarity of how things are done in this project. I like it that you guys are responsive. It's a nice place for a dev :) (In reply to comment #9) > Mario, thank you for your exhaustive answers. All makes sense after these explanations. > Actually I missed the LayoutTests/platform/gtk directory so this part was especially helpful: > You're welcome. Very happy to be helpful! :) > [..] > > I recommend you to read a post I wrote last year (and not because of self-promotion purposes), > > I guess you might find it handy to understand some of the messy concepts here . Here's the link: > > http://mariospr.org/2013/02/03/accessibility-in-webkitgtk/ > > Never too much promotion about this spot on article. The things explained there fill the gap, > because they are nowhere expressed so clearly and completely. Yes, that's exactly what I thought when I wrote it and also precisely why I did it: I felt there was a gap, or at least that "my life would have been much easier if I had access to that info, all together in the same place, when I started" :) > It is a must for anyone willing to discover accessibility framework on Linux. Thanks for your kind words. Happy to see it's useful, because it did take me quite some time to put it together and write it. Actually, I even asked Joanie for help to "proof read" it and make comments and changes before publishing, because I did want to make sure I avoided confusing people even more when it comes to this topic (Big thanks to Joanie, btw). > I read it thoroughly 2 weeks ago and also bookmarked it permanently.I wanted > to say thanks at the bottom, because the last "thank you" was in 2013, but the > page did not allow. So now expressing my gratitude directly :) Yes, the WordPress is configured to close comments after a while, to help me fight spam, but thank you anyway. > From the first glance at caret layout tests I see no additional api is needed. > But a javascript function may probably be added to perform the same things I > implemented in testatk.c. I think now I can get back to work and return with > something for platform/gtk/accessibility directory. Make sure you enable caret browsing in the test, so you can then set the caret anywhere (not only in editable content) and move it around using either the DOM selection API or the eventSender object. See some examples here: LayoutTests/platform/gtk/accessibility/caret-browsing-select-focus.html LayoutTests/platform/gtk/accessibility/caret-browsing-text-focus.html > >the best idea would be to continue Joanie's work on converting those leftovers in testatk.c into layout tests > > Actually I don't intend to become a full-time webkit contributor, so I don't plan to rewrite many tests from testatk.c. That's fine, we're all busy people :). Any contribution you make will be valuable on its own, don't worry about that. > But the few things I plan to do, I would like to do them completely and in a good style. > In the meantime I'll gather some knowledge about webkit, and probably will be coming > back when another bug trace leads from orca to webkit. Sounds great > I like the webkit code, its documentation, its webpage. I like the order and > clarity of how things are done in this project. I like it that you guys are > responsive. It's a nice place for a dev :) Welcome :) Suggested implementation started in #132527. Comment on attachment 230405 [details] auto tests for testatk.c Cleared review? from attachment 230405 [details] so that this bug does not appear in http://webkit.org/pending-review. If you would like this patch reviewed, please attach it to a new bug (or re-open this bug before marking it for review again). |