Fixes Select elements drop-down doesn't follow update of the element position/size

Prior to this fix, when a select element's popup is open, a script
in the background can modify the page's DOM that changes the position
of select element on the screen. However, select popup remains
stationary thus appearing disconnect from the select element. The select
popup also doesn't update to the correct size if the script adds
an new option with a long text.

The reason is that select element doesn't check to see if the position
or size of the select element popup relative to the document changed.
For example, after the popup is displayed, a script could modify the
document that changes the position of the select element. It doesn't
notify its popup, thus makes them appear as disconnected.

The fix is to update the select popup, its position and size, at the
end of frame's life-cycle phase. This will guarantee that all elements
in the frame have been styled and layout correctly. At this point,
we can update the popup if its owner's position and size has been
changed. The fix also removes the PostTask as a performance and
simplicity optimization.

An alternative proposal, use intersection observer on the owner element,
was discussed. However, it still has the draw backs of post task. Plus,
it may not capture all cases.

In addition, I also considered and tried using PostTask within the
UpdateLifecycle method. However, that's not optimal since within the
popup's update method, it requires another call to UpdateStyleAndLayout
because the layout could have been dirtied since the time PostTask was
added into the queue. InternalPopupMenu::Update() also updates the
select element's internal state, options. I saw some intermittent
failures in couple of tests that validate options index. These tests
use SetTimeout(), which may partly be aggravate by PostTask. I didn't
investigate further as this is not the approach to take.

This fix works for both Linux and Windows. I updated test
popup-menu-resize-after-open to reflect that. In addition, I removed
the (TODO) in listPicker.js. This fixes select popup when placed inside
an iframe.
For platforms that use external_popup_menu, mac and android, will be
migrated to use internal_popup_menu when forms-refresh is rolled out to
those platforms in M83.

One of the biggest problem I encountered when doing this fix is that popup
content doesn't updated when the popup is moved only. This is the same
issue as crbug.com/633140. This review,
https://codereview.chromium.org/2228093003, goes into more details on
popup bounds synchronization between browser and renderer. Regardless,
I had to make a similar fix in  web_page_popup_impl.cc,
 calling SetWindowRect() when force_update is true.


