Summary: | AX: Move Mac subrole logic to new subrolePlatformString AXCoreObject interface method | ||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Tyler Wilcock <tyler_w> | ||||||||||||||||
Component: | Accessibility | Assignee: | Tyler Wilcock <tyler_w> | ||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||
Severity: | Normal | CC: | aboxhall, andresg_22, apinheiro, cfleizach, dmazzoni, ews-watchlist, jcraig, jdiggs, samuel_white, webkit-bug-importer | ||||||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||||||
Version: | WebKit Nightly Build | ||||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||||
OS: | Unspecified | ||||||||||||||||||
Attachments: |
|
Description
Tyler Wilcock
2022-01-20 12:07:50 PST
Created attachment 449601 [details]
Patch
Created attachment 449605 [details]
Patch
Comment on attachment 449605 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=449605&action=review > Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm:2020 > + return Accessibility::retrieveValueFromMainThread<String>([protectedSelf = retainPtr(self)] () -> String { should we cache this rather than hitting main thread? Created attachment 449638 [details]
Patch
Created attachment 449639 [details]
Patch
Comment on attachment 449639 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=449639&action=review > Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm:1807 > + && (!backingObject->isModel() || backingObject->modelElementChildren().isEmpty()) I think even if the model is empty we want to return the special subrole so that VO can land on it directly and know that it's a model Created attachment 449658 [details]
Patch
(In reply to chris fleizach from comment #7) > Comment on attachment 449639 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=449639&action=review > > > Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm:1807 > > + && (!backingObject->isModel() || backingObject->modelElementChildren().isEmpty()) > > I think even if the model is empty we want to return the special subrole so > that VO can land on it directly and know that it's a model Fixed. (In reply to Tyler Wilcock from comment #8) > Created attachment 449658 [details] > Patch --- a/Source/WebCore/accessibility/AccessibilityObject.cpp +++ a/Source/WebCore/accessibility/AccessibilityObject.cpp +String AccessibilityObject::subrolePlatformString() const +{ + return String(); +} + I think we do these hallowed implementations as inlines in the header. --- a/Source/WebCore/accessibility/isolatedtree/mac/AXIsolatedObjectMac.mm +++ a/Source/WebCore/accessibility/isolatedtree/mac/AXIsolatedObjectMac.mm + setProperty(AXPropertyName::SubrolePlatformString, object.subrolePlatformString().isolatedCopy()); We can do this in the cross-platform initializeData since it is the same for all platforms. --- a/Source/WebCore/accessibility/mac/AccessibilityObjectMac.mm +++ a/Source/WebCore/accessibility/mac/AccessibilityObjectMac.mm +String AccessibilityObject::subrolePlatformString() const +{ This can not be implemented twice in AccessibilityObject.cpp and Mac.mm. --- a/Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm +++ a/Source/WebCore/accessibility/mac/WebAccessibilityObjectWrapperMac.mm + if (subrole != String()) + return subrole; Change to if (!subrole.isEmpty()) Created attachment 449684 [details]
Patch
> +String AccessibilityObject::subrolePlatformString() const
> +{
> + return String();
> +}
> +
> I think we do these hallowed implementations as inlines in the header.
> --- a/Source/WebCore/accessibility/mac/AccessibilityObjectMac.mm
> +++ a/Source/WebCore/accessibility/mac/AccessibilityObjectMac.mm
>
> +String AccessibilityObject::subrolePlatformString() const
> +{
>
> This can not be implemented twice in AccessibilityObject.cpp and Mac.mm.
The patch diff doesn't show enough context for you to have seen this, but it's not implemented twice. I added the hollow implementation of subrolePlatformString to the .cpp source file to match how we do it for roles, as that is already wrapped in an #if !PLATFORM(MAC) guard:
#if !PLATFORM(MAC)
String AccessibilityObject::rolePlatformString() const
{
return Accessibility::roleToPlatformString(roleValue());
}
String AccessibilityObject::rolePlatformDescription() const
{
// FIXME: implement in other platforms.
return String();
}
String AccessibilityObject::subrolePlatformString() const
{
return String();
}
#endif
Then we provide the real implementation in AccessibilityObjectMac.mm.
I can still change it if you'd like, though.
Fixed your other two comments, thanks!
Created attachment 449686 [details]
Patch
Committed r288390 (246287@main): <https://commits.webkit.org/246287@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 449686 [details]. |