Fixes #3505 Calling focus on hidden elements breaks page on ie8 #3516

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
5 participants
Contributor

sgrebnov commented Jan 31, 2012

No description provided.

Contributor

sgrebnov commented Jan 31, 2012

$.mobile.focusPage() fails in jquery.mobile.transition.js in outInTransitionHandler( name, reverse, $to, $from ) function. I see that if I remove this call from this place and change last parameter in 'deferred.resolve' call from true to false which means that the page has not been already focused then $.mobile.focusPage() is called in jquery.mobile.navigation.js - transitionPages.done function. In this cause page is visible and method call is ok. I only worry about that it is ok to comment focusPage in transition.js, please let me know if there are cases it won't work.

I also wrapped .focus call to try try/catch block to prevent similar errors in the feature.

Contributor

johnbender commented Jan 31, 2012

@sgrebnov

I think this just transplanted the issue that jQuery had up into our code. The problem with swallowing the exception is that it's the same as not calling focus at all in IE. If we remove the focus call altogether then it doesn't much matter but if the focus call is important then we need to move the focus call to the point at which the element is visible.

@toddparker

Can you comment on how important that focus is? (We focus the title on page load I believe)

Contributor

toddparker commented Jan 31, 2012

If you're asking if it's important to set focus on the active page, yes it is. Otherwise, the focus could be stuck on a non-active page which is bad for both accessibility and keyboard navigation.

Contributor

johnbender commented Jan 31, 2012

@sgrebnov

So given that we can't disable the focus and that it's actually really important for accessibility in IE (which from my conversations with @Wilto is one of the most used for that purpose), we have to defer the focus until that element is visible.

johnbender closed this Jan 31, 2012

Contributor

sgrebnov commented Feb 1, 2012

@johnbender
In my changes above .focus is successfully executed, I just made the focusPage method call to be executed in another place (after page become visible) - I commented it in jquery.mobile.transition.js, corrected parameter, after that it is successfully executed in jquery.mobile.navigation.js.

So the conclusion here then that jQuery Mobile is sacrificing IE8 support in favour of 'accessibility and keyboard navigation' on other browsers?

Contributor

sgrebnov commented Feb 3, 2012

Hi @davidosullivan. No, it does not, we still working on this bug.

cool, thats good news ;) Was just a bit confused by the fact the issue is marked as closed... I don't really understand Github- I'll have a read of the Help section...

Contributor

toddparker commented Feb 3, 2012

@davidosullivan This is a pull request, not an issue so we decided to close the PR because the suggested code approach didn't satisfy our requirements for both IE8 and accessibility support. @sgrebnov will create a new PR with a revised solution that will cover both requirements.

ahh ok I get it now ;) Thanks for taking the time to explain ;)

Contributor

johnbender commented Feb 3, 2012

@sgrebnov

Did you submit another PR, or did you intend to update this one with the changes you mentioned. I don't see either. Either way thanks for the change of tack!

Contributor

sgrebnov commented Feb 5, 2012

HI @johnbender,
Actually my changes mentioned are part of this PR (js/jquery.mobile.transition.js file), changing last parameter from 'true' to 'false' when calling deferred.resolve( name, reverse, $to, $from, false ) make the trick of setting focus later (as per my experiments). I'm only not sure about special cases where commented by me // $.mobile.focusPage( $to ); is really needed.

Contributor

scottjehl commented Feb 27, 2012

Hey all

I was just going through commits to figure out why transitions aren't feeling as good as when they landed. This change appears to be part of it...

Unfortunately, I think this change ended up clobbering remembered scroll distances when you return to a page. It'll also introduce a jump sometimes after pageshow, especially if iOS console is open (which isn't an actual issue really, but it clued me in that something was off on the timing).

In the transitions rewrite earlier this year, I moved this call from where it is again now, into the transitions handler to ensure that focus was called on a page before it's shown or scrolled, otherwise, focus will jump the page to the top again after the scroll.

It looks like this change undid that 51b0677

Of course, there appears to be an actual issue here to fix in IE8, but I'm pretty sure we should find another way of fixing it. that doesn't require moving the focus call back out of the transition handler again.

I'll take a look, and make a new issue.

Contributor

scottjehl commented Feb 27, 2012

Okay, some more info on this one...

IE8 only throws this error on the initial page load, and it's because this class:

.ui-mobile-rendering > * { visibility: hidden; }

...is still present when the page is transitioning in. I realized this when I noticed that the error was occurring because while page was indeed display:block, it was set to "visibility: hidden", meaning this isn't really a transitions bug, but rather one with init. I'll dig into getting that class removed earlier now.

Contributor

scottjehl commented Feb 27, 2012

Reopening to fix this another way...

@jasonlcrane jasonlcrane pushed a commit to jasonlcrane/jquery-mobile that referenced this pull request Feb 28, 2012

scottjehl Moved the call to remove the ui-mobile-rendering class from the page …
…transition done() promise to init JS. Fixes #3505 and Fixes #3516
4feaa46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment