Bug 252193 – AX: Move the AttributedString for TextMarkerRange functionality out of the Mac wrapper into the AXCoreObject implementation so that it can be cached in isolated tree mode.

• Component: Accessibility
• Status: RESOLVED FIXED

[Andres Gonzalez]

Add it to the AXCoreObject interface and provide a common implementation for Mac and iOS.

[Radar WebKit Bug Importer]

<rdar://problem/105416277>

[Andres Gonzalez]

Created attachment 464979 [details]
Patch

[Tyler Wilcock]

Comment on attachment 464979 [details]
Patch

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

> Source/WebCore/accessibility/cocoa/AccessibilityObjectCocoa.mm:2
> + * Copyright (C) 2019 Apple Inc. All rights reserved.

I know this is largely a copy-paste, but might as well update the copyright year to 2023 since we're adding this new file.

> Source/WebCore/accessibility/cocoa/AccessibilityObjectCocoa.mm:43
> +PlainTextRange::PlainTextRange(NSRange r)

I think WebKit coding style doesn't allow for shortening variable names (so this should probably be `range` rather than `r`).

[Tyler Wilcock]

Comment on attachment 464979 [details]
Patch

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

> COMMIT_MESSAGE:1
> +AX: Move the AttributedString for TextMarkerRange functionality out of the Mac wrapper into the AX objet implementation so that
 it can be cached in isolated tree mode.
 

Typo — "AX objet"

[Andres Gonzalez]

(In reply to Tyler Wilcock from comment #3)
> Comment on attachment 464979 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=464979&action=review
> 
> > Source/WebCore/accessibility/cocoa/AccessibilityObjectCocoa.mm:2
> > + * Copyright (C) 2019 Apple Inc. All rights reserved.
> 
> I know this is largely a copy-paste, but might as well update the copyright
> year to 2023 since we're adding this new file.

It is not a new file, it was mac/AccessibilityObjectMac.mm and now is cocoa/AccessibilityObjectCocoa.mm (with additions), but git refuses to think so despite using git mv ... Changed the copyright year nevertheless, although I'm not sure what the policy is regarding file renames + additions.
> 
> > Source/WebCore/accessibility/cocoa/AccessibilityObjectCocoa.mm:43
> > +PlainTextRange::PlainTextRange(NSRange r)
> 
> I think WebKit coding style doesn't allow for shortening variable names (so
> this should probably be `range` rather than `r`).

Fixed, this is part of the old content of the file.

[Andres Gonzalez]

(In reply to Tyler Wilcock from comment #4)
> Comment on attachment 464979 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=464979&action=review
> 
> > COMMIT_MESSAGE:1
> > +AX: Move the AttributedString for TextMarkerRange functionality out of the Mac wrapper into the AX objet implementation so that
>  it can be cached in isolated tree mode.
>  
> 
> Typo — "AX objet"

Fixed, AXCoreObject to be precise.

[Andres Gonzalez]

Created attachment 465059 [details]
Patch

[Andres Gonzalez]

Created attachment 465061 [details]
Patch

Fixes merge problems.

[Tyler Wilcock]

Comment on attachment 465061 [details]
Patch

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

Looks like your patch message is missing the block listing modified files.

> COMMIT_MESSAGE:11
> +- Lots of code cleanup and strenghening.

Typo — strenghening should be strengthening.

> Source/WebCore/accessibility/ios/AccessibilityObjectIOS.mm:203
> +    auto matchFunc = [] (const AXCoreObject& axObject) {

Can this be typed as const AccessibilityObject&?

> Source/WebCore/accessibility/ios/AccessibilityObjectIOS.mm:207
> +    if (const auto* parent = Accessibility::findAncestor<AXCoreObject>(*object, true, WTFMove(matchFunc)))

Can this be Accessibility::findAncestor<AccessibilityObject> rather than <AXCoreObject>? This code checks DOM nodes which isn't going to work for isolated objects, so would be nice to type this specifically if possible.

[Andres Gonzalez]

Created attachment 465147 [details]
Patch

[Andres Gonzalez]

(In reply to Tyler Wilcock from comment #9)
> Comment on attachment 465061 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=465061&action=review
> 
> Looks like your patch message is missing the block listing modified files.

Yes, no easy way that I know of to recreate the list of modified files and functions after many commit amends. git show shows all the differences so the list that the WebKit script creates is redundant in that sense.
> 
> > COMMIT_MESSAGE:11
> > +- Lots of code cleanup and strenghening.
> 
> Typo — strenghening should be strengthening.

Fixed.
> 
> > Source/WebCore/accessibility/ios/AccessibilityObjectIOS.mm:203
> > +    auto matchFunc = [] (const AXCoreObject& axObject) {
> 
> Can this be typed as const AccessibilityObject&?
> 
> > Source/WebCore/accessibility/ios/AccessibilityObjectIOS.mm:207
> > +    if (const auto* parent = Accessibility::findAncestor<AXCoreObject>(*object, true, WTFMove(matchFunc)))
> 
> Can this be Accessibility::findAncestor<AccessibilityObject> rather than
> <AXCoreObject>? This code checks DOM nodes which isn't going to work for
> isolated objects, so would be nice to type this specifically if possible.

Fixed.

[Tyler Wilcock]

(In reply to Andres Gonzalez from comment #11)
> (In reply to Tyler Wilcock from comment #9)
> > Comment on attachment 465061 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=465061&action=review
> > 
> > Looks like your patch message is missing the block listing modified files.
> 
> Yes, no easy way that I know of to recreate the list of modified files and
> functions after many commit amends. git show shows all the differences so
> the list that the WebKit script creates is redundant in that sense.
You can do it with this set of steps:

1. Copy your current commit message to Notes / TextEdit / whatever.
2. Run `git reset HEAD^`. This undoes your head commit and unstages all the changes.
3. git add .
4. git commit
5. Notice all the changed files are back. Copy-paste the previous commit message you saved.

There might be an easier way, but this works fine.

[Andres Gonzalez]

(In reply to Tyler Wilcock from comment #12)
> (In reply to Andres Gonzalez from comment #11)
> > (In reply to Tyler Wilcock from comment #9)
> > > Comment on attachment 465061 [details]
> > > Patch
> > > 
> > > View in context:
> > > https://bugs.webkit.org/attachment.cgi?id=465061&action=review
> > > 
> > > Looks like your patch message is missing the block listing modified files.
> > 
> > Yes, no easy way that I know of to recreate the list of modified files and
> > functions after many commit amends. git show shows all the differences so
> > the list that the WebKit script creates is redundant in that sense.
> You can do it with this set of steps:
> 
> 1. Copy your current commit message to Notes / TextEdit / whatever.
> 2. Run `git reset HEAD^`. This undoes your head commit and unstages all the
> changes.
> 3. git add .
> 4. git commit
> 5. Notice all the changed files are back. Copy-paste the previous commit
> message you saved.
> 
> There might be an easier way, but this works fine.

thanks for the tip, but I don't really think it is worth the trouble in this case and many others where you don't really need to comment in the commit message at that level of granularity. If the change doesn't make sense in the code, then you have a problem in the code or needs to be better commented in the code itlself, not in the commit message. In some cases it may be good to have some comments in individual functions in the commit message, but not in most cases. Just think it is a vestige of the ChangeLogs predating git.

[EWS]

Committed 260800@main (89395210e335): <https://commits.webkit.org/260800@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 465147 [details].