Bug 25897 - [Gtk] Extraneous object of ROLE_PANEL in hierarchy for entries
Summary: [Gtk] Extraneous object of ROLE_PANEL in hierarchy for entries
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Accessibility (show other bugs)
Version: 528+ (Nightly build)
Hardware: PC Linux
: P3 Normal
Assignee: Nobody
URL:
Keywords: Gtk
Depends on:
Blocks: 25531
  Show dependency treegraph
 
Reported: 2009-05-20 15:58 PDT by Joanmarie Diggs (irc: joanie)
Modified: 2009-10-28 14:36 PDT (History)
4 users (show)

See Also:


Attachments
Add ability for platforms to ignore objects (9.75 KB, patch)
2009-10-26 11:27 PDT, Joanmarie Diggs (irc: joanie)
no flags Details | Formatted Diff | Diff
Add ability for platforms to ignore objects - Shorter Name Edition (9.15 KB, patch)
2009-10-26 12:10 PDT, Joanmarie Diggs (irc: joanie)
no flags Details | Formatted Diff | Diff
Add ability for platforms to ignore objects - Cleaned up comments as per review (9.12 KB, patch)
2009-10-27 06:50 PDT, Joanmarie Diggs (irc: joanie)
no flags Details | Formatted Diff | Diff
textcontrolsatk.diff (7.78 KB, patch)
2009-10-27 06:52 PDT, Xan Lopez
no flags Details | Formatted Diff | Diff
Remove the extraneous panel (2.41 KB, patch)
2009-10-28 13:32 PDT, Joanmarie Diggs (irc: joanie)
xan.lopez: review-
Details | Formatted Diff | Diff
Remove the extraneous panel - Take 2 (2.41 KB, patch)
2009-10-28 13:57 PDT, Joanmarie Diggs (irc: joanie)
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Joanmarie Diggs (irc: joanie) 2009-05-20 15:58:13 PDT
Steps to reproduce:

1. Type some text in an entry (e.g. Google search entry).

2. Examine the accessible associated with the entry in Accerciser.

Preferred results: The entry itself would implement the accessible text interface itself rather than have a descendant which does so. This would be consistent with what other applications do.

Expected results (Based on looking at the hierarchy of other accessible objects in WebKit which contain text):

- ROLE_ENTRY
  - ROLE_TEXT

Actual results: 

- ROLE_ENTRY
  - ROLE_PANEL
    - ROLE_TEXT

If there is some fundamental reason why that object of ROLE_PANEL must remain in the accessible hierarchy, assistive technologies can punt and still get at the text. However, if this object is indeed extraneous, it would be great if it could be removed from the hierarchy.
Comment 1 Joanmarie Diggs (irc: joanie) 2009-10-26 11:27:43 PDT
Created attachment 41877 [details]
Add ability for platforms to ignore objects

Expands on -- and modifies -- what was done to fix bug 27085. Now platforms can indicate that they:

* Want to include an object (i.e. one which accessibilityIsIgnored()
  would otherwise ignore, aka bug 27085)

* Want to ignore an object (i.e. one which accessibilityIsIgnored()
  would include, aka this bug)

* Want to let accessibilityIsIgnored() decide.

What I like:

* More control at the platform level to deal with unique needs/issues.

* Fixes both of the issues in question.

What I don't like much:

* The names. Y'all decide what they should be, and I will make it so. :-)

Also, Xan: I narrowed the logic down from your pastebin patch to just look at text-based controls. Looking at all controls seems like it might accidentally cause us to ignore parts of the hierarchy of certain funky widgets I've seen in Aria, Dojo, etc. And maybe even combo boxes -- which I still have to implement, but which have hierarchies like:

-> combo box
   -> menu
      -> menu item
      -> menu item

Anyhoo.... Thoughts?
Comment 2 Joanmarie Diggs (irc: joanie) 2009-10-26 12:10:24 PDT
Created attachment 41882 [details]
Add ability for platforms to ignore objects - Shorter Name Edition

Xan and I chatted about this in IRC. I was (mistakenly) under the impression that everything had to start with Accessibility.

