Bug 52148 – [Gtk] Implement support for Embedded Objects

• Component: Accessibility
• Status: RESOLVED FIXED

[Joanmarie Diggs]

Created attachment 78400 [details]
test case

WebKitGtk currently does not have support for embedded objects. In particular, given the attached test case:

1. The AtkText implementation for each paragraph should contain an embedded object character for each embedded object:

* Here's our logo: <eoc> It reminds me of airport security screenings.

* Choose: <eoc> <eoc> (pick one or more)

* Choose: <eoc> <eoc> (just pick one)

* Choose: <eoc> <eoc> (just pick one)

2. Each embedded object should implement AtkHyperlink.

3. Each paragraph should implement AtkHypertext.

(I realize this is likely at least three bugs/rfes. But I will let Mario triage and make this an embedded object metabug if and how he sees fit. :-) )

[Joanmarie Diggs]

D'oh! On the last item, the expected text is:

Choose: <eoc> (just pick one)

Sorry for the spam....

[Mario Sanchez Prada]

Thanks for filing this. Adding myself to CC now...

[Mario Sanchez Prada]

(In reply to comment #0)
> Created an attachment (id=78400) [details]
> test case
> 
> WebKitGtk currently does not have support for embedded objects. In particular, given the attached test case:
> 
> 1. The AtkText implementation for each paragraph should contain an embedded object character for each embedded object:

You mean some kind of special character like those found in ff, right? 
Something like this:

 In [2]: t.getText(0, -1)
 Out[2]: "Here's our logo: \xef\xbf\xbc It reminds me of airport security
          screenings. "

> * Here's our logo: <eoc> It reminds me of airport security screenings.
> 
> * Choose: <eoc> <eoc> (pick one or more)
> 
> * Choose: <eoc> <eoc> (just pick one)
> 
> * Choose: <eoc> <eoc> (just pick one)

My main question is "how that special character helps to screen readers"? 
I guess the answer is that ORCA would use it to know that it should dive in the object to extract more info (actually I recall you talking to me about this in Seville, not sure if I recall correctly, though :-)), but I wonder whether it shouldn't be better to provide a textual representation for them.

Of course, I'm talking from my vast ignorance on the issue :-), and I realize that then we'd need to agree on a specific text representation for each kind of object (e.g. 'checked/unchecked' for check boxes, 'selected/unselected' for radio buttons...) but asking is not harmful either.


> 2. Each embedded object should implement AtkHyperlink.

Is this really always needed for each embedded object of any kind? 

I can see it for the image, but don't know how this interface would be implemented for other objects, such as the check boxes or the radio buttons (e.g. what should they return in the atk_hyperlink_get_uri() method?).

Taking a look at the following document (look for the text "Check one or more" to easily find the part I'm talking about)...

http://accessibility.linuxfoundation.org/~gk4/a11y/ddc06/guide/atkguideddc06.html

...it seems they suggest using AthHyperText/Link stuff for the case of the image, but not for check buttons and their associated text, which would just implement AtkAction and AtkText, respectively.

But of course, we could decide to do something else. Just pointing out that link as a reference trying to confirm things.


> 3. Each paragraph should implement AtkHypertext.

Makes sense.


> (I realize this is likely at least three bugs/rfes. But I will let Mario triage and make this an embedded object metabug if and how he sees fit. :-) )

At the moment, things are not perfectly clear so I'll keep this one as reference, but as soon as this translates into actual code and patches you can be sure I'll probably file more specific bugs depending on this one.

Thanks again, and sorry for the delay from my side on this topic.

[Joanmarie Diggs]

