? Success

User tests: Successful: Unsuccessful:

avatar dgt41
dgt41
16 Jun 2015

This PR drops the current custom code for sticking the two top toolbars together and instead use the already loaded bootstrap script!

Should work with all browsers but the idea was to fix #5970 and safari!

@wilsonge If this gets merged you got a nice conflict with 3.5-dev ???? Sorry!

@losedk @JoomliC and anyone with safari ever experienced this glitch can you give this a test?

avatar dgt41 dgt41 - open - 16 Jun 2015
avatar dgt41 dgt41 - change - 16 Jun 2015
The description was changed
avatar joomla-cms-bot joomla-cms-bot - change - 16 Jun 2015
The description was changed
Labels Added: ?
avatar dgt41 dgt41 - change - 16 Jun 2015
The description was changed
avatar dgt41 dgt41 - change - 16 Jun 2015
The description was changed
avatar losedk
losedk - comment - 17 Jun 2015

Seems to fix the problem :)

However noticed two things. The affix kicks in immediately on scroll and when clicking on menu items in the sidebar menu it jumps a bit

avatar dgt41
dgt41 - comment - 17 Jun 2015

@losedk So I fixed a bug by introducing another one? ????
For me the instant effect is not a problem and the jump of the sidebar is really unnoticeable...

avatar losedk
losedk - comment - 17 Jun 2015

Hehe :smile: Wouldn't call them bugs, just minor issues. But let's get some people to test :)

94d7ae6 17 Jun 2015 avatar dgt41 nope
avatar dgt41 dgt41 - change - 17 Jun 2015
The description was changed
avatar dgt41
dgt41 - comment - 17 Jun 2015

@losedk actually this is a problem caused by safari’s rubber band, all other browsers are totally fine (with or without this PR). But also I think is better to use already loaded scripts (bootstrap) instead of writing our own custom ones.
And also with this PR you will never end up with a messed up screen

avatar dgt41 dgt41 - change - 17 Jun 2015
The description was changed
avatar dgt41 dgt41 - change - 17 Jun 2015
The description was changed
avatar dgt41 dgt41 - change - 17 Jun 2015
The description was changed
avatar losedk
losedk - comment - 17 Jun 2015

@dgt41 Ahh okay, I didn't know that. I totally agree that it's much better to use the native BS.

avatar dgt41 dgt41 - change - 17 Jun 2015
The description was changed
avatar dgt41 dgt41 - change - 17 Jun 2015
The description was changed
avatar dgt41 dgt41 - change - 17 Jun 2015
The description was changed
avatar zero-24 zero-24 - change - 17 Jun 2015
Category JavaScript
avatar zero-24 zero-24 - change - 17 Jun 2015
Status New Pending
Easy No Yes
avatar dgt41
dgt41 - comment - 17 Jun 2015

@infograf768 JM can you test this with com_localize as I am touching the sidebar script here and AFAIK there were some compatibility issues when sidebar was introduced. Thanks

avatar dgt41 dgt41 - change - 17 Jun 2015
The description was changed
avatar infograf768
infograf768 - comment - 18 Jun 2015

@dgt41
Will do asap.

avatar infograf768
infograf768 - comment - 19 Jun 2015

@dgt41
Looks like it's fine in com_localise.

avatar zero-24 zero-24 - alter_testresult - 19 Jun 2015 - infograf768: Tested successfully
avatar zero-24 zero-24 - alter_testresult - 19 Jun 2015 - losedk: Tested successfully
avatar zero-24 zero-24 - change - 19 Jun 2015
Status Pending Ready to Commit
avatar zero-24
zero-24 - comment - 19 Jun 2015

RTC based on Tests by @losedk and @infograf768 Thanks :smile:


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

avatar zero-24 zero-24 - change - 19 Jun 2015
Labels Added: ?
avatar dgt41
dgt41 - comment - 23 Jun 2015

I just found another less intruding way to solve this:

html {
height: 100%;
}

That will not require a change for our scripts here

avatar wilsonge
wilsonge - comment - 23 Jun 2015

From a technical perspective I'd imagine it's better to use the bootstrap affix though than hack it ourselves and then add in fixes when the functionality already exists? I mean I'd rather get it right than bug fix the hacks :P

avatar dgt41
dgt41 - comment - 23 Jun 2015

Apart from the wrong decision to affix also the sidebar (that’s a big change from the previous behavior and also is buggy), I totally agree with you.

avatar infograf768
infograf768 - comment - 24 Jun 2015

@test
Still OK here for core and com_localise.

avatar brianpeat
brianpeat - comment - 24 Jun 2015

Seems to fix the issue for me in 2 sites once I remembered to clear my cache.


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

avatar brianpeat brianpeat - test_item - 24 Jun 2015 - Tested successfully
avatar brianteeman
brianteeman - comment - 24 Jun 2015

Multiple good tests setting to RTC

@wilsonge would be good to get this into 3.4.2


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

avatar wilsonge wilsonge - change - 24 Jun 2015
Milestone Added:
avatar rdeutz rdeutz - change - 28 Jun 2015
Milestone Added:
avatar rdeutz rdeutz - change - 28 Jun 2015
Milestone Removed:
avatar dgt41
dgt41 - comment - 29 Jun 2015

@rdeutz Robert are there any particular reasons to postpone this for 3.4.3?

avatar Bakual
Bakual - comment - 29 Jun 2015

We only fix critical things during an RC. As far as I understood this is only a minor visual thing.

