Bug 204056 – AX: Implement isolated tree support for math objects

• Component: Accessibility
• Status: RESOLVED FIXED

[chris fleizach]

Implement isolated tree support for math objects

[Radar WebKit Bug Importer]

<rdar://problem/57054644>

[chris fleizach]

Created attachment 383238 [details]
patch

[chris fleizach]

Comment on attachment 383238 [details]
patch

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

> Source/WebCore/accessibility/isolatedtree/AXIsolatedTreeNode.cpp:-37
> -AXIsolatedObject::AXIsolatedObject(const AXCoreObject& object)

Removing the const here because some of the math objects access children() which is non-const

[chris fleizach]

Created attachment 383239 [details]
patch

[chris fleizach]

     setProperty(AXPropertyName::HelpText, object.helpTextAttributeValue().isolatedCopy());
+

AG: don't think the code style rules state this, but I would prefer (kindly
request) blank lines to have zero spaces. Otherwise VO reads "space, space, ..."
very annoying. There is an option in Xcode to automatically get rid off
trailing spaces, and that does the right thing.


-    static Ref<AXIsolatedObject> create(const AXCoreObject&);
+    static Ref<AXIsolatedObject> create(AXCoreObject&);

AG: Fine for now. But we should have the create function and the constructor to
take an AccessibilityObject, constructor marked as explicit. And a separate
copy constructor that takes an AXIsolatedObject.


+    AXIsolatedObject(AXCoreObject&);
+    void initializeAttributeData(AXCoreObject&);

AG: As in create, we should have an explicit constructor that takes an
AccessibilityObject, and a separate copy constructor. initializeAttributeData
should take an AccessibilityObject.

[chris fleizach]

(In reply to chris fleizach from comment #5)
>      setProperty(AXPropertyName::HelpText,
> object.helpTextAttributeValue().isolatedCopy());
> +
> 
> AG: don't think the code style rules state this, but I would prefer (kindly
> request) blank lines to have zero spaces. Otherwise VO reads "space, space,
> ..."
> very annoying. There is an option in Xcode to automatically get rid off
> trailing spaces, and that does the right thing.
> 
> 
> -    static Ref<AXIsolatedObject> create(const AXCoreObject&);
> +    static Ref<AXIsolatedObject> create(AXCoreObject&);
> 
> AG: Fine for now. But we should have the create function and the constructor
> to
> take an AccessibilityObject, constructor marked as explicit. And a separate
> copy constructor that takes an AXIsolatedObject.
> 
> 
> +    AXIsolatedObject(AXCoreObject&);
> +    void initializeAttributeData(AXCoreObject&);
> 
> AG: As in create, we should have an explicit constructor that takes an
> AccessibilityObject, and a separate copy constructor. initializeAttributeData
> should take an AccessibilityObject.

Talked with Andres and we discussed at this time we have to deal with the fact that children() is non-const so we need to handle this here because we're calling children() now for the math things

[chris fleizach]

Created attachment 383513 [details]
patch

[WebKit Commit Bot]

Comment on attachment 383513 [details]
patch

Clearing flags on attachment: 383513

Committed r252457: <https://trac.webkit.org/changeset/252457>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Darin Adler]

Comment on attachment 383513 [details]
patch

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

> Source/WebCore/accessibility/isolatedtree/AXIsolatedTreeNode.cpp:201
> +    AXID nodeID = WTF::switchOn(value,
> +        [&] (Optional<AXID> typedValue) {
> +            if (!typedValue)
> +                return InvalidAXID;
> +        return typedValue.value();
> +        },
> +        [] (auto&) { return InvalidAXID; }
> +    );

I don’t understand the reason behind the [&] capture; that mistake seems to be repeated over and over again in the other similar functions.

Also maybe we could write this in a way that's a little more readable using valueOr(InvalidAXID).

I don’t understand why we have both InvalidAXID and Optional<AXID>; two ways of doing the same thing. We should choose one idiom or the other rather than mixing the two. This here is extra code to translate between them.

Indentation mistake here. This return statement should be indented.

[chris fleizach]

(In reply to Darin Adler from comment #10)
> Comment on attachment 383513 [details]
> patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=383513&action=review
> 
> > Source/WebCore/accessibility/isolatedtree/AXIsolatedTreeNode.cpp:201
> > +    AXID nodeID = WTF::switchOn(value,
> > +        [&] (Optional<AXID> typedValue) {
> > +            if (!typedValue)
> > +                return InvalidAXID;
> > +        return typedValue.value();
> > +        },
> > +        [] (auto&) { return InvalidAXID; }
> > +    );
> 
> I don’t understand the reason behind the [&] capture; that mistake seems to
> be repeated over and over again in the other similar functions.
> 
> Also maybe we could write this in a way that's a little more readable using
> valueOr(InvalidAXID).
> 
> I don’t understand why we have both InvalidAXID and Optional<AXID>; two ways
> of doing the same thing. We should choose one idiom or the other rather than
> mixing the two. This here is extra code to translate between them.
> 
> Indentation mistake here. This return statement should be indented.

Thanks for your feedback. Will look into make those changes

[chris fleizach]

> Thanks for your feedback. Will look into make those changes

https://bugs.webkit.org/show_bug.cgi?id=204233