Modal popup bugs and anomolies

B

Bauer

Well-Known Member
I have never used the "ajaxified links" popup feature in lists, simply because I could never get it to work as expected.
Here are the issues...
1. The popup width and height settings specified in the list configuration do not work.
2. There is no way to drag the popup.
3. If the popup is scrolled the entire page (viewport) scrolls with it.
4. There is no title shown in the popup
5. The popup's close and expand icons don't seem to appear where you would expect them to be.
(See 'before1.png' attachment)

This weekend I decided to try to figure out why. Here's what I've found...
1. There was a typo in the fabrik.js file that was assigning the popup height value set in the list configuration as the popup width!
a. That was easy enough to fix by changing one word in the fabrik.js file. A pull request has been added at github.
b. The height works correctly unless there is an image in the popup (e.g. a google map) - in which case the height will expand to contain the image (or scroll if the popup height is greater than the browser viewport.) (see before2.png) I suppose I can live with that - but still, it's not respecting the 'popup height' setting set in the list configuration in all cases.
2. I have no idea why the Fabrik 'draggable' javascript code isn't working. (But it didn't work in the CSV Export popup either - which I've been trying to get my pull request fixes at github merged for 2 months now.) But, as with the CSV popup, that's easily fixed with the jQuery code below.
3. Also easily fixed with the jQuery code below.
4. Again, I'm not sure if the Fabrik javascript is supposed to be finding some title to use in the popup - but if it is, it just isn't working. This can be fixed (for form popups anyhow) with the jQuery code below.
5. By giving the popup a title, the close and expand icons then look like you would expect them to appear.

Adding these few lines of javascript/jQuery code to the 'makeWindow' function in windows.js file takes care of all the issues listed above...
JavaScript: 
/* adds draggable feature to modal popup */
jQuery('div.modal').draggable({
    handle: '.modal-header'
});
jQuery('.modal-header').css('cursor','move');

/* Prevent browser window from being scrolled */
jQuery('body').css({'height':'100%','overflow':'hidden'});

/* Allow browser window to be scrolled again when modal is released from DOM */
jQuery('div.modal').on('remove', function () {
    jQuery('body').css({'height':'initial','overflow':'initial'});
});

/* Use form title if modal handlelabel is blank */
if(jQuery('div.modal-header .handlelabel').text().length==0){
    if(jQuery('div.itemContentPadder form').context.title.length){
        jQuery('div.modal-header .handlelabel').text(jQuery('div.itemContentPadder form').context.title);
    } 
}
(See 'after1.png' attachment)

Other than fixing the fabrik.js typo, I didn't do anything further at github because someone who is more familiar with the Fabrik javascript code might want to do it differently. Like maybe remove some of the javascript/mootools code in favor of jQuery - and/or add some of the other features I included in the CSV Export popup javascript - like better re-centering and handling of the popup when the popup is moved or the main viewport is resized. I'm just posting this so you know the issues and suggestions on how to fix them.

And finally, IMO, the preferred place to set the popup height and width for "ajaxified" links in a list would be on an individual basis - in the element's 'List view settings' - not a "one-size-fits-all" height/width setting in the list configuration. That would be a welcome fix that I'm sure all users would appreciate.
 

Attachments

  • before1.png
    before1.png
    136 KB · Views: 294
  • before2.png
    before2.png
    138.9 KB · Views: 283
  • after1.png
    after1.png
    136.5 KB · Views: 280
troester said:
There are some more issues
http://fabrikar.com/forums/index.ph...are-breaking-other-buttons-on-the-site.44105/

The title is missing if you are using the element link. With the add/view/edit buttons there's one displayed.

I've merged your PR (hopefully it's correct, I don't think I've tried to merge before).
Click to expand...
Thank you troester! :)
How that simple typo never got recognized until now is beyond me. I mean, no one ever complained that the popup width setting wasn't working?:confused:

