Make WordPress Core

Opened 10 years ago

Closed 10 years ago

Last modified 10 years ago

#27279 closed defect (bug) (fixed)

TinyMCE styling regressions

Reported by: hanni's profile Hanni Owned by:
Milestone: 3.9 Priority: normal
Severity: normal Version: 3.9
Component: TinyMCE Keywords: has-patch
Focuses: ui, administration Cc:

Description (last modified by SergeyBiryukov)

There many be some unintentional style regressions as a result of the integration of the TinyMCE update.

Here's the included image in PNG format, as the tiff doesn't appear to display directly in all browsers.

As can be seen by the above direct comparison between trunk ( at 27368 earlier today) and 3.8.1.

  1. The toggle button between the compact and expanded versions of the visual editor toolbar no longer has a distinct hover and active state.
  1. Spacing changes which appear to be not uniquely related to having fewer buttons.

Spotted whilst trying to figure out what might be needed for #27100.

Attachments (26)

tinymce 3.8.1 versus trunk.png (108.7 KB) - added by Hanni 10 years ago.
Here's the included image in PNG format, as the tiff doesn't appear to display directly in all browsers.
27279.patch (1.4 KB) - added by iseulde 10 years ago.
Screen Shot 2014-03-05 at 16.11.25.png (20.0 KB) - added by iseulde 10 years ago.
27279.2.patch (409 bytes) - added by iseulde 10 years ago.
27279.3.patch (1.3 KB) - added by iseulde 10 years ago.
Screen Shot 2014-03-27 at 10.36.54.png (21.1 KB) - added by iseulde 10 years ago.
Screen Shot 2014-03-27 at 10.36.36.png (20.1 KB) - added by iseulde 10 years ago.
Screenshot_2014-03-27-12-30-43.png (28.6 KB) - added by azaozz 10 years ago.
27279.4.patch (3.1 KB) - added by iseulde 10 years ago.
Screen Shot 2014-03-28 at 15.35.55.png (16.9 KB) - added by iseulde 10 years ago.
Screen Shot 2014-03-28 at 15.36.18.png (20.0 KB) - added by iseulde 10 years ago.
Screen Shot 2014-03-28 at 15.37.21.png (15.1 KB) - added by iseulde 10 years ago.
Screen Shot 2014-03-28 at 15.47.45.png (10.5 KB) - added by iseulde 10 years ago.
Old buttons on hover.
Screen Shot 2014-03-29 at 14.10.04.png (25.2 KB) - added by iseulde 10 years ago.
TinyMCE: hover/focus state and active state + fullscreen button.
Screen Shot 2014-03-29 at 14.04.17.png (20.5 KB) - added by iseulde 10 years ago.
TinyMCE: fullscreen button with toolbar collapsed.
Screen Shot 2014-03-29 at 14.04.36.png (21.0 KB) - added by iseulde 10 years ago.
TinyMCE: fullscreen button on a small screen.
Screen Shot 2014-03-29 at 14.03.56.png (29.2 KB) - added by iseulde 10 years ago.
Quicktags: new buttons and fullscreen button.
Screen Shot 2014-03-29 at 14.04.58.png (32.5 KB) - added by iseulde 10 years ago.
Quicktags: small screen.
Screen Shot 2014-03-29 at 14.06.19.png (23.9 KB) - added by iseulde 10 years ago.
DFW: New button group + border.
27279.5.patch (13.2 KB) - added by iseulde 10 years ago.
27279.6.patch (13.2 KB) - added by iseulde 10 years ago.
Style :active state, use class instead of #qt_content_fullscreen.
27279.7.patch (13.2 KB) - added by iseulde 10 years ago.
Get rid of !important, adjust toolbar padding.
27279.8.patch (15.1 KB) - added by iseulde 10 years ago.
Add .wp-core-ui to link modal and DFW. Increase the z-index of DFW.
27279.9.patch (1.8 KB) - added by iseulde 10 years ago.
firefox.png (2.0 KB) - added by azaozz 10 years ago.
chrome.png (1.2 KB) - added by azaozz 10 years ago.

Download all attachments as: .zip

Change History (72)

@Hanni
10 years ago