So 'tis now:

+enum AccessibilityObjectPlatformInclusion {
+    IncludeObject,
+    IgnoreObject,
+    DefaultBehavior,
+};
+

Also, Xan reminded me of the one-thing-per-patch rule. Sorry! :-) So this patch *only* has the expansion in functionality; it does not include the fix for this bug. I'll re-submit that once we decide about this bit.

Thanks!
Comment 3 Jan Alonzo 2009-10-27 03:50:46 PDT
Comment on attachment 41882 [details]
Add ability for platforms to ignore objects - Shorter Name Edition

I just have some nits.

> +    // gives platforms the opportunity to indicate if and how an object should
> +    // be included

No need to wrap.

>  #if HAVE(ACCESSIBILITY)
> -    bool accessibilityPlatformIncludesObject() const;
> +    AccessibilityObjectPlatformInclusion accessibilityPlatformIncludesObject() const;
>  #else
> -    bool accessibilityPlatformIncludesObject() const { return false; }
> +    // this won't ever get called....
> +    AccessibilityObjectPlatformInclusion accessibilityPlatformIncludesObject() const { return DefaultBehavior; }

It can be called if people don't want AX enabled. Aside from that patch looks OK.

> Also, Xan reminded me of the one-thing-per-patch rule. Sorry! :-) So this patch

One-thing-per-bug rule would've been nice as well. :)
Comment 4 Jan Alonzo 2009-10-27 03:53:18 PDT
Comment on attachment 41882 [details]
Add ability for platforms to ignore objects - Shorter Name Edition

I just have some nits.

> +    // gives platforms the opportunity to indicate if and how an object should
> +    // be included

No need to wrap.

>  #if HAVE(ACCESSIBILITY)
> -    bool accessibilityPlatformIncludesObject() const;
> +    AccessibilityObjectPlatformInclusion accessibilityPlatformIncludesObject() const;
>  #else
> -    bool accessibilityPlatformIncludesObject() const { return false; }
> +    // this won't ever get called....
> +    AccessibilityObjectPlatformInclusion accessibilityPlatformIncludesObject() const { return DefaultBehavior; }

It can be called if people don't want AX enabled. Aside from that patch looks OK.

> Also, Xan reminded me of the one-thing-per-patch rule. Sorry! :-) So this patch

One-thing-per-bug rule would've been nice as well. :)
Comment 5 Joanmarie Diggs (irc: joanie) 2009-10-27 06:50:59 PDT
Created attachment 41949 [details]
Add ability for platforms to ignore objects - Cleaned up comments as per review

Nits addressed. Thanks!
Comment 6 Xan Lopez 2009-10-27 06:52:51 PDT
Created attachment 41950 [details]
textcontrolsatk.diff

Make text controls implement the text interfaces properly, a small piece in the puzzle.
Comment 7 Jan Alonzo 2009-10-28 00:26:18 PDT
Comment on attachment 41950 [details]
textcontrolsatk.diff

> +static void test_webkit_atk_get_text_at_offset_forms(void)

forms?

