GNOME Bugzilla – Bug 793587
adding ScrollTo to AtkComponent
Last modified: 2018-05-16 13:46:44 UTC
Created attachment 368546 [details] [review] proposed implementation Here is a proposed atk implementation for scrollto, as discussed in https://bugzilla.gnome.org/show_bug.cgi?id=792820
Alex, could you review this when you have some time? The related AT-SPI patches seem fine to me, but I would prefer to commit all of them in parallel and be sure that ATK and AT-SPI remain in synch. (Goes back to the question raised at a hackfest in terms of whether AT-SPI and ATK really need to be separate these days, but, alas, trying to combine them would take quite a bit of time for someone.)
Just making sure that Alejandro (I guess that's whom Mike meant) knows we are waiting for input.
(In reply to Samuel Thibault from comment #2) > Just making sure that Alejandro (I guess that's whom Mike meant) knows we > are waiting for input. Sorry, due the bug title, I assumed that this was a at-spi bug (didn't check the component). Will edit to avoid confusion. Will take a look to the patches now.
(In reply to Mike Gorse from comment #1) > Alex, could you review this when you have some time? Will take a look now, just a comment below. > The related AT-SPI patches seem fine to me, but I would prefer to commit all > of them in parallel and be sure that ATK and AT-SPI remain in synch. Yes me too. > (Goes > back to the question raised at a hackfest in terms of whether AT-SPI and ATK > really need to be separate these days, but, alas, trying to combine them > would take quite a bit of time for someone.) Merging both has been a desired feature for some time. But that would need, imho, more that a bit of time for someone. Somewhat off-topic here, but the main problem is that in order to merge both, technically it makes more sense to kill ATK, get at-spi to be mostly generated from the dbus xml definitions, with some manual tweaks. In order to do that, the xml definition would be need to be updated and cleaned-up. But that has the problem that all the implementors implement ATK not AT-SPI. So it would make more people needed to change. Sincerely, I doubt all this to be done soon.
Review of attachment 368546 [details] [review]: In general seems reasonable, although I have added some questions/suggestions. But in any case, my main concern is: have you tried to implement this? You mentioned at Bug 792820 comment 16 something about the gtk implementation of atk_component_set_position. Perhaps you could use as a reference for an early implementation? Im mentioning this because in the past we had some issues with adding new interfaces on atk/at-spi, merging them on the library, and then finding issues when we tried to implement them. Since them, I try and advice to try to implement the early versions of the new API (even if it is an incomplete implementation) to test if there is something missing. ::: atk/atkcomponent.c @@ +589,3 @@ + * + * Makes an object visible on the screen at a given position by scrolling all + * necessary parents. Bug 792820 comment 14 makes a good point about the similarities of this method with the already existing atk_component_set_position. You explain why both are relevant on following comments. That explanation (or at least a summary) should be included there. ::: atk/atkcomponent.h @@ +35,3 @@ + *window. + *@ATK_SCROLL_BOTTOM_RIGHT: Scroll the object to the bottom right corner of + *the window. Why LEFT and RIGHT are used only for TOP and BOTTOM respectively. I guess that it is in order to not having too many elements on this enum (something that I appreciate), but in any case, why TOP_RIGHT is not desired? @@ +36,3 @@ + *@ATK_SCROLL_BOTTOM_RIGHT: Scroll the object to the bottom right corner of + *the window. + *@ATK_SCROLL_TOP_EDGE: Scroll the object to the top edge of the window. Probably a non-native-speaker question, but what EDGE means in this case? Also, if we have LEFT and RIGHT, does ATK_SCROLL_TOP_EDGE implies centered? @@ +44,3 @@ + *window. + *@ATK_SCROLL_ANYWHERE: Scroll the object to application-dependent position + *on the window. What exactly means application-dependent? And in any case, would users of the API use anywhere at all? @@ +188,3 @@ + /* + * Scrolls this object so it becomes visible on the screen. + * Since ATK 2.27.2 Just a reminder: now it will be a more recent release, so this will need to be updated (although this is due not realizing about this patch, sorry for that). Also, at some point we decided to use the numbers of the first stable release with the API available. The advantage of that is that we would not need to update the numbers as unstable releases advance (reminder to myself: we need to write this agreement somewhere). So it would be 2.30 @@ +260,3 @@ gdouble atk_component_get_alpha (AtkComponent *component); +ATK_AVAILABLE_IN_ALL This is a new method, so it is not available in all. It would need to use ATK_AVAILABLE_IN_2_28. Take a look to atkdocument.h as a reference. @@ +264,3 @@ + AtkScrollType type); + +ATK_AVAILABLE_IN_ALL Ditto. ::: atk/atkutil.h @@ +180,3 @@ * top-level window + *@ATK_XY_PARENT: specifies xy coordinates relative to the widget's + * immediate parent. Taking into account that accessibility tree tends to have a lot of intermediate container fillers, is ATK_XY_PARENT really useful?
> have you tried to implement this? I simply plugged the firefox implementation for testing > Why LEFT and RIGHT are used only for TOP and BOTTOM respectively I wondered about it. I just followed the IAccessible2 interface to keep coherency with it. We can indeed add to it the other corners. > what EDGE means in this case? It means the scrolling gets the object to move up to reaching the top of the screen > Also, if we have LEFT and RIGHT, does ATK_SCROLL_TOP_EDGE implies centered? AIU IAccessible2 it means just scroll vertically to get it there. > What exactly means application-dependent? It means the application can decide the preferred way to have it shown on the screen. I know it's vague, that's IAccessible2. > And in any case, would users of the API use anywhere at all? Actually, this is what is used internally by firefox to show the currently focused widget, their algorithm is basically "minimal scrolling to get the object into view". > is ATK_XY_PARENT really useful? It was for coherency with IAccessible2. I agree that the actual usefulness is questionable.
(In reply to Samuel Thibault from comment #6) Oh true, IAccessible2 already have these methods. Just skimming, current implementation is basically the ATK/at-spi equivalent of those methods (yes, now I know that you already mentioned it on bug 792820 comment 0, but I missed it until you mentioned it, sorry). Then I guess that my concern "was this implemented somehow?" are done. Some comments extra comments below. > > have you tried to implement this? > > I simply plugged the firefox implementation for testing Ok, thanks for confirming. > > > Why LEFT and RIGHT are used only for TOP and BOTTOM respectively > > I wondered about it. I just followed the IAccessible2 interface to keep > coherency with it. We can indeed add to it the other corners. Well, as I mentioned, I having as less enum values as possible is a good thing. I don't like too much not knowing why those corners are not present, but I prefer to keep it simple. Let's keep them those, and if someone asks, it would be for coherency with IAccessible2 (that if possible, and unless we totally dislike what IAccessible2 does, it is a good thing). > > what EDGE means in this case? > > It means the scrolling gets the object to move up to reaching the top of the > screen > > > Also, if we have LEFT and RIGHT, does ATK_SCROLL_TOP_EDGE implies centered? > > AIU IAccessible2 it means just scroll vertically to get it there. Ah ok, so then I guess that RIGHT/LEFT will do also an horizontal move if needed, but EDGE will just do a vertical move. Is that correct? If that is the case, could add a small comment to the documentation? > > > What exactly means application-dependent? > > It means the application can decide the preferred way to have it shown on > the screen. I know it's vague, that's IAccessible2. Your explanation here is good. Could you expand the documentation of the method to include your explanation? > > And in any case, would users of the API use anywhere at all? > > Actually, this is what is used internally by firefox to show the currently > focused widget, their algorithm is basically "minimal scrolling to get the > object into view". > > > is ATK_XY_PARENT really useful? > > It was for coherency with IAccessible2. I agree that the actual usefulness > is questionable. Hmmm, ok, I guess that it is okish to add it. Let's then keep it. So in summary, I think that the patch look good, except for some minor things (sorry if some are nitpicks) I mentioned. Thanks for the patience answering my questions, and thanks for the patch.
(In reply to Samuel Thibault from comment #6) > > have you tried to implement this? > > I simply plugged the firefox implementation for testing Do you have a bug open for this? I will add support to Orca once things land, but I'd like to have something to test with.
Created attachment 371616 [details] [review] proposed implementation Here is the reworked patch
Joanmarie: I have opened bug https://bugzilla.mozilla.org/show_bug.cgi?id=1458548 with the patches I am currently using with firefox.
Review of attachment 371616 [details] [review]: LGTM.
Just wondering: is there anything missing for this, or are we just waiting for 2.30 to be about to be released before committing this?
(In reply to Samuel Thibault from comment #12) > Just wondering: is there anything missing for this, or are we just waiting > for 2.30 to be about to be released before committing this? Ah sorry, I'm used to review the patches, and let the author to push them to master if approved. Do you need me to push the patch to master?
I don't have a gnome account yet, I had started the request, but some vouching would be needed: https://rt.gnome.org/Ticket/Display.html?id=15942
(In reply to Samuel Thibault from comment #14) > I don't have a gnome account yet, I had started the request, but some > vouching would be needed: > > https://rt.gnome.org/Ticket/Display.html?id=15942 I have just committed the patch. Thanks for writing it. Sorry for the misunderstanding, and taking so long to push it. Closing the bug.
I just pushed a quick build fix, adding ATK_VERSION_2_30. I also noticed AtkScrollType and ATK_XY_PARENT are missing Since tags.
(In reply to Michael Catanzaro from comment #16) > I just pushed a quick build fix, adding ATK_VERSION_2_30. Ups. Ok, thanks. > I also noticed AtkScrollType and ATK_XY_PARENT are missing Since tags. I just pushed a quick fix for that. Thanks for noticing.
I noticed another build failure today, and pushed another fix for the fix. ;) Let's see if we've got it right now....