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

Editor: Fix tinymce float toolbar not hide (fixes #1486) #2084

Merged
merged 1 commit into from
Jan 15, 2016

Conversation

ralder
Copy link
Contributor

@ralder ralder commented Jan 4, 2016

Reason of this issue was that on blur event activeToolbar was hidden but after if window is got scroll event it was shown again.

Changed 'resize' to 'resizewindow' because it's right for window target (tested in ie11 on win10; ff, chrome, safari on mac os)

PS:

@ralder ralder changed the title fix tinymce float toolbar not hide (fixes #1486) Editor: Fix tinymce float toolbar not hide (fixes #1486) Jan 4, 2016
@lancewillett lancewillett added OSS Citizen [Feature] Post/Page Editor The editor for editing posts and pages. [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. labels Jan 4, 2016
@aduth
Copy link
Contributor

aduth commented Jan 5, 2016

Hi @ralder, thanks for the pull request!

As a bit of background, the wpcom TinyMCE plugin is a forked version of the core WordPress wordpress TinyMCE plugin, found here. We try to keep these files in sync as changes are made, so it can help to check for more recent changes to the WordPress file, or vice versa, if we make changes, it's probably a good idea to submit a patch to the core WordPress Trac.

To the changes here, I think adding the condition for blur makes sense, as below there is an event binding for blur which is never otherwise accounted for in the hide function.

However, I'm not so certain about the resizewindow event. I can't find any reference in the spec about a resizewindow event, and based on the event bindings for calling hide, it wouldn't appear to reach the function anyways. Where did you find information about resizewindow?

@aduth aduth added [Status] Needs Author Reply and removed [Status] Needs Review The PR is ready for review. This also triggers e2e canary tests and wp-desktop tests automatically. labels Jan 5, 2016
@ralder
Copy link
Contributor Author

ralder commented Jan 5, 2016

resizewindow isn't event. It's value of event.type for resize event when it binding to window. I also not found any specification for this. But browsers do this (I tested in ie11 on win and chrome, ff, safari on macos)

@nylen
Copy link
Contributor

nylen commented Jan 6, 2016

I'm seeing event.type === 'resize' on my browser as well. As expected, given the spec and the list of available event names.

I'm also seeing that your other change does not fix #1486 - I'm able to reproduce it intermittently in both master and this branch.

In master, we still call activeToolbar.hide() to hide the toolbar when a blur event occurs. What you changed was to prevent the toolbar from being shown again after the editor is blurred and then refocused. I think the existing behavior (hide the toolbar, but allow it to be shown again when the editor is refocused) is correct.

Related to #1486 - the editor doesn't always get that blur event when I click the notifications icon, so none of this code is ever called and the toolbar remains active. I'm not sure why this is happening.

@ralder
Copy link
Contributor Author

ralder commented Jan 6, 2016

I'm sorry 😳. @nylen is right, browsers send event.type = 'resize' by spec. But here tinymce rewrites it to 'resizewindow'

I think the existing behavior (hide the toolbar, but allow it to be shown again when the editor is refocused) is correct.

In my test in browser I cant do this when I refocused editor by click him. If I clicked by previous element with hidden toolbar(toolbar is shown again) if I clicked by element without toolbar (previous toolbar is shown and then hides straight away), i.e. i cant found element in editor for click for just got focus for showing hidden toolbar. And when I changed in blur handler activeToolbar = false I got same behavior.
If i'm wrong here and there is approach to do this, then we need here additional variable 'isFocused' for editor, because in hide function hidden toolbar will be shown again for events 'scroll' and 'resize' even editor hasn't focus.

If you not get blur event for click notification icon, say whats environment you have. I always got it except touch devices

@nylen
Copy link
Contributor

nylen commented Jan 15, 2016

Sorry for the delay @ralder, I've been meaning to come back to this but got pulled onto another project and haven't had time.

I didn't look at the event type in the context of that code - you're correct that it is resizewindow here due to TinyMCE (fired as ResizeWindow then lower-cased).

I'm still seeing that this PR does not fix #1486 because the blur event is not always fired. I'm seeing this in OS X and latest Chrome, but the problem doesn't appear consistently.

Still, these are good changes. They make more sense to me now, thanks for explaining. There are 2 different issues that this PR fixes.

Inline toolbar reappears after blur then scroll

  • Click an image or do something else to show an inline toolbar.
  • Click outside the editor to fire a blur event; the inline toolbar should disappear.
  • Scroll the window; the inline toolbar will reappear because it wasn't disabled on blur.

Fixed by clearing the active toolbar on blur.

Inline toolbar does not reappear after resizing window

  • Click an image or do something else to show an inline toolbar.
  • Resize the window; the inline toolbar should disappear briefly then reappear, but it doesn't. (Then, if you scroll the window, the inline toolbar reappears again.)

Fixed by testing for event.type === 'resizewindow' (non-standard, but set by TinyMCE as described above).

nylen added a commit that referenced this pull request Jan 15, 2016
Editor: Fix tinymce float toolbar not hide (fixes #1486)
@nylen nylen merged commit 5372759 into Automattic:master Jan 15, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[Feature] Post/Page Editor The editor for editing posts and pages. OSS Citizen
Projects
None yet
5 participants