? ? Pending

User tests: Successful: Unsuccessful:

avatar PhilETaylor
PhilETaylor
2 May 2017

Pull Request for Issue #15710 .

Summary of Changes

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

Testing Instructions

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!!!)

Two requested NEW features that this PR contains

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

Votes

# of Users Experiencing Issue
1/1
Average Importance Score
5.00

avatar PhilETaylor PhilETaylor - open - 2 May 2017
avatar PhilETaylor PhilETaylor - change - 2 May 2017
Status New Pending
avatar joomla-cms-bot joomla-cms-bot - change - 2 May 2017
Category JavaScript
avatar PhilETaylor PhilETaylor - change - 2 May 2017
Labels Added: ?
avatar Bakual
Bakual - comment - 2 May 2017

This works fine except for one case:

  • Create a new article, select another tab, save. Tab jumps back to first tab.

That could be a tricky to solve of course.

Anyway this PR already improves thing a great deal imho.

avatar PhilETaylor
PhilETaylor - comment - 2 May 2017

yes thats the only one that would be near impossible to track in a pure JS solution.

avatar tonypartridge
tonypartridge - comment - 2 May 2017

I have tested this item successfully on 1b49474

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


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/15748.

avatar tonypartridge tonypartridge - test_item - 2 May 2017 - Tested successfully
avatar PhilETaylor PhilETaylor - change - 2 May 2017
The description was changed
avatar PhilETaylor PhilETaylor - edited - 2 May 2017
avatar PhilETaylor
PhilETaylor - comment - 2 May 2017

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.

avatar franz-wohlkoenig
franz-wohlkoenig - comment - 3 May 2017

I have tested this item successfully on 254f112


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/15748.

avatar franz-wohlkoenig franz-wohlkoenig - test_item - 3 May 2017 - Tested successfully
avatar brianteeman
brianteeman - comment - 3 May 2017

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

avatar PhilETaylor
PhilETaylor - comment - 3 May 2017

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.

avatar brianteeman
brianteeman - comment - 3 May 2017

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

avatar PhilETaylor
PhilETaylor - comment - 3 May 2017

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.

avatar tonypartridge
tonypartridge - comment - 3 May 2017

@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.

avatar PhilETaylor
PhilETaylor - comment - 3 May 2017

storing a value via PHP in the session or input

Which is overkill for such a small improvement imho...

avatar tonypartridge
tonypartridge - comment - 3 May 2017

@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.

avatar PhilETaylor
PhilETaylor - comment - 3 May 2017

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 :)

avatar tonypartridge
tonypartridge - comment - 3 May 2017

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.

avatar PhilETaylor
PhilETaylor - comment - 3 May 2017

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.

avatar brianteeman
brianteeman - comment - 3 May 2017

@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

avatar PhilETaylor
PhilETaylor - comment - 3 May 2017

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 :) :)

avatar PhilETaylor PhilETaylor - change - 3 May 2017
The description was changed
avatar PhilETaylor PhilETaylor - edited - 3 May 2017
avatar C-Lodder
C-Lodder - comment - 3 May 2017

Would be great if we could get this in vanilla JS now and tested fully as opposed to converting at a later date.

avatar PhilETaylor
PhilETaylor - comment - 3 May 2017

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...

avatar C-Lodder
C-Lodder - comment - 3 May 2017

Yeah don't worry, I'll make some changes my end and submit a PR to your branch

avatar PhilETaylor
PhilETaylor - comment - 3 May 2017

Also as it relies on bootstrap tabs anyway, its safe to assume that bootstrap/jQuery is available

avatar dgt41
dgt41 - comment - 3 May 2017

@C-Lodder basically in J4 we have to move all this code to toolbar apply button and make it work with the custom elements tab (vanilla js). Or maybe integrate this functionality to the custom element itself...

avatar PhilETaylor
PhilETaylor - comment - 3 May 2017

This code currently works in Joomla 4 as well as joomla-cms/staging - its tested in both.