> +{
> +    WebKitWebView* webView;
> +    AtkObject *obj;

'*' on the wrong side.

> +    AtkObject *obj;

Same.

Kindly fix before landing. r=me.
Comment 8 Jan Alonzo 2009-10-28 00:45:39 PDT
Comment on attachment 41949 [details]
Add ability for platforms to ignore objects - Cleaned up comments as per review

LGTM. r=me.
Comment 9 Xan Lopez 2009-10-28 03:16:06 PDT
(In reply to comment #7)
> (From update of attachment 41950 [details])
> > +static void test_webkit_atk_get_text_at_offset_forms(void)
> 
> forms?

as in HTML forms.

> 
> > +{
> > +    WebKitWebView* webView;
> > +    AtkObject *obj;
> 
> '*' on the wrong side.
> 
> > +    AtkObject *obj;
> 
> Same.

Right, this was already in the file and I was just moving it around, but good catch.

> 
> Kindly fix before landing. r=me.
Comment 10 Xan Lopez 2009-10-28 03:20:29 PDT
Comment on attachment 41950 [details]
textcontrolsatk.diff

Landed in r50208, clearing flags.
Comment 11 WebKit Commit Bot 2009-10-28 10:02:23 PDT
Comment on attachment 41949 [details]
Add ability for platforms to ignore objects - Cleaned up comments as per review

Clearing flags on attachment: 41949

Committed r50220: <http://trac.webkit.org/changeset/50220>
Comment 12 WebKit Commit Bot 2009-10-28 10:02:28 PDT
All reviewed patches have been landed.  Closing bug.
Comment 13 Joanmarie Diggs (irc: joanie) 2009-10-28 10:14:26 PDT
Re-opening to address the removal of the extraneous object of ROLE_PANEL.
Comment 14 Joanmarie Diggs (irc: joanie) 2009-10-28 13:32:13 PDT
Created attachment 42052 [details]
Remove the extraneous panel

Another piece of the puzzle. This patch removes the offending panel in question. It does NOT address the additional object of ROLE_TEXT. Different patch at the very least. And arguably, a different bug. :-)
Comment 15 Xan Lopez 2009-10-28 13:43:05 PDT
Comment on attachment 42052 [details]
Remove the extraneous panel

> AccessibilityObjectPlatformInclusion AccessibilityObject::accessibilityPlatformIncludesObject() const
> {
>+    AccessibilityObject* parent = parentObject();
>+    if (not parent)

Come again?

>+        return DefaultBehavior;

The only case where this happens is with the WebView itself, right?

>+
>     // When a list item is made up entirely of children (e.g. paragraphs)
>     // the list item gets ignored. We need it.
>-    if (isGroup()) {
>-        AccessibilityObject* parent = parentObject();
>-        if (parent && parent->isList())
>-            return IncludeObject;
>-    }
>+    if (isGroup() && parent->isList())
>+        return IncludeObject;
>+
>+    // Entries and password fields have extraneous children which we want to ignore.
>+    if (parent->isPasswordField() || parent->isTextControl())
>+        return IgnoreObject;
> 
>     return DefaultBehavior;
> }
>-- 
>1.6.3.3
>

Looks good to me otherwise, but r- because of the "not".
Comment 16 Joanmarie Diggs (irc: joanie) 2009-10-28 13:57:04 PDT
Created attachment 42053 [details]
Remove the extraneous panel - Take 2

> > {
> >+    AccessibilityObject* parent = parentObject();
> >+    if (not parent)
> 
> Come again?

Oh hell... Sorry. :-( 3 years of Python; 3 weeks of C++.

> The only case where this happens is with the WebView itself, right?

That I'm aware of, yet. But given some other hierarchical bogusty I'm seeing (but have not yet pinned down), I wouldn't bet money that this is the only case. Not yet anyway.
Comment 17 Xan Lopez 2009-10-28 14:12:29 PDT
Comment on attachment 42053 [details]
Remove the extraneous panel - Take 2

r=me
Comment 18 Xan Lopez 2009-10-28 14:12:58 PDT
(In reply to comment #16)
> Created an attachment (id=42053) [details]
> Remove the extraneous panel - Take 2
> 
> > > {
> > >+    AccessibilityObject* parent = parentObject();
> > >+    if (not parent)
> > 
> > Come again?
> 
> Oh hell... Sorry. :-( 3 years of Python; 3 weeks of C++.

Let the compiler catch them for you! ;)

> 
> > The only case where this happens is with the WebView itself, right?
> 
> That I'm aware of, yet. But given some other hierarchical bogusty I'm seeing
> (but have not yet pinned down), I wouldn't bet money that this is the only
> case. Not yet anyway.
Comment 19 WebKit Commit Bot 2009-10-28 14:36:47 PDT
Comment on attachment 42053 [details]
Remove the extraneous panel - Take 2

Clearing flags on attachment: 42053

Committed r50238: <http://trac.webkit.org/changeset/50238>
Comment 20 WebKit Commit Bot 2009-10-28 14:36:52 PDT
All reviewed patches have been landed.  Closing bug.