(In reply to comment #3)

> You mean some kind of special character like those found in ff, right? 

Yup, FF and OOo Writer.

> Something like this:
> 
>  In [2]: t.getText(0, -1)
>  Out[2]: "Here's our logo: \xef\xbf\xbc It reminds me of airport security
>           screenings. "

Yup.

> My main question is "how that special character helps to screen readers"? 

It's a placeholder. For instance, in the case of a radio button, we need to know there's an object of ATK_ROLE_RADIO_BUTTON, where it is exactly (and thus where to present it). We also need a means to get that radio button (i.e. the AtkObject representation of that radio button) so we can also tell the user it's state (checked/unchecked, required, etc.)

> I guess the answer is that ORCA would use it to know that it should dive in the object to extract more info (actually I recall you talking to me about this in Seville, not sure if I recall correctly, though :-))

You do. :-)

>but I wonder whether it shouldn't be better to provide a textual representation for them.

Well, it's up to the screen reader to handle how to present the state, allow users to have 'brief' and 'verbose' output, represent embedded objects in braille (often as symbols < > and <x>, rather than text), etc., etc., etc. That something WebKitGtk should take on?? ;-) (rhetorical question)

> Of course, I'm talking from my vast ignorance on the issue :-), and I realize that then we'd need to agree on a specific text representation for each kind of object (e.g. 'checked/unchecked' for check boxes, 'selected/unselected' for radio buttons...) but asking is not harmful either.

And you'd also have to get all those strings localized into all the languages gnome is localized into, and figure out brief braille symbols, and verbose braille symbols. In a way, I'd love to be able to say, "Yeah, you're right, this isn't Orca's job; it's WebKitGtk's." :-) But right now, I'm pretty sure we're stuck with it Orca-side. Feel free to keep trying to convince me I'm wrong, as I might let you win. ;-)

It's never, ever harmful to ask questions. I hope I've answered them. If not, ask again and I shall try again.

> > 2. Each embedded object should implement AtkHyperlink.
> 
> Is this really always needed for each embedded object of any kind? 

What we really need is:

* atk_hyperlink_get_start_index()
* atk_hyperlink_get_end_index()
* atk_hyperlink_get_object()

> I can see it for the image, but don't know how this interface would be implemented for other objects, such as the check boxes or the radio buttons (e.g. what should they return in the atk_hyperlink_get_uri() method?).

Since the return type is a string, and since these objects would not have an associated uri, I would think it would return an empty string.

What we really care about is where is the thing w.r.t. the text (start, end indices) and to be able to say "give me that object" for the purpose of presenting its role, states, coming up with its braille representation, etc., etc.

HTH. Thanks!!

[Mario Sanchez Prada]

(In reply to comment #4)
> (In reply to comment #3)
> 
> > You mean some kind of special character like those found in ff, right? 
> 
> Yup, FF and OOo Writer.
> 
> > Something like this:
> > 
> >  In [2]: t.getText(0, -1)
> >  Out[2]: "Here's our logo: \xef\xbf\xbc It reminds me of airport security
> >           screenings. "
> 
> Yup.
> 
> > My main question is "how that special character helps to screen readers"? 
> 
> It's a placeholder. For instance, in the case of a radio button, we need
> to know there's an object of ATK_ROLE_RADIO_BUTTON, where it is exactly
> (and thus where to present it).

Ok.

> We also need a means to get that radio button (i.e. the AtkObject
> representation of that radio button) so we can also tell the user
> it's state (checked/unchecked, required, etc.)

That should work well enough through the atk_hyperlink_get_object() method, I guess

> > but I wonder whether it shouldn't be better to provide a textual
> > representation for them.
> 
> Well, it's up to the screen reader to handle how to present the state, allow
> users to have 'brief' and 'verbose' output, represent embedded objects in
> braille (often as symbols < > and <x>, rather than text), etc., etc., etc.
> That something WebKitGtk should take on?? ;-) (rhetorical question)

Yeah, forget about my proposal. I don't really think now such a thing should be done in WKGTK's side.

> > Of course, I'm talking from my vast ignorance on the issue :-), and I
> > realize that then we'd need to agree on a specific text representation for
> > each kind of object (e.g. 'checked/unchecked' for check boxes, 
> > 'selected/unselected' for radio buttons...) but asking is not harmful
> > either.
> 
> And you'd also have to get all those strings localized into all the
> languages gnome is localized into, and figure out brief braille symbols,
> and verbose braille symbols. In a way, I'd love to be able to say, "Yeah,
> you're right, this isn't Orca's job; it's WebKitGtk's." :-) But right now,
> I'm pretty sure we're stuck with it Orca-side. Feel free to keep trying
> to convince me I'm wrong, as I might let you win. ;-)

No, no... :-) I think you're perfectly right, and therefore I'm perfectly wrong. Actually I also thought of the l10n problem at some point after posting my question, and you just confirmed my doubts :-)