avatar dgt41
dgt41 - comment - 3 May 2017

J4 needs to be jQuery free, core moves away from unneeded dependencies...

avatar PhilETaylor
PhilETaylor - comment - 3 May 2017

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.

avatar dgt41
dgt41 - comment - 3 May 2017

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

avatar PhilETaylor
PhilETaylor - comment - 3 May 2017

so reinventing the wheel for reinventing sake... gotcha. #NeverLearnTheLessonsFromThePast

avatar C-Lodder
C-Lodder - comment - 3 May 2017

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

avatar dgt41
dgt41 - comment - 3 May 2017

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!

avatar brianteeman
brianteeman - comment - 3 May 2017

This PR is for 3,7 - if you want to do it differently for j4 then thats fine but dont hijaak this issue

avatar brianteeman brianteeman - change - 3 May 2017
Status Pending Closed
Closed_Date 0000-00-00 00:00:00 2017-05-03 11:26:03
Closed_By brianteeman
avatar brianteeman brianteeman - close - 3 May 2017
avatar brianteeman brianteeman - change - 3 May 2017
Status Closed New
Closed_Date 2017-05-03 11:26:03
Closed_By brianteeman
avatar brianteeman brianteeman - change - 3 May 2017
Status New Pending
avatar brianteeman brianteeman - reopen - 3 May 2017
avatar brianteeman
brianteeman - comment - 3 May 2017

(oops hit close by mistake)

avatar dgt41
dgt41 - comment - 3 May 2017

I have tested this item successfully on 9b9498e

Works as advertised


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/15748.

avatar dgt41 dgt41 - test_item - 3 May 2017 - Tested successfully
avatar laoneo
laoneo - comment - 3 May 2017

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?

avatar PhilETaylor
PhilETaylor - comment - 3 May 2017

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)

avatar Quy
Quy - comment - 3 May 2017

No tab is active when you click Save & New button.

avatar PhilETaylor
PhilETaylor - comment - 3 May 2017

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.

avatar laoneo
laoneo - comment - 3 May 2017

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.

avatar PhilETaylor
PhilETaylor - comment - 3 May 2017

Line 84 is NOT e_id, its a_id and is related to front end content editing.

avatar Quy
Quy - comment - 3 May 2017

I have tested this item successfully on 9b9498e


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/15748.

avatar Quy Quy - test_item - 3 May 2017 - Tested successfully
avatar laoneo
laoneo - comment - 3 May 2017

So what happens when an extension uses e_I'd as parameter? Anyway I will test that case later.

avatar PhilETaylor
PhilETaylor - comment - 3 May 2017

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.

avatar franz-wohlkoenig franz-wohlkoenig - change - 3 May 2017
Status Pending Ready to Commit
avatar franz-wohlkoenig
franz-wohlkoenig - comment - 3 May 2017

RTC after two successful tests.

avatar laoneo
laoneo - comment - 3 May 2017

Maintainers, can we put this on hold. I would like to test something.

avatar tonypartridge
tonypartridge - comment - 3 May 2017

I have tested this item successfully on 9b9498e

Working very well in all 12 tests I have tested.


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/15748.

avatar tonypartridge tonypartridge - test_item - 3 May 2017 - Tested successfully
avatar ggppdk
ggppdk - comment - 3 May 2017

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

  • i want to quickly access / modify e.g. the publishing TAB in a several records now i have to click the publising TAB again and again and again
  • also after editing a several records e.g. 20 records and then returning e.g. to record 454 which can be after e.g. an hour, you get whatever TAB you had opened which probably is no longer relevant nor needed, and gives the feeling that the TAB was randomly selected

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

avatar tonypartridge
tonypartridge - comment - 3 May 2017

@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.

avatar PhilETaylor PhilETaylor - comment - 3 May 2017
avatar PhilETaylor PhilETaylor - comment - 3 May 2017
avatar ggppdk
ggppdk - comment - 3 May 2017

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