Bug: 137495
Change-Id: I7ef4c09a6d3d7bee3ef8988c815e07efa72d4a79
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2013470
Commit-Queue: Yu Han <yuzhehan@chromium.org>
Reviewed-by: Chris Harrelson <chrishtr@chromium.org>
Cr-Commit-Position: refs/heads/master@{#740964}

third_party/blink/renderer/core/exported/web_page_popup_impl.cc

@@ -53,6 +53,7 @@
 #include "third_party/blink/renderer/core/frame/visual_viewport.h"
 #include "third_party/blink/renderer/core/frame/web_frame_widget_base.h"
 #include "third_party/blink/renderer/core/frame/web_local_frame_impl.h"
+#include "third_party/blink/renderer/core/geometry/dom_rect.h"
 #include "third_party/blink/renderer/core/input/event_handler.h"
 #include "third_party/blink/renderer/core/layout/layout_view.h"
 #include "third_party/blink/renderer/core/loader/empty_clients.h"
@@ -322,6 +323,8 @@ void WebPagePopupImpl::Initialize(WebViewImpl* web_view,
   frame->SetPageZoomFactor(popup_client_->ZoomFactor());
   frame->ForceSynchronousDocumentInstall("text/html", std::move(data));
 
+  popup_owner_client_rect_ =
+      popup_client_->OwnerElement().getBoundingClientRect();
   WidgetClient()->Show(WebNavigationPolicy());
   SetFocus(true);
 }
@@ -340,6 +343,20 @@ void WebPagePopupImpl::PostMessageToPopup(const String& message) {
   MainFrame().DomWindow()->DispatchEvent(*MessageEvent::Create(message));
 }
 
+void WebPagePopupImpl::Update() {
+  if (!page_ && !popup_client_)
+    return;
+
+  DOMRect* dom_rect = popup_client_->OwnerElement().getBoundingClientRect();
+  bool forced_update = (*dom_rect != *popup_owner_client_rect_);
+  if (forced_update)
+    popup_owner_client_rect_ = dom_rect;
+
+  popup_client_->Update(forced_update);
+  if (forced_update)
+    SetWindowRect(WindowRectInScreen());
+}
+
 void WebPagePopupImpl::DestroyPage() {
   page_->WillCloseAnimationHost(nullptr);
   page_->WillBeDestroyed();

third_party/blink/renderer/core/exported/web_page_popup_impl.h

@@ -51,6 +51,7 @@ class PagePopupChromeClient;
 class PagePopupClient;
 class WebViewImpl;
 class LocalDOMWindow;
+class DOMRect;
 
 class CORE_EXPORT WebPagePopupImpl final : public WebPagePopup,
                                            public PageWidgetEventHandler,
@@ -103,6 +104,7 @@ class CORE_EXPORT WebPagePopupImpl final : public WebPagePopup,
 
   // PagePopup implementation.
   void PostMessageToPopup(const String& message) override;
+  void Update() override;
 
   // PageWidgetEventHandler implementation.
   WebInputEventResult HandleKeyEvent(const WebKeyboardEvent&) override;
@@ -172,6 +174,7 @@ class CORE_EXPORT WebPagePopupImpl final : public WebPagePopup,
   bool is_accelerated_compositing_active_ = false;
 
   bool suppress_next_keypress_event_ = false;
+  Persistent<DOMRect> popup_owner_client_rect_;
 
   friend class WebPagePopup;
   friend class PagePopupChromeClient;

third_party/blink/renderer/core/exported/web_view_impl.cc

@@ -1165,6 +1165,11 @@ void WebViewImpl::CleanupPagePopup() {
   DisablePopupMouseWheelEventListener();
 }
 
+void WebViewImpl::UpdatePagePopup() {
+  if (page_popup_)
+    page_popup_->Update();
+}
+
 void WebViewImpl::EnablePopupMouseWheelEventListener(
     WebLocalFrameImpl* local_root) {
   DCHECK(!popup_mouse_wheel_event_listener_);
@@ -1595,6 +1600,8 @@ void WebViewImpl::UpdateLifecycle(WebWidget::LifecycleUpdate requested_update,
   if (requested_update != WebWidget::LifecycleUpdate::kAll)
     return;
 
+  UpdatePagePopup();
+
   // There is no background color for non-composited WebViews (eg printing).
   if (does_composite_) {
     MainFrameImpl()->FrameWidgetImpl()->Client()->SetBackgroundColor(

third_party/blink/renderer/core/exported/web_view_impl.h

@@ -307,6 +307,9 @@ class CORE_EXPORT WebViewImpl final : public WebView,
   // Callback from PagePopup when it is closed, which it can be done directly
   // without coming through WebViewImpl.
   void CleanupPagePopup();
+  // Ensure popup's size and position is correct based on its owner element's
+  // dimensions.
+  void UpdatePagePopup();
   LocalDOMWindow* PagePopupWindow() const;
 
   PageScheduler* Scheduler() const override;

third_party/blink/renderer/core/frame/web_frame_widget_impl.cc

@@ -375,6 +375,7 @@ void WebFrameWidgetImpl::UpdateLifecycle(LifecycleUpdate requested_update,
       LocalRootImpl()->GetFrame()->GetDocument()->Lifecycle());
   PageWidgetDelegate::UpdateLifecycle(*GetPage(), *LocalRootImpl()->GetFrame(),
                                       requested_update, reason);
+  View()->UpdatePagePopup();
 }
 
 void WebFrameWidgetImpl::ThemeChanged() {

third_party/blink/renderer/core/geometry/dom_rect.h

@@ -34,6 +34,13 @@ class CORE_EXPORT DOMRect final : public DOMRectReadOnly {
   void setHeight(double height) { height_ = height; }
 };
 
+constexpr bool operator==(const DOMRect& lhs, const DOMRect& rhs) {
+  return lhs.x() == rhs.x() && lhs.y() == rhs.y() &&
+         lhs.width() == rhs.width() && lhs.height() == rhs.height();
+}
+constexpr bool operator!=(const DOMRect& lhs, const DOMRect& rhs) {
+  return !(lhs == rhs);
+}
 }  // namespace blink
 
-#endif
+#endif  // THIRD_PARTY_BLINK_RENDERER_CORE_GEOMETRY_DOM_RECT_H_

third_party/blink/renderer/core/html/forms/internal_popup_menu.cc

@@ -508,20 +508,12 @@ void InternalPopupMenu::Hide() {
 }
 
 void InternalPopupMenu::UpdateFromElement(UpdateReason) {
-  if (needs_update_)
-    return;
   needs_update_ = true;
-  OwnerElement()
-      .GetDocument()
-      .GetTaskRunner(TaskType::kUserInteraction)
-      ->PostTask(FROM_HERE,
-                 WTF::Bind(&InternalPopupMenu::Update, WrapPersistent(this)));
 }
 
-void InternalPopupMenu::Update() {
-  if (!popup_ || !owner_element_)
+void InternalPopupMenu::Update(bool force_update) {
+  if (!popup_ || !owner_element_ || (!needs_update_ && !force_update))
     return;
-  OwnerElement().GetDocument().UpdateStyleAndLayoutTree();
   // disconnectClient() might have been called.
   if (!owner_element_)
     return;

third_party/blink/renderer/core/html/forms/internal_popup_menu.h

@@ -28,7 +28,7 @@ class CORE_EXPORT InternalPopupMenu final : public PopupMenu,
   ~InternalPopupMenu() override;
   void Trace(Visitor*) override;
 
-  void Update();
+  void Update(bool force_update) override;
 
   void Dispose();
 

third_party/blink/renderer/core/html/forms/resources/listPicker.js

@@ -100,12 +100,8 @@ ListPicker.prototype._handleWindowMessage = function(event) {
             window.updateData.anchorRectInScreen.width ||
         this._config.anchorRectInScreen.height !==
             window.updateData.anchorRectInScreen.height) {
-      // TODO(tkent): Don't fix window size here due to a bug of Aura or
-      // compositor. crbug.com/863770
-      if (!navigator.platform.startsWith('Win')) {
         this._config.anchorRectInScreen = window.updateData.anchorRectInScreen;
         this._fixWindowSize();
-      }
     }
   }
   delete window.updateData;

third_party/blink/renderer/core/page/page_popup.h

@@ -45,6 +45,7 @@ class PagePopup {
   virtual AXObject* RootAXObject() = 0;
   virtual void SetWindowRect(const IntRect&) = 0;
   virtual void PostMessageToPopup(const String& message) = 0;
+  virtual void Update() = 0;
 
  protected:
   virtual ~PagePopup() = default;

third_party/blink/renderer/core/page/page_popup_client.h

@@ -85,6 +85,9 @@ class CORE_EXPORT PagePopupClient {
   // This is called whenever a PagePopup was closed.
   virtual void DidClosePopup() = 0;
 
+  // This is called when popup content or its owner's position changed.
+  virtual void Update(bool force_update) {}
+
   virtual ~PagePopupClient() = default;
 
   // Helper functions to be used in PagePopupClient::writeDocument().

third_party/blink/web_tests/fast/forms/select-popup/popup-menu-move-after-open.html

+<!DOCTYPE html>
+<title>Test popup menu positioned correctly after layout change</title>
+<div id="err" style="height:50px; display:block">
+  Test select popup positions correctly after an event causes update to select element positioning.
+</div>
+<select id="menu">
+    <option value="one">One</option>
+    <option value="two">Two</option>
+</select>
+<script>
+  menu.focus();
+  menu.onkeydown = removeError;
+
+  function removeError() {
+    err.style.display = 'none';
+    if (window.testRunner){
+      testRunner.notifyDone();
+    }
+  }
+
+  if (window.testRunner) {
+    testRunner.waitUntilDone();
+  }
+  eventSender.keyDown(" ", null);
+</script>
\ No newline at end of file

third_party/blink/web_tests/platform/linux/fast/forms/select-popup/popup-menu-move-after-open-expected.html

+<!DOCTYPE html>
+<select id="menu">
+    <option value="one">One</option>
+    <option value="two">Two</option>
+</select>
+<script>
+   menu.focus();
+   eventSender.keyDown(" ", null);
+</script>

third_party/blink/web_tests/platform/win/fast/forms/select-popup/popup-menu-move-after-open-expected.html

+<!DOCTYPE html>
+<select id="menu">
+    <option value="one">One</option>
+    <option value="two">Two</option>
+</select>
+<script>
+    menu.focus();
+    eventSender.keyDown(" ", null);
+</script>

third_party/blink/web_tests/platform/win/fast/forms/select-popup/popup-menu-resize-after-open-expected.txt

+PASS popupWindow.innerWidth is 100
+PASS popupWindow.innerWidth is 150
+PASS successfullyParsed is true
+
+TEST COMPLETE
+
+o1
+o2