Here's the included image in PNG format, as the tiff doesn't appear to display directly in all browsers.

@iseulde
10 years ago

#1 follow-up: @iseulde
10 years ago

I've been meaning to write down some similar things.

  • As Hanni noted there is no distinct active and hover state for any of the TinyMCE buttons, nor the Quicktags. On top of that, Quicktags uses the TinyMCE button's hover state as a normal state and has a totally different active and hover state.
  • The kitchensink button is active when you toggle it, but it's not toggled when you refresh the page.
  • Distraction free writing isn't responsive.
  • Hovering over the Quicktags doesn't change your cursor to a pointer.
  • In 3.8 you can't insert a link without selecting something, the buttons are disabled. Now they're enabled, but inserting a link will fail.
  • The font is still Helvetica Neue, not Open Sans.
  • As azaozz suggested, we should probably create a new skin for TinyMCE instead of overwriting everything. I'd be happy to do that.

The patch above makes the buttons bigger on small screen (same size as Quicktags). It also adds that cursor: pointer;.

#2 @SergeyBiryukov
10 years ago

  • Description modified (diff)
  • Milestone changed from Awaiting Review to 3.9

#3 @azaozz
10 years ago

The buttons padding in responsive mode was fixed in another ticket.

#4 in reply to: ↑ 1 ; follow-up: @nacin
10 years ago

Replying to avryl:

  • As Hanni noted there is no distinct active and hover state for any of the TinyMCE buttons, nor the Quicktags. On top of that, Quicktags uses the TinyMCE button's hover state as a normal state and has a totally different active and hover state.

Fixed.

  • The kitchensink button is active when you toggle it, but it's not toggled when you refresh the page.

While it would be nice, it's never been something that saves state. Future release enhancement.

  • Distraction free writing isn't responsive.

Being worked on by azaozz partially in #26907.

  • Hovering over the Quicktags doesn't change your cursor to a pointer.

Can't reproduce. Fixed?

  • In 3.8 you can't insert a link without selecting something, the buttons are disabled. Now they're enabled, but inserting a link will fail.

I remember this in trunk, but it's fixed now.

  • The font is still Helvetica Neue, not Open Sans.

NEEDS-PATCH

  • As azaozz suggested, we should probably create a new skin for TinyMCE instead of overwriting everything. I'd be happy to do that.

Probably a little late for this. Would be a lot of work and would require a lot of testing.

#5 in reply to: ↑ 4 @iseulde
10 years ago

The cursor issue is still there, but easy to fix. I'll submit a patch for the font, we just need to be careful not to overwrite TinyMCE's icon font. And yep, I think we said before that TinyMCE 4 needs to grow a little bit more before creating a skin.

@iseulde
10 years ago

#6 @iseulde
10 years ago

Last patch: hovering over the buttons in the 'Text' editor doesn't change the cursor to pointer.

@iseulde
10 years ago

#7 @iseulde
10 years ago

  • Keywords has-patch added

Last patch changes the font size to Open Sans, except for the the things that use the tinymce icon font. This patch also contains the preview patch solving the cursor issue and also changes the hr icon to a dashicon.

#8 @iseulde
10 years ago

Moved the point about the TinyMCE skin to #27545.

#9 follow-ups: @iseulde
10 years ago

The quicktag buttons are still a lot bigger than the ones in the TinyMCE toolbar. Fix or not?

#10 @iseulde
10 years ago

Compare the screenshot a bit higher up to see how the TinyMCE buttons would look when they have the same height.

#11 @azaozz
10 years ago

In 27791:

Set the default font family to inherit (Open Sans) in all TinyMCE modals, change the cursor to pointer when hovering over Quicktags buttons. Props avryl, see #27279

#12 in reply to: ↑ 9 @samuelsidler
10 years ago

Replying to avryl:

The quicktag buttons are still a lot bigger than the ones in the TinyMCE toolbar. Fix or not?

I say fix.

Outside of that, is there anything left for this ticket?

#13 in reply to: ↑ 9 @azaozz
10 years ago

Replying to avryl:

The quicktag buttons are still a lot bigger than the ones in the TinyMCE toolbar.