I remember reading your post in the pro support forum when you posted it - and just figured 'someone' would fix it. When I get some time I'll try to see if I can't figure out what's going on there. FOLLOWUP EDIT: Guess what - I can't duplicate (or maybe I just don't understand the problem?). I have a list that is 'Ajaxified" with a GroupBy setup and tried opening and closing the popups - from both a list element and the action buttons - and everything seems to work OK.

Now if only I could get someone to merge the changes (#1720) for the CSV Export popup that I've been trying to get done for a while now, I'll be a happy camper.

The xml menu configuration settings I added for the CSV popup width option was merged 2 weeks ago - but they are useless (not even 'seen') without the changes in list.js that are in PR #1720. If someone does that, I promise to come up with a way of making similar options in the Element 'List view settings' for use in this window.js - which would take care of my suggestion in the last paragraph of the initial post in this thread. I think that controlling the size of the popup windows on a per-item basis is something that really needs to be done.
 
Last edited:
troester said:
There are some more issues
http://fabrikar.com/forums/index.ph...are-breaking-other-buttons-on-the-site.44105/

The title is missing if you are using the element link. With the add/view/edit buttons there's one displayed.
Click to expand...
I don't have access to pro support to respond in that thread - but "after further review" I am able to see the other issues you are talking about after all. What was happening for me was the list would lose all the data and the only thing that showed after the popup form was saved was the list heading and footer (and the buttons no longer worked - just as you have said).

It looks to me like the problems are all caused by the fact that the list is never updated after the ajax call that updates the table from the popup form.

I added this one line in the media/com_fabrik/js/list.js file at approx. line #1206 - just after the line: self._updateRows(json); - and it seems to have fixed those issues by reloading the list....
window.location.href=window.history.state.nav.url;

(If you want to test it out - you'll have to minimize that file after the edit and copy it to the dev folder - or rename the list.js file in the dev folder and just copy the changed non-minimized version there while you test.)

"Ajaxified" of not - I can't see how the list and all the associated filters and actions can get updated without reloading the entire list (one would probably take just s long as the other) - unless someone has written routine that does a one-row update for a list "in real time" that I'm not aware of. Until a better fix comes along, the url used in that line of code at least has the current pagination and filter settings to use when the page is reloaded, with the list containing the updated data - and all the buttons working correctly again.

And if you do a similar tweak - by adding those these few lines of javascript/jQuery code to the 'makeWindow' function in windows.js file, as mentioned 2 posts back - you should have your popups looking and working more like you might expect them to work.
 
Thanks for the investigation.
But
  • I don't have an issue with losing data - after saving an "edit" popup the list data is updated correctly
  • Your list.js seems not to match the current one - self._updateRows(json); is around line 1086 in my version
  • Your fix fixes the button issue, but only if you are editing. The issue is also if you are just viewing (so no row to update at all after closing). So I think it's something more in principal.
BTW: for testing JS you don't need to minify, just enable DebugJS in Fabrik Options/Debugging, then it's always loading the un-minified file (you only have to keep in mind that the minified version is still the old one;))
 
Bauer said:
Now if only I could get someone to merge the changes (#1720) for the CSV Export popup that I've been trying to get done for a while now, I'll be a happy camper.
Click to expand...

I'm trying to merge those atm, but Github is having a bad hair day (it's lagging horribly all round) and can't figure out whether it can merge or not, it's stuck at ...

Checking for ability to merge automatically?
Hang in there while we check the branch's status.

I'll try again later.

-- hugh
 
troester said:
Thanks for the investigation.
But
  • I don't have an issue with losing data - after saving an "edit" popup the list data is updated correctly
  • Your list.js seems not to match the current one - self._updateRows(json); is around line 1086 in my version
  • Your fix fixes the button issue, but only if you are editing. The issue is also if you are just viewing (so no row to update at all after closing). So I think it's something more in principal.
BTW: for testing JS you don't need to minify, just enable DebugJS in Fabrik Options/Debugging, then it's always loading the un-minified file (you only have to keep in mind that the minified version is still the old one;))
Click to expand...
Yep, you got it all covered, troester - as usual.

Playing with this more today I found that just adding a call to the 'doFilter()' function in list.php - as the last line in the _UpdateRows function - will also fix the dropdowns. But that still doesn't fix the weirdness that happens if you hit a Cancel/Go Back button in a popup form - or close a Details view popup - or simply close the popup window.

I'm hoping Hugh or Rob can identify or shed some light on why all the javascript on the main page seems to lose functionality when the popup comes up - and then never regains it. If I knew where how that is happening I could probably figure out how to restore the functionality when the popup window is closed without any type of edit. I've been trying to figure that out for hours.

Also, while messing with this today, I came to realize that the main page should probably be disabled or have some sort of overlay when the popup is active. Otherwise you can just keep clicking and adding more popups - and who knows what nightmares that might bring on.
 
Bauer said:
I was just out dancing in the street.:D
Click to expand...

In return, can you summarize this thread so I know where we're at with it. I've kind of lost the plot.

We committed some changes today which fixed issues with buttons / dropdowns not working after a popup closed.

-- hugh
 
Last edited:
cheesegrits said:
In return, can you summarize this thread so I know where we're at with it. I've kind of lost the plot.

We committed some changes today which fixed issues with buttons / dropdowns not working after a popup closed.

-- hugh
Click to expand...
OK. I'm just about to update from github. Let me do that first so I'm not whining about things that just got fixed.:p
 
cheesegrits said:
In return, can you summarize this thread so I know where we're at with it. I've kind of lost the plot.

We committed some changes today which fixed issues with buttons / dropdowns not working after a popup closed.
Click to expand...
OK - The good news is that the issues that troester brings up all seem to be fixed. And those are the ones that really prevented you from using the popup feature.

But the cosmetic problems that I listed numerically in the initial post at the top of this thread still remain - and insertion of the code I suggested (added to the 'makeWindow' function in the window.js file) will fix them.

The only change or question since that post (else I'd add a PR with my 'fixes' at github) is that I noticed that the resize and drag handler is already there in that makeWindow function - but it is surrounded by an if condition that prevents it from being added if type = 'modal'. And my question is WHY? Why shouldn't a modal popup be movable and resizeable?
 
But the cosmetic problems that I listed numerically in the initial post at the top of this thread still remain - and insertion of the code I suggested (added to the 'makeWindow' function in the window.js file) will fix them.
Click to expand...

Can you submit that as a PR, as I'm not sure how you intended that to play with the existing draggable code.

And my question is WHY? Why shouldn't a modal popup be movable and resizeable?
Click to expand...

No idea. That's a @rob thing.

-- hugh
 
That's weird. Now I can't stop mine from NOT being draggable! I removed the remark to allow that if condition again - and the popups are still draggable. (And yes I checked the window.js code in chrome developer toolbar and the old if condition is being used.) Maybe its a cache thing - but I cleared all the caches and still no difference. Have I told you lately I hate javascript?o_O

After fartin' around with this again, it seems like the key 'fix' to that (not draggable) problem that I've been complaining about is the code that adds a title to the popup window if it's empty.

The action buttons always had a popup window title - and I suppose a normal element link would too. But in my list the ajaxified element links are all custom links to filtered forms - so they had no title.

If there is no title there is no handle to drag. That's the crux of the problem.

But my 'fix' assumes the popup is a form (probably because my custom links are all forms:p) - so I'm not so sure that's the real solution either. I'll try adding another else to that code so it will just uses a   for the title if the popup is not a form - and see if it that creates a viable handle - otherwise some hard-coded catch-all title like 'Form' might be the way to go??? Or... in the element configuration where you can add the custom link - add an input option for a title? But that sounds too much like work.:(

You're the boss on this one , I'm just making suggestions.
 
Last edited:
Just worked on my suggestions a bit.
Using .text(' ') still doesn't give a handle, even though the spaces show in the div.
Using .text(' ') gives a handle that shows the title as " " verbatim!
Using .html(' ') works!!!

I also found I don't need to specify a form object in the if condition - the .itemContentPadder context has the same title property.

So the new code to fix the no-title-no-handle issue (and not have to worry about whether the popup is a form) is...
JavaScript: 
if(jQuery('div.modal-header .handlelabel').text().length==0){
    if(jQuery('div.itemContentPadder').context.title.length){
        jQuery('div.modal-header .handlelabel').text(jQuery('div.itemContentPadder').context.title);
    }else{
        jQuery('div.modal-header .handlelabel').html(" ");      
    }
}
I'll wait until we iron out this issue with draggable before creating a PR.
Maybe we should just trash the javascript that is there and use the jQueryUI draggable instead?
If you want to try that this works for you...
JavaScript: 
/* adds draggable feature to modal popup */
jQuery('div.modal').draggable({
    handle: '.modal-header'
});
 
And my question is WHY? Why shouldn't a modal popup be movable and resizeable?
Click to expand...
Well there are two ways to render things

Modal - should NOT be resizable or draggable
Window - should be resisable or draggable


So I'll be uncommeting that if statement you commented out as its needed for various things. If you need to get the window to be draggable etc then you defined that in the options : modal = false.

Also the title code is going to be brittle in that its dependant upon the jLayout being used. E.g for UIKit or bootstrap3 it may not work, but not a big issue as it will just fail silently in these cases
 
rob said:
Well there are two ways to render things

Modal - should NOT be resizable or draggable
Window - should be resisable or draggable


So I'll be uncommeting that if statement you commented out as its needed for various things. If you need to get the window to be draggable etc then you defined that in the options : modal = false.

Also the title code is going to be brittle in that its dependant upon the jLayout being used. E.g for UIKit or bootstrap3 it may not work, but not a big issue as it will just fail silently in these cases
Click to expand...
OK, I think I got it.
But in either case the parent container (the popup window) has the class 'modal', correct? I think that is what is so confusing to me.
 
But in either case the parent container (the popup window) has the class 'modal', correct? I think that is what is so confusing to me.
Click to expand...
unfortunately that kind of depends on which UI framework you are using :D Yes for bootstrap no for UIKit (it uses uk-modal). We have to have those classes to get the modal/window styling so yeah it is confusing :)
 
You must log in or register to reply here.

Useful links

  • Create a Fabrik Account (Downloads)
  • Create a Forum Account
  • Fabrik Wiki
  • Fabrik 4 Changelog /Install /Upgrade
    • We are in need of some funding.
    More details.

    Thank you.

    Members online

    Total: 86 (members: 2, guests: 84)
    Back
    Top