Bug 25897

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?

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 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 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. :)

Created attachment 41949 [details] Add ability for platforms to ignore objects - Cleaned up comments as per review Nits addressed. Thanks!

Created attachment 41950 [details] textcontrolsatk.diff Make text controls implement the text interfaces properly, a small piece in the puzzle.

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 on attachment 41949 [details] Add ability for platforms to ignore objects - Cleaned up comments as per review LGTM. r=me.

(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 on attachment 41950 [details] textcontrolsatk.diff Landed in r50208, clearing flags.

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>

All reviewed patches have been landed. Closing bug.

Re-opening to address the removal of the extraneous object of ROLE_PANEL.

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 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".

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 on attachment 42053 [details] Remove the extraneous panel - Take 2 r=me

(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 on attachment 42053 [details] Remove the extraneous panel - Take 2 Clearing flags on attachment: 42053 Committed r50238: <http://trac.webkit.org/changeset/50238>

All reviewed patches have been landed. Closing bug.