WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
Bug 25897
[Gtk] Extraneous object of ROLE_PANEL in hierarchy for entries
https://bugs.webkit.org/show_bug.cgi?id=25897
Summary
[Gtk] Extraneous object of ROLE_PANEL in hierarchy for entries
Joanmarie Diggs
Reported
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.
Attachments
Add ability for platforms to ignore objects
(9.75 KB, patch)
2009-10-26 11:27 PDT
,
Joanmarie Diggs
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
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
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
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
no flags
Details
Formatted Diff
Diff
Show Obsolete
(3)
View All
Add attachment
proposed patch, testcase, etc.
Joanmarie Diggs
Comment 1
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?
Joanmarie Diggs
Comment 2
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!
Jan Alonzo
Comment 3
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. :)
Jan Alonzo
Comment 4
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. :)
Joanmarie Diggs
Comment 5
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!
Xan Lopez
Comment 6
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.
Jan Alonzo
Comment 7
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.
Jan Alonzo
Comment 8
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.
Xan Lopez
Comment 9
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.
Xan Lopez
Comment 10
2009-10-28 03:20:29 PDT
Comment on
attachment 41950
[details]
textcontrolsatk.diff Landed in
r50208
, clearing flags.
WebKit Commit Bot
Comment 11
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
>
WebKit Commit Bot
Comment 12
2009-10-28 10:02:28 PDT
All reviewed patches have been landed. Closing bug.
Joanmarie Diggs
Comment 13
2009-10-28 10:14:26 PDT
Re-opening to address the removal of the extraneous object of ROLE_PANEL.
Joanmarie Diggs
Comment 14
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. :-)
Xan Lopez
Comment 15
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".
Joanmarie Diggs
Comment 16
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.
Xan Lopez
Comment 17
2009-10-28 14:12:29 PDT
Comment on
attachment 42053
[details]
Remove the extraneous panel - Take 2 r=me
Xan Lopez
Comment 18
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.
WebKit Commit Bot
Comment 19
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
>
WebKit Commit Bot
Comment 20
2009-10-28 14:36:52 PDT
All reviewed patches have been landed. Closing bug.
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug