Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

loop calling of scope.updateSelectedStyles() after switching window #443

Closed
marbug opened this issue Dec 8, 2014 · 17 comments
Closed

loop calling of scope.updateSelectedStyles() after switching window #443

marbug opened this issue Dec 8, 2014 · 17 comments
Labels
Milestone

Comments

@marbug
Copy link
Contributor

marbug commented Dec 8, 2014

Hello.

During #423, #424, #425 issues investigation the following small issue was found (for 1.3.0-pre15).

Chrome, FireFox

  1. add console logging like the following:
    scope.updateSelectedStyles = function(){ console.log( 'scope.updateSelectedStyles' );

  2. click the TA's input field

  3. switch to another window (Alt+Tab)

  4. switch back
    See in console the loop calling of scope.updateSelectedStyles()
    I.e. scope._bUpdateSelectedStyles is not set to false

  5. click the TA's input field - looping is stopped.

@SimeonC
Copy link
Collaborator

SimeonC commented Dec 9, 2014

That's an odd bug. Thanks, I'll have a look when I can.

@SimeonC SimeonC added the bug label Dec 11, 2014
@SimeonC
Copy link
Collaborator

SimeonC commented Dec 15, 2014

I can't actually replicate this on OS X - something to do with how our windows work.

I think the following should fix it, could you test it for me and let me know please?

scope.updateSelectedStyles = function(){
    console.log('updateSelectedStyles');
    var _selection;
    // test if the common element ISN'T the root ta-text node
    if((_selection = taSelection.getSelectionElement()) !== undefined && _selection.parentNode !== scope.displayElements.text[0]){
        _toolbars.updateSelectedStyles(angular.element(_selection));
    }else _toolbars.updateSelectedStyles();
    // used to update the active state when a key is held down, ie the left arrow
    /* istanbul ignore else: browser only check */
    if(scope._bUpdateSelectedStyles && $document.hasFocus()) $timeout(scope.updateSelectedStyles, 200);
    else scope._bUpdateSelectedStyles = false
};

@SimeonC SimeonC added this to the 1.3.0 milestone Dec 16, 2014
SimeonC pushed a commit that referenced this issue Dec 16, 2014
@SimeonC
Copy link
Collaborator

SimeonC commented Dec 16, 2014

Or you can test it in the Pre-Release 17 version.

@marbug
Copy link
Contributor Author

marbug commented Dec 16, 2014

TypeError: undefined is not a function
at h.scope.updateSelectedStyles (http://localhost/marbug/textAngularUpgrade/textAngular/src/textAngular.js:1989:51)

I.e. $document does not have hasFocus() function

P.S. Chrome version 39.0.2171.95 (64-bit)

@marbug
Copy link
Contributor Author

marbug commented Dec 16, 2014

I tried the following and it helped in IE11, Chrome, FireFox:

            $($window).on( 'blur', function() { console.log('window has lost focus');
                scope._bUpdateSelectedStyles = false;
            });

@marbug
Copy link
Contributor Author

marbug commented Dec 16, 2014

I.e.

            // the following is for applying the active states to the tools that support it
            scope._bUpdateSelectedStyles = false;

            $($window).on( 'blur', function() { console.log('window has lost focus');
                scope._bUpdateSelectedStyles = false;
            });

            // loop through all the tools polling their activeState function if it exists
            scope.updateSelectedStyles = function(){ console.log( 'scope.updateSelectedStyles' );
                var _selection;
                // test if the common element ISN'T the root ta-text node
                if((_selection = taSelection.getSelectionElement()) !== undefined && _selection.parentNode !== scope.displayElements.text[0]){
                    _toolbars.updateSelectedStyles(angular.element(_selection));
                }else _toolbars.updateSelectedStyles();
                // used to update the active state when a key is held down, ie the left arrow
                if(scope._bUpdateSelectedStyles) $timeout(scope.updateSelectedStyles, 200);
            };

@marbug
Copy link
Contributor Author

marbug commented Dec 16, 2014

P.S. The additional issue is found. When a page URL is copied in IE11, then a loop calling of scope.updateSelectedStyles() also occurs.

STR:

  1. add console.log:
        scope.updateSelectedStyles = function(){ console.log( 'scope.updateSelectedStyles' );
  1. click on TA input field
  2. press any keyboard key
  3. click on the page URL (it should become selected)
  4. press Ctrl + Insert on keyboard (i.e. copy the selected url)
  5. see loop calling of scope.updateSelectedStyles() in console.
  6. click on TA input field - loop calling is stopped.

@SimeonC
Copy link
Collaborator

SimeonC commented Dec 16, 2014

Ok, I think I got it this time, try pre-release 18.

@marbug
Copy link
Contributor Author

marbug commented Dec 17, 2014

The main issue is fixed now. But the additional issue with IE11 still exists.

@SimeonC
Copy link
Collaborator

SimeonC commented Dec 17, 2014

I haven't got easy access to an IE11 machine to test it, I've added another check in the pre-release 19 that may fix this.

@marbug
Copy link
Contributor Author

marbug commented Dec 18, 2014

Unfortunately this did not help.

Very strange, but when focus is at the browser's URL

  1. IE11 fires keydown event (i.e. _keydown function is called)
  2. and IE11 does not fire keyup event (i.e. _keyup function is not called).

keydown event is fired only after pressing Ctrl button but not for others.

As a workaround _keydown function may:

  1. ignore pressing Ctrl button
  2. or check if focus is at TA fields.

IMHO 2) is better but indeed it's up to you.

P.S. I would prefer something like:

                            // start updating on keydown
                            _keydown = function(){ console.log('keydown');
                                    if ( !isFocusAtTAField() ) {
                                        scope._bUpdateSelectedStyles = false;
                                        return;
                                    }
                                    /* istanbul ignore else: don't run if already running */
                                    if(!scope._bUpdateSelectedStyles){

... (skipped)

                            function isFocusAtTAField() {
                                ...
                            }

@marbug
Copy link
Contributor Author

marbug commented Dec 18, 2014

@SimeonC
Copy link
Collaborator

SimeonC commented Dec 18, 2014

2 would indeed be better, can you try something for me in the _focusout = function(e){ event can you double-check that the blur event is called if you go from the page to the url please?

As long as that triggers correctly in IE11 I can use that, otherwise I'll use 1 - if that's possible, I'll have to check.

@marbug
Copy link
Contributor Author

marbug commented Dec 19, 2014

yes. _focusout is called in such case.

SimeonC pushed a commit that referenced this issue Jan 4, 2015
@SimeonC
Copy link
Collaborator

SimeonC commented Jan 4, 2015

OK, I've added that fix into the new pre-release can you check please?

@marbug
Copy link
Contributor Author

marbug commented Jan 8, 2015

Hello. The issue has been fixed.

@SimeonC
Copy link
Collaborator

SimeonC commented Jan 8, 2015

Awesome, thanks for getting back to us.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants