Bug 246737 – AX: Make animated image experimental feature respect preferences

• Component: Accessibility
• Status: RESOLVED FIXED

[chris fleizach]

Make animated images experimental feature respect preferences

[Radar WebKit Bug Importer]

<rdar://problem/101326045>

[chris fleizach]

Created attachment 463087 [details]
patch

[Darin Adler]

Comment on attachment 463087 [details]
patch

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

> Source/WebCore/PAL/pal/spi/cocoa/AccessibilitySupportSoftLink.cpp:32
>  SOFT_LINK_LIBRARY_FOR_SOURCE(PAL, libAccessibility)

Wouldn’t we want this in an #if ENABLE(ACCESSIBILITY_ISOLATED_TREE) || HAVE(ACCESSIBILITY_ANIMATED_IMAGES) block?

> Source/WebKit/WebProcess/WebPage/WebPage.cpp:785
> -
> +    

Let’s not add leading whitespace like this.

[Tyler Wilcock]

Comment on attachment 463087 [details]
patch

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

> Source/WebCore/page/SettingsBase.cpp:-491
> -void SettingsBase::setImageAnimationControlEnabledChanged()
> -{
> -    if (m_page && !m_page->settings().imageAnimationControlEnabled())
> -        m_page->setImageAnimationEnabled(true);
> -}

I'm not sure if we can remove this until we remove the experimental flag entirely. This code is important because it turns animations back on if the feature flag is disabled so users aren't stuck with a page of disabled animations.

> Source/WebCore/testing/Internals.cpp:1055
> -    if (auto* page = contextDocument() ? contextDocument()->page() : nullptr) {
> -        if (page->settings().imageAnimationControlEnabled())
> -            page->setImageAnimationEnabled(enabled);
> -    }
> +    if (auto* page = contextDocument() ? contextDocument()->page() : nullptr)
> +        page->setImageAnimationEnabled(enabled);

Why remove:

if (page->settings().imageAnimationControlEnabled())

here? Is it no longer necessary?

> Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:1129
> +    _page->process().updateAnimatedImagesState();

Where is updateAnimatedImagesState defined? I couldn't find it in this patch or in WebKit `main`.

[chris fleizach]

Comment on attachment 463087 [details]
patch

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

Apologies - did not mean to get official review on this yet, so I hadn't cleaned it up to that quality yet

>> Source/WebCore/PAL/pal/spi/cocoa/AccessibilitySupportSoftLink.cpp:32
>>  SOFT_LINK_LIBRARY_FOR_SOURCE(PAL, libAccessibility)
> 
> Wouldn’t we want this in an #if ENABLE(ACCESSIBILITY_ISOLATED_TREE) || HAVE(ACCESSIBILITY_ANIMATED_IMAGES) block?

👍

>> Source/WebCore/page/SettingsBase.cpp:-491
>> -}
> 
> I'm not sure if we can remove this until we remove the experimental flag entirely. This code is important because it turns animations back on if the feature flag is disabled so users aren't stuck with a page of disabled animations.

or they can just reload the page.

but I wanted opinions on this anyway...

basically the Client side is sending over the preference for whether this is on. that bit + feature flag determines if it's on

In this case here we only have access to the final state + the new experimental state... so basically you could have a different state than with the preference

then I wondered do we even need experimental flag if we're gaiting with a preference? thoughts?

>> Source/WebCore/testing/Internals.cpp:1055
>> +        page->setImageAnimationEnabled(enabled);
> 
> Why remove:
> 
> if (page->settings().imageAnimationControlEnabled())
> 
> here? Is it no longer necessary?

this is internals only. It seemed unnecessary to tie Internals to the experimental feature flag. do you have a strong opinion?

>> Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:1129
>> +    _page->process().updateAnimatedImagesState();
> 
> Where is updateAnimatedImagesState defined? I couldn't find it in this patch or in WebKit `main`.

good question...

>> Source/WebKit/WebProcess/WebPage/WebPage.cpp:785
>> +    
> 
> Let’s not add leading whitespace like this.

👍

[Tyler Wilcock]

