Bug 197222 - Clean up dependencies with accessibility and UIKit
Summary: Clean up dependencies with accessibility and UIKit
Status: RESOLVED DUPLICATE of bug 197279
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Megan Gardner
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2019-04-23 17:52 PDT by Megan Gardner
Modified: 2019-04-25 13:13 PDT (History)
16 users (show)

See Also:


Attachments
Patch (13.92 KB, patch)
2019-04-23 17:56 PDT, Megan Gardner
no flags Details | Formatted Diff | Diff
Patch (11.86 KB, patch)
2019-04-23 18:29 PDT, Megan Gardner
no flags Details | Formatted Diff | Diff
Patch (12.00 KB, patch)
2019-04-24 11:16 PDT, Megan Gardner
no flags Details | Formatted Diff | Diff
Patch (12.06 KB, patch)
2019-04-24 13:30 PDT, Megan Gardner
no flags Details | Formatted Diff | Diff
Patch (12.05 KB, patch)
2019-04-24 13:52 PDT, Megan Gardner
no flags Details | Formatted Diff | Diff
Patch (12.05 KB, patch)
2019-04-24 14:49 PDT, Megan Gardner
no flags Details | Formatted Diff | Diff
Patch (11.91 KB, patch)
2019-04-24 15:04 PDT, Megan Gardner
no flags Details | Formatted Diff | Diff
Patch (11.02 KB, patch)
2019-04-25 11:31 PDT, Megan Gardner
no flags Details | Formatted Diff | Diff
Patch (10.93 KB, patch)
2019-04-25 11:42 PDT, Megan Gardner
megan_gardner: review?
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Megan Gardner 2019-04-23 17:52:06 PDT
Clean up dependencies with accessibility and UIKit
Comment 1 Megan Gardner 2019-04-23 17:56:38 PDT
Created attachment 368093 [details]
Patch
Comment 2 chris fleizach 2019-04-23 18:02:51 PDT
Comment on attachment 368093 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=368093&action=review

> Source/WebCore/accessibility/ios/WebAccessibilityObjectWrapperIOS.mm:1184
>  {

I don’t think this change is correct. Ax is expecting this method name
Comment 3 Tim Horton 2019-04-23 18:07:20 PDT
Comment on attachment 368093 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=368093&action=review

>> Source/WebCore/accessibility/ios/WebAccessibilityObjectWrapperIOS.mm:1184
>>  {
> 
> I don’t think this change is correct. Ax is expecting this method name

Yeah
Comment 4 Megan Gardner 2019-04-23 18:29:14 PDT
Created attachment 368096 [details]
Patch
Comment 5 Simon Fraser (smfr) 2019-04-23 18:43:50 PDT
Comment on attachment 368096 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=368096&action=review

> Source/WebKit/Platform/spi/ios/UIKitSPI.h:1074
> +#if HAVE_ACCESSIBILITY_DEFINED_IN_UIKIT

It's odd to use the term "defined" here (too easily confused with preprocessor macro defining). If it's only about UIAccessibilityScrollDirection and NSAttachmentCharacter, maybe there should be two HAVE_ with better names.
Comment 6 Megan Gardner 2019-04-24 11:16:56 PDT
Created attachment 368147 [details]
Patch
Comment 7 chris fleizach 2019-04-24 12:06:23 PDT
Comment on attachment 368147 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=368147&action=review

> Source/WebCore/accessibility/ios/WebAccessibilityObjectWrapperIOS.mm:1184
> +#pragma clang diagnostic ignored "-Wdeprecated-implementations"

whats the warning that gets output? the actual header says

@property(nullable, nonatomic, copy) NSArray *accessibilityHeaderElements UIKIT_AVAILABLE_TVOS_ONLY(9_0);

but it shouldn't stop us from implementing it right?

> Source/WebKit/Platform/spi/ios/UIKitSPI.h:1087
> +static NSString * const UIAccessibilityTextAttributeContext = @"UIAccessibilityTextAttributeContext";

these are in iOS13 so we could also just do a > IOS13 #if where we use them

[attrString addAttribute:PAL::get_UIKit_UIAccessibilityTextAttributeContext() value:PAL::get_UIKit_UIAccessibilityTextualContextSourceCode() range:range];

> Tools/ChangeLog:10
> +        * DumpRenderTree/ios/AccessibilityUIElementIOS.mm:

where there any tool changes, not seeing them here
Comment 8 Megan Gardner 2019-04-24 13:30:17 PDT
Created attachment 368168 [details]
Patch
Comment 9 Megan Gardner 2019-04-24 13:52:52 PDT
Created attachment 368173 [details]
Patch
Comment 10 Megan Gardner 2019-04-24 14:49:29 PDT
Created attachment 368179 [details]
Patch
Comment 11 Build Bot 2019-04-24 14:52:39 PDT
Attachment 368179 [details] did not pass style-queue:


ERROR: Source/WebKit/Platform/spi/ios/UIKitSPI.h:1073:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/WebKit/Platform/spi/ios/UIKitSPI.h:1092:  Should have a space between // and comment  [whitespace/comments] [4]
Total errors found: 2 in 11 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 12 Megan Gardner 2019-04-24 15:04:43 PDT
Created attachment 368184 [details]
Patch
Comment 13 Build Bot 2019-04-24 15:08:47 PDT
Attachment 368184 [details] did not pass style-queue:


ERROR: Source/WebCore/PAL/pal/spi/ios/UIKitSPI.h:150:  Should have a space between // and comment  [whitespace/comments] [4]
ERROR: Source/WebCore/PAL/pal/spi/ios/UIKitSPI.h:169:  Should have a space between // and comment  [whitespace/comments] [4]
Total errors found: 2 in 11 files


If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 14 chris fleizach 2019-04-25 11:06:52 PDT
Don't feel like current patch addresses any of this feedback

(In reply to chris fleizach from comment #7)
> Comment on attachment 368147 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=368147&action=review
> 
> > Source/WebCore/accessibility/ios/WebAccessibilityObjectWrapperIOS.mm:1184
> > +#pragma clang diagnostic ignored "-Wdeprecated-implementations"
> 
> whats the warning that gets output? the actual header says
> 
> @property(nullable, nonatomic, copy) NSArray *accessibilityHeaderElements
> UIKIT_AVAILABLE_TVOS_ONLY(9_0);
> 
> but it shouldn't stop us from implementing it right?
> 
> > Source/WebKit/Platform/spi/ios/UIKitSPI.h:1087
> > +static NSString * const UIAccessibilityTextAttributeContext = @"UIAccessibilityTextAttributeContext";
> 
> these are in iOS13 so we could also just do a > IOS13 #if where we use them
> 
> [attrString
> addAttribute:PAL::get_UIKit_UIAccessibilityTextAttributeContext()
> value:PAL::get_UIKit_UIAccessibilityTextualContextSourceCode() range:range];
> 
> > Tools/ChangeLog:10
> > +        * DumpRenderTree/ios/AccessibilityUIElementIOS.mm:
> 
> where there any tool changes, not seeing them here
Comment 15 Megan Gardner 2019-04-25 11:31:56 PDT
Created attachment 368253 [details]
Patch
Comment 16 Megan Gardner 2019-04-25 11:42:42 PDT
Created attachment 368255 [details]
Patch
Comment 17 Megan Gardner 2019-04-25 13:13:17 PDT
https://bugs.webkit.org/show_bug.cgi?id=197279

*** This bug has been marked as a duplicate of bug 197279 ***