REGRESSION(257219@main): [GTK] Cursor blinks oddly in text fields, even if blinking disabled #12737
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
EWS run on previous version of this PR (hash 525120d)
|
carlosgcampos
approved these changes
Apr 14, 2023
| return shouldBlink ? 500_us * time : 0_s; | ||
| if (shouldBlink) | ||
| return { 500_us * time }; | ||
| return { }; |
There was a problem hiding this comment.
Shouldn't this be std::nullopt?
There was a problem hiding this comment.
Empty initializer list is equivalent to std::nullopt, I have seen both styles in WebKit. I can change it before landing if you prefer.
Ms2ger
reviewed
Apr 14, 2023
| return; | ||
| } | ||
|
|
||
| // If blinking is disabled, set isBlinkingSuspended() would have made the | ||
| // previuos check return early and at this point there must be an interval. |
There was a problem hiding this comment.
Suggested change
| // previuos check return early and at this point there must be an interval. | |
| // previous check return early and at this point there must be an interval. |
There was a problem hiding this comment.
Thanks, I will fix the typo before landing 👌🏼
webkit-ews-buildbot
added
the
merging-blocked
aperezdc
removed
the
merging-blocked
aperezdc
force-pushed
the
eng/gtk-caret-animation
branch
from
525120d
to
55fc48d
Compare
aperezdc
added
the
unsafe-merge-queue
…en if blinking disabled https://bugs.webkit.org/show_bug.cgi?id=252687 Reviewed by Carlos Garcia Campos. Before 257219@main if RenderTheme::caretBlinkInterval() returned zero then the timer controlling the blink animation was never started. When introducing CaretAnimator + SimpleCaretAnimator the check was not carried over, resulting in the animator trying to paint the caret on-and-off as fast as possible when blinking should have been disabled. The fix reintroduces the check, reusing setBlinkingSuspended() to enable (or disable) blinking depending on RenderTheme::caretBlinkInterval(), which in turn is changed to return std::optional<Seconds>, with std::nullopt signalling that blinking is disables. The improved typing ensures that the code explicitly handles the case of blinking being disabled. While at it, remove the RenderThemeGtk.{h,cpp} files, because they only contained an implementation of caretBlinkInterval() that was exactly the same code guarded with PLATFORM(GTK) in RenderThemeAdwaita. * Source/WebCore/SourcesGTK.txt: Remove RenderThemeGtk.cpp from the list. * Source/WebCore/platform/CaretAnimator.h: (WebCore::CaretAnimator::didStart): Adapt to use std::optional<Seconds>. * Source/WebCore/platform/SimpleCaretAnimator.cpp: (WebCore::SimpleCaretAnimator::updateAnimationProperties): Arrange to enable or disable caret blinking, adapting to use std::optional<Seconds>. * Source/WebCore/rendering/RenderTheme.h: (WebCore::RenderTheme::caretBlinkInterval const): Change the return type to std::optional<Seconds>. * Source/WebCore/rendering/RenderThemeAdwaita.cpp: (WebCore::RenderTheme::singleton): Remove PLATFORM(GTK) guard. (WebCore::RenderThemeAdwaita::caretBlinkInterval const): Adapt to use std::optional<Seconds>. * Source/WebCore/rendering/RenderThemeAdwaita.h: Ditto. * Source/WebCore/rendering/RenderThemeGtk.cpp: Removed. * Source/WebCore/rendering/RenderThemeGtk.h: Removed. Canonical link: https://commits.webkit.org/262954@main
|
EWS run on current version of this PR (hash 55fc48d)
|
webkit-commit-queue
force-pushed
the
eng/gtk-caret-animation
branch
from
55fc48d
to
6a0a835
Compare
|
Committed 262954@main (6a0a835): https://commits.webkit.org/262954@main Reviewed commits have been landed. Closing PR #12737 and removing active labels. |
webkit-commit-queue
removed
the
unsafe-merge-queue
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
6a0a835
55fc48d
🧪 wpe-wk2🧪 ios-wk2🧪 api-mac🧪 ios-wk2-wpt🧪 mac-wk1🧪 gtk-wk2🧪 api-ios🧪 mac-wk2🧪 api-gtk🧪 mac-AS-debug-wk2🧪 mac-wk2-stress