Bug 204241 – AX: support more attributes for AXIsolatedTreeNode

• Component: Accessibility
• Status: RESOLVED FIXED

[chris fleizach]

Support more attributes for AXIsolatedTreeNode

    bool isChecked() 
    bool isEnabled() 
    bool isSelected() 
    bool isFocused()
    bool isHovered() 
    bool isIndeterminate() 
    bool isLoaded() 
    bool isMultiSelectable() 
    bool isOnScreen() 
    bool isPressed() 
    bool isUnvisited() 
    bool isVisited() 
    bool isRequired() 
    bool supportsRequiredAttribute() 
    bool isLinked() 
    bool isExpanded()
    bool isVisible() 
    bool isCollapsed() 
    void setIsExpanded(bool) 
    bool isSelectedOptionActive()

[Radar WebKit Bug Importer]

<rdar://problem/57237606>

[chris fleizach]

Created attachment 383637 [details]
patch

[chris fleizach]

Created attachment 383638 [details]
patch

[Andres Gonzalez]

(In reply to chris fleizach from comment #3)
> Created attachment 383638 [details]
> patch

--- a/Source/WebCore/accessibility/AccessibilityObjectInterface.h
+++ a/Source/WebCore/accessibility/AccessibilityObjectInterface.h
@@ -569,7 +569,7 @@ public:
     virtual bool isMultiSelectable() const = 0;
     // FIXME should need just one since onscreen should be !offscreen.
     virtual bool isOnScreen() const = 0;
-    virtual bool isOffScreen() const = 0;
+    virtual bool isOffScreen() const { return !isOnScreen(); }

Should we make it final? Do we need to keep both isOnScreen and isOffScreen? If so, let's remove the FIXME comment.

--- a/Source/WebCore/accessibility/isolatedtree/AXIsolatedTreeNode.h
+++ a/Source/WebCore/accessibility/isolatedtree/AXIsolatedTreeNode.h
@@ -41,6 +41,8 @@
 #include <wtf/Vector.h>
 #include <wtf/WeakPtr.h>

+#define AX_REQUIRES_IMPLEMENTATION ASSERT_NOT_REACHED
+
 namespace WebCore {

Shouldn't AX_REQUIRES_IMPLEMENTATION be in the WebCore namespace?

[Andres Gonzalez]

> (In reply to chris fleizach from comment #3)
> > Created attachment 383638 [details]
> > patch
> 
> --- a/Source/WebCore/accessibility/AccessibilityObjectInterface.h
> +++ a/Source/WebCore/accessibility/AccessibilityObjectInterface.h
> @@ -569,7 +569,7 @@ public:
>      virtual bool isMultiSelectable() const = 0;
>      // FIXME should need just one since onscreen should be !offscreen.
>      virtual bool isOnScreen() const = 0;
> -    virtual bool isOffScreen() const = 0;
> +    virtual bool isOffScreen() const { return !isOnScreen(); }
> 
> Should we make it final? Do we need to keep both isOnScreen and isOffScreen?
> If so, let's remove the FIXME comment.

The style checker complains about virtual and final in the same method declaration. And since this is the base class, you cannot have just final because the compiler requires virtual. So in a similar situation, I opted for having neither virtual nor final.

The question about keeping both methods and removing the FIXME is still valid.

> 
> --- a/Source/WebCore/accessibility/isolatedtree/AXIsolatedTreeNode.h
> +++ a/Source/WebCore/accessibility/isolatedtree/AXIsolatedTreeNode.h
> @@ -41,6 +41,8 @@
>  #include <wtf/Vector.h>
>  #include <wtf/WeakPtr.h>
> 
> +#define AX_REQUIRES_IMPLEMENTATION ASSERT_NOT_REACHED
> +
>  namespace WebCore {
> 
> Shouldn't AX_REQUIRES_IMPLEMENTATION be in the WebCore namespace?

[chris fleizach]

(In reply to Andres Gonzalez from comment #5)
> > (In reply to chris fleizach from comment #3)
> > > Created attachment 383638 [details]
> > > patch
> > 
> > --- a/Source/WebCore/accessibility/AccessibilityObjectInterface.h
> > +++ a/Source/WebCore/accessibility/AccessibilityObjectInterface.h
> > @@ -569,7 +569,7 @@ public:
> >      virtual bool isMultiSelectable() const = 0;
> >      // FIXME should need just one since onscreen should be !offscreen.
> >      virtual bool isOnScreen() const = 0;
> > -    virtual bool isOffScreen() const = 0;
> > +    virtual bool isOffScreen() const { return !isOnScreen(); }
> > 
> > Should we make it final? Do we need to keep both isOnScreen and isOffScreen?
> > If so, let's remove the FIXME comment.
> 
> The style checker complains about virtual and final in the same method
> declaration. And since this is the base class, you cannot have just final
> because the compiler requires virtual. So in a similar situation, I opted
> for having neither virtual nor final.
> 
> The question about keeping both methods and removing the FIXME is still
> valid.

There are overrides of both isOffscreen() and isOnscreen(). Untangling that seems out of scope for this patch. feel like it's probably better to leave as is for now

> 
> > 
> > --- a/Source/WebCore/accessibility/isolatedtree/AXIsolatedTreeNode.h
> > +++ a/Source/WebCore/accessibility/isolatedtree/AXIsolatedTreeNode.h
> > @@ -41,6 +41,8 @@
> >  #include <wtf/Vector.h>
> >  #include <wtf/WeakPtr.h>
> > 
> > +#define AX_REQUIRES_IMPLEMENTATION ASSERT_NOT_REACHED
> > +
> >  namespace WebCore {
> > 
> > Shouldn't AX_REQUIRES_IMPLEMENTATION be in the WebCore namespace?

[chris fleizach]

Created attachment 384060 [details]
patch

[EWS Watchlist]

Comment on attachment 384060 [details]
patch

Attachment 384060 [details] did not pass win-ews (win):
Output: https://webkit-queues.webkit.org/results/13271438

New failing tests:
fast/workers/worker-cloneport.html

[EWS Watchlist]

Created attachment 384083 [details]
Archive of layout-test-results from ews214 for win-future

The attached test failures were seen while running run-webkit-tests on the win-ews.
Bot: ews214  Port: win-future  Platform: CYGWIN_NT-10.0-17763-3.0.5-338.x86_64-x86_64-64bit

[chris fleizach]

Unrelated

(In reply to Build Bot from comment #9)
> Created attachment 384083 [details]
> Archive of layout-test-results from ews214 for win-future
> 
> The attached test failures were seen while running run-webkit-tests on the
> win-ews.
> Bot: ews214  Port: win-future  Platform:
> CYGWIN_NT-10.0-17763-3.0.5-338.x86_64-x86_64-64bit

[WebKit Commit Bot]

Comment on attachment 384060 [details]
patch

Clearing flags on attachment: 384060

Committed r252748: <https://trac.webkit.org/changeset/252748>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.