The TinyMCE buttons are about the same height as all the rest of the buttons in responsive mode. The Quicktags buttons seem very large covering most of the screen when the keyboard is open (also compared to the keyboard keys).

Last edited 10 years ago by azaozz (previous) (diff)

#14 follow-up: @iseulde
10 years ago

Normal buttons: 39px;
TinyMCE buttons: 32px;
Quicktag buttons: 40px.

@iseulde
10 years ago

#15 @iseulde
10 years ago

Let's use the standard buttons for the quick tags toolbar instead of the old ones. Now the buttons in the toolbars all have the same height as well. I wonder what the hover state of the TinyMCE buttons would look like if they had the hover state of the standard buttons, so I'll try that out soon. Looks like the icon in the 'Add Media' button could use some more padding on the right on small screens.

@iseulde
10 years ago

Old buttons on hover.

#16 @nacin
10 years ago

I was surprised I'd like 27279.4.patch, but I do. They do take up a bit more space, though. I don't think we need to design to 1024px screens a whole lot anymore, but the old ones wrapped at 1052px, while the new ones wrap at 1148px.

(FWIW: When you exclude the fullscreen button, relatively new compared to the others in quicktags, it used to wrap at 986px, under 1024px.)

Maybe a simple responsive style can be used to decrease padding between buttons a bit at narrower widths? If you're deliberately keeping a narrow browser window (split screen etc) you'd rather have a slightly smaller target area than wrapping (non-mobile at least).

@iseulde
10 years ago

TinyMCE: hover/focus state and active state + fullscreen button.

@iseulde
10 years ago

TinyMCE: fullscreen button with toolbar collapsed.

@iseulde
10 years ago

TinyMCE: fullscreen button on a small screen.

@iseulde
10 years ago

Quicktags: new buttons and fullscreen button.

@iseulde
10 years ago

Quicktags: small screen.

@iseulde
10 years ago

DFW: New button group + border.

#17 @iseulde
10 years ago

Patch below:

  • Use standard button styles for the quick tag buttons.
  • Use standard button styles for the TinyMCE buttons (just hover/focus/active/disabled).
  • Add buttons.css as a dependency to the editor.
  • Nacin asked me to try to move the fullscreen button to the right. Works perfectly in the patch. The same button style is used for both toolbars. On smaller screens the button keeps using some space on the right. I don't mind it that way, but maybe at some point it should go back between the other buttons. (?)
  • I've changed the old visual/text buttons to a new button-group. Looks much better. I've also added a border on the bottom, just like the toolbars outside DFW.

Replying to nacin:

While it would be nice, it's never been something that saves state. Future release enhancement.

  • FIXED! Enhanced with just one line of code.

TODO:

  • Remove the space on the right when the fullscreen button is removed form the editor.
  • Since the fullscreen button makes DFW easier to discover, we should really work on the responsiveness of it. I also think I'd be good to replace 'Exit fullscreen' with a dashicon (the fullscreen icon with inverted arrows).

@iseulde
10 years ago

This ticket was mentioned in IRC in #wordpress-dev by avryl_. View the logs.


10 years ago

#19 follow-up: @nacin
10 years ago

This is amazing. I have two small things:

  • So it's squared up in the corner, the right spacing of the DFW button should be 5px, versus 8px. (I recognize the 8px was to replicate the screenshot I had presented using a simple float.)
  • TinyMCE buttons don't have an active state. (Try the horizontal rule button for example.) This is a regression from 3.8. I recognize the markup changes probably make this difficult (:active is on the button while the styling is not).

I'm probably going to commit this with the first bullet point handled, leaving just the second bullet point.

#20 in reply to: ↑ 19 @iseulde
10 years ago

Replying to nacin:

  • So it's squared up in the corner, the right spacing of the DFW button should be 5px, versus 8px. (I recognize the 8px was to replicate the screenshot I had presented using a simple float.)

Okay, but then the padding around the whole toolbar should be 3px instead of 6px right and left. The distance between the edge and the buttons should be the same on both sides.

And yeah, I realised the point about the active state after I uploaded the patch. I'll add it to the next one.

#21 @iseulde
10 years ago

(That becomes 5px and 8px including the margin around the buttons.)

#22 in reply to: ↑ 14 @azaozz
10 years ago

Looking at 27279.5.patch:

  • #qt_content_fullscreen will only work when the editor ID is content. There's nothing stopping us to add classes to the Quicktags buttons, that would also improve the existing styling for b, i, link, etc.
  • Changing the styling for .mce-btn-group would introduce discrepancies when "button groups" are used. TinyMCE has the notion of grouping the buttons and keeping these groups together when the toolbar is wrapped. Groups are separated with | when defining button order. This is not used by default in WordPress, but can be added by a plugin.
  • !important is evil! :) There are plenty of nested elements and selectors that can be used to override some style.

Needs testing with the different TinyMCE configuration options, modals, etc.

Last edited 10 years ago by azaozz (previous) (diff)

#23 @iseulde
10 years ago

Thanks azaozz, I'll add a button group to test with and use a class instead of #qt_content_fullscreen.

@iseulde
10 years ago

Style :active state, use class instead of #qt_content_fullscreen.

@iseulde
10 years ago

Get rid of !important, adjust toolbar padding.

#24 @iseulde
10 years ago

Updated the patch with all the points above. .mce-btn-group wasn't styled before (the buttons just look the same as all the others), and this patch doesn't change anything about that.

Last edited 10 years ago by iseulde (previous) (diff)

@iseulde
10 years ago

Add .wp-core-ui to link modal and DFW. Increase the z-index of DFW.

#25 @iseulde
10 years ago

Oh, the z-index of TinyMCE's fullscreen wrap, not DFW.

#26 @azaozz
10 years ago

In 27857:

Editor:

  • Use standard button styles for the Quicktags buttons.
  • Better style for the TinyMCE buttons (hover/focus/active/disabled).
  • Move the fullscreen (DFW) button to the right.
  • Better style for the Visual/Text buttons in DFW.

Props avryl, see #27279

#27 @azaozz
10 years ago

Thinking it's better to open another ticket regarding .wp-core-ui on the front-end. Just adding the class won't help much and there are some other options.

#28 @azaozz
10 years ago

In 27858:

Add class wp-core-ui to DFW and wpLink as it is required by buttons.css. Props avryl, see #27279

#29 @nacin
10 years ago

The help dialog is hidden by DFW.

#30 @nacin
10 years ago

More info: The help dialog does actually appear above DFW. But the button wasn't working. When I exited fullscreen, the DFW button wouldn't open the dialog, either, and that's what I had reported. Saving the post meant everything worked. Something is clearly causing these buttons to not respond.

Nothing in the console beyond the known TinyMCE 3.x deprecated warnings for the window manager.

#31 @nacin
10 years ago

Even while DFW didn't work, Bold/Italic/etc did. I didn't check to see if Help (outside of DFW) didn't work. I'll try everything if I can reproduce it again.

#32 @nacin
10 years ago

Other DFW issues:

  • "Saved." (and "Save failed.") are about 5 pixels too far to the right and two pixels short on line height (seems like it should be 9px and 28px respectively). This is likely a regression from either 3.8 or 3.7. The spinner is also slightly high (might need more line height). The button's height is 28px, which is where that number comes from.
  • Clicking "Save" again should immediately hide "Saved." (or "Save failed.") when showing the spinner. This is likely not a regression.

#33 @iseulde
10 years ago

Other points:

  • The buttons in DFW don't seem to ever have an active state. To reproduce, just add a link or make something bold.
  • The link button is never disabled.
  • TinyMCE's tooltips are enabled, but there are no tooltips for the quicktag buttons (and no tooltip for the fullscreen button). There are also no tooltips for the buttons in DFW.
  • In DFW the 'Save' button has the text 'Save', while in the Publish box it has the text 'Update'.
  • For new posts there is no publish button, and it is unclear at first whether or not the 'Save' button will save the post as a draft or publish it, since it's a primary button.

I tried the help button several times, and for me it works as expected.

To make DFW more responsive, we could:

  • Give the toolbar a min-width. It's based on the content width, but this can be really small.
  • Use a dashicon for 'Exit fullscreen'. I've asked melchoyce and she asked empireoflight. In the meantime we could maybe use an 'X'.
  • On small screens, we could move the media button to the first row, and maybe hide the help button, since the modal is not responsive anyway and there is no way to close it atm.

