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

toolbar does not disappear #371

Closed
kel093 opened this issue Nov 4, 2015 · 13 comments

Comments

Projects
None yet
3 participants
@kel093
Copy link

commented Nov 4, 2015

Hello. I am apologise if i duplicate the issue, but i don't found it in list of issue's.
I am using the AlloyEditor and today saw strange behaviour. I selected the text after this the toolbar is appears, then i clicked on the any place on the screen and the toolbar does not disappear.
bug

I investigated how plugin works and understood the problem, the problem is that editor.status == "ready" and listener "contentDom" will not be called. That's means that listeners "mousedown" and "keydown" are not be added to document.

I fixed this problem, please see my changes in function componentDidMount().

        if (editor && editor.status == "ready") {
            document.addEventListener('mousedown', this._mousedownListener);
            document.addEventListener('keydown', this._keyDownListener);
        } else {
            editor.once('contentDom', (function () {
                document.addEventListener('mousedown', this._mousedownListener);
                document.addEventListener('keydown', this._keyDownListener);
            }).bind(this));
        }
@ipeychev

This comment has been minimized.

Copy link
Contributor

commented Nov 4, 2015

Hey @kel093,

Interesting issue! How exactly we can reproduce it?
Also, could you see if the following code snippet will fix your issue too:

editor.once('instanceReady', function() {
    document.addEventListener('mousedown', this._mousedownListener);
    document.addEventListener('keydown', this._keyDownListener);
}.bind(this));

Maybe we can just change the event and avoid the other "if".

Thanks,

@kel093

This comment has been minimized.

Copy link
Author

commented Nov 4, 2015

I tried to change event "contentDom" on "instanceReady", but didn't help.

It's easy to reproduce, just call destroy() then init() on earlier initialized AlloyEditor.

Look like on this code:

var editor2 = AlloyEditor.editable('editable'); //init the AlloyEditor
setTimeout(function(){
    //reinit AlloyEditor
    editor2.destroy();
    editor2.init();
},1000);
@ipeychev

This comment has been minimized.

Copy link
Contributor

commented Nov 5, 2015

Ah, I see! This way of reinitializing is unsupported however. The function "init" is not supposed to be called manually.
The recommended way after destroying is to get a new fresh instance of the editor, like this:

var editor2 = AlloyEditor.editable('editable'); //init the AlloyEditor
setTimeout(function(){
    //reinit AlloyEditor
    editor2.destroy();
    editor2 = AlloyEditor.editable('editable');
},1000)

Thanks,

@kel093

This comment has been minimized.

Copy link
Author

commented Nov 5, 2015

I tried this way, but It didn't help too.

@ipeychev

This comment has been minimized.

Copy link
Contributor

commented Nov 5, 2015

Hm. Okay. Chema, would you have 5 mins to check this one? If not, I can try this evening.

@jbalsas

@jbalsas

This comment has been minimized.

Copy link
Member

commented Nov 5, 2015

Yes, I'll check this one out later today

@jbalsas

This comment has been minimized.

Copy link
Member

commented Nov 5, 2015

I just checked, and yeah, it does make sense that it would fail. I think the best fix would be to change the event from contentDom to focus. This way, we can even save the event initialization if the editor never gets really used.

I'll put a PR with this in a little while.

@kel093, I tested this and it even fixes the unsupported call to init instead of the static initialization, but could you verify this fixes the issue for you as well?

@kel093

This comment has been minimized.

Copy link
Author

commented Nov 5, 2015

@jbalsas, I just tested and it looks well for me too.
Thanks for quick fix.

@jbalsas jbalsas added this to the 0.6.1 milestone Nov 5, 2015

@jbalsas jbalsas added the bug label Nov 5, 2015

@jbalsas jbalsas closed this in e2a8540 Nov 6, 2015

ipeychev added a commit that referenced this issue Nov 6, 2015

ipeychev added a commit that referenced this issue Nov 6, 2015

@ipeychev

This comment has been minimized.

Copy link
Contributor

commented Nov 6, 2015

Hey @jbalsas,

Changing the event to focus breaks one of the tests on Firefox and all IEs.
After looking also on the comments here, let's try another fix, which will be to not wait for "contentDom or "focus" here.
The change is committed, could you please check if that is OK for everybody?

@kel093

Thanks,

@jbalsas

This comment has been minimized.

Copy link
Member

commented Nov 6, 2015

mmm... that's weird, they were passing for me... I guess only added the change for the ae-editable class part at the end and forgot to re-run the tests :(

Of course it makes sense that test will fail, we should update the initialization of the editors to fire a focus event before doing anything else. This is what will actually happen in practice when a user tries to work with the editor, isn't it?

The only thing that worried me was that some styling could flicker but I didn't see anything weird.

In any case I don't think attaching the events always will cause other issues. Will check later.

Thanks!

@ipeychev

This comment has been minimized.

Copy link
Contributor

commented Nov 6, 2015

Yeah, the first thing I tried was to add focus in the initialization, bit it didn't work. However, please check again, it was 4:30am :)

Focus is nasty event in general, very hard to deal, especially for testing. Had multiple issues previously with Firefox and since then I'm trying to avoid it. In this case it makes sense, if we can do it properly. Otherwise, let's attach the other events even before user starts to interact with the editor.

@jbalsas

This comment has been minimized.

Copy link
Member

commented Nov 6, 2015

I think we can take the safe route here and avoid focus alltogether. I even have the feeling that this might save us from some issues in the future, so I'm fine with it.

And btw, I'm going to ask for a prohibition from 23pm to 6:30am on commits on this repo so you can get some more sleep 😜

@ipeychev

This comment has been minimized.

Copy link
Contributor

commented Nov 6, 2015

Your request was declined. I will sleep when I get retired ;)

dpobel added a commit to ezsystems/PlatformUIAssetsBundle that referenced this issue Dec 7, 2015

Fix EZP-25181: The richtext toolbar don't close when we change field …
…focus

Temporarily use our own AlloyEditor fork to get a 0.6 version with the fix for
liferay/alloy-editor#371

dpobel added a commit to ezsystems/PlatformUIAssetsBundle that referenced this issue Dec 7, 2015

Fix EZP-25181: RichText editor toolbar stays visible when loosing focus
Temporarily use our own AlloyEditor fork to get a 0.6 version with the fix for
liferay/alloy-editor#371
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.