Skip to content

Relation controller popup: fix size, add cssClass and allowDismiss #1044

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.

Sign up for GitHub

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

Open
AIC-BV wants to merge 13 commits into
base: develop
Choose a base branch
from
+39 −8
Open

Relation controller popup: fix size, add cssClass and allowDismiss #1044

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
13 commits
Select commit Hold shift + click to select a range
5708357
Update RelationController.php
AIC-BV Jan 22, 2024
72c6238
Update popup.js
AIC-BV Jan 22, 2024
b078572
Update storm-min.js
AIC-BV Jan 22, 2024
21edb7f
Update winter.relation.js
AIC-BV Jan 22, 2024
bde98dd
[fix] missing properties
AIC-BV Jan 22, 2024
4ec18a0
[fix] size is not filled in
AIC-BV Jan 22, 2024
fc77e5b
[add] allowDismiss
AIC-BV Feb 23, 2024
137b717
[add] allowDismiss
AIC-BV Feb 23, 2024
c296dbe
[add] allowDismiss
AIC-BV Feb 23, 2024
55701c2
[add] allowDismiss
AIC-BV Feb 23, 2024
3a1511a
Update popup.js
AIC-BV Aug 28, 2024
f4bf2ec
Update storm-min.js
AIC-BV Aug 28, 2024
929c822
Merge branch 'develop' into relation-controller-popup-size
LukeTowers Jan 7, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 8 additions & 2 deletions modules/backend/behaviors/RelationController.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -685,12 +685,18 @@ protected function makeViewWidget()
$config->recordUrl = $this->getConfig('view[recordUrl]');
$config->customViewPath = $this->getConfig('view[customViewPath]');
$config->noRecordsMessage = $this->getConfig('view[noRecordsMessage]');
$config->size = $this->getConfig('manage[size]');
$config->cssClass = $this->getConfig('manage[cssClass]');
$config->allowDismiss = $this->getConfig('manage[allowDismiss]');

$defaultOnClick = sprintf(
"$.wn.relationBehavior.clickViewListRecord(':%s', '%s', '%s')",
"$.wn.relationBehavior.clickViewListRecord(':%s', '%s', '%s', '%s', '%s', '%s')",
$this->relationModel->getKeyName(),
$this->relationGetId(),
$this->relationGetSessionKey()
$this->relationGetSessionKey(),
$config->size,
$config->cssClass,
$config->allowDismiss
);

if ($config->recordUrl) {
Expand Down
10 changes: 8 additions & 2 deletions modules/backend/behaviors/relationcontroller/assets/js/winter.relation.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,14 +9,20 @@
$(el).closest('.control-list').listWidget('toggleChecked', [el])
}

this.clickViewListRecord = function(recordId, relationId, sessionKey) {
this.clickViewListRecord = function(recordId, relationId, sessionKey, size, cssClass, allowDismiss) {
var newPopup = $('<a />'),
$container = $('#'+relationId),
requestData = paramToObj('data-request-data', $container.data('request-data'))

if (!size) {
size = 'huge'
}

newPopup.popup({
handler: 'onRelationClickViewList',
size: 'huge',
size: size,
cssClass: cssClass,
allowDismiss: allowDismiss,
extraData: $.extend({}, requestData, {
'manage_id': recordId,
'_session_key': sessionKey
Expand Down
18 changes: 17 additions & 1 deletion modules/system/assets/ui/js/popup.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,9 @@
content: null,
size: null,
adaptiveHeight: false,
zIndex: null
zIndex: null,
cssClass: null,
allowDismiss: false
}

Popup.prototype.init = function(){
Expand Down Expand Up @@ -207,8 +209,22 @@
if (this.options.adaptiveHeight)
modalDialog.addClass('adaptive-height')

if (this.options.cssClass)
modalDialog.addClass(this.options.cssClass)

if (this.options.zIndex !== null)
modal.css('z-index', this.options.zIndex + 20)

if (this.options.allowDismiss) {
modal.on('mousedown', function(e) {
const target = e.target;
if (target.classList.contains('control-popup')) {
modal.hide()
$('.popup-backdrop').remove()
$(document.body).removeClass('modal-open')
}
});
}
Comment on lines +218 to +227
Copy link

@coderabbitai coderabbitai bot Jan 7, 2026

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🔴 Critical

Critical: allowDismiss implementation bypasses popup lifecycle and lock mechanism.

The current implementation has several critical flaws:

  1. Line 222 uses modal.hide() (jQuery's hide method) instead of calling the Popup instance's hide() method (line 334). This bypasses:

    • The allowHide lock check (line 340) which prevents closing locked popups
    • The hide event triggers (lines 337-338)
    • The proper cleanup flow managed by Bootstrap modal events

  2. Lines 223-224 manually remove backdrop and body class instead of letting the Bootstrap modal hide events handle cleanup (lines 148-154). This can leave the popup in an inconsistent state.

  3. Missing unsaved changes protection: Per PR discussion, there is no change monitoring to warn users about unsaved data before dismissing. This creates a risk of accidental data loss.

Recommendation: Based on the PR discussion where LukeTowers requested removing allowDismiss until safer handling is implemented, consider either:

  • Remove the allowDismiss feature from this PR
  • Integrate Winter UI Input Monitor to detect unsaved changes before allowing dismiss
  • Use the popup's lock mechanism (this.lock(true)) when changes are detected
🔧 Proposed fix to use proper popup hide method 
         if (this.options.allowDismiss) {
+            var self = this
             modal.on('mousedown', function(e) {
                 const target = e.target;
                 if (target.classList.contains('control-popup')) {
-                    modal.hide()
-                    $('.popup-backdrop').remove()
-                    $(document.body).removeClass('modal-open')
+                    // Use the popup instance's hide method to ensure proper cleanup
+                    self.hide()
                 }
             });
         }

Note: This fix addresses the lifecycle bypass but does not solve the missing change monitoring concern.

🤖 Prompt for AI Agents 
In @modules/system/assets/ui/js/popup.js around lines 218 - 227, The
allowDismiss mousedown handler bypasses the Popup lifecycle by calling jQuery's
modal.hide() and manually removing backdrop/body classes; replace that handler
so it calls the Popup instance's hide() method (use this.hide() bound to the
popup context or call popup.hide()), remove manual $('.popup-backdrop').remove()
and $(document.body).removeClass('modal-open') calls so Bootstrap/Popup cleanup
and hide events run, and ensure the hide flow honors the allowHide/lock checks
(use this.lock(true) or check current lock state before dismissing). If you
prefer the safer option requested in PR, remove or disable the allowDismiss
feature entirely in this patch instead of re-enabling it until InputMonitor (or
equivalent unsaved-changes detection) is integrated.


return modal.append(modalDialog.append(modalContent))
}
Expand Down
9 changes: 6 additions & 3 deletions modules/system/assets/ui/storm-min.js
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Loading