Summary: | AX: WKWebView does not shift Accessibility Focus for Catalyst | ||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Eric Liang <ericliang> | ||||||||||||||
Component: | Accessibility | Assignee: | Per Arne Vollan <pvollan> | ||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||
Severity: | Normal | CC: | aboxhall, apinheiro, bfulgham, cfleizach, commit-queue, dbates, dmazzoni, ews-watchlist, jcraig, jdiggs, pvollan, samuel_white, thorton, webkit-bug-importer | ||||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||||
Version: | WebKit Nightly Build | ||||||||||||||||
Hardware: | All | ||||||||||||||||
OS: | All | ||||||||||||||||
Attachments: |
|
Description
Eric Liang
2019-11-03 16:45:17 PST
Created attachment 382712 [details]
Patch
Comment on attachment 382712 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=382712&action=review > Source/WebKit/UIProcess/ios/WKContentView.mm:578 > + [self _updateRemoteAccessibilityRegistration:NO]; there are a number of places to cleanup. check the other places where we do accessibility cleanup You should also get a WebKit2 contributors point of view, like Daniel Bates or Brent Fulgham (In reply to chris fleizach from comment #3) > Comment on attachment 382712 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=382712&action=review > > > Source/WebKit/UIProcess/ios/WKContentView.mm:578 > > + [self _updateRemoteAccessibilityRegistration:NO]; > > there are a number of places to cleanup. check the other places where we do > accessibility cleanup Even though we do more cleanups in other places, if the process identifier didn't change, do we need to unregister the pid for ax? (In reply to chris fleizach from comment #3) > Comment on attachment 382712 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=382712&action=review > > > Source/WebKit/UIProcess/ios/WKContentView.mm:578 > > + [self _updateRemoteAccessibilityRegistration:NO]; > > there are a number of places to cleanup. check the other places where we do > accessibility cleanup the Mac version cleans up when 1) page closed, 2) process swaps or exits Comment on attachment 382712 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=382712&action=review >>>> Source/WebKit/UIProcess/ios/WKContentView.mm:578 >>>> + [self _updateRemoteAccessibilityRegistration:NO]; >>> >>> there are a number of places to cleanup. check the other places where we do accessibility cleanup >> >> Even though we do more cleanups in other places, if the process identifier didn't change, do we need to unregister the pid for ax? > > the Mac version cleans up when 1) page closed, 2) process swaps or exits ok I think that's right Comment on attachment 382712 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=382712&action=review > Source/WebKit/UIProcess/ios/WKContentView.mm:73 > +SOFT_LINK_FRAMEWORK(AppKit) > +SOFT_LINK_CLASS(AppKit, NSAccessibilityRemoteUIElement) Why are you softlinking AppKit? WebKit links AppKit directly. > Source/WebKit/UIProcess/ios/WKContentView.mm:79 > +@interface NSObject(NSAccessibilityRemoteUIElement_Private) > +- (void)registerRemoteUIProcessIdentifier:(pid_t)pid; > +- (void)unregisterRemoteUIProcessIdentifier:(pid_t)pid; > +@end > +#endif This should be in a SPI header, not here. Comment on attachment 382712 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=382712&action=review >> Source/WebKit/UIProcess/ios/WKContentView.mm:73 >> +SOFT_LINK_CLASS(AppKit, NSAccessibilityRemoteUIElement) > > Why are you softlinking AppKit? WebKit links AppKit directly. Correct me if I'm wrong, but this is the iOS code so I don't think we can link or import anything from AppKit because of unavailability build errors. (In reply to Eric Liang from comment #9) > Comment on attachment 382712 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=382712&action=review > > >> Source/WebKit/UIProcess/ios/WKContentView.mm:73 > >> +SOFT_LINK_CLASS(AppKit, NSAccessibilityRemoteUIElement) > > > > Why are you softlinking AppKit? WebKit links AppKit directly. > > Correct me if I'm wrong, but this is the iOS code so I don't think we can > link or import anything from AppKit because of unavailability build errors. Oh, macCatalyst might not link AppKit directly, that's a fair point. In that case, you should have -Softlinking headers like other frameworks. Created attachment 382757 [details]
Patch
Comment on attachment 382757 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=382757&action=review > Source/WebKit/UIProcess/ios/AppKitSoftLink.h:2 > + * Copyright (C) 2018 Apple Inc. All rights reserved. 2019 > Source/WebKit/UIProcess/ios/AppKitSoftLink.mm:2 > + * Copyright (C) 2018 Apple Inc. All rights reserved. 2019 Created attachment 382774 [details]
Patch
Comment on attachment 382774 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=382774&action=review > Source/WebKit/UIProcess/ios/AppKitSoftLink.h:38 > +@interface NSObject(NSAccessibilityRemoteUIElement_Private) > +- (void)registerRemoteUIProcessIdentifier:(pid_t)pid; > +- (void)unregisterRemoteUIProcessIdentifier:(pid_t)pid; > +@end These go in AppKitSPI, not AppKitSoftLink. Probably in a macCatalyst-only section. Also, the category name should have a space before the opening paren. Created attachment 382786 [details]
Patch
Comment on attachment 382786 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=382786&action=review > Source/WebCore/PAL/pal/spi/cocoa/NSAccessibilitySPI.h:66 > +SOFT_LINK_FRAMEWORK_FOR_HEADER(PAL, AppKit) > +SOFT_LINK_CLASS_FOR_HEADER(PAL, NSAccessibilityRemoteUIElement) You moved too much :) These (soft linking bits) should be in -SoftLinking.h/.mm files like they were. Just the forward declarations needed to move here. Other than that this looks good. Created attachment 382995 [details]
Patch
Comment on attachment 382995 [details] Patch Clearing flags on attachment: 382995 Committed r252175: <https://trac.webkit.org/changeset/252175> All reviewed patches have been landed. Closing bug. Reopening to attach new patch. Created attachment 383143 [details]
Patch
Comment on attachment 383143 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=383143&action=review > Source/WebKit/ChangeLog:3 > + AX: WKWebView does not shift Accessibility Focus for Catalyst seems like the wrong bug right? Comment on attachment 383143 [details]
Patch
Seems like a typo
|