#34 @iseulde
10 years ago

In Firefox the fullscreen button moves vertically by 1px when switching tabs. All the margins and paddings are right, it seems to be caused by the position of the dashicon. I also noticed that the dash icons for the TinyMCE buttons are placed a bit higher on the button than in other browsers.

@iseulde
10 years ago

#35 follow-up: @iseulde
10 years ago

.9

  • Removes the padding on the right in both toolbars if the editor doesn't have a DFW mode.
  • Gives the fullscreen button in the quicktags toolbar styling for an active state as well.

#36 in reply to: ↑ 35 @azaozz
10 years ago

Replying to avryl:

Removes the padding on the right in both toolbars if the editor doesn't have a DFW mode.

Adding a class to the wrapper will not work when a plugin adds DFW (wp_fullscreen).

Other inconsistencies after [27858]:

  • The buttons look different/quite worse in Firefox.
  • No hover state for active buttons.
  • Disabled buttons get border on hover (maybe OK).
Last edited 10 years ago by azaozz (previous) (diff)

#37 @iseulde
10 years ago

Adding a class to the wrapper will not work when a plugin adds DFW (wp_fullscreen).

Right, let's check in the button array then.

The buttons look different/quite worse in Firefox.

Could you take a screenshot? :) They look okay to me... They look exactly the same.

No hover state for active buttons.

Is looks like buttons.css doesn't add styling for that. It's rare to have active buttons that you can hover on outside TinyMCE, but it'd be good to add some styling for it there. I'm not sure who designed the buttons, but they should probably be asked for their opinion.

Disabled buttons get border on hover (maybe OK).

That's by design. I gave all states button styles except the default state. I think it looks fine like this.

This ticket was mentioned in IRC in #wordpress-dev by azaozz. View the logs.


10 years ago

@azaozz
10 years ago

@azaozz
10 years ago

#39 @azaozz
10 years ago

Firefox (on Win7) seems to have some problems with the current styles. Also perhaps better to have some subtle :hover for non-disabled buttons. It used to make the icons darker.

Another problem with having (heavier) box-shadow at the top is that it makes the icons look a bit off center. That style is used for the other buttons only on :active, but in TinyMCE there is an "activated" state (depending on the currently selected node in the editor). Perhaps we need different styling for that.

Last edited 10 years ago by azaozz (previous) (diff)

#40 @azaozz
10 years ago

In 27941:

TinyMCE: better calculation for editor height when switching Text to Visual and back. Add stopping of editor resizing when the mouse leaves the browser window. See #27279

#41 @nacin
10 years ago

Use a dashicon for 'Exit fullscreen'. I've asked melchoyce and she asked empireoflight. In the meantime we could maybe use an 'X'.

I'm not sure we should change that for now. Icons are not everything and I'd rather not trap someone in this mode. It's been like this since 3.2.

#42 @azaozz
10 years ago

In 27978:

TinyMCE:

  • Tighten up button styles, add :hover for .mce-active buttons.
  • Pad only the first toolbar row and remove the padding in both toolbars if the editor doesn't have a DFW mode.

Part props avryl, see #27279

#43 @azaozz
10 years ago

In 27983:

TinyMCE: couple more small css tweaks for the menubar and tabs in native modals, see #27279

This ticket was mentioned in IRC in #wordpress-dev by nacin. View the logs.


10 years ago

#45 follow-up: @nacin
10 years ago

  • Resolution set to fixed
  • Status changed from new to closed

I'm closing this one out. Great job, all. New tickets for new issues.

I imagine there are a number of outstanding DFW issues still here, as enumerated by avryl, me, and others. avryl, could you compile all of them into a single list and put them in a new ticket?

#46 in reply to: ↑ 45 @nacin
10 years ago

Replying to nacin:

I'm closing this one out. Great job, all. New tickets for new issues.

I imagine there are a number of outstanding DFW issues still here, as enumerated by avryl, me, and others. avryl, could you compile all of them into a single list and put them in a new ticket?

Actually, I've opened #27709 — though we'll still need to collect the list of issues.

Note: See TracTickets for help on using tickets.