avatar laoneo
laoneo - comment - 4 May 2017

I tested it with DPCalendar which uses e_id in the url and it is the old behavior:

webinstaller-1

Don't know if the problem is on line 84 which has hardcoded a_id.

avatar Bakual
Bakual - comment - 4 May 2017

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.

avatar PhilETaylor PhilETaylor - comment - 4 May 2017
avatar PhilETaylor PhilETaylor - change - 4 May 2017
Labels Added: ?
avatar PhilETaylor PhilETaylor - comment - 4 May 2017
avatar laoneo
laoneo - comment - 4 May 2017

Would it not be better to have something like a wildcard instead of a hardcoded list of id parameters?

avatar PhilETaylor PhilETaylor - comment - 4 May 2017
avatar PhilETaylor PhilETaylor - comment - 4 May 2017
avatar laoneo
laoneo - comment - 4 May 2017

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").

avatar PhilETaylor PhilETaylor - comment - 4 May 2017
avatar joomdonation
joomdonation - comment - 4 May 2017

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

avatar PhilETaylor PhilETaylor - comment - 4 May 2017
avatar laoneo
laoneo - comment - 4 May 2017

The please add the variables t_id, b_id, c_id also to the list as I have more entities in my extensions.

avatar PhilETaylor PhilETaylor - comment - 4 May 2017
avatar joomdonation
joomdonation - comment - 4 May 2017

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.

avatar PhilETaylor PhilETaylor - comment - 4 May 2017
avatar laoneo
laoneo - comment - 4 May 2017

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.

avatar PhilETaylor PhilETaylor - comment - 4 May 2017
avatar Bakual
Bakual - comment - 4 May 2017

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.

avatar PhilETaylor PhilETaylor - comment - 4 May 2017
avatar Bakual
Bakual - comment - 4 May 2017

Yep, that's exactly the same in our case as well. Backend works fine, frontend is broken.

avatar laoneo
laoneo - comment - 4 May 2017

That was also my intention, I could do the video also with front end editing.

avatar laoneo
laoneo - comment - 4 May 2017

I also tough that a_id stands for article id, so if you have an event I should use e_id.

avatar laoneo
laoneo - comment - 4 May 2017

I don't know what you are doing in your extension @PhilETaylor but this was the understanding for me as extension dev.

avatar PhilETaylor PhilETaylor - comment - 4 May 2017
avatar Bakual
Bakual - comment - 4 May 2017

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.

avatar PhilETaylor PhilETaylor - comment - 4 May 2017
avatar Bakual
Bakual - comment - 4 May 2017

No, it isn't. Before it was at least stored per type correctly.

avatar laoneo
laoneo - comment - 4 May 2017

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.

avatar Bakual
Bakual - comment - 4 May 2017

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.

avatar PhilETaylor PhilETaylor - comment - 4 May 2017
avatar Bakual
Bakual - comment - 4 May 2017

Who deviate from best practice and don't use id to represent id!

If so, then core doesn't use best practice as well.

avatar PhilETaylor PhilETaylor - comment - 4 May 2017
avatar Bakual
Bakual - comment - 4 May 2017

In a SINGLE place.

Exactly what I do, as said.

avatar PhilETaylor PhilETaylor - comment - 4 May 2017
avatar laoneo
laoneo - comment - 4 May 2017

I have tested this item successfully on 1134e60

Tested with DPCalendar which uses e_id in the url and with articles on front and back end. All is working fine.


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/15748.

avatar laoneo laoneo - test_item - 4 May 2017 - Tested successfully
avatar laoneo
laoneo - comment - 4 May 2017

Thanks @PhilETaylor for listening.

avatar Bakual
Bakual - comment - 4 May 2017

Works like a charm now.

avatar Bakual
Bakual - comment - 4 May 2017

I have tested this item successfully on 1134e60


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/15748.

avatar Bakual Bakual - test_item - 4 May 2017 - Tested successfully
avatar ggppdk
ggppdk - comment - 4 May 2017
  • JTable allows the DB column name to be any name,
  • and also possibly to have more than 1 columns composing the primary key

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

  • reading optional configuration (optionally set via PHP code of the extension) like @dgt41 said using $document->addScriptOptions()

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

avatar Bakual
Bakual - comment - 4 May 2017

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.

avatar PhilETaylor PhilETaylor - comment - 4 May 2017
avatar ggppdk
ggppdk - comment - 4 May 2017

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

avatar PhilETaylor
PhilETaylor - comment - 4 May 2017

@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

screen shot 2017-05-04 at 10 53 55

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.

avatar PhilETaylor
PhilETaylor - comment - 4 May 2017

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

avatar dgt41
dgt41 - comment - 4 May 2017

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).

avatar PhilETaylor
PhilETaylor - comment - 4 May 2017

Why?

THERE SIMPLY IS NO REASON FOR PHP TO BE INVOLVED HERE!

avatar dgt41
dgt41 - comment - 4 May 2017

Well, then keep things hardcoded and do the guestimation thing...

avatar PhilETaylor
PhilETaylor - comment - 4 May 2017

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.

avatar dgt41
dgt41 - comment - 4 May 2017

If you are breaking 3rd PD apps then I guess the RTC is not that valid...

avatar PhilETaylor
PhilETaylor - comment - 4 May 2017

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

avatar dgt41
dgt41 - comment - 4 May 2017
  • JTable allows the DB column name to be any name
  • 'id=|[a-z]{1}_id=' is not covering all cases

I rest my case

This is RTC!

avatar PhilETaylor
PhilETaylor - comment - 4 May 2017

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.

avatar C-Lodder
C-Lodder - comment - 4 May 2017

I have tested this item ? unsuccessfully on 1134e60

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.

avatar C-Lodder C-Lodder - test_item - 4 May 2017 - Tested unsuccessfully
avatar PhilETaylor
PhilETaylor - comment - 4 May 2017

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

avatar C-Lodder
C-Lodder - comment - 4 May 2017

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)

avatar PhilETaylor
PhilETaylor - comment - 4 May 2017

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.

avatar C-Lodder
C-Lodder - comment - 4 May 2017

assuming the minified stuff you now added are polyfills, they should go into separate JS files, like the rest

untitled

avatar PhilETaylor
PhilETaylor - comment - 4 May 2017

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.

avatar zero-24 zero-24 - change - 4 May 2017
Status Ready to Commit Pending
Labels
avatar C-Lodder
C-Lodder - comment - 4 May 2017

Seems to be working now

avatar PhilETaylor
PhilETaylor - comment - 4 May 2017

Excellent - I'll get the pollyfill created correctly then later today.

avatar PhilETaylor PhilETaylor - change - 4 May 2017
Labels Removed: ?
avatar joomla-cms-bot joomla-cms-bot - change - 4 May 2017
Category JavaScript Libraries JavaScript
avatar PhilETaylor
PhilETaylor - comment - 4 May 2017

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

avatar C-Lodder
C-Lodder - comment - 4 May 2017

The polyfill in the uncompressed file is still minified. Is there an unminified version available to put in there?

avatar PhilETaylor
PhilETaylor - comment - 4 May 2017

I could not find one, only the sources, https://github.com/google/wicked-good-xpath

avatar ggppdk
ggppdk - comment - 4 May 2017

@Bakual

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

avatar Quy
Quy - comment - 5 May 2017

Under Global Configuration, going from one section to another section, no tab is selected. Restarted the browser before testing and using latest staging.

2017-05-04_20-09-29

avatar PhilETaylor
PhilETaylor - comment - 5 May 2017

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 :))

avatar PhilETaylor
PhilETaylor - comment - 5 May 2017

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...

avatar PhilETaylor
PhilETaylor - comment - 5 May 2017

@brianteeman Please test your two requested NEW features that this PR contains

  • 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
