- Revision
- 259938
- Author
- [email protected]
- Date
- 2020-04-11 17:43:53 -0700 (Sat, 11 Apr 2020)
Log Message
[macOS] [WK1] Touch Bar flashes when typing in Vietnamese in Mail
https://bugs.webkit.org/show_bug.cgi?id=210394
<rdar://problem/60099560>
Reviewed by Tim Horton.
Source/WebCore:
See WebKitLegacy/mac/ChangeLog for more details.
Currently, many users of TemporarySelectionChange use it to temporarily avoid propagating selection changes to
the client layer during temporary selection changes. This involves creating a TemporarySelectionChange without
a new selection, but with the `IgnoreSelectionChanges` option specified, which makes us call `Editor::
setIgnoreSelectionChanges` to suppress selection change notifications.
Do a bit of cleanup in this area by introducing IgnoreSelectionChangeForScope, which wraps a
TemporarySelectionChange and makes it easier for a handful of call sites that currently use
TemporarySelectionChange to hide selection changes from the client layer to get their desired behavior.
Test: CandidateTests.DoNotHideCandidatesDuringTextReplacement
* editing/Editor.cpp:
(WebCore::Editor::respondToChangedSelection):
* editing/Editor.h:
(WebCore::TemporarySelectionChange::TemporarySelectionChange):
(WebCore::IgnoreSelectionChangeForScope::IgnoreSelectionChangeForScope):
* page/DragController.cpp:
(WebCore::DragController::performDragOperation):
(WebCore::DragController::insertDroppedImagePlaceholdersAtCaret):
Replace these:
`TemporarySelectionChange ignoreSelectionChanges { frame, WTF::nullopt, TemporarySelectionOption::IgnoreSelectionChanges };`
...with these instead:
`IgnoreSelectionChangeForScope ignoreSelectionChanges { *frame };`
Source/WebKitLegacy/mac:
In recent versions of macOS, changes to the animation of the candidate list touch bar item when it is expanded
or collapsed using -updateWithInsertionPointVisibility: mean that back-to-back calls to hide and show the
candidate list during the same runloop now result in a visible flicker, whereas it would remain still in prior
releases.
Combined with how `-[WebHTMLView insertText:]` in WebKitLegacy causes multiple selection change updates if the
argument is an attributed string with the text replacement attribute, this means that there is now a visible
flicker in the touch bar when using input methods that rely on the `NSTextInputReplacementRangeAttributeName`
attribute. One such input method is Simple Telex (Vietnamese), when typing causes characters in other parts of
the word to gain diacritics.
To fix this, we make two small adjustments in WebKitLegacy (see below).
* WebView/WebHTMLView.mm:
(-[WebHTMLView insertText:]):
While inserting text (which may be an NSAttibutedString or NSString), we may come across an attribute indicating
which part of the document to replace. In this case, we will select this range in the document before inserting
the given text.
This selection change is propagated to the client layer, which makes us briefly think that we have a ranged
selection, causing the flicker described above. Address this by suppressing selection change notifications while
we're selecting the range to replace, using the new `IgnoreSelectionChangeForScope` RAII object (see WebCore
changes for more detail).
* WebView/WebView.mm:
(-[WebView updateTextTouchBar]):
When updating the text touch bar (which contains the candidate list item) after a selection change, we currently
handle the case where selection changes are ignored by hiding the candidate list; this doesn't seem right, since
selection changes that are ignored should simply be ignored by the client. To fix this, push the
`ignoreSelectionChanges()` check into the if statement.
Tools:
Add an API test that inserts an attributed string with the text replacement attribute, and verifies that we
show the candidate list when inserting the string, and also don't temporarily hide the candidate list in the
process of doing so.
To achieve this, we swizzle out calls to -[NSCandidateListTouchBarItem updateWithInsertionPointVisibility:] to
detect when the candidate list is either hidden or made visible.
* TestWebKitAPI/Tests/TestWebKitAPI/mac/AppKitSPI.h:
* TestWebKitAPI/Tests/mac/CandidateTests.mm:
Fix a leak in an adjacent test too, while I'm touching this file.
(TestWebKitAPI::updateCandidateListWithVisibility):
Modified Paths
Diff
Modified: trunk/Source/WebCore/ChangeLog (259937 => 259938)
--- trunk/Source/WebCore/ChangeLog 2020-04-11 23:56:46 UTC (rev 259937)
+++ trunk/Source/WebCore/ChangeLog 2020-04-12 00:43:53 UTC (rev 259938)
@@ -1,3 +1,41 @@
+2020-04-11 Wenson Hsieh <[email protected]>
+
+ [macOS] [WK1] Touch Bar flashes when typing in Vietnamese in Mail
+ https://bugs.webkit.org/show_bug.cgi?id=210394
+ <rdar://problem/60099560>
+
+ Reviewed by Tim Horton.
+
+ See WebKitLegacy/mac/ChangeLog for more details.
+
+ Currently, many users of TemporarySelectionChange use it to temporarily avoid propagating selection changes to
+ the client layer during temporary selection changes. This involves creating a TemporarySelectionChange without
+ a new selection, but with the `IgnoreSelectionChanges` option specified, which makes us call `Editor::
+ setIgnoreSelectionChanges` to suppress selection change notifications.
+
+ Do a bit of cleanup in this area by introducing IgnoreSelectionChangeForScope, which wraps a
+ TemporarySelectionChange and makes it easier for a handful of call sites that currently use
+ TemporarySelectionChange to hide selection changes from the client layer to get their desired behavior.
+
+ Test: CandidateTests.DoNotHideCandidatesDuringTextReplacement
+
+ * editing/Editor.cpp:
+ (WebCore::Editor::respondToChangedSelection):
+ * editing/Editor.h:
+ (WebCore::TemporarySelectionChange::TemporarySelectionChange):
+ (WebCore::IgnoreSelectionChangeForScope::IgnoreSelectionChangeForScope):
+ * page/DragController.cpp:
+ (WebCore::DragController::performDragOperation):
+ (WebCore::DragController::insertDroppedImagePlaceholdersAtCaret):
+
+ Replace these:
+
+ `TemporarySelectionChange ignoreSelectionChanges { frame, WTF::nullopt, TemporarySelectionOption::IgnoreSelectionChanges };`
+
+ ...with these instead:
+
+ `IgnoreSelectionChangeForScope ignoreSelectionChanges { *frame };`
+
2020-04-11 Simon Fraser <[email protected]>
[Async overflow] Can't scroll overflow:scroll in sideways-scrollable RTL document
Modified: trunk/Source/WebCore/editing/Editor.h (259937 => 259938)
--- trunk/Source/WebCore/editing/Editor.h 2020-04-11 23:56:46 UTC (rev 259937)
+++ trunk/Source/WebCore/editing/Editor.h 2020-04-12 00:43:53 UTC (rev 259938)
@@ -119,8 +119,8 @@
class TemporarySelectionChange {
public:
- TemporarySelectionChange(Frame&, Optional<VisibleSelection> = WTF::nullopt, OptionSet<TemporarySelectionOption> = { });
- ~TemporarySelectionChange();
+ WEBCORE_EXPORT TemporarySelectionChange(Frame&, Optional<VisibleSelection> = WTF::nullopt, OptionSet<TemporarySelectionOption> = { });
+ WEBCORE_EXPORT ~TemporarySelectionChange();
private:
void setSelection(const VisibleSelection&);
@@ -134,6 +134,19 @@
Optional<VisibleSelection> m_selectionToRestore;
};
+class IgnoreSelectionChangeForScope {
+public:
+ IgnoreSelectionChangeForScope(Frame& frame)
+ : m_selectionChange(frame, WTF::nullopt, TemporarySelectionOption::IgnoreSelectionChanges)
+ {
+ }
+
+ ~IgnoreSelectionChangeForScope() = default;
+
+private:
+ TemporarySelectionChange m_selectionChange;
+};
+
class Editor {
WTF_MAKE_FAST_ALLOCATED;
public:
Modified: trunk/Source/WebCore/page/DragController.cpp (259937 => 259938)
--- trunk/Source/WebCore/page/DragController.cpp 2020-04-11 23:56:46 UTC (rev 259937)
+++ trunk/Source/WebCore/page/DragController.cpp 2020-04-12 00:43:53 UTC (rev 259938)
@@ -250,7 +250,7 @@
removeAllDroppedImagePlaceholders();
SetForScope<bool> isPerformingDrop(m_isPerformingDrop, true);
- TemporarySelectionChange ignoreSelectionChanges(m_page.focusController().focusedOrMainFrame(), WTF::nullopt, TemporarySelectionOption::IgnoreSelectionChanges);
+ IgnoreSelectionChangeForScope ignoreSelectionChanges { m_page.focusController().focusedOrMainFrame() };
m_documentUnderMouse = m_page.mainFrame().documentAtPoint(dragData.clientPosition());
@@ -1388,7 +1388,7 @@
if (!frame)
return;
- TemporarySelectionChange selectionChange(*frame, WTF::nullopt, { TemporarySelectionOption::IgnoreSelectionChanges });
+ IgnoreSelectionChangeForScope selectionChange { *frame };
auto fragment = DocumentFragment::create(*document);
for (auto& size : imageSizes) {
Modified: trunk/Source/WebKitLegacy/mac/ChangeLog (259937 => 259938)
--- trunk/Source/WebKitLegacy/mac/ChangeLog 2020-04-11 23:56:46 UTC (rev 259937)
+++ trunk/Source/WebKitLegacy/mac/ChangeLog 2020-04-12 00:43:53 UTC (rev 259938)
@@ -1,3 +1,44 @@
+2020-04-11 Wenson Hsieh <[email protected]>
+
+ [macOS] [WK1] Touch Bar flashes when typing in Vietnamese in Mail
+ https://bugs.webkit.org/show_bug.cgi?id=210394
+ <rdar://problem/60099560>
+
+ Reviewed by Tim Horton.
+
+ In recent versions of macOS, changes to the animation of the candidate list touch bar item when it is expanded
+ or collapsed using -updateWithInsertionPointVisibility: mean that back-to-back calls to hide and show the
+ candidate list during the same runloop now result in a visible flicker, whereas it would remain still in prior
+ releases.
+
+ Combined with how `-[WebHTMLView insertText:]` in WebKitLegacy causes multiple selection change updates if the
+ argument is an attributed string with the text replacement attribute, this means that there is now a visible
+ flicker in the touch bar when using input methods that rely on the `NSTextInputReplacementRangeAttributeName`
+ attribute. One such input method is Simple Telex (Vietnamese), when typing causes characters in other parts of
+ the word to gain diacritics.
+
+ To fix this, we make two small adjustments in WebKitLegacy (see below).
+
+ * WebView/WebHTMLView.mm:
+ (-[WebHTMLView insertText:]):
+
+ While inserting text (which may be an NSAttibutedString or NSString), we may come across an attribute indicating
+ which part of the document to replace. In this case, we will select this range in the document before inserting
+ the given text.
+
+ This selection change is propagated to the client layer, which makes us briefly think that we have a ranged
+ selection, causing the flicker described above. Address this by suppressing selection change notifications while
+ we're selecting the range to replace, using the new `IgnoreSelectionChangeForScope` RAII object (see WebCore
+ changes for more detail).
+
+ * WebView/WebView.mm:
+ (-[WebView updateTextTouchBar]):
+
+ When updating the text touch bar (which contains the candidate list item) after a selection change, we currently
+ handle the case where selection changes are ignored by hiding the candidate list; this doesn't seem right, since
+ selection changes that are ignored should simply be ignored by the client. To fix this, push the
+ `ignoreSelectionChanges()` check into the if statement.
+
2020-04-10 Darin Adler <[email protected]>
Move more from live range to SimpleRange: callers of absoluteTextRects
Modified: trunk/Source/WebKitLegacy/mac/WebView/WebHTMLView.mm (259937 => 259938)
--- trunk/Source/WebKitLegacy/mac/WebView/WebHTMLView.mm 2020-04-11 23:56:46 UTC (rev 259937)
+++ trunk/Source/WebKitLegacy/mac/WebView/WebHTMLView.mm 2020-04-12 00:43:53 UTC (rev 259938)
@@ -6713,6 +6713,7 @@
if (replacementRange.location != NSNotFound) {
WebRangeIsRelativeTo rangeIsRelativeTo = needToRemoveSoftSpace ? WebRangeIsRelativeTo::Paragraph : WebRangeIsRelativeTo::EditableRoot;
if (auto domRange = [[self _frame] _convertToDOMRange:replacementRange rangeIsRelativeTo:rangeIsRelativeTo]) {
+ WebCore::IgnoreSelectionChangeForScope selectionChange { *coreFrame };
coreFrame->selection().setSelection(WebCore::VisibleSelection(*domRange, WebCore::SEL_DEFAULT_AFFINITY));
replacesText = replacementRange.length;
}
Modified: trunk/Source/WebKitLegacy/mac/WebView/WebView.mm (259937 => 259938)
--- trunk/Source/WebKitLegacy/mac/WebView/WebView.mm 2020-04-11 23:56:46 UTC (rev 259937)
+++ trunk/Source/WebKitLegacy/mac/WebView/WebView.mm 2020-04-12 00:43:53 UTC (rev 259938)
@@ -9866,10 +9866,8 @@
[_private->_plainTextTouchBar setCustomizationIdentifier:@"WebPlainTextTouchBar"];
}
- if ([NSSpellChecker isAutomaticTextCompletionEnabled] && !_private->_isCustomizingTouchBar) {
- BOOL shouldShowCandidateList = !coreFrame->selection().selection().isRange() || coreFrame->editor().ignoreSelectionChanges();
- [self.candidateList updateWithInsertionPointVisibility:shouldShowCandidateList];
- }
+ if ([NSSpellChecker isAutomaticTextCompletionEnabled] && !_private->_isCustomizingTouchBar && !coreFrame->editor().ignoreSelectionChanges())
+ [self.candidateList updateWithInsertionPointVisibility:!coreFrame->selection().selection().isRange()];
if (coreFrame->selection().selection().isInPasswordField()) {
// We don't request candidates for password fields. If the user was previously in a non-password field, then the
Modified: trunk/Tools/ChangeLog (259937 => 259938)
--- trunk/Tools/ChangeLog 2020-04-11 23:56:46 UTC (rev 259937)
+++ trunk/Tools/ChangeLog 2020-04-12 00:43:53 UTC (rev 259938)
@@ -1,3 +1,25 @@
+2020-04-11 Wenson Hsieh <[email protected]>
+
+ [macOS] [WK1] Touch Bar flashes when typing in Vietnamese in Mail
+ https://bugs.webkit.org/show_bug.cgi?id=210394
+ <rdar://problem/60099560>
+
+ Reviewed by Tim Horton.
+
+ Add an API test that inserts an attributed string with the text replacement attribute, and verifies that we
+ show the candidate list when inserting the string, and also don't temporarily hide the candidate list in the
+ process of doing so.
+
+ To achieve this, we swizzle out calls to -[NSCandidateListTouchBarItem updateWithInsertionPointVisibility:] to
+ detect when the candidate list is either hidden or made visible.
+
+ * TestWebKitAPI/Tests/TestWebKitAPI/mac/AppKitSPI.h:
+ * TestWebKitAPI/Tests/mac/CandidateTests.mm:
+
+ Fix a leak in an adjacent test too, while I'm touching this file.
+
+ (TestWebKitAPI::updateCandidateListWithVisibility):
+
2020-04-11 Aakash Jain <[email protected]>
Buildbot: Force crash log submission after each test run (follow-up fix)
Modified: trunk/Tools/TestWebKitAPI/Tests/TestWebKitAPI/mac/AppKitSPI.h (259937 => 259938)
--- trunk/Tools/TestWebKitAPI/Tests/TestWebKitAPI/mac/AppKitSPI.h 2020-04-11 23:56:46 UTC (rev 259937)
+++ trunk/Tools/TestWebKitAPI/Tests/TestWebKitAPI/mac/AppKitSPI.h 2020-04-12 00:43:53 UTC (rev 259938)
@@ -65,6 +65,7 @@
NSString * const NSInspectorBarTextBackgroundColorItemIdentifier = @"NSInspectorBarTextBackgroundColorItemIdentifier";
NSString * const NSInspectorBarFontStyleItemIdentifier = @"NSInspectorBarFontStyleItemIdentifier";
NSString * const NSInspectorBarTextAlignmentItemIdentifier = @"NSInspectorBarTextAlignmentItemIdentifier";
+NSString * const NSTextInputReplacementRangeAttributeName = @"NSTextInputReplacementRangeAttributeName";
@interface __NSInspectorBarItemController : NSObject
- (instancetype)initWithInspectorBar:(NSInspectorBar *)bar NS_DESIGNATED_INITIALIZER;
Modified: trunk/Tools/TestWebKitAPI/Tests/mac/CandidateTests.mm (259937 => 259938)
--- trunk/Tools/TestWebKitAPI/Tests/mac/CandidateTests.mm 2020-04-11 23:56:46 UTC (rev 259937)
+++ trunk/Tools/TestWebKitAPI/Tests/mac/CandidateTests.mm 2020-04-12 00:43:53 UTC (rev 259938)
@@ -25,11 +25,13 @@
#include "config.h"
+#import "AppKitSPI.h"
+#import "InstanceMethodSwizzler.h"
#import "PlatformUtilities.h"
#import <WebKit/WebViewPrivate.h>
#import <wtf/RetainPtr.h>
-#if PLATFORM(MAC) && __MAC_OS_X_VERSION_MAX_ALLOWED >= 101201
+#if PLATFORM(MAC)
static bool webViewWasDeallocated = false;
static bool didFinishLoad = false;
@@ -123,10 +125,10 @@
TEST(CandidateTests, DoNotRequestCandidatesForPasswordInput)
{
- WebView *webView = [[WebView alloc] initWithFrame:NSMakeRect(0, 0, 100, 100)];
+ auto webView = adoptNS([[WebView alloc] initWithFrame:NSMakeRect(0, 0, 100, 100)]);
[webView forceRequestCandidatesForTesting];
- CandidateRequestFrameLoadDelegate *delegate = [[CandidateRequestFrameLoadDelegate alloc] init];
- [webView setFrameLoadDelegate:delegate];
+ auto delegate = adoptNS([[CandidateRequestFrameLoadDelegate alloc] init]);
+ [webView setFrameLoadDelegate:delegate.get()];
NSURL *contentURL = [[NSBundle mainBundle] URLForResource:@"focus-inputs" withExtension:@"html" subdirectory:@"TestWebKitAPI.resources"];
[[webView mainFrame] loadRequest:[NSURLRequest requestWithURL:contentURL]];
@@ -139,10 +141,40 @@
if ([webView shouldRequestCandidates])
candidatesWereRequested = true;
- [webView release];
EXPECT_FALSE(candidatesWereRequested);
}
+static BOOL candidateListWasHidden = NO;
+static BOOL candidateListWasShown = NO;
+static void updateCandidateListWithVisibility(id, SEL, BOOL visible)
+{
+ if (visible)
+ candidateListWasShown = YES;
+ else
+ candidateListWasHidden = YES;
}
-#endif // PLATFORM(MAC) && __MAC_OS_X_VERSION_MAX_ALLOWED >= 101201
+TEST(CandidateTests, DoNotHideCandidatesDuringTextReplacement)
+{
+ InstanceMethodSwizzler visibilityUpdateSwizzler { NSCandidateListTouchBarItem.class, @selector(updateWithInsertionPointVisibility:), reinterpret_cast<IMP>(updateCandidateListWithVisibility) };
+
+ auto webView = adoptNS([[WebView alloc] initWithFrame:NSMakeRect(0, 0, 100, 100)]);
+ [webView forceRequestCandidatesForTesting];
+
+ auto delegate = adoptNS([[CandidateRequestFrameLoadDelegate alloc] init]);
+ [webView setFrameLoadDelegate:delegate.get()];
+ [[webView mainFrame] loadHTMLString:@"<body contenteditable>Hello world<script>document.body.focus()</script>" baseURL:nil];
+ TestWebKitAPI::Util::run(&didFinishLoad);
+
+ auto textToInsert = adoptNS([[NSMutableAttributedString alloc] initWithString:@"Goodbye"]);
+ [textToInsert addAttribute:NSTextInputReplacementRangeAttributeName value:NSStringFromRange(NSMakeRange(0, 5)) range:NSMakeRange(0, 7)];
+ [webView insertText:textToInsert.get()];
+
+ EXPECT_WK_STREQ("Goodbye world", [webView stringByEvaluatingJavaScriptFromString:@"document.body.innerText"]);
+ EXPECT_FALSE(candidateListWasHidden);
+ EXPECT_TRUE(candidateListWasShown);
+}
+
+}
+
+#endif // PLATFORM(MAC)