avatar brianpeat
brianpeat - comment - 29 Jun 2015

I would not consider this minor. There are times when I am completely unable to get to buttons because the header never expands. It's enough to make me almost want to stop using Safari because at times the admin screens are unusable (can't name something, can't access the New button, etc).

avatar dgt41
dgt41 - comment - 29 Jun 2015

@Bakual Fair enough, I just guessed since we have this fix that annoy a lot of people maybe we could also include that as well.

avatar mbabker
mbabker - comment - 29 Jun 2015

@dgt41 IMO this is a step backwards as now we again have the jumping subnav bar.

avatar dgt41
dgt41 - comment - 29 Jun 2015

@mbabker jumping? You mean the animation is not as smooth as it was or you get some other glitch?

avatar mbabker
mbabker - comment - 29 Jun 2015

No animation at all, just jumps... http://cl.ly/0s0v0i0e0S17

avatar zero-24 zero-24 - change - 2 Jul 2015
Milestone Removed:
avatar zero-24 zero-24 - change - 2 Jul 2015
Milestone Removed:
avatar zero-24 zero-24 - change - 2 Jul 2015
Milestone Added:
avatar zero-24 zero-24 - change - 2 Jul 2015
Milestone Added:
avatar dgt41
dgt41 - comment - 4 Jul 2015

I do not quite agree with @mbabker that this is a step backwards, but I also realize that a better animation can be applied here. So with the latest commit I try to follow the same route Joomla did with sidebar: use css3 transition.

I would like to ask if you agree with this animation or if it is too long/short?
So @losedk @mbabker @brianpeat @infograf768 your feedback here is essential, Thanks

avatar mbabker
mbabker - comment - 4 Jul 2015

The animation is only triggering for me in Safari; Chrome and Firefox still just jump.

So here's why I say it's a step backwards. Right now, for the most part, the toolbar smoothly scrolls with the page and affixes when it reaches the target point. With this PR applied, scrolling one pixel causes it to jump up to its affixed spot. So in fixing an issue for one case, we're introducing a less than optimal behavior for another case.

FWIW, this same JavaScript logic is in use on the Bootstrap based joomla.org template and there I can't duplicate the behavior in Safari of the page content not being below the second toolbar (in the .org case it's the site menu). Actually, when the page loads "properly", unpatched I see the body contents get pushed down during rendering in the Joomla admin, but the .org sites I tested against (API which is using pure HTML and an unaltered Bootstrap source and Developer which is a CMS install) don't exhibit the same behaviors.

avatar dgt41
dgt41 - comment - 4 Jul 2015

The problem is that the current script used in Isis is somehow conflicting with the bootstrap collapse for the same toolbar. If you remove the collapse from the subhead everything works nicely. I think we have many competing scripts (sidebar, collapse, affix) and somehow there is an interference as all of them trying at the same time to calculate the save values e.g. height, width

avatar mbabker
mbabker - comment - 4 Jul 2015

Without redesigning the whole top section I don't see there being a good fix for this whole thing. Either something has to stop being affixed or we have to deal with rather ugly behaviors on scroll.

avatar dgt41
dgt41 - comment - 4 Jul 2015

@mbabker I will agree on that redesign! Anyways I gave it one more try fixing FF,chrome,opera,IE11 animation

avatar dgt41
dgt41 - comment - 4 Jul 2015

@mbabker messing around with the old script I found a root of the problem, so indeed we don’t have to drop the old script. Here it is:

            function processScroll()
            {
                if ($('.subhead').length) {
                    var scrollTop = $(window).scrollTop();
                    if (scrollTop >= navTop && !isFixed) {
                        isFixed = true;
                        $('.subhead').addClass('subhead-fixed');

                        // Fix the container top
                        $(".container-main").css("top", $('.subhead').offset().top + $('nav.navbar').height());
                    } else if (scrollTop <= navTop && isFixed) {
                        isFixed = false;
                        $('.subhead').removeClass('subhead-fixed');
                    }
                }
            }
avatar dgt41
dgt41 - comment - 4 Jul 2015

OK I am gonna leave the decision to PLT if they want to merge this or #7342 which solves the problem with the old script ????

avatar JoomliC
JoomliC - comment - 7 Jul 2015

It is not having the expected behavior for me (just tested on FF), and with issue on mobile device (the last pixel up render smaller toolbar buttons)...
In the same time, #7342 is working nice, and don't change the behavior (i understand your goal with this PR, but not sure it's an easy job to introduce it in Isis (and not yet working with fluidity), or to keep some new stuff for a future new admin template ;-) )


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

avatar JoomliC JoomliC - test_item - 7 Jul 2015 - Tested unsuccessfully
avatar zero-24 zero-24 - close - 7 Jul 2015
avatar wilsonge
wilsonge - comment - 7 Jul 2015

As this still seems buggy for people and as metioned #7342 is working ok. going to close this for #7342 - because at the end of the day we need something that works.

avatar wilsonge wilsonge - change - 7 Jul 2015
Status Ready to Commit Closed
Closed_Date 0000-00-00 00:00:00 2015-07-07 14:01:50
Closed_By wilsonge
avatar wilsonge wilsonge - close - 7 Jul 2015
avatar wilsonge wilsonge - close - 7 Jul 2015
avatar zero-24 zero-24 - change - 7 Jul 2015
Milestone Removed:
avatar zero-24 zero-24 - change - 14 Oct 2015
Labels Removed: ?

Add a Comment

Login with GitHub to post a comment