Bug 263735 – AX: Sometimes unable to see play/pause animation context menu item when setting is toggled

• Status: RESOLVED
• Resolution: FIXED
• Component: Accessibility

[Joshua Hoffman]

When the Autoplay Animated Images setting is toggled off and on, the setting often remains stale, so the context menu item does not reappear when it should.

[Radar WebKit Bug Importer]

<rdar://problem/117543410>

[Joshua Hoffman]

rdar://117215059

[Joshua Hoffman]

Created attachment 468355 [details]
Patch

[chris fleizach]

Comment on attachment 468355 [details]
Patch

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

> Source/WebCore/page/Page.h:706
>      bool imageAnimationEnabled() const { return m_imageAnimationEnabled; }

why isn't the value of imageAnimationEnabled() sufficient for this? it ends up just being the inverse of whatever is set for imageAnimationEnabled

[Tyler Wilcock]

Comment on attachment 468355 [details]
Patch

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

> Source/WebCore/page/Page.cpp:2186
> +
> +void Page::setSystemAllowsAnimationControls(bool isAllowed)
> +{
> +    m_systemAllowsAnimationControls = isAllowed;
> +}
>  #endif

Seems like this block has grown long enough to comment this #endif.

#endif // PLATFORM(FOO)

[Joshua Hoffman]

Comment on attachment 468355 [details]
Patch

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

>> Source/WebCore/page/Page.cpp:2186
>>  #endif
> 
> Seems like this block has grown long enough to comment this #endif.
> 
> #endif // PLATFORM(FOO)

Will add that!

>> Source/WebCore/page/Page.h:706
>>      bool imageAnimationEnabled() const { return m_imageAnimationEnabled; }
> 
> why isn't the value of imageAnimationEnabled() sufficient for this? it ends up just being the inverse of whatever is set for imageAnimationEnabled

m_imageAnimationEnabled can be true even with the preference is off (for example, if the user has clicked "Play all animations"). See: anywhere we call setImageAnimationEnabled.

[Joshua Hoffman]

Created attachment 468358 [details]
Patch

[chris fleizach]

Comment on attachment 468358 [details]
Patch

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

> Source/WebCore/page/Page.h:1240
> +    bool m_systemAllowsAnimationControls { false };

should this concept be called

m_imageAnimationControlsEnabled

[Joshua Hoffman]

(In reply to chris fleizach from comment #8)
> Comment on attachment 468358 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=468358&action=review
> 
> > Source/WebCore/page/Page.h:1240
> > +    bool m_systemAllowsAnimationControls { false };
> 
> should this concept be called
> 
> m_imageAnimationControlsEnabled

That does make sense, but I worry it is too similar to ImageAnimationControlEnabled, a preference which uses the ACCESSIBILITY_ANIMATION_CONTROL compile flag to determine if we should allow animation controls at all (used alongside the AX preference).

[chris fleizach]

(In reply to Joshua Hoffman from comment #9)
> (In reply to chris fleizach from comment #8)
> > Comment on attachment 468358 [details]
> > Patch
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=468358&action=review
> > 
> > > Source/WebCore/page/Page.h:1240
> > > +    bool m_systemAllowsAnimationControls { false };
> > 
> > should this concept be called
> > 
> > m_imageAnimationControlsEnabled
> 
> That does make sense, but I worry it is too similar to
> ImageAnimationControlEnabled, a preference which uses the
> ACCESSIBILITY_ANIMATION_CONTROL compile flag to determine if we should allow
> animation controls at all (used alongside the AX preference).

M_systemImageAnimationEnabled ?

[Joshua Hoffman]

(In reply to chris fleizach from comment #10)
> (In reply to Joshua Hoffman from comment #9)
> > (In reply to chris fleizach from comment #8)
> > > Comment on attachment 468358 [details]
> > > Patch
> > > 
> > > View in context:
> > > https://bugs.webkit.org/attachment.cgi?id=468358&action=review
> > > 
> > > > Source/WebCore/page/Page.h:1240
> > > > +    bool m_systemAllowsAnimationControls { false };
> > > 
> > > should this concept be called
> > > 
> > > m_imageAnimationControlsEnabled
> > 
> > That does make sense, but I worry it is too similar to
> > ImageAnimationControlEnabled, a preference which uses the
> > ACCESSIBILITY_ANIMATION_CONTROL compile flag to determine if we should allow
> > animation controls at all (used alongside the AX preference).
> 
> M_systemImageAnimationEnabled ?

works for me!

[Joshua Hoffman]

Created attachment 468362 [details]
Patch

[Tyler Wilcock]

Comment on attachment 468362 [details]
Patch

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

> Source/WebCore/page/Page.h:1240
> +    bool m_systemImageAnimationEnabled { false };

This new name and the default value of false feel inconsistent to me. All systems / platforms allow animation by default, contrary to this default value of false. It also might be confusing to have two flags that are named so similarly (m_imageAnimationEnabled and m_systemImageAnimationEnabled). If we called this something like m_systemAllowsAnimationControls that might be more precise, and distinct from m_imageAnimationEnabled. What do y'all think?

[Joshua Hoffman]

(In reply to Tyler Wilcock from comment #13)
> Comment on attachment 468362 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=468362&action=review
> 
> > Source/WebCore/page/Page.h:1240
> > +    bool m_systemImageAnimationEnabled { false };
> 
> This new name and the default value of false feel inconsistent to me. All
> systems / platforms allow animation by default, contrary to this default
> value of false. It also might be confusing to have two flags that are named
> so similarly (m_imageAnimationEnabled and m_systemImageAnimationEnabled). If
> we called this something like m_systemAllowsAnimationControls that might be
> more precise, and distinct from m_imageAnimationEnabled. What do y'all think?

This does make sense—I think the more we can separate the two the better as to not make it more convoluted.

[Joshua Hoffman]

Created attachment 468372 [details]
Patch

[EWS]

Committed 269878@main (38398649280b): <https://commits.webkit.org/269878@main>

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