Bug 1530188 - Make nsChildView::GetEditorView() use eQueryContentState without flushing layout r=smaug a=pascalc
authorMasayuki Nakano <masayuki@d-toybox.com>
Wed, 03 Apr 2019 10:27:13 +0000
changeset 526161 d9a704ef462353c6427f316faad23af8bd76832d
parent 526160 afc3ac77b7750eae6581dae4cdf65b0678e8bea2
child 526162 e54cf3b64083747317a9675b587b1975dfa49607
push id2032
push userffxbld-merge
push dateMon, 13 May 2019 09:36:57 +0000
treeherdermozilla-release@455c1065dcbe [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewerssmaug, pascalc
bugs1530188
milestone67.0
Bug 1530188 - Make nsChildView::GetEditorView() use eQueryContentState without flushing layout r=smaug a=pascalc `nsChildView::GetEditorView()` is called by `TextInputHandlerBase::GetWindowLevel()` which is called when Cocoa requests window level of focused widget. It currently gets widget including focused element (e.g., it may be in a XUL `<panel>`) with `eQueryTextContent` event. However, it requires only the widget (i.e., when a XUL `<panel>` has focused element, the widget for the panel). Therefore, it does not require to flush the layout. However, on macOS, `ContentEventHandler` always flushes layout even with `eQueryContentState` which does not require any layout information. Whether it requires flushing layout or not is considered with `WidgetQueryContentEvent::mNeedsToFlushLayout` but this is set to false only when `IMEContentObserver` notifies widget (and IME) of focus set. At this time, only on macOS, IME caches the layout information, for example, the character coordinates, but we don't have a way to update it. This is the reason why we always flush layout on macOS. Unfortunately, when a menu popup frame is created, widget for the popup is created synchronously. Then, Cocoa retrieves window level of the widget including focused element. But this is unsafe to flush the layout. So, we need to stop flushing layout in this case. Therefore, this patch moves the `#ifdef` from `TextEvents.h` to `IMEContentObserver.cpp`, then, makes `nsChildView::GetEditorView()` use `eQueryContentState` which is the simplest query content event, and finally, sets `mNeedsToFlushLayout` to `false`. Differential Revision: https://phabricator.services.mozilla.com/D25912
dom/events/IMEContentObserver.cpp
widget/TextEvents.h
widget/cocoa/nsChildView.mm
--- a/dom/events/IMEContentObserver.cpp
+++ b/dom/events/IMEContentObserver.cpp
@@ -1808,19 +1808,25 @@ void IMEContentObserver::IMENotification
              "SendFocusSet(), retrying to send NOTIFY_IME_OF_FOCUS...",
              this));
     observer->PostFocusSetNotification();
     return;
   }
 
   observer->mIMEHasFocus = true;
   // Initialize selection cache with the first selection data.
+#ifdef XP_MACOSX
+  // We need to flush layout only on macOS because character coordinates are
+  // cached by cocoa with this call, but we don't have a way to update them
+  // after that.  Therefore, we need the latest layout information right now.
+  observer->UpdateSelectionCache(true);
+#else
   // We avoid flushing for focus in the general case.
   observer->UpdateSelectionCache(false);
-
+#endif  // #ifdef XP_MACOSX #else
   MOZ_LOG(sIMECOLog, LogLevel::Info,
           ("0x%p IMEContentObserver::IMENotificationSender::"
            "SendFocusSet(), sending NOTIFY_IME_OF_FOCUS...",
            this));
 
   MOZ_RELEASE_ASSERT(observer->mSendingNotification == NOTIFY_IME_OF_NOTHING);
   observer->mSendingNotification = NOTIFY_IME_OF_FOCUS;
   IMEStateManager::NotifyIME(IMENotification(NOTIFY_IME_OF_FOCUS),
--- a/widget/TextEvents.h
+++ b/widget/TextEvents.h
@@ -996,23 +996,17 @@ class WidgetQueryContentEvent : public W
                                  const Options& aOptions = Options()) {
     NS_ASSERTION(mMessage == eQueryTextRectArray,
                  "wrong initializer is called");
     mInput.mOffset = aOffset;
     mInput.mLength = aLength;
     Init(aOptions);
   }
 
-  bool NeedsToFlushLayout() const {
-#ifdef XP_MACOSX
-    return true;
-#else
-    return mNeedsToFlushLayout;
-#endif
-  }
+  bool NeedsToFlushLayout() const { return mNeedsToFlushLayout; }
 
   void RequestFontRanges() {
     NS_ASSERTION(mMessage == eQueryTextContent, "not querying text content");
     mWithFontRanges = true;
   }
 
   uint32_t GetSelectionStart(void) const {
     NS_ASSERTION(mMessage == eQuerySelectedText, "not querying selection");
--- a/widget/cocoa/nsChildView.mm
+++ b/widget/cocoa/nsChildView.mm
@@ -1682,22 +1682,26 @@ void nsChildView::GetEditCommands(Native
   keyBindings->GetEditCommands(aEvent, aCommands);
 }
 
 NSView<mozView>* nsChildView::GetEditorView() {
   NSView<mozView>* editorView = mView;
   // We need to get editor's view. E.g., when the focus is in the bookmark
   // dialog, the view is <panel> element of the dialog.  At this time, the key
   // events are processed the parent window's view that has native focus.
-  WidgetQueryContentEvent textContent(true, eQueryTextContent, this);
-  textContent.InitForQueryTextContent(0, 0);
-  DispatchWindowEvent(textContent);
-  if (textContent.mSucceeded && textContent.mReply.mFocusedWidget) {
+  WidgetQueryContentEvent queryContentState(true, eQueryContentState, this);
+  // This may be called during creating a menu popup frame due to creating
+  // widget synchronously and that causes Cocoa asking current window level.
+  // In this case, it's not safe to flush layout on the document and we don't
+  // need any layout information right now.
+  queryContentState.mNeedsToFlushLayout = false;
+  DispatchWindowEvent(queryContentState);
+  if (queryContentState.mSucceeded && queryContentState.mReply.mFocusedWidget) {
     NSView<mozView>* view = static_cast<NSView<mozView>*>(
-        textContent.mReply.mFocusedWidget->GetNativeData(NS_NATIVE_WIDGET));
+        queryContentState.mReply.mFocusedWidget->GetNativeData(NS_NATIVE_WIDGET));
     if (view) editorView = view;
   }
   return editorView;
 }
 
 #pragma mark -
 
 void nsChildView::CreateCompositor() {