(In reply to chris fleizach from comment #5)
> Comment on attachment 463087 [details]
> patch
> >> Source/WebCore/page/SettingsBase.cpp:-491
> >> -}
> > 
> > I'm not sure if we can remove this until we remove the experimental flag entirely. This code is important because it turns animations back on if the feature flag is disabled so users aren't stuck with a page of disabled animations.
> 
> or they can just reload the page.
> 
> but I wanted opinions on this anyway...
> 
> basically the Client side is sending over the preference for whether this is
> on. that bit + feature flag determines if it's on
> 
> In this case here we only have access to the final state + the new
> experimental state... so basically you could have a different state than
> with the preference
> 
> then I wondered do we even need experimental flag if we're gaiting with a
> preference? thoughts?
So if I understand correctly, you're saying we don't have access to the preference state in this context, and by allowing this function to set image animation to true when the feature flag is changed to disabled, the "image animation enabled" pref could be disabled while WebKit is actually allowing animations?

If so, I think that's the experimental feature flag functioning as expected and therefore fine behavior. The user themselves is the one changing the feature flag state, and is knowingly overriding their preference choice. I imagine they'd be surprised if it _didn't_ cause a change in the page.

I also imagine this feature flag should be deleted entirely (with "on" being the only behavior) before this setting is even exposed to users, making this problem moot. But maybe you had other plans?

What do you think?

> >> Source/WebCore/testing/Internals.cpp:1055
> >> +        page->setImageAnimationEnabled(enabled);
> > 
> > Why remove:
> > 
> > if (page->settings().imageAnimationControlEnabled())
> > 
> > here? Is it no longer necessary?
> 
> this is internals only. It seemed unnecessary to tie Internals to the
> experimental feature flag. do you have a strong opinion?
True. No strong opinion here — keeping or removing is fine.

[chris fleizach]

Comment on attachment 463087 [details]
patch

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

>>>> Source/WebCore/page/SettingsBase.cpp:-491
>>>> -}
>>> 
>>> I'm not sure if we can remove this until we remove the experimental flag entirely. This code is important because it turns animations back on if the feature flag is disabled so users aren't stuck with a page of disabled animations.
>> 
>> or they can just reload the page.
>> 
>> but I wanted opinions on this anyway...
>> 
>> basically the Client side is sending over the preference for whether this is on. that bit + feature flag determines if it's on
>> 
>> In this case here we only have access to the final state + the new experimental state... so basically you could have a different state than with the preference
>> 
>> then I wondered do we even need experimental flag if we're gaiting with a preference? thoughts?
> 
> So if I understand correctly, you're saying we don't have access to the preference state in this context, and by allowing this function to set image animation to true when the feature flag is changed to disabled, the "image animation enabled" pref could be disabled while WebKit is actually allowing animations?
> 
> If so, I think that's the experimental feature flag functioning as expected and therefore fine behavior. The user themselves is the one changing the feature flag state, and is knowingly overriding their preference choice. I imagine they'd be surprised if it _didn't_ cause a change in the page.
> 
> I also imagine this feature flag should be deleted entirely (with "on" being the only behavior) before this setting is even exposed to users, making this problem moot. But maybe you had other plans?
> 
> What do you think?

yea that was what I was thinking. this will go away anyway and wondering whether I should remove it as experimental with this PR

[Tyler Wilcock]

(In reply to chris fleizach from comment #7)
> yea that was what I was thinking. this will go away anyway and wondering
> whether I should remove it as experimental with this PR
Actually yeah, that makes sense. I'm good with deleting it.

[chris fleizach]

Created attachment 463116 [details]
Patch

[chris fleizach]

Created attachment 463118 [details]
Patch

[chris fleizach]

Created attachment 463136 [details]
Patch

[chris fleizach]

Created attachment 463151 [details]
Patch

[chris fleizach]

Created attachment 463172 [details]
Patch

[chris fleizach]

Created attachment 463186 [details]
Patch

[EWS]

Committed 255916@main (bc9fcf475055): <https://commits.webkit.org/255916@main>

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