Adopt non-blinking cursor accessibility API
<rdar://problem/122473135>
rdar://118550285
Created attachment 469760 [details] Patch
Comment on attachment 469760 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=469760&action=review > Source/WebCore/platform/CaretAnimator.h:100 > + bool isBlinkingSuspended() const { return m_isBlinkingSuspended || (page() && page()->prefersNonBlinkingCursor()); } How often is this called? 60 times a second (every frame)? Every paint? If either is true, I fear that this will introduce a performance regression — we'd either want to do something like this to only compute page() once: auto* page = this->page(); return page && page->prefersNonBlinkingCursor(); But probably that won't be enough either, instead we may need to sync the state between the page -> every document in the page -> Document::m_selection -> FrameSelection::m_caretAnimator (this was only the thing I could find that hangs on to a caret animator)
Created attachment 469782 [details] Patch
(In reply to Joshua Hoffman from comment #5) > Created attachment 469782 [details] > Patch diff --git a/Source/WebCore/page/Page.h b/Source/WebCore/page/Page.h index 3a0d2b86bb9e..cdade05808ff 100644 --- a/Source/WebCore/page/Page.h +++ b/Source/WebCore/page/Page.h @@ -710,6 +710,11 @@ public: bool imageAnimationEnabled() const { return m_imageAnimationEnabled; } bool systemAllowsAnimationControls() const { return m_systemAllowsAnimationControls; } +#if ENABLE(ACCESSIBILITY_NON_BLINKING_CURSOR) + WEBCORE_EXPORT void setPrefersNonBlinkingCursor(bool enabled); +#endif + bool prefersNonBlinkingCursor() const { return m_prefersNonBlinkingCursor; }; AG: maybe you can put the definition of setPrefersNonBlinkingCursor also here instead of in the cpp, since these two are trivial accessors. + void userStyleSheetLocationChanged(); const String& userStyleSheet() const; @@ -1246,6 +1251,7 @@ private: bool m_systemAllowsAnimationControls { false }; // Elements containing animations that are individually playing (potentially overriding the page-wide m_imageAnimationEnabled state). WeakHashSet<HTMLImageElement, WeakPtrImplWithEventTargetData> m_individuallyPlayingAnimationElements; + bool m_prefersNonBlinkingCursor { false }; AG: wondering why we add the member variable and getter even when the feature is not enabled. TimerThrottlingState m_timerThrottlingState { TimerThrottlingState::Disabled }; MonotonicTime m_timerThrottlingStateLastChangedTime;
(In reply to Andres Gonzalez from comment #6) > (In reply to Joshua Hoffman from comment #5) > > Created attachment 469782 [details] > > Patch > > diff --git a/Source/WebCore/page/Page.h b/Source/WebCore/page/Page.h > index 3a0d2b86bb9e..cdade05808ff 100644 > --- a/Source/WebCore/page/Page.h > +++ b/Source/WebCore/page/Page.h > @@ -710,6 +710,11 @@ public: > bool imageAnimationEnabled() const { return m_imageAnimationEnabled; } > bool systemAllowsAnimationControls() const { return > m_systemAllowsAnimationControls; } > > +#if ENABLE(ACCESSIBILITY_NON_BLINKING_CURSOR) > + WEBCORE_EXPORT void setPrefersNonBlinkingCursor(bool enabled); > +#endif > + bool prefersNonBlinkingCursor() const { return > m_prefersNonBlinkingCursor; }; > > AG: maybe you can put the definition of setPrefersNonBlinkingCursor also > here instead of in the cpp, since these two are trivial accessors. Good point, that was leftover from a non-trivial implementation. I can fix that. > void userStyleSheetLocationChanged(); > const String& userStyleSheet() const; > > @@ -1246,6 +1251,7 @@ private: > bool m_systemAllowsAnimationControls { false }; > // Elements containing animations that are individually playing > (potentially overriding the page-wide m_imageAnimationEnabled state). > WeakHashSet<HTMLImageElement, WeakPtrImplWithEventTargetData> > m_individuallyPlayingAnimationElements; > + bool m_prefersNonBlinkingCursor { false }; > > AG: wondering why we add the member variable and getter even when the > feature is not enabled. You are right, I will move this behind the macro.
diff --git a/Source/WebCore/platform/CaretAnimator.h b/Source/WebCore/platform/CaretAnimator.h index 984e6df9ffc1..d14c3b24fb03 100644 --- a/Source/WebCore/platform/CaretAnimator.h +++ b/Source/WebCore/platform/CaretAnimator.h @@ -97,7 +97,11 @@ public: virtual String debugDescription() const = 0; virtual void setBlinkingSuspended(bool suspended) { m_isBlinkingSuspended = suspended; } - bool isBlinkingSuspended() const { return m_isBlinkingSuspended; } + bool isBlinkingSuspended() const { return m_isBlinkingSuspended || m_prefersNonBlinkingCursor; } + +#if ENABLE(ACCESSIBILITY_NON_BLINKING_CURSOR) + void setPrefersNonBlinkingCursor(bool enabled) { m_prefersNonBlinkingCursor = enabled; } +#endif virtual void setVisible(bool) = 0; @@ -110,7 +114,11 @@ protected: explicit CaretAnimator(CaretAnimationClient& client) : m_client(client) , m_blinkTimer(*this, &CaretAnimator::scheduleAnimation) - { } + { +#if ENABLE(ACCESSIBILITY_NON_BLINKING_CURSOR) + m_prefersNonBlinkingCursor = page() && page()->prefersNonBlinkingCursor(); +#endif + } virtual void updateAnimationProperties() = 0; @@ -141,6 +149,7 @@ private: bool m_isActive { false }; bool m_isBlinkingSuspended { false }; + bool m_prefersNonBlinkingCursor { false }; AG: same comment here. }; static inline CaretAnimator::PresentationProperties::BlinkState operator!(CaretAnimator::PresentationProperties::BlinkState blinkState)
diff --git a/Source/WebCore/testing/Internals.cpp b/Source/WebCore/testing/Internals.cpp index 5ad97a71653d..5f7678e62035 100644 --- a/Source/WebCore/testing/Internals.cpp +++ b/Source/WebCore/testing/Internals.cpp @@ -1132,6 +1132,17 @@ void Internals::pauseImageAnimation(HTMLImageElement& element) } #endif // ENABLE(ACCESSIBILITY_ANIMATION_CONTROL) +#if ENABLE(ACCESSIBILITY_NON_BLINKING_CURSOR) +void Internals::setPrefersNonBlinkingCursor(bool enabled) +{ + Document* document = contextDocument(); + if (!document || !document->frame()) + return; + + return document->frame()->selection().setPrefersNonBlinkingCursor(enabled); AG: extra blank line and extra return. can be written: auto* document = contextDocument(); if (document && document->frame()) document->frame()->selection().setPrefersNonBlinkingCursor(enabled); AG: is frame() a trivial accessor? +} +#endif + unsigned Internals::imagePendingDecodePromisesCountForTesting(HTMLImageElement& element) { return element.pendingDecodePromisesCountForTesting();
diff --git a/Source/WebKit/Shared/AccessibilityPreferences.h b/Source/WebKit/Shared/AccessibilityPreferences.h index 1c64367892d6..7a7da544fe9e 100644 --- a/Source/WebKit/Shared/AccessibilityPreferences.h +++ b/Source/WebKit/Shared/AccessibilityPreferences.h @@ -87,6 +87,7 @@ struct AccessibilityPreferences { #endif bool imageAnimationEnabled { true }; bool enhanceTextLegibilityOverall { false }; + bool prefersNonBlinkingCursor { false }; AG: does this need the ENABLED guard as well? }; } // namespace WebKit
diff --git a/Source/WebKit/WebProcess/WebPage/WebPage.cpp b/Source/WebKit/WebProcess/WebPage/WebPage.cpp index d7365e937678..b44f30582f8a 100644 --- a/Source/WebKit/WebProcess/WebPage/WebPage.cpp +++ b/Source/WebKit/WebProcess/WebPage/WebPage.cpp @@ -1025,6 +1025,9 @@ WebPage::WebPage(PageIdentifier pageID, WebPageCreationParameters&& parameters) #if ENABLE(ACCESSIBILITY_ANIMATION_CONTROL) updateImageAnimationEnabled(); #endif +#if ENABLE(ACCESSIBILITY_NON_BLINKING_CURSOR) + updatePrefersNonBlinkingCursor(); +#endif #if ENABLE(ADVANCED_PRIVACY_PROTECTIONS) setLinkDecorationFilteringData(WTFMove(parameters.linkDecorationFilteringData)); setAllowedQueryParametersForAdvancedPrivacyProtections(WTFMove(parameters.allowedQueryParametersForAdvancedPrivacyProtections)); @@ -8925,6 +8928,18 @@ void WebPage::playAllAnimations(CompletionHandler<void()>&& completionHandler) } #endif // ENABLE(ACCESSIBILITY_ANIMATION_CONTROL) +#if ENABLE(ACCESSIBILITY_NON_BLINKING_CURSOR) +void WebPage::updatePrefersNonBlinkingCursor() +{ + if (auto* page = corePage()) { AG: RefPtr ? + page->setPrefersNonBlinkingCursor(WebProcess::singleton().prefersNonBlinkingCursor()); + page->forEachDocument([&](auto& document) { + document.selection().setPrefersNonBlinkingCursor(WebProcess::singleton().prefersNonBlinkingCursor()); + }); + } +} +#endif + bool WebPage::isUsingUISideCompositing() const { #if PLATFORM(COCOA)
diff --git a/Source/WebKit/WebProcess/WebProcess.h b/Source/WebKit/WebProcess/WebProcess.h index 28ab3422fd93..a1810515b851 100644 --- a/Source/WebKit/WebProcess/WebProcess.h +++ b/Source/WebKit/WebProcess/WebProcess.h @@ -421,6 +421,7 @@ public: bool isLockdownModeEnabled() const { return m_isLockdownModeEnabled; } bool imageAnimationEnabled() const { return m_imageAnimationEnabled; } + bool prefersNonBlinkingCursor() const { return m_prefersNonBlinkingCursor; } void setHadMainFrameMainResourcePrivateRelayed() { m_hadMainFrameMainResourcePrivateRelayed = true; } bool hadMainFrameMainResourcePrivateRelayed() const { return m_hadMainFrameMainResourcePrivateRelayed; } @@ -625,6 +626,7 @@ private: #endif void accessibilityPreferencesDidChange(const AccessibilityPreferences&); + void updatePageAccessibilitySettings(); #if HAVE(MEDIA_ACCESSIBILITY_FRAMEWORK) void setMediaAccessibilityPreferences(WebCore::CaptionUserPreferences::CaptionDisplayMode, const Vector<String>&); #endif @@ -840,6 +842,7 @@ private: bool m_imageAnimationEnabled { true }; bool m_hasEverHadAnyWebPages { false }; bool m_hasPendingAccessibilityUnsuspension { false }; + bool m_prefersNonBlinkingCursor { false }; AG: should we put these behind the feature enabled directive? HashSet<WebCore::RegistrableDomain> m_allowedFirstPartiesForCookies; String m_mediaKeysStorageDirectory;
(In reply to Joshua Hoffman from comment #7) > (In reply to Andres Gonzalez from comment #6) > > (In reply to Joshua Hoffman from comment #5) > > > Created attachment 469782 [details] > > > Patch > > > > diff --git a/Source/WebCore/page/Page.h b/Source/WebCore/page/Page.h > > index 3a0d2b86bb9e..cdade05808ff 100644 > > --- a/Source/WebCore/page/Page.h > > +++ b/Source/WebCore/page/Page.h > > @@ -710,6 +710,11 @@ public: > > bool imageAnimationEnabled() const { return m_imageAnimationEnabled; } > > bool systemAllowsAnimationControls() const { return > > m_systemAllowsAnimationControls; } > > > > +#if ENABLE(ACCESSIBILITY_NON_BLINKING_CURSOR) > > + WEBCORE_EXPORT void setPrefersNonBlinkingCursor(bool enabled); > > +#endif > > + bool prefersNonBlinkingCursor() const { return > > m_prefersNonBlinkingCursor; }; > > > > AG: maybe you can put the definition of setPrefersNonBlinkingCursor also > > here instead of in the cpp, since these two are trivial accessors. > > Good point, that was leftover from a non-trivial implementation. I can fix > that. > I take that back, I forgot the reason we have to keep the setPrefersNonBlinkingCursor implementation in the CPP is because WebKit doesn't allow WEBCORE_EXPORT on inline methods.
Created attachment 469785 [details] Patch
Created attachment 469786 [details] Patch
Created attachment 469793 [details] Patch
Comment on attachment 469793 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=469793&action=review > Source/WebCore/platform/CaretAnimator.cpp:41 > +bool CaretAnimator::isBlinkingSuspended() const > +{ > +#if ENABLE(ACCESSIBILITY_NON_BLINKING_CURSOR) > + if (m_prefersNonBlinkingCursor) > + return true; > +#endif > + return m_isBlinkingSuspended; > +} Sorry for the late-breaking idea, but thinking about this some more...I wonder if it makes more sense to check m_prefersNonBlinkingCursor in the setBlinkingSuspended implementations (of which there are multiple) rather than here. For example, consider OpacityCaretAnimator::setBlinkingSuspended. With our current approach, when that setBlinkingSuspended(false) is called, we're still going to start a timer and update state, even though we aren't really respecting that request to un-suspend the caret. It also seems like it would be surprising for future us when `m_isBlinkingSuspended` is false, but the caret still isn't animating — it feels more coherent not to allow m_isBlinkingSuspended to become false when m_prefersNonBlinkingCursor is true. What do you think?
(In reply to Tyler Wilcock from comment #17) > Comment on attachment 469793 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=469793&action=review > > > Source/WebCore/platform/CaretAnimator.cpp:41 > > +bool CaretAnimator::isBlinkingSuspended() const > > +{ > > +#if ENABLE(ACCESSIBILITY_NON_BLINKING_CURSOR) > > + if (m_prefersNonBlinkingCursor) > > + return true; > > +#endif > > + return m_isBlinkingSuspended; > > +} > > Sorry for the late-breaking idea, but thinking about this some more...I > wonder if it makes more sense to check m_prefersNonBlinkingCursor in the > setBlinkingSuspended implementations (of which there are multiple) rather > than here. For example, consider OpacityCaretAnimator::setBlinkingSuspended. > With our current approach, when that setBlinkingSuspended(false) is called, > we're still going to start a timer and update state, even though we aren't > really respecting that request to un-suspend the caret. It also seems like > it would be surprising for future us when `m_isBlinkingSuspended` is false, > but the caret still isn't animating — it feels more coherent not to allow > m_isBlinkingSuspended to become false when m_prefersNonBlinkingCursor is > true. > > What do you think? I totally understand the concern here, but I fear that doing so may create more complexity in managing state. For example, let's say we have prefersNonBlinkingCursor enabled while the blinking is also suspended. But, then a client comes along and calls setBlinkingSuspended(true). Once prefersNonBlinkingCursor is disabled again, we will want the cursor to immediately start animating, but we will have never started the timer in this case. We would have to be careful and call `setBlinkingSuspended(true)` on the Caret animator when accessibility preferences change, but maybe that is an okay solution?
(In reply to Joshua Hoffman from comment #18) > (In reply to Tyler Wilcock from comment #17) > > Comment on attachment 469793 [details] > > Patch > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=469793&action=review > > > > > Source/WebCore/platform/CaretAnimator.cpp:41 > > > +bool CaretAnimator::isBlinkingSuspended() const > > > +{ > > > +#if ENABLE(ACCESSIBILITY_NON_BLINKING_CURSOR) > > > + if (m_prefersNonBlinkingCursor) > > > + return true; > > > +#endif > > > + return m_isBlinkingSuspended; > > > +} > > > > Sorry for the late-breaking idea, but thinking about this some more...I > > wonder if it makes more sense to check m_prefersNonBlinkingCursor in the > > setBlinkingSuspended implementations (of which there are multiple) rather > > than here. For example, consider OpacityCaretAnimator::setBlinkingSuspended. > > With our current approach, when that setBlinkingSuspended(false) is called, > > we're still going to start a timer and update state, even though we aren't > > really respecting that request to un-suspend the caret. It also seems like > > it would be surprising for future us when `m_isBlinkingSuspended` is false, > > but the caret still isn't animating — it feels more coherent not to allow > > m_isBlinkingSuspended to become false when m_prefersNonBlinkingCursor is > > true. > > > > What do you think? > > I totally understand the concern here, but I fear that doing so may create > more complexity in managing state. > > For example, let's say we have prefersNonBlinkingCursor enabled while the > blinking is also suspended. But, then a client comes along and calls > setBlinkingSuspended(true). Once prefersNonBlinkingCursor is disabled again, > we will want the cursor to immediately start animating, but we will have > never started the timer in this case. > > We would have to be careful and call `setBlinkingSuspended(true)` on the > Caret animator when accessibility preferences change, but maybe that is an > okay solution? The other tricky thing here is if the preference prefersNonBlinkingCursor updates to false, and then we call setBlinkingSuspended(true), we may run the risk of overriding another place that actually wants the cursor to suspend.
Created attachment 469856 [details] Patch
Comment on attachment 469856 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=469856&action=review > Source/WebCore/testing/Internals.cpp:1140 > + Document* document = contextDocument(); auto*? > Source/WebCore/testing/Internals.cpp:1905 > +ExceptionOr<bool> Internals::isCaretBlinkingSuspended(Document& document) > +{ > + if (!document.frame()) > + return Exception { ExceptionCode::InvalidAccessError }; > + > + return document.frame()->selection().isCaretBlinkingSuspended(); > +} Can we change the implementation of Internals::isCaretBlinkingSuspended() to call this method with `contextDocument()` so we aren't duplicating the logic? > Source/WebCore/testing/Internals.idl:508 > boolean isCaretBlinkingSuspended(); > + boolean isCaretBlinkingSuspended(Document document); Maybe we can add a comment explaining what document the parameter-less method queries now that we have two methods with the same name? > LayoutTests/accessibility/mac/prefers-non-blinking-cursor.html:15 > +let iframe = document.getElementById("iframe") Can this iframe be const? Also it's missing a semi-colon at the end of the statement, might be nice to consistent with all of our other statements which end in semicolons.
cc'ing a few other folks who more familiar with this area for a final look as well.
Created attachment 469857 [details] Patch
(In reply to Tyler Wilcock from comment #21) > Comment on attachment 469856 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=469856&action=review > > > Source/WebCore/testing/Internals.cpp:1140 > > + Document* document = contextDocument(); > > auto*? > > > Source/WebCore/testing/Internals.cpp:1905 > > +ExceptionOr<bool> Internals::isCaretBlinkingSuspended(Document& document) > > +{ > > + if (!document.frame()) > > + return Exception { ExceptionCode::InvalidAccessError }; > > + > > + return document.frame()->selection().isCaretBlinkingSuspended(); > > +} > > Can we change the implementation of Internals::isCaretBlinkingSuspended() to > call this method with `contextDocument()` so we aren't duplicating the logic? > > > Source/WebCore/testing/Internals.idl:508 > > boolean isCaretBlinkingSuspended(); > > + boolean isCaretBlinkingSuspended(Document document); > > Maybe we can add a comment explaining what document the parameter-less > method queries now that we have two methods with the same name? > > > LayoutTests/accessibility/mac/prefers-non-blinking-cursor.html:15 > > +let iframe = document.getElementById("iframe") > > Can this iframe be const? Also it's missing a semi-colon at the end of the > statement, might be nice to consistent with all of our other statements > which end in semicolons. Just addressed all of these, thank you!
Does anyone have other feedback on this? I will land tomorrow, otherwise.
Created attachment 469899 [details] Patch
Committed 274792@main (322e588ea0da): <https://commits.webkit.org/274792@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 469899 [details].