tdf#164093 tdf#157001 a11y: Improve menu window disposal

In MenuFloatingWindow::dispose, unset the accessible
before calling the base class implementation, to prevent
the accessible from getting disposed there as well.
This is necessary because the MenuFloatingWindow
doesn't create (and therefore own) its accessible
object, but returns that from its menu in
MenuFloatingWindow::CreateAccessible. Therefore,
the PopupMenu as the owner is responsible for disposing the
accessible as well, not the MenuFloatingWindow.

This logic was already implemented in Menu::dispose,
but that doesn't help in cases where the MenuFloatingWindow
gets disposed independently.

In Menu::dispose, drop the extra logic
and just call `m_pWindow.disposeAndClear`
so that MenuFloatingWindow::dispose can
take care of everything that's needed.
(For the MenuBar case, MenuBar::dispose
calls MenuBar::ImplDestroy, which unsets
Menu::m_pWindow, so other than for PopupMenu,
this change to MenuBar::ImplDestroy method doesn't
make a difference for that class.)

Don't dispose the current Menu::m_pWindow in
PopupMenu::FinishRun, but instead in
PopupMenu::ImplExecute (if set) before assigning
a new one, to ensure that the old one gets disposed.
Drop the

SAL_WARN_IF(GetWindow(), "vcl", "Win?!");

because that case is handled fine now. Without this
change in place, I saw that warning getting triggered
while debugging (potentially because PopupMenu::FinishRun
never got reached as the menu didn't show,  maybe due to
switching focus to the IDE when reaching a breakpoint
or different timing,...).

This commit by itself doesn't yet fix the crash
from tdf#164093 seen on Windows with NVDA running
because the menu's accessible still incorrectly gets
disposed by the MenuFloatingWindow's VCLXWindow
(toolkit window peer). That will be addressed
in a separate commit.

Change-Id: Ia2931bee23204395e8b3396927acf4fa1d0f077c
Reviewed-on: https://gerrit.libreoffice.org/c/core/+/177883
Tested-by: Jenkins
Reviewed-by: Michael Weghorn <m.weghorn@posteo.de>

2 files changed, 8 insertions, 11 deletions

diff --git a/vcl/source/window/menu.cxx b/vcl/source/window/menu.cxx
index a8c460629dd4..12c4490aa9df 100644
--- a/vcl/source/window/menu.cxx
+++ b/vcl/source/window/menu.cxx
@@ -189,15 +189,7 @@ void Menu::dispose()
 {
     ImplCallEventListeners( VclEventId::ObjectDying, ITEMPOS_INVALID );
 
-    // at the window free the reference to the accessible component
-    // and make sure the MenuFloatingWindow knows about our destruction
-    if (m_pWindow)
-    {
-        MenuFloatingWindow* pFloat = static_cast<MenuFloatingWindow*>(m_pWindow.get());
-        if( pFloat->pMenu.get() == this )
-            pFloat->pMenu.clear();
-        m_pWindow->SetAccessible( css::uno::Reference< css::accessibility::XAccessible >() );
-    }
+    m_pWindow.disposeAndClear();
 
     // dispose accessible components
     comphelper::disposeComponent(mxAccessible);
@@ -2890,7 +2882,6 @@ sal_uInt16 PopupMenu::ImplExecute(const VclPtr<vcl::Window>& pParentWin, const t
                                                            | FloatWinPopupEndFlags::CloseAll);
     }
 
-    SAL_WARN_IF(GetWindow(), "vcl", "Win?!");
     tools::Rectangle aRect(rRect);
     aRect.SetPos(pParentWin->OutputToScreenPixel(aRect.TopLeft()));
 
@@ -2951,6 +2942,8 @@ sal_uInt16 PopupMenu::ImplExecute(const VclPtr<vcl::Window>& pParentWin, const t
         pWin->SetBorderStyle( WindowBorderStyle::NOBORDER );
     else
         pWin->SetBorderStyle( pWin->GetBorderStyle() | WindowBorderStyle::MENU );
+
+    m_pWindow.disposeAndClear();
     m_pWindow = pWin;
 
     Size aSz = ImplCalcSize( pWin );
@@ -3084,7 +3077,6 @@ void PopupMenu::FinishRun(const VclPtr<MenuFloatingWindow>& pWin, const VclPtr<v
         pWin->StopExecute();
 
     pWin->doShutdown();
-    m_pWindow.disposeAndClear();
     ImplClosePopupToolBox(pParentWin);
     ImplFlushPendingSelect();
 }
diff --git a/vcl/source/window/menufloatingwindow.cxx b/vcl/source/window/menufloatingwindow.cxx
index 8beb36be0760..d49469a7ab2c 100644
--- a/vcl/source/window/menufloatingwindow.cxx
+++ b/vcl/source/window/menufloatingwindow.cxx
@@ -125,6 +125,11 @@ void MenuFloatingWindow::dispose()
     pMenu.clear();
     pActivePopup.clear();
     xSaveFocusId.clear();
+
+    // unset accessible taken from the PopupMenu (s. CreateAccessible),
+    // it is owned and therefore disposed by the PopupMenu
+    SetAccessible(nullptr);
+
     FloatingWindow::dispose();
 }