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.
<rdar://problem/117543410>
rdar://117215059
Created attachment 468355 [details] Patch
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
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)
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.
Created attachment 468358 [details] Patch
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
(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).
(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 ?
(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!
Created attachment 468362 [details] Patch
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?
(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.
Created attachment 468372 [details] Patch
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].