User tests: Successful: Unsuccessful:
Pull Request for Issue #15710 .
better handling of multiple tab widgets per page (by full xpath) and not saving the state if there is no id in the url (like on the CREATE page of content. giving better future proofing as well
Edit Article 1 - click a tab - click save - same tab should be selected
Edit Article 1 - click PERMISSIONS tab, Click an ACL Level - click save - same TABS should be selected
SAVE and CLOSE Article 1
Edit Article 2 - First Tab should be selected
SAVE and CLOSE Article 2
Create new article - NOTE first tab is selected.
Select the LAST tab, Click cancel
Create new article - NOTE first tab is selected.
Edit Article 1 - click the LAST TAB
Click cancel - go to installer and see FIRST TAB correctly shown and loaded (bonus fix!!!)
Ability to preserve state from CREATE to EDIT on first save.
Ability to link directly to a tabs #anchor using the hash of a url and have that tab be active on page load
Status | New | ⇒ | Pending |
Category | ⇒ | JavaScript |
Labels |
Added:
?
|
yes thats the only one that would be near impossible to track in a pure JS solution.
I have tested this item
Same issue as Bakual, but this is a small issue compared to the current issues.
If you can fix the tab select on new article then great and I'll test again. Otherwise I'm very happy with this solution.
Many thanks for your hard work on this @PhilETaylor
ok some more tiny improvements made, around saving on the frontend and also on initial states to fix the dreaded "no tab is actually selected" on page load bug.
I have tested this item
Almost but not quite.
Works perfectly on an existing item
But on a new item you dont save the state and this causes an issue - the one that tab-state is intended to resolve.
To replicate this go to com-content and create a new article
Go to the permissions tab and then select Save.
Expected behaviour is to stay on the permissions tab
Actual behaviour is to be redirected to the first tab
To replicate this go to com-content and create a new article
Go to the permissions tab and then select Save.
Expected behaviour is to stay on the permissions tab
Actual behaviour is to be redirected to the first tab
Exactly. This is the one state that CANNOT be saved using the current approach as the tab state is stored based on the URL and the URL on a CREATE screen has no id, and after saving it does.
But doing exactly what you are asking for, the last person broke tab states globally.
I'll see if I can find a "cute" solution today, but it will be separate to fixing the saving of states globally within the app correctly, which this PR addresses.
EDIT:
Like others already stated:
That could be a tricky to solve of course.
Same issue as Bakual, but this is a small issue compared to the current issues.
If you can fix the tab select on new article then great and I'll test again. Otherwise I'm very happy with this solution.
The problem is that in my long memory the reason that tab-state storage was created in the first place was for the exact scenario that isnt working. the other scenarios were side effects
Well now the 99% is fixed, thats a good thing and a huge step forward.
As I said, once I finish dealing with crap in my mailbox, I'll take another look at the create state and see if there is an easy win to be had with that too so that this PR exceeds your expectations.
@brianteeman whilst I agree, you edit a ton more than you do to create. Therefore this approach is better and then the side effect is just on new events.
The other option for @PhilETaylor would be to look at storing a value via PHP in the session or input and rendering it on load whereby the JS can check if it exists to activate as an active tab. Just my two cents.
storing a value via PHP in the session or input
Which is overkill for such a small improvement imho...
@PhilETaylor I agree, but cannot see a way of setting the ID in JS like you already are as the save of a new article happens server side when the ID is generated.
I agree, but cannot see a way of setting the ID in JS like you already are as the save of a new article happens server side when the ID is generated.
And therein lies the issue :)
Indeed! You could almost do with a reserve ID usage case, whereby the table ID is set as X and the auto-incremental number is increased therefore allowing that ID to be saved. But again, likely a massive project since you need to make everything using tabs compatible.
Sometimes I amaze myself... especially when I pretend to be a JS developer....
A pure JS solution has been found for @brianteeman use case - improving this PR now to keep it all in one place.
@PhilETaylor great thanks
@tonypartridge not quite - think of something with a lot of config options that you want to save and see what is happening etc before closing
Ok changes ready to test. The JS is now compressed, uncompressed version also there, the JS has been through JSLint and so it "better" than it was.
The solution now stores the UNSAVED_ITEM state, and uses this if no state is present on first page load of a URL (which is what happens after a new item is saved, happily)
The solution also fixes all the other niggles with unselected tabs showing in random pages.
PLEASE TEST A LOT as this has knock on effects if its wrong :) :)
Would be great if we could get this in vanilla JS now and tested fully as opposed to converting at a later date.
Would be great if we could get this in vanilla JS now and tested fully as opposed to converting at a later date.
No chance... Not by me...
Yeah don't worry, I'll make some changes my end and submit a PR to your branch
Also as it relies on bootstrap tabs anyway, its safe to assume that bootstrap/jQuery is available
This code currently works in Joomla 4 as well as joomla-cms/staging - its tested in both.
J4 needs to be jQuery free, core moves away from unneeded dependencies...
Well currently its not, and so the proposed solution is still valid.
It would not take a Javascript guru too long to recode away from the jQuery.
I am not not a Javascript guru. Im barely a human most days.
Currently the only thing that uses jQuery in the core is bootstrap and for that we have a solution named: custom elements, PR: #14333, repo: https://github.com/joomla-projects/custom-elements
We (as @C-Lodder , @ciar4n and me) are trying to finalise this (documented as well) and publish it before JAB
so reinventing the wheel for reinventing sake... gotcha. #NeverLearnTheLessonsFromThePast
Just attachting a start to this PR for future reference so I can find it. (completely untested so probably won't work)
https://gist.github.com/C-Lodder/cc5b573ce84f210bd68e803fdfbcb96e
so reinventing the wheel for reinventing sake
We are not reinventing anything there, we are moving forward, using technologies that will be the industry standard in couple of years instead of messing around with the ones that are dying...
Tabs, modals etc in essence are very simple things (change the active,show state) so why the heck you need jQuery for that, the platform provides all the tools to do it natively. You are totally missing the point here!
This PR is for 3,7 - if you want to do it differently for j4 then thats fine but dont hijaak this issue
Status | Pending | ⇒ | Closed |
Closed_Date | 0000-00-00 00:00:00 | ⇒ | 2017-05-03 11:26:03 |
Closed_By | ⇒ | brianteeman |
Status | Closed | ⇒ | New |
Closed_Date | 2017-05-03 11:26:03 | ⇒ | |
Closed_By | brianteeman | ⇒ |
Status | New | ⇒ | Pending |
(oops hit close by mistake)
I have tested this item
Works as advertised
Will tabstate still work when an extension is using something different than id as name? Would it be possible to make the id attribute somehow configurable?
Did you read the JS code?
This PR specifically DOESN'T by design use any ID to select the tabs - it uses xpath instead for exactly that reason (and others)
No tab is active when you click Save & New
button.
No tab is active when you click Save & New button.
Please restart your browser before testing
When I click save and new on an existing item, when on the fields tab, I am taken back to the first tab, which is expected.
When I saw line 84, that makes me wondering when my key is e_id if it still works. I will test it when I'm back home.
Line 84 is NOT e_id, its a_id and is related to front end content editing.
I have tested this item
So what happens when an extension uses e_I'd as parameter? Anyway I will test that case later.
Im sorry you are going to have to be more specific in what you are trying to ask.
With this PR each URL stores every instance of tabs on that page in its own storage. If there are tabs in an extension it should just work like it did before.
Status | Pending | ⇒ | Ready to Commit |
RTC after two successful tests.
Maintainers, can we put this on hold. I would like to test something.
I have tested this item
Working very well in all 12 tests I have tested.
Besides the fix for new records which is better behaviour
and besides better handling of multiple tab widgets
The new behaviour of TAB storage for existing records is quite annoying and less usefull than @dgt41 solution had given
Examples
new behaviour for existing records works as described
but is just not better choice than existing behaviour,
it results in a rather annoying / confusing UX
@ggppdk I have to disagree, your usage scenario is unique. If you are really bulk editing that many of the same tab you should be using an export tool or a bulk edit tool of sorts. Not article by article, since that's port UX from the start.
Most people would edit an article and it would not be the same edit to another article.
It's poor ux whereby I currently open a permissions tab on a component go to another component an it's open already. It shouldn't be because I haven't configured the first tab.
This pull provides the correct UX IMHO.
I supposed @PhilETaylor could look at adding a timestamp to the tab store and clearing after x time. But I don't think it's necessary.
So beside the fact that its millions times
millions ? ok, i just said what is better behaviour in the above mentioned cases
i would not make any claims on millions, billion or trillions
Which was a completely broken solution (incorrect handling of arrays)
that was not the point of my comment, i was commenting on desired / best behaviour
i was not commenting about existing implementation being broken / wrong for some cases !
In your opinion. No one else has yet agreed with you.
i would not know, i did not ask everyone,
nor we have waited for that long, lets wait some weeks or months
how much time did we take to find out annoying issues about existing behaviour ?
Finally i am not commenting on your efforts or your code in this PR,
please take no offence, i mean none
please dont take different opinions as offences or anything
my opinion is that i find new behaviour annoying in some cases and in some cases confusing was explained above, in other cases it is indeed useful.
This is hopefully my last comment in this PR
Same for me with SermonSpeaker (uses "s_id" in frontend). It always gets saved as an "UNSAVED_ITEM"
A side effect is also that when I afterwards create a new article, it gets the last selected tab (eg "published" tab) from my SermonSpeaker item. I think because it's stored as "UNSAVED_ITEM" without any reference to the item type.
Labels |
Added:
?
|
Would it not be better to have something like a wildcard instead of a hardcoded list of id parameters?
The urlParam function is already using a regex. I'm not a regex guru, but somebody with some better knowledge can probably change that function to support like $.urlParam("*_id")
.
I don't know about JS but in PHP, to remember model states, we are using the combination of option (com_content for example) and view (article for example) as the key. Can it be used for this case? So the key to store tab state can be com_content.article or something like that.
Ignore my comment if un-related. I don't know much about JS and haven't read the code in this PR yet
The please add the variables t_id, b_id, c_id also to the list as I have more entities in my extensions.
I didn't mean introduce PHP here. I just want to point out the key we use to store model states in our PHP code and thought it might be possible to use the same kind of key for js side.
I think it is very possible that extensions do have more than one entity like in DPCalendar I have tickets bookings and locations beside events. Just tried to explain you my situation.
No. How can you have a wildcard when you simply do not know what crap a 3pd developer is going to us as an id? Why cant they just use id, is that too simple? Thats what Joomla does!
Actually, in my case I took it just from com_content and assumed there was a reason Joomla does this. All I did is changing the a_id
from com_content to a more sensible s_id
in my case. So it's the same behavior Joomla has, not some "crap" 3rd parties invented. So please calm down a bit here.
Yep, that's exactly the same in our case as well. Backend works fine, frontend is broken.
That was also my intention, I could do the video also with front end editing.
I also tough that a_id stands for article id, so if you have an event I should use e_id.
I don't know what you are doing in your extension @PhilETaylor but this was the understanding for me as extension dev.
The ONLY think your extensions will not inherit from this PR, is the ability to preserve state from the CREATE mode through the save, to the EDIT mode. All other tabs will persist just like they always have done.
In frontend, it doesn't work for my extension since even edits are detected as "create" and thus there is only one record saved for each edit form. It's not even per type since the URL isn't stored in this case.
No, it isn't. Before it was at least stored per type correctly.
I guess you have the input from two extension developers. What you do with it is up to you @PhilETaylor. I guess this pr is a nice addition for the core, but it will not work for some extensions.
If that line 84 could do a wildcard like Allon suggested ("*_id") it would work great.
Another approach may be to store the "UNSAVED_ITEM" item with a different KEY so it includes the URL. Then it would at least be the same behavior with today.
Who deviate from best practice and don't use id to represent id!
If so, then core doesn't use best practice as well.
In a SINGLE place.
Exactly what I do, as said.
I have tested this item
Tested with DPCalendar which uses e_id in the url and with articles on front and back end. All is working fine.
Thanks @PhilETaylor for listening.
Works like a charm now.
I have tested this item
and there was also effort to support the above e.g. when implenent reordering etc
Is the above "crap" choice ?
because some extensions do not use 'id' as primary column name in DB ?
No it is not
Was there ever are requirement to use id or a_id or anything similar as the URL variable for identifying the record and or even a suggestion to map ther URL variable 'id' it to a custom name column of primary key ?
Why is it evil or crap to make this -optionally- configurable and avoid hardcoding ?
It should be straightforward to add support into the js code for
e.g. this
$document->addScriptOptions('js-tabstate', array('keys'=>array('e_id')));
$document->addScriptOptions('js-tabstate', array('keys'=>array('e_id', 'recno')));
or if you just want to support 1 key, like this:
$document->addScriptOptions('js-tabstate', array('key'=>'s_id'));
and the just keeping the 'id' as the default
If you make the "id" configurable and default to "ID", then it will break for all extensions that use a different identifier until they update their code and add that configuration. So that is not really an option.
If you make it configurable but fall back to the current proposal where "id" and "*_id" work both work, then it may be an interesting addition. But could as well be done in a different PR.
If you make the "id" configurable and default to "ID", then it will break for all extensions that use a different identifier until they update their code and add that configuration. So that is not really an option.
adding / not adding , in both cases the extensions that do no use the hardcoded options 'id', 's_id' etc are already broken
@ggppdk extensions that do not use the hardcoded options 'id', 's_id' etc are already broken
Wrong. They are NOT broken - did you even test?
TEST:
<ul class="nav nav-tabs">
<li class="active"><a data-toggle="tab" href="#home">Home</a></li>
<li><a data-toggle="tab" href="#menu1">Menu 1</a></li>
<li><a data-toggle="tab" href="#menu2">Menu 2</a></li>
</ul>
<div class="tab-content">
<div id="home" class="tab-pane fade in active">
<h3>HOME</h3>
<p>Some content.</p>
</div>
<div id="menu1" class="tab-pane fade">
<h3>Menu 1</h3>
<p>Some content in menu 1.</p>
</div>
<div id="menu2" class="tab-pane fade">
<h3>Menu 2</h3>
<p>Some content in menu 2.</p>
</div>
</div>
If you have tabs on the /index.php page (or any other page from a 3pd extension) these will persist (if the tab state behaviour is loaded) they store and retrieve from the UNSAVED_STATE key
There is simply no way of telling if a URL is a 3PD implemented edit page with a non-standard id, a 3PD CREATE page or a normal page just with tabs on, therefore this PR is backward compatible.
With Joomla core, if an id is in the url then we know we are not on a create page, but either an edit or view type layout.
If you make the "id" configurable and default to "ID", then it will break for all extensions that use a different identifier until they update their code and add that configuration. So that is not really an option.
Agreed
If you make the "id" configurable and default to "ID", then it will break for all extensions that use a different identifier until they update their code and add that configuration. So that is not really an option.
Why?
The tab state is loaded through JHtml and the proposed code should be there (and invisible to dev).
Why?
THERE SIMPLY IS NO REASON FOR PHP TO BE INVOLVED HERE!
Well, then keep things hardcoded and do the guestimation thing...
Discussion over.
This PR is already repeatedly tested and set RTC.
If you dont like it you can propose your own PR.
I have a day job to look after today.
If you are breaking 3rd PD apps then I guess the RTC is not that valid...
If you are breaking 3rd PD apps then I guess the RTC is not that valid...
DID YOU EVEN TEST?
DID YOU EVEN READ?
Tested with DPCalendar which uses e_id in the url and with articles on front and back end. All is working fine.
Works like a charm now.
/unsubscribe
'id=|[a-z]{1}_id='
is not covering all casesI rest my case
This is RTC!
You are wrong.
'id=|[a-z]{1}_id=' is not covering all cases
All cases of what exactly? Exactly, you dont know and don't care to test. This is actually relating to the NEW FUNCTIONALITY, we cover all Joomla's use cases for this NEW FUNCTIONALITY that this PR provides in addition to the bug fixes as well as RETAINING ALL BACKWARD COMPATIBILITY. We dont cover all use cases for all 3pd for this NEW FUNCTIONALITY however all the code is BACKWARD COMPATIBILE
So by your own admission, you have not bothered testing the PR.
This PR covers everything that Joomla used to do (badly), fixes the bugs, and provides new functionality at the same time as preserving backward compatibility.
If you dont like like it - tuff titty! Go do it yourself your own way - but don't go sprouting false lies that it is invalid and breaking 3pd if you cannot be bothered to even test it and provide accurate feedback
Im done. Seriously done.
I have tested this item
Internet Explorer 8
SCRIPT438: Object doesn't support property or method 'forEach'
eval code (41) (1,855)
This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/15748.
Good catch, IE doesnt like forEach, refactored to use
for (var i = 0; i < activeTabsHrefs.length; i++) {
if (activeTabsHrefs[i].indexOf(tabCollection) > -1) {
activeTabsHrefs.splice(i, 1);
}
}
(Ironically my first google result just said use jQuery.each to overcome browser compatibility issues LMAO!)
Please test @C-Lodder
Now getting:
Object doesn't support property or method 'evaluate'
tabs-state.js (1,585)
and:
Object doesn't support this action
html5fallback.js (1,1565)
well thats a killer... document.evaluate
is not available in IE...
That kills this whole approach for now. So unset RTC. This is not mergeable if it cannot work in IE8
I have added Wicked Good XPath
which takes the file to 30k compressed... please see if that works in IE as I dont have an IE machine here to test - if it does and 30k is acceptable great - if not then the use of xpath would need to be reworked and removed :(
Wicked Good XPath is a Google-authored pure JavaScript implementation of the DOM Level 3 XPath specification. It enables XPath evaluation for HTML documents in every browser. We believe it to be the fastest XPath implementation available in JavaScript.
eventually - yes
Can you test and tell me if the PR currently works in IE with the wicked stuff - if so then I can do the polyfil correctly - my time is limited today.
Status | Ready to Commit | ⇒ | Pending |
Labels |
Seems to be working now
Excellent - I'll get the pollyfill created correctly then later today.
Labels |
Removed:
?
|
Category | JavaScript | ⇒ | Libraries JavaScript |
Ok This is now tested in IE8, IE9, IE10, IE11 and standard Chrome and Safari on mac and looks like everything is working
everyone PLEASE RETEST to get this back to RTC
The polyfill in the uncompressed file is still minified. Is there an unminified version available to put in there?
I could not find one, only the sources, https://github.com/google/wicked-good-xpath
If you make it configurable but fall back to the current proposal where "id" and "*_id" work both work, then it may be an interesting addition. But could as well be done in a different PR.
yes i meant keep whatever hardcoded value you want as default fallback,
anyway this can be added in a different PR, without removing something from this PR
yup - something is broken today, will take another look.
(All these niggles have come as a result of trying to implement to persistent state from the create screen to the edit screen - without that NEW feature, there would not be so many cases to handle :))
Ok I have now refactored this again, with all bugs reported now fixed.
This push also includes NEW FUNCTIONALITY where you can specify the #hash in the url to trump any stored state (this is what @brianteeman wants, to be able to link to a tab) Note that only one has can be provided so you cannot deep link to the "Manager" Tab inside the "Permissions" Tab using just a hash... (You have never been able to do this so again this doesnt break b/c)
This push should continue to be compatible with 3PD extensions
Please test...
@brianteeman Please test your two requested NEW features that this PR contains
It is not working here Components > Smart Search > Search Filters
since it is using filter_id
.
@Quy It is not working here Components > Smart Search > Search Filters since it is using filter_id.
This is unrelated to this PR and is a bug in Joomla where the com_filter, filter view is not requesting the html behaviour for tabstate
I have added that to this PR now for completeness
To be clear, this fact that tab states did not work on Components > Smart Search > Search Filters NOT BECAUSE it is using filter_id BUT because the JS is not even loaded into the page - this is a bug in Joomla, not in this PR.
Category | JavaScript Libraries | ⇒ | Administration com_finder Libraries JavaScript |
I have tested this item
THANK YOU!
I have tested this item
tested on FF, Chrome, Edge and IE
Can you give an example @brianteeman because the appending of a #hash to a url DOESNT change the url and so it would be impossible to get what you are getting by just appending a #hash to the url
Ah I get what you mean now...
E.g. YOU CANNOT ever link to a tab inside something that needs checking out first. This is because Joomla has to first check out that item, and then redirect to the edit page. Joomla doesnt pass on the #hash in that case
e.g. you cannot just click a link like http://joomla-cms/administrator/index.php?option=com_content&view=article&layout=edit&id=3#attrib-fields-0 unless you have checked that item out for your use in this session already.
This is not a tab state issue, This is how Joomla works.
HOWEVER if you are in an extension, or some 3pd use of the tabs, then you can use the new feature to link to the tab on that page.
Maybe you misunderstood my request then - the objective was for example to be able to link to the permissions tab of a module directly from any page
So if you were in the admin home page you could follow a link to go directly to
/administrator/index.php?option=com_modules&view=module&layout=edit&id=27#permissions
Ability to link directly to a tabs #anchor using the hash of a url and have that tab be active on page load
Ah i think I missed the crucial word in the sentence above - hash -
So if i build the links with the hash they will work? If not how can I use or test this feature
I see the problem
YOU are linking to the final page of
and not to the CHECKOUT link of
http://joomla-cms/administrator/index.php?option=com_modules&task=module.edit&id=93#permissions
When you use the link with &task=module.edit
in it, Joomla will checkout an item and redirect to the destination &view=module&layout=edit&id=93#permissions
WITH the hash.
so YOU need to be using links like http://joomla-cms/administrator/index.php?option=com_modules&task=module.edit&id=93#permissions
which DO work and DO correctly load the permissions tab after this PR is applied :) :) :) :)
Almost but not quite
It does load the permissions tab but not with the anything highlighted on the left
Which in my use case would be ok as I would want to link to a specific usergroup permissions so according to your statement (and my understanding) then this will work
http://joomla-cms/administrator/index.php?option=com_modules&task=module.edit&id=93#permission-4
but it doesnt :(
So like I said above:
Note that only one has can be provided so you cannot deep link to the "Manager" Tab inside the "Permissions" Tab using just a hash... (You have never been able to do this so again this doesnt break b/c)
but you want that new feature too? It might be easy to add (after another strong coffee)
ok @brianteeman, I have just pushed yet more new features to this tab state PR, now you can link to
http://joomla-cms/administrator/index.php?option=com_modules&task=module.edit&id=93#permission-4
This will correctly trump any previous stored state for module id 93 url, and will correctly link directly to the Permissions Tab, and the Correct Permissions Level Tab in that pane.
Please test everything again but it should be b/c with the other fixes/features
(note it removed the #hash from the url on page load after saving the tab state so as to preserve the tab state correctly too)
I have tested this item
Working beautifully for me on my components, a few 3rd party tests and all core tests run all under Chrome.
There is a lot that could be refactored, fact is at the moment it works, and has several tests and should be RTC based on that. Further refinement could be made but I would prefer this to make Joomla 3.7.1 instead of making small untested tweaks and having to get people to retest to get to RTC.
On 9 May 2017, at 17:12, Lodder notifications@github.com wrote:
@C-Lodder commented on this pull request.
In media/system/js/tabs-state-uncompressed.js:
// if we managed to locate its selector directly
if (tabToClick.selector) {
// highlight tab of the tabs if the hash matches
tabsToClick.push(tabToClick);
} else {
// highlight first tab of the tabs
tabsToClick.push(tabToClick.first());
}
var parentPane = tabToClick.closest('.tab-pane');
// bubble up for nested tabs (like permissions tabs in the permissions pane
if (parentPane) {
This could be moved into 1 if statement, like so:
var parentPane = tabToClick.closest('.tab-pane'),
id = parentPane.attr('id'),
parentTabToClick = $(parentPane).find("a[href='#" + id + "']");if (parentTabToClick) {
tabsToClick.push(parentTabToClick);
}parentTabToClick will of course only exist if parentPane and id do.
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub, or mute the thread.
This is no longer the case. Clicking New
the second time, the last selected tab is active.
Create new article - NOTE first tab is selected.
Select the LAST tab, Click cancel
Create new article - NOTE first tab is selected.
This is no longer the case
Correct. By Design.
A lot has happened in this PR since the start.
I have tested this item
Status | Pending | ⇒ | Ready to Commit |
RTC after two successful tests.
Labels |
Added:
?
|
Status | Ready to Commit | ⇒ | Fixed in Code Base |
Closed_Date | 0000-00-00 00:00:00 | ⇒ | 2017-05-22 18:41:37 |
Closed_By | ⇒ | rdeutz |
This works fine except for one case:
That could be a tricky to solve of course.
Anyway this PR already improves thing a great deal imho.