avatar PhilETaylor PhilETaylor - change - 5 May 2017
The description was changed
avatar PhilETaylor PhilETaylor - edited - 5 May 2017
avatar Quy
Quy - comment - 7 May 2017

It is not working here Components > Smart Search > Search Filters since it is using filter_id.

avatar PhilETaylor
PhilETaylor - comment - 7 May 2017

@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.

avatar joomla-cms-bot joomla-cms-bot - change - 7 May 2017
Category JavaScript Libraries Administration com_finder Libraries JavaScript
avatar Quy
Quy - comment - 7 May 2017

I have tested this item successfully on f058746

THANK YOU!


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/15748.

avatar Quy Quy - test_item - 7 May 2017 - Tested successfully
avatar C-Lodder
C-Lodder - comment - 8 May 2017

I have tested this item successfully on f058746

tested on FF, Chrome, Edge and IE


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/15748.

avatar C-Lodder C-Lodder - test_item - 8 May 2017 - Tested successfully
avatar brianteeman
brianteeman - comment - 8 May 2017

Everything works for me EXCEPT for

Ability to link directly to a tabs #anchor using the hash of a url and have that tab be active on page load

I always get a message similar to

You are not permitted to use that link to directly access that page (#9).

avatar PhilETaylor
PhilETaylor - comment - 8 May 2017

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

avatar PhilETaylor
PhilETaylor - comment - 8 May 2017

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.

avatar brianteeman
brianteeman - comment - 8 May 2017

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

avatar brianteeman
brianteeman - comment - 8 May 2017

looks like we typed at the same time

The use case was specifically for #14944

Before I reverted the last change there was a js script to break the stored tab state and direct linking did work - albeit a little unreliably

avatar brianteeman
brianteeman - comment - 8 May 2017

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

avatar PhilETaylor
PhilETaylor - comment - 8 May 2017

I see the problem

YOU are linking to the final page of

http://joomla-cms/administrator/index.php?option=com_modules&view=module&layout=edit&id=93#permissions

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 :) :) :) :)

avatar brianteeman
brianteeman - comment - 8 May 2017

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 :(

avatar PhilETaylor
PhilETaylor - comment - 8 May 2017

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)

avatar PhilETaylor
PhilETaylor - comment - 8 May 2017

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

screen shot 2017-05-08 at 14 56 24

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

avatar PhilETaylor
PhilETaylor - comment - 8 May 2017

(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)

avatar tonypartridge
tonypartridge - comment - 9 May 2017

I have tested this item successfully on 3278d14

Working beautifully for me on my components, a few 3rd party tests and all core tests run all under Chrome.


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/15748.

avatar tonypartridge tonypartridge - test_item - 9 May 2017 - Tested successfully
avatar PhilETaylor
PhilETaylor - comment - 9 May 2017

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.

avatar Quy
Quy - comment - 10 May 2017

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.

avatar PhilETaylor
PhilETaylor - comment - 10 May 2017

This is no longer the case

Correct. By Design.

A lot has happened in this PR since the start.

avatar Quy
Quy - comment - 10 May 2017

I have tested this item successfully on 3278d14


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/15748.

avatar Quy Quy - test_item - 10 May 2017 - Tested successfully
avatar franz-wohlkoenig franz-wohlkoenig - change - 10 May 2017
Status Pending Ready to Commit
avatar franz-wohlkoenig
franz-wohlkoenig - comment - 10 May 2017

RTC after two successful tests.

avatar PhilETaylor PhilETaylor - change - 10 May 2017
Labels Added: ?
avatar PhilETaylor
PhilETaylor - comment - 10 May 2017

@rdeutz added this to the Joomla 3.7.2 milestone 21 minutes ago

Boo. Hiss.

avatar rdeutz rdeutz - change - 22 May 2017
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
avatar rdeutz rdeutz - close - 22 May 2017
avatar rdeutz rdeutz - merge - 22 May 2017

Add a Comment

Login with GitHub to post a comment