Skip to content

fix(OpenUI5Support): fix sap.m.Select inside a ui5-dialog #11939

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 13 commits into from
Jul 23, 2025

Conversation

TeodorTaushanov
Copy link
Member

@TeodorTaushanov TeodorTaushanov commented Jul 17, 2025

Organize all web components and OpenUI5 popups into a single array to ensure that focus is managed by the uppermost popup.

fixes #11933

@TeodorTaushanov TeodorTaushanov requested review from pskelin, Stoev, vladitasev and a team July 21, 2025 07:51
Copy link

@Stoev Stoev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please consider, if you want to make the header a bit more descriptive, although you already have this info, more or less, in the commit body:

fix(OpenUI5Support): unify popups registry for correct focus management

change: function (oEvent) {
console.error("Selected item:", oEvent.getParameter("selectedItem").getText());
}
}).placeAt("dilog1content");
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this a typo?
placeAt("dilog1content") > placeAt("dialog1content")

@@ -81,6 +94,7 @@
<ui5-button id="myButton">Open WebC Dialog</ui5-button>
</div>
<ui5-dialog id="dialog1" header-text="This is an WebC Dialog 1">
<div id="dilog1content"></div>
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this a typo?
"dilog1content" > "dialog1content"

@@ -14,6 +16,24 @@ type OpenUI5Popup = {
}
};

// contains all OpenUI5 and Web Component popups that are currently opened
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Alternatively, if you consider more descriptive:

Registry containing all currently opened OpenUI5 and Web Component popups.

Copy link
Contributor

@alexandar-mitsev alexandar-mitsev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

there 4 focus related bugs, but are not caused by the change (I will report them separately)

  • if you replace the select with combobox, pressing escape closes the full dialog and not only the combobox popover. Similar behaviour can be reproduced if combobox is in ui5 dialog
  • if you have combobox in openui5 dialog, start writing so that the popover appears - change the focus with Tab - there is a console error
  • if you use keyboard only to open all dialogs, esc does not close the openui5 dialog properly
  • the select is directly opened when the dialog is opened

patchPopup,
addOpenedPopup,
removeOpenedPopup,
getTopMostPopup,
Copy link
Contributor

@alexandar-mitsev alexandar-mitsev Jul 22, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you can also use the single word "topmost", which means exactly this. Although "top most" is not too wrong either.

@TeodorTaushanov TeodorTaushanov merged commit a02011f into main Jul 23, 2025
67 of 76 checks passed
@TeodorTaushanov TeodorTaushanov deleted the focusPatcher branch July 23, 2025 13:43
@TeodorTaushanov TeodorTaushanov restored the focusPatcher branch July 23, 2025 13:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[OpenUI5Support]: sap.m.Select broken inside a <ui5-dialog>
4 participants