> It's never, ever harmful to ask questions. I hope I've answered them.
> If not, ask again and I shall try again.

You have. Thanks

> > > 2. Each embedded object should implement AtkHyperlink.
> > 
> > Is this really always needed for each embedded object of any kind? 
> 
> What we really need is:
> 
> * atk_hyperlink_get_start_index()
> * atk_hyperlink_get_end_index()
> * atk_hyperlink_get_object()
> 

Good to know, thanks for confirming.

> > I can see it for the image, but don't know how this interface would
> > be implemented for other objects, such as the check boxes or the
> > radio buttons (e.g. what should they return in the
> > atk_hyperlink_get_uri() method?).
> 
> Since the return type is a string, and since these objects would not
> have an associated uri, I would think it would return an empty string.

Ok.

> What we really care about is where is the thing w.r.t. the text (start,
> end indices) and to be able to say "give me that object" for the
> purpose of presenting its role, states, coming up with its braille
> representation, etc., etc.

Now I understand much better the problem and a way to a possible and actual solution.

Thanks for the awesome feedback!

[Mario Sanchez Prada]

I have a working implementation of a patch for this bug, but before attaching it I'd like to comment some things on top of this description, for clarity.

(In reply to comment #0)
> [...]
> 1. The AtkText implementation for each paragraph should contain an embedded object character for each embedded object:
> 
> * Here's our logo: <eoc> It reminds me of airport security screenings.
> 
> * Choose: <eoc> <eoc> (pick one or more)
> 
> * Choose: <eoc> <eoc> (just pick one)
> 
> * Choose: <eoc> <eoc> (just pick one)

As I commented via mail to Joanie, there's a problem for this and it's that, even though the example uses <input type="radiobutton">text</input> to associate the 'text' string with the button, in practise such relationship doesn't get reflected in the DOM tree since the </input> end tag is not correct, hence it will be ignored.

According to the HTML spec:
http://www.w3.org/TR/html4/interact/forms.html#h-17.4

 "17.4 The INPUT element
  [...]
  Start tag: required, End tag: forbidden"
                       ^^^^^^^^^^^^^^^^^^

That explains while in the DOM, rendering and a11y trees the object associated to the INPUT element has no child with the text, but just a sibling holding both the text that was originally between the <input> and </input> marks, as well as any other text that might be present in that line.

Hence, I think that we have no other option that representing this as firefox currently does, something like:

   * Choose: <eoc>apples <eoc>oranges (just pick one)

...and then AT's would need to do some extra work to figure out which text is related to which input elements. In the case of the GTK toolkit is different because both check and radio buttons have internal attributes for holding a (optional) string as a label, but for the case of HTML that's not that way, and we can't either make assumptions like "the text at the right/left side of the button", or the like. 

Guess that the only option would be to explicitly use the <label> tag to 'label' those objects in some way...

> 2. Each embedded object should implement AtkHyperlink.

Done in the current patch. Evey embedded object implements this the AtkHyperlinkImpl interface, allowing to get a instance of the AtkHyperlink class for every embedded object.

Also, every instance of the AtkHyperlink class can return the associated AtkObject by calling to atk_hyperlink_get_object().

> 3. Each paragraph should implement AtkHypertext.

Done.

> (I realize this is likely at least three bugs/rfes. But I will let Mario triage and make this an embedded object metabug if and how he sees fit. :-) )

I think it's ok having it under the same bug :-)

[Mario Sanchez Prada]

Created attachment 85344 [details]
Patch proposal + Unit test

Attaching patch proposal + Unit test

[chris fleizach]

Comment on attachment 85344 [details]
Patch proposal + Unit test

View in context: https://bugs.webkit.org/attachment.cgi?id=85344&action=review

Accessibility changes generally look pretty good. You should get someone else to take a look at the text iterator changes to make sure something isn't being missed. Accessibility side (modulus changes) have my review

> Source/WebCore/accessibility/AccessibilityRenderObject.cpp:1156
> +#endif

Seems like we should have a method like textIteratorBehavior() that will consolidate this logic

> Source/WebCore/accessibility/gtk/AccessibilityObjectWrapperAtk.cpp:2408
> +

Instead of referencing AccessibilityRenderObject in platform code, which i don't like, I think we should either have a method renderer() on AccessibilityObject or isRendererReplaced() on accessibility object.

[Mario Sanchez Prada]

(In reply to comment #8)
> (From update of attachment 85344 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=85344&action=review
> 
> Accessibility changes generally look pretty good. You should get someone else 
> to take a look at the text iterator changes to make sure something isn't 
> being missed. Accessibility side (modulus changes) have my review

Thanks. Adding Enrica Casucci to CC, as the person that already took a look into other patches I wrote in the past, related to text editing.

> 
> > Source/WebCore/accessibility/AccessibilityRenderObject.cpp:1156
> > +#endif
> 
> Seems like we should have a method like textIteratorBehavior() that will
> consolidate this logic

Ok.

> > Source/WebCore/accessibility/gtk/AccessibilityObjectWrapperAtk.cpp:2408
> > +
> 
> Instead of referencing AccessibilityRenderObject in platform code, which i 
> don't like, I think we should either have a method renderer() on 
> AccessibilityObject or isRendererReplaced() on accessibility object.

Ok, but as AccessibilityRenderObject is also being referenced in several other parts of the GTK's wrapper, I wonder whether this change shouldn't go in a separate patch to this one, and now focus in fixing this bug.

What do you think?

[Mario Sanchez Prada]

Created attachment 88447 [details]
Patch proposal + Unit test

Attaching a new patch with some changes over it, based on Chris's previous comments.

(In reply to comment #9)
> [...]
> > > Source/WebCore/accessibility/AccessibilityRenderObject.cpp:1156
> > > +#endif
> > 
> > Seems like we should have a method like textIteratorBehavior() that will
> > consolidate this logic
> 
> Ok.

Done.

> > > Source/WebCore/accessibility/gtk/AccessibilityObjectWrapperAtk.cpp:2408
> > > +
> > 
> > Instead of referencing AccessibilityRenderObject in platform code, which i 
> > don't like, I think we should either have a method renderer() on 
> > AccessibilityObject or isRendererReplaced() on accessibility object.
> 
> Ok, but as AccessibilityRenderObject is also being referenced in several
> other parts of the GTK's wrapper, I wonder whether this change shouldn't
> go in a separate patch to this one, and now focus in fixing this bug.

Done, by reporting a new bug (bug 57955) for fixing that, providing a patch there (currently pending on review) and developing the new patch for this bug on top of that one.

[Mario Sanchez Prada]

Setting dependency on bug 57955, since the current patch here was developed on top of the patch currently proposed for fixing that other one.

[chris fleizach]

Comment on attachment 88447 [details]
Patch proposal + Unit test

changes look good to me. if we can just get someone familiar with text editing code to ensure that's the best way to handle this case, this should be r+

[Enrica Casucci]

Comment on attachment 88447 [details]
Patch proposal + Unit test

The changes look reasonable to me.

[chris fleizach]

Comment on attachment 88447 [details]
Patch proposal + Unit test

r=me, considering feedback from enrica

[Mario Sanchez Prada]

Committed r83493: <http://trac.webkit.org/changeset/83493>