Make WordPress Core

Opened 5 years ago

Closed 4 years ago

Last modified 4 years ago

#49288 closed defect (bug) (fixed)

Metabox holders missing their border and "Drag boxes here" text

Reported by: xkon's profile xkon Owned by: audrasjb's profile audrasjb
Milestone: 5.5 Priority: highest omg bbq
Severity: blocker Version:
Component: Administration Keywords: has-screenshots has-patch commit dev-reviewed fixed-major
Focuses: ui, accessibility, administration Cc:

Description

Splitting this up from #39074. While editing Posts/Pages the advanced-sortables was missing a visible border and it was of 0px height so kind of hard to even drag a postbox there even if you knew it existed.

This was unfortunately extended into all metabox holders within the Edit so even if the sidebar was emptied there was no indication then that a draggable placeholder exists there.

I've managed to track down changes regarding this back to ticket #26399 & changeset 36295.

After some investigation, I also realized that the Drag boxes here text was also hidden from the Dashboard and all other areas for me. This was due to the @media rules pointing to specific ranges of a max 1800px.

Attachments (34)

49288.diff (2.3 KB) - added by xkon 5 years ago.
edit_before.jpg (120.3 KB) - added by xkon 5 years ago.
edit_after.jpg (129.5 KB) - added by xkon 5 years ago.
dashboard_before_over_1800wide.jpg (100.1 KB) - added by xkon 5 years ago.
dashboard_after_over_1800wide.jpg (102.4 KB) - added by xkon 5 years ago.
49288.2.diff (3.0 KB) - added by xkon 5 years ago.
49288.2_preview.gif (3.1 MB) - added by xkon 5 years ago.
49288 current patch.png (117.1 KB) - added by afercia 5 years ago.
49288 empty side sortable.png (79.2 KB) - added by afercia 5 years ago.
49288 dragging into side sortable.png (27.8 KB) - added by afercia 5 years ago.
49288.3.diff (7.3 KB) - added by afercia 5 years ago.
dashboard.gif (269.8 KB) - added by afercia 5 years ago.
post.gif (524.1 KB) - added by afercia 5 years ago.
49288 placeholder height.png (38.8 KB) - added by afercia 5 years ago.
Sortable placeholder height issue in Chrome when zooming.
dashboard-1200.gif (3.3 MB) - added by audrasjb 5 years ago.
Dashboard looks good to me (viewport width 1200px)
Capture d’écran 2020-06-06 à 05.57.48.png (87.0 KB) - added by audrasjb 5 years ago.
Edit post viewport width 1200px - no drag boxes here message
d08afd063388dfe33a59e49999b11474.mp4 (2.9 MB) - added by audrasjb 5 years ago.
Attachement screen - works fine
49288 advanced sortables wp 3.2.gif (274.6 KB) - added by afercia 4 years ago.
WordPress 3.2: dragging a box to the "advanced" area
49288.4.diff (9.5 KB) - added by afercia 4 years ago.
49288 edge case.png (144.2 KB) - added by afercia 4 years ago.
49288.5.diff (11.4 KB) - added by afercia 4 years ago.
49288 toggling dashboard widgets.gif (791.7 KB) - added by afercia 4 years ago.
New behavior on the Dashboard.
49288.6.diff (11.4 KB) - added by afercia 4 years ago.
49288.7.diff (11.3 KB) - added by afercia 4 years ago.
empty-containers.png (53.2 KB) - added by azaozz 4 years ago.
The outline and "Drag here..." text probably shouldn't be visible when there's nothing to add there.
empty-containers-without-patch.png (118.2 KB) - added by azaozz 4 years ago.
Same as above without 49288.7.diff. There's no empty-container class on the drop area.
dragging postboxes.2.gif (219.9 KB) - added by afercia 4 years ago.
49288.8.diff (11.4 KB) - added by afercia 4 years ago.
49288 dashboard 4 sortbles.png (151.7 KB) - added by afercia 4 years ago.
Emulating a large screen with 4 sortable areas available (but no visual hint) and 1 visible box.
49288 logged in as contributor.png (195.3 KB) - added by afercia 4 years ago.
Screenshot from trunk before the patch: Dashboard: four available "boxes" for the Contributor role.
49288 wp 5.4 subscriper.png (386.1 KB) - added by afercia 4 years ago.
WordPress 5.4: subscriber role: 2 widgets and 4 areas (all visible)
tablepress-empty-container.png (139.9 KB) - added by TobiasBg 4 years ago.
49288.9.diff (4.6 KB) - added by afercia 4 years ago.
49288.10.diff (5.3 KB) - added by afercia 4 years ago.

Change History (121)

@xkon
5 years ago

#1 @xkon
5 years ago

  • Keywords has-patch needs-testing added

49288.diff adjusts the CSS rules and alters the postbox.js to be able to read all metabox holders to add the empty class.

Instead of the default 250px height (as seen on Dashboard) the Edit empty holders will have a 50px height to avoid a big blank space at the bottom of the page (i.e. for advanced-sortables). It also enables the Drag boxes here message for resolutions above 1800px.

@xkon
5 years ago

@xkon
5 years ago

#2 @xkon
5 years ago

  • Keywords has-screenshots added

This ticket was mentioned in Slack in #accessibility by xkon. View the logs.


5 years ago

#4 @xkon
5 years ago

  • Summary changed from Adjust the CSS/JS of the empty metabox holders to Metabox holders missing their border and "Drag boxes here" text

This ticket was mentioned in Slack in #accessibility by xkon. View the logs.


5 years ago

#6 @audrasjb
5 years ago

  • Keywords needs-design-feedback added
  • Milestone changed from Awaiting Review to 5.4
  • Owner set to audrasjb
  • Status changed from new to reviewing

#7 @afercia
5 years ago

@xkon thanks for the patch!

Re: "Drag boxes here": since we're here, it would be nice to make the text have a sufficient color contrast. The current grey #ccc has a very low contrast of 1.42:1 see https://jdlsn.com/color/?type=hex&color=cccccc&color2=f1f1f1. We could use #606a73 which is already used in the Dashboard for some elements.

Re: the "advanced" area: Seems to me the "regression" is a bit more... ancient 🙂. WordPress 3.0 used to have these styles to set a min-height on the sortable areas:

.inner-sidebar #side-sortables {
	width: 280px;
	min-height: 300px;
}

#post-body #normal-sortables {
	min-height: 50px;
}

#post-body #advanced-sortables {
	min-height: 20px;
}

So in WordPress 3.0 it was actually possible to move a meta box to the "advanced" area. The UI wasn't that great: there was no visible indication of the advanced area but it worked.

The "normal" and "side" min-heights are still used today. Instead, the "advanced" min-height was removed in [18975] see the related ticket #18314. This change seems completely unrelated to the original ticket purpose and probably not intended. Since then, it is not possible to move meta boxes to the "advanced" area. /Cc @azaozz may know more.

Worth also considering jQuery UI Sortable has a start event. By using it, it would be possible to add a CSS class to the body e.g. is-dragging-metaboxes and style anything in the page. This could help to highlight the available areas in some way. Then, remove the CSS class on the stop event which is the already used in postbox.js.

I do realize the legacy Edit Post page has a low priority now that there's Gutenberg but this is a small, long-standing, regression that can be quickly fixed. The dashboard issue instead needs to be fixed anyways :)

@xkon
5 years ago

@xkon
5 years ago

#8 @xkon
5 years ago

Thanks for going even further back and pinpointing a commit @afercia!

I was aware of the start() stop() but I didn't have an idea on how to use them. The color correction to #606a73 gave me the idea though that instead of having them hidden/visible on drag we could also make them even more visible and a11y friendly since the border is still at #b4b9be which isn't a good difference with the background also.

The changes in the 49288.2.diff are re-introducing the older min-height:20px on advanced-sortables as well as adjusting the Drag boxes here to permanently use #606a73.

Also adds/removes the is-dragging-metaboxes class on drag/drop which also adjusts the border color to #606a73 as well. This way we tackle both a visual hint and also making it compatible to standards.

Side note: I did also play around with fully hiding the sortables while empty and show them on drag but this was resulting either on "flickering" of the screen if the height was also changed ( i.e. with a display:none or height: 20px) or you just ended up having a huge gap at the end of the page that didn't look nice :-). That's why I ended up just bumping the border color to have the best of both.

Preview 49288.2_preview.gif

#9 @karmatosed
5 years ago

  • Keywords needs-design-feedback removed

From a design perspective, this doesn't seem to change the interface much, unless I am mistaken. As result removing the keyword as looks like it is an improvement fix.

This ticket was mentioned in Slack in #accessibility by afercia. View the logs.


5 years ago

#11 @afercia
5 years ago

This ticket was discussed during today's accessibility bug-scrub: the proposed patch appears to just need a final review.

This ticket was mentioned in Slack in #accessibility by afercia. View the logs.


5 years ago

#13 @afercia
5 years ago

  • Milestone changed from 5.4 to 5.5

This ticket was discussed during today's accessibility bug-scrub: With 5.4 Beta 3 approaching, agreed to move this ticket to the next release cycle.

This ticket was mentioned in Slack in #accessibility by afercia. View the logs.


5 years ago

#15 @afercia
5 years ago

  • Keywords needs-refresh added; needs-testing removed

Testing 49288.2.diff, the "drop area" below the post content is a bit too big, see attached screenshot. There are some subtleties in the way the edit post columns work and the way the columns behavior changed over time that make touching this code not that easy. Given that the classic edit post screen is legacy, I'd tend to think we should focus on a solution that is as simple as possible. On it.

#16 @afercia
5 years ago

RIght now, then the right column (#side-sortables) is empty (or contains only hidden meta boxes) it gets a tick dashed border. See screenshot attached below.

Version 0, edited 5 years ago by afercia (next)

#17 @afercia
5 years ago

When dragging a meta box inside the right column, the meta box placeholder has a thinner border. This way, the two different border styles help distinguishing the drop area from the placeholder. See screenshot attached below.

I'd tend to think the best option would be using the same pattern for the two drop areas below the post (#normal-sortables and #advanced-sortables) with a way smaller height and only while dragging.

Lastly, ensure all this doesn't break when the layout is set to 1-column under "Screen options".

Last edited 5 years ago by afercia (previous) (diff)

@afercia
5 years ago

#18 @afercia
5 years ago

  • Keywords needs-refresh removed

49288.3.diff builds on the previous patch and seeks to introduce a few improvements.

To recap, the main purpose of this ticket is:

  • on the Dashboard: display the "Drag boxes here" text also on large screens
  • on the edit post pages: give the ability to move the post boxes to all the available areas, including the "advanced-sortables" one which isn't possible since [18975]

Some testing would be nice. Please test at various resolutions and with the 2 column or 1 column layout, where applicable:

  • dashboard
  • edit post/page pages
  • edit attachment page

Details, TL;DR

  • adds a is-dragging-metaboxes class to the body when dragging
  • simplifies the JS and adds the is-dragging-metaboxes CSS class to all the sortables in the Dashboard and in the various edit posts pages
  • displays the CSS generated content "Drag boxes here" only on the Dashboard
  • since this text is meant to appear only on the Dashboard, moves the related CSS to dashboard.css
  • makes sure the text is displayed also on large screens with a viewport larger than 1800 pixels
  • better scopes the CSS differentiating between the one for #dashboard-widgets and the one for #post-body
  • removes the CSS rulesets related to .inner-sidebar and .has-right-sidebar as they're no longer used since WordPress 3.4, see [20272] / #20015

Visual changes for the Dashboard:

  • when dragging, the "empty" drop areas get a darker border
  • note: the empty areas are the ones that are really empty or that contain only hidden boxes

Visual changes for the edit post pages:

  • when dragging, adds a visual indication of all the drop areas, making it possible to move post boxes also to the "advanced-sortables" area
  • to respect the original implementation intent, there's no change in the 1 column layout

Some relevant history related to columns and sortables:
[18975] / #18314 / [20272] / #20015

#19 @afercia
5 years ago

See below an animated GIF to illustrate the behavior on the Dashboard.

Question: would it be preferable to always show the drop areas borders while dragging? The fact that borders are only shown for empty containers is a bit odd to me.

@afercia
5 years ago

#20 @afercia
5 years ago

See below an animated GIF to illustrate the behavior on the Edit post screen.

@afercia
5 years ago

#21 @afercia
5 years ago

Aside: while working on this I noticed a pre-existing issue.

With Chrome, when zooming out or in, at some zoom levels the "sortable placeholder" (the rectangle with a thin dashed border) has no height. It just shows like a doubled dashed line. See screenshot below.

When the placeholder height isn't explicitly set via CSS, jQuery UI sortable is supposed to compute a height based on the dragged element. See https://github.com/jquery/jquery-ui/blob/1.11.4/ui/sortable.js#L816-L817
This is a good thing because the placeholder will adapt to the post box collapsed / expanded height.

However, seems this computation fails because Chrome returns a height value which is 0 with some decimals e.g. 0.02083400000000002 thus when the Sortable code checks for ! element.height(), it fails. I guess jQuery UI sortable should prevent browser roundings by using parseInt() on all those values. Thinking this should be reported upstream. Although WordPress is using a pretty old version of jQuery / jQuery UI, this issue doesn't seem fixed on latest master as well.

@afercia
5 years ago

Sortable placeholder height issue in Chrome when zooming.

This ticket was mentioned in Slack in #accessibility by afercia. View the logs.


5 years ago

@audrasjb
5 years ago

Dashboard looks good to me (viewport width 1200px)

@audrasjb
5 years ago

Edit post viewport width 1200px - no drag boxes here message

#23 @audrasjb
5 years ago

@afercia

Question: would it be preferable to always show the drop areas borders while dragging? The fact that borders are only shown for empty containers is a bit odd to me.

Yes, it looks odd to me as well. I'm in favor of always showing the drop area borders when dragging.

Otherwise, the patch works fine on my side, great work :)

@audrasjb
5 years ago

Attachement screen - works fine

#24 @afercia
4 years ago

Thx for testing @audrasjb, will refresh the patch to always show the borders while dragging.

Attaching one more animated GIF to illustrate how in very old versions of WordPress (3.2) it was possible to drag the boxes to the "advanced" area. Note that the placeholder area was grey on a white background. The border was the placeholder border, not a border set on the sortables area.

@afercia
4 years ago

WordPress 3.2: dragging a box to the "advanced" area

@afercia
4 years ago

#25 @afercia
4 years ago

49288.4.diff:

  • highlights all the drop areas while dragging
  • switches from border to outline, as it just works better with outline

Some testing would be nice, especially for edge cases.

For example, in the dashboard it is possible to move a box to the 3rd or 4th area when the page is zoomed out (or it's displayed on a large screen). Then when zooming to normal level or switching to another device with a different screen size, the layout may change and show "empty" drop areas between the other ones. See attached screenshot for an example. This is pre-existing behavior, there's just the need to check these edge cases still allow reordering the boxes.

#26 @afercia
4 years ago

Related: #47541. The "Drag boxes here" text isn't that appropriate in the (edge) case where there are no visible boxes in the Dashboard.

This ticket was mentioned in Slack in #accessibility by audrasjb. View the logs.


4 years ago

@afercia
4 years ago

#28 follow-ups: @afercia
4 years ago

  • Keywords commit added

49288.5.diff refreshes the patch and includes a fix for #47541.

Some testing and review would be nice :)

  • please double check the removed check for visible == 1 as I'm not sure I understand why it was there in the first place [edit] whether it's still used (it was introduced in [20272])
  • removes the postBoxL10n and uses more wp-i18n
  • provides an alternative text to Drag boxes here when all sortable areas are empty, fixing #47541

See attached animated GIF that illustrates the new behavior on the Dashboard. Notice the text changes when all sortable areas are empty.

Last edited 4 years ago by afercia (previous) (diff)

@afercia
4 years ago

New behavior on the Dashboard.

#29 in reply to: ↑ 28 @ocean90
4 years ago

Replying to afercia:

  • removes the postBoxL10n and uses more wp-i18n

Thanks for removing the variable. But please use $scripts->set_translations() for the script handle to properly register the translations instead of adding wp-i18n as a dependency.

#30 @afercia
4 years ago

  • Keywords commit removed

Thanks @ocean90 will have a look at it. Hm, I think I saw more cases where this should be done as well? Not 100% sure, but I've seen patches that just added wp-i18n.

@afercia
4 years ago

#31 @afercia
4 years ago

  • Keywords commit added

49288.6.diff adds set_translations()

@afercia
4 years ago

#32 @afercia
4 years ago

49288.7.diff omits to add the wp-i18n dependency to the postbox script, as it already uses set_translations().

#33 in reply to: ↑ 28 ; follow-up: @afercia
4 years ago

To me, the patch looks good to go for Beta. I'd greatly appreciate some input on the following point, please. /Cc @azaozz

  • please double check the removed check for visible == 1 as I'm not sure I understand why it was there in the first place [edit] whether it's still used (it was introduced in [20272])

#34 in reply to: ↑ 33 @azaozz
4 years ago

  • Keywords commit removed

Replying to afercia:

  • please double check the removed check for visible == 1 as I'm not sure I understand why it was there in the first place [edit] whether it's still used (it was introduced in [20272])

Hmm, can't remember (it was 8 years ago!). Seems it had something to do with hiding the outlines for empty drop areas on the Dashboard when there is only one "box" available for dragging. Then it doesn't make sense to show other areas as there's nothing to move there.

49288.7.diff seems to work well on the Dashboard, but doesn't work on the old Edit Post screen (need Classic Editor to test). When there are no boxes under the editor, or when all of them are (visibly) hidden, you cannot drag a box from the side to under the editor.

Seem caused by

#normal-sortables.empty-container, #post-body #advanced-sortables.empty-container {
    height: auto;
    ...

For Sortables the drop area has to have width and height before you start to drag, or there's nowhere to drop. Removing the above CSS seem to fix it.

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

@azaozz
4 years ago

The outline and "Drag here..." text probably shouldn't be visible when there's nothing to add there.

@azaozz
4 years ago

Same as above without 49288.7.diff. There's no empty-container class on the drop area.

#35 @afercia
4 years ago

Thanks @azaozz ! Will try to check all your points:

First:

doesn't work on the old Edit Post screen (need Classic Editor to test). When there are no boxes under the editor, or when all of them are (visibly) hidden, you cannot drag a box from the side to under the editor.

Seems to work for me. See attached animated GIF. I do know the sortables drop areas need a height to work correctly. That is why a min-height is added while dragging. However, I do see that when "hovering" a drop area, the "placeholder" appears only when you move a bit around the drop area. This is clearly visible also in the GIF. The placeholder should appear immediately and probably a height needs to be set earlier.

On the other hand, there's the need to reset the height inherited from common.css otherwise there will be a huge blank space below the editor.

I experimented a bit and looks like the sortables need any initial height to work better: even a 1 pixel initial height seems to improve things. Then, when dragging, min-height: 60px; makes the drop areas height bigger. Of course, we can adjust these numbers if necessary. Patch incoming.

@afercia
4 years ago

#36 @afercia
4 years ago

49288.8.diff adds a 1 pixel initial height to the sortables drop areas in the Edit Post screen. In my testing, this makes things work better, as the placeholder appears immediately when dragging over.

#37 @afercia
4 years ago

Second point:

The outline and "Drag here..." text probably shouldn't be visible when there's nothing to add there.

I think this is related to the visible == 1 bit :) Glad to know we traced it back to its original purpose. I have no strong opinion on this. We can restore it and not show the dashed outline + text when there's only 1 visible box.

On the other hand, with so many large screens in use today we should take into consideration many desktop users will likely see 4 sortables areas on the Dashboard.

Theoretically, users could end up in a situation like the one in the screenshot attached below. I'd tend to think in that scenario, it would be best to show the outline + text. But again I have no strong opinion, I'd defer the decision to you.

@afercia
4 years ago

Emulating a large screen with 4 sortable areas available (but no visual hint) and 1 visible box.

#38 @joedolson
4 years ago

I think that the outlines should be visible even when there's only one movable box visible. It would help make it more clear that there's a reason for a box to floating in the middle of neutral territory, and also help people to understand what spaces are available.

Also, as long as it is possible to drag a box into another position, it should be possible to identify the locations of those position.

#39 follow-up: @afercia
4 years ago

  • Keywords commit added

@joedolson thanks. @azaozz 49288.8.diff looks good to me. I'd like to commit, pending your final review and final input on the visibility thing. When you have a chance :)

This ticket was mentioned in Slack in #accessibility by audrasjb. View the logs.


4 years ago

#41 in reply to: ↑ 39 ; follow-up: @azaozz
4 years ago

Replying to afercia:

Right, 49288.8.diff fixes the problems on the old Edit Post screen. Also fixes start-location placeholders when starting to drag. Looks good here.

I think that the outlines should be visible even when there's only one movable box visible.

@joedolson Thinking this will be a "UX regression". Moving boxes is not the main purpose on the Dashboard, it is a settings option. Perhaps all placeholders can be visible when opening the Screen Options, even when nothing to drag there, but having drop areas that are always empty and cannot be used and cannot be hidden is not a good UI :)

For example "contributor" level users can only see one box on the Dashboard. Why would they need to be constantly asked to drag it to another place? Ideally the empty drop areas will match the number of available/visible boxes (had a plan to add this long time ago, never got to it...).

+1 to commit now, lets look into refining it during beta.

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

#42 @afercia
4 years ago

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

In 48340:

Accessibility: Administration: Improve the sortable postboxes areas on the Dashboard and Classic Editor pages.

  • makes the postboxes areas in the Dashboard visible also on large screens
  • uses a more meaningful text when all postboxes areas are empty instead of "Drag boxes here"
  • restores the ability to drag boxes to the "advanced" area in the Classic Editor page
  • makes the postboxes areas in the Classic Editor page visible while dragging so that users have a clue what the available areas are
  • improves the color contrast of the postboxes areas while dragging
  • uses wp.i18n for translatable strings in wp-admin/js/postbox.js

Props xkon, karmatosed, audrasjb, ocean90, joedolson, afercia, azaozz.
See #20491.
Fixes #49288, #47541.

#43 in reply to: ↑ 41 @afercia
4 years ago

Thanks @azaozz !

Replying to azaozz:

For example "contributor" level users can only see one box on the Dashboard. Why would they need to be constantly asked to drag it to another place? Ideally the empty drop areas will match the number of available/visible boxes (had a plan to add this long time ago, never got to it...).

Yep, that would be something interesting to explore in future iterations.

Worth noting Editors and Contributors can see 4 post boxes on the Dashboard, see attached screenshot. I think you meant the Subscriber role which traditionally had only one visible postbox. In the last releases though, subscribers can see also "WordPress Events and News".

@afercia
4 years ago

Screenshot from trunk before the patch: Dashboard: four available "boxes" for the Contributor role.

#44 follow-up: @azaozz
4 years ago

Uh, right, subscriber level users :)

Yeah, the problem is that before this patch "metabox holders" were not visible when there was only one visible box on the Dashboard. Now there will be up to three, telling the user to drag the box when they clearly don't need to do that.

Still thinking this is a bad UI/UX that was better before the patch, and possibly there will be some unhappy/confused users.

#45 @joedolson
4 years ago

Subscriber level users can see *two* boxes, I believe; Events & News and Activity.

What I think is that any droppable area should be exposed - if there is an area available, it should be visible. But I'd be totally in favor of removing droppable areas if there aren't enough boxes available to fill them.

#46 in reply to: ↑ 44 ; follow-up: @afercia
4 years ago

Replying to azaozz:

Still thinking this is a bad UI/UX that was better before the patch, and possibly there will be some unhappy/confused users.

I do agree the behavior on the Dashboard can be improved :) I'm not sure I understand in which way this change is a "regression" as you said above. Currently, on stable 5.4, subscribers can see up to two dashboard widgets and four dashed areas. Of course, this depends on the viewport width that on 5.4 needs to be not greater than 1800 pixels to see all the 4 areas. The dashed area "disappeared" above 1800 pixels, which is a bug fixed by this change.

See screenshot.

@afercia
4 years ago

WordPress 5.4: subscriber role: 2 widgets and 4 areas (all visible)

#47 in reply to: ↑ 46 @azaozz
4 years ago

Replying to afercia:

Replying to azaozz:

Still thinking this is a bad UI/UX that was better before the patch, and possibly there will be some unhappy/confused users.

I do agree the behavior on the Dashboard can be improved :) I'm not sure I understand in which way this change is a "regression" as you said above.

It's not so much of a "code" regression, but when there is only one box visible, now there will be up to three "drop areas" asking the users to "Drag boxes here". See

https://core.trac.wordpress.org/raw-attachment/ticket/49288/empty-containers-without-patch.png

The users can select to have only one box visible on the Dashboard, also plugins can disable some of the default boxes. That's why I'm expecting there may be some confusion :)

In any case, can look into polishing this during beta.

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

#48 @afercia
4 years ago

Yeah, that's the visible == 1 bit :) However, even if the dashed areas were not visible on 5.4 with only one enabled widget, the areas were still fully functional and users were able to drag boxes there. A better way to improve this would be not playing with the visibility of the "dashed outlines" but actually disabling the areas. Which is a bit more complicated :)

#49 @azaozz
4 years ago

Hmm, think Sortables has a reject/return callback on drop, can probably look into using that based on the number of visible boxes. Or can just reinitialize Sortables when boxes are shown or hidden from Screen Options.

#50 @azaozz
4 years ago

Opened #50699 as a follow-up.

Looking at arranging/dragging of metaboxes, it is part of "Screen Options" and doesn't make sense to do when not setting the screen options. As that would be an enhancement, not just a bugfix for the visibility of the drop area, lets look at it for 5.6.

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

#51 @TobiasBg
4 years ago

I'm not too fond of the now always visible drop zones here. I just tested my plugin TablePress on 5.5-beta2 and now see the empty containers on most screens as well, see the screenshot.

I haven't done deeper testing, but assume that it's related to that visible == 1 check.

I could probably fix this by introducing plugin CSS to make that box hidden again, but I'd rather not to.

#52 follow-up: @afercia
4 years ago

@TobiasBg thanks for your report!

I looked a bit into your plugin. I see the order of the postbox-container-1/2 is different from the one in core and you're using default and also custom context for do_meta_boxes(). I guess that's fine, not arguing :)

I think the root problem is that your plugin uses the post-body ID for the meta box holders. Historically, the ID post-body is used in core to identify the edit pages of posts, comments, menu items. It's heavily used to target the core functionality.

Your collaboration would be greatly appreciated to make the core improvements play nicely with your plugin. My suggestion is to try to not use the post-body ID and adjust accordingly where necessary. Hopefully that should prevent visual changes in your plugin.

I'd also like to point you at the changes from the ticket #39074 and the related changesets [48373], [48460], [48465]. The ability for users to reorder meta boxes is now much improved. Now, wherever there are "sortables" areas defined, users will be able to reorder meta boxes also by teh means of dedicated buttons.

#53 @TobiasBg
4 years ago

Hi @afercia!

Thanks for taking a look so quickly!

This part of the code is some of the oldest in TablePress, so unfortunately my memory about design decisions is not the best here :-(

That postbox-container-1/2 thing is one of them. The raw framework that I used came from edit-form-advanced.php (I think) and that change (according to the commit message) is related to #21042. In some tests, the order didn't seem to matter anymore (as positioning is solely handled by CSS and no longer via HTML code order, which might have played a role back then. It does however not seem to have an influence on the now visible dropzones.

I also tested your suggestion of removing the post-body ID. This did indeed fix the issue of the now visible dropzones. Unfortunately, it creates a new problem:
On the "About" screen of TablePress (screenshot), there are normally two columns of postboxes. After removing the post-body ID, everything is in only column, presumably because CSS from common.css (lines 1960-1968) no longer applies. So, post-body seems to be necessary to get a two-column layout.

The newly introduced arrow buttons seem to work nicely! This really is a great improvement of drag&drop, thanks!

#54 @afercia
4 years ago

Hi @TobiasBg yep I see the problem on your plugin "About" page.

I'm afraid you will have to add some lines of CSS then :) Please consider that the core CSS is meant to be used for the pages with post-body, that is: edit post, edit CPTs, comments, etc. It is not meant to be reused on other pages, nor this layout is part of any official "API" that is guaranteed to work forever. In other words, while plugins authors can absolutely reuse this HTML and CSS, they do it at their own risk as both HTML and CSS may change at any time.

Let me briefly explain how this new bit of CSS works and I'm confident it can work for your usec case.

The core CSS uses this to make the sortable areas not visible:

/* Resets height and outline inherited from common.css. */
#post-body.columns-1 #side-sortables.empty-container,
#post-body #normal-sortables.empty-container,
#post-body #additional-sortables.empty-container {
	/* Sortables need some initial height to work correctly. */
	height: 1px;
	outline: none;
}
  • it sets a 1px min-height that is necessary to make jQuery sortable work
  • it resets the outline so the dotted line around the sortables areas isn't visible

I think using the same CSS on your plugin #additional-sortables area should solve the first step of the issue.

Then, when the meta boxes are being dragged, the core CSS makes the sortables area visible so that users can actually see which "drop areas" are available. This part was buggh in the previous WordPress versions and it's one of the issues this change aims to solve.

To reveal these areas while dragging, core uses the following CSS:

/* Only highlight drop zones when dragging and only in the 2 columns layout. */
.is-dragging-metaboxes #post-body.columns-2 #side-sortables,
.is-dragging-metaboxes #post-body #normal-sortables,
.is-dragging-metaboxes #post-body #additional-sortables {
	outline: 3px dashed #606a73;
	/* Prevent margin on the child from collapsing with margin on the parent. */
	display: flow-root;
	/*
	 * This min-height is meant to limit jumpiness while dragging. It's equivalent
	 * to the minimum height of the sortable-placeholder which is given by the height
	 * of a collapsed post box (36px + 1px top and bottom borders) + the placeholder
	 * bottom margin (20px) + 2 additional pixels to compensate browsers rounding.
	 */
	min-height: 60px;
	margin-bottom: 20px;
}

Using the same CSS for your plugin #additional-sortables area should solve the second part of the issue. I quickly tested it on TablePress and seems to work nicely.

#55 @TobiasBg
4 years ago

Hi @afercia!

Thanks for going the extra mile here and even looking up the CSS code! It's highly appreciated!

And please don't get me wrong, I never expected that the CSS/HTML that I took from core would work like that forever :-)

The CSS solution should work just fine, and I can simply add that to TablePress' common.css file, which it loads on all its pages.

Thanks again for all your hard work here!

#56 @afercia
4 years ago

@TobiasBg you're welcome. Thank you for the kind way you reported your issue!

#57 in reply to: ↑ 52 ; follow-up: @azaozz
4 years ago

  • Keywords needs-testing needs-backwards-compatibility added; commit removed
  • Priority changed from normal to highest omg bbq
  • Resolution fixed deleted
  • Severity changed from normal to blocker
  • Status changed from closed to reopened

Replying to afercia:

I think the root problem is that your plugin uses the post-body ID for the meta box holders. Historically, the ID post-body is used in core to identify the edit pages of posts, comments, menu items. It's heavily used to target the core functionality.

This is incorrect. The #post-body ID is used in core as a container for the writing area, and is reused by many plugins for the same purpose. A quick search shows that about 7,700 plugins reuse it: https://wpdirectory.net/search/01EE3JR3KJVSZFNR4R2MH6S8S0.

In that terms the code committed here introduces a serious back-compat problem.

Looking a bit deeper, why were some of the (older) default styles removed? They seem unrelated to this ticket: https://core.trac.wordpress.org/changeset/48340/trunk/src/wp-admin/css/common.css. Looks like there are 32,700 plugins that may be reusing some of these: https://wpdirectory.net/search/01EE3GHZPD2KPSS1J24KGXE4CB. Was this unrelated change even tested in any way?

Considering the above two backwards compatibility problems (that seem easily avoidable), and the fact that the committed code reverts previous (long time ago) fix, thinking this should be reverted for 5.5, then fixed and committed in 5.6.

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

This ticket was mentioned in Slack in #core by azaozz. View the logs.


4 years ago

#59 follow-up: @joedolson
4 years ago

Can you describe the problem that's resulting in a backwards compatibility issue? I don't clearly see what the problem is; if it's just the fact that drop areas are unexpectedly visible, I think there should be a solution that's short of reverting everything. But looking at what you've written above, I'm not at all clear what the issue is.

#60 in reply to: ↑ 59 @azaozz
4 years ago

Replying to joedolson:

if it's just the fact that drop areas are unexpectedly visible, I think there should be a solution...

Yeah, that doesn't look like a "huge" problem (as previously discussed). Thinking this can be fixed before, or even during RC if need be.

The bigger problem is removing some CSS and changing other/existing CSS that seems used in many many plugins, with unpredictable results and seemingly without any testing :(

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

#61 @joedolson
4 years ago

I can see that there's a consequence to the removed CSS that eliminates sidebars for plug-ins using those styles; I can't see any reason why that CSS was removed in this discussion or in the commit message - it seems like restoring that CSS might be all that's required, although with no idea why it was removed in the first place, it's hard to judge.

#62 follow-up: @joedolson
4 years ago

OK; I see the removal: https://core.trac.wordpress.org/ticket/49288#comment:18

It looks like those patterns were only removed because they weren't used in core, so it's a completely unrelated change. It's been in the patch since May 25th, so we really should have caught it sooner.

It looks to me like restoring that CSS is all that's needed.

#63 in reply to: ↑ 62 @azaozz
4 years ago

Replying to joedolson:

It looks to me like restoring that CSS is all that's needed.

Yeah, same here. Think this CSS was kept for backwards compatibility's sake, don't see a good reason to remove it in 5.5.

Hopefully the other CSS changes that affect back-compat/plugins can be fixed and tested in time for 5.5. If not, seems the only option would be to revert it, then fix, test, and add in 5.6.

#64 in reply to: ↑ 57 ; follow-up: @afercia
4 years ago

@azaozz I'm all for reviewing better this change in the next release cycle if there are concerns for a broad impact on the plugins ecosystem. Your call :) Of course, I have no objections to a revert.

However, for the future, this leads us to a question: to what extent is the HTML and CSS provided by core for specific pages really "supported"? I'd tend to think it is supported, to some extent, for the specific layout provided by core for the specific kind of pages where it it's used. For example the legacy edit post page: any page that reuses that layout can use the HTML and CSS provided by core. Instead, I'm not sure that HTML and CSS can be "supported" when plugins reuse them in other pages, like their settings pages. As I see it, core shouldn't support this usage as it's basically a "doing it wrong".

In any case, plugins that "hack" (in the good sense) HTML and CSS used by core for layout, they do it at their own risk. There's no official support, nor an API, that "guarantee" core HTML and CSS won't ever change. Actually, they can change at any time and this isn't unprecedented in the WordPress history.

Replying to azaozz:

This is incorrect. The #post-body ID is used in core as a container for the writing area

Hm, isn't this what I said above? "...used in core to identify the edit pages of posts, comments, menu items..."

Looking a bit deeper, why were some of the (older) default styles removed? They seem unrelated to this ticket: https://core.trac.wordpress.org/changeset/48340/trunk/src/wp-admin/css/common.css. Looks like there are 32,700 plugins that may be reusing some of these: https://wpdirectory.net/search/01EE3GHZPD2KPSS1J24KGXE4CB. Was this unrelated change even tested in any way?

This is documented in comment:18 on this ticket:

removes the CSS rulesets related to .inner-sidebar and .has-right-sidebar as they're no longer used since WordPress 3.4, see [20272] / #20015

where the emphasis on they're no longer used since WordPress 3.4 is mine :) Which leads us to one more question: how long core should keep supporting CSS that isn't used any longer in core? WordPress 3.4 was released on June 13, 2012, that's more than 8 years ago.

the fact that the committed code reverts previous (long time ago) fix

I may have missed what this fix was. Asking genuinely, can you please clarify when you have a chance?

#65 follow-up: @joedolson
4 years ago

It's worth having a discussion about how long we should maintain core CSS, given the arguments you present; but regardless, this ticket is not the appropriate place for removing old CSS; it's not related to the core purpose of the ticket, and that will make tracking the history of removals more difficult.

If we do remove core CSS that's used by thousands of plugins, it would need a dev note - preferably published sooner than RC1. It's not a big deal, as it doesn't break any functionality, but it's still a disservice to plug-in developers to make changes like this without adequate communication. Let's just restore those styles, and we can have a discussion about long-term maintenance of CSS separately.

#66 in reply to: ↑ 64 ; follow-up: @azaozz
4 years ago

Replying to afercia:

I'm all for reviewing better this change in the next release cycle if there are concerns for a broad impact on the plugins ecosystem.

Sorry but I'm not sure how to read this. I hope it doesn't mean "it is okay to potentially break up to 7,000 plugins installed on few millions of sites in 5.5, then "review" it and maybe fix it after 3-4 months. Releasing a WP version with known incompatibilities is... unthinkable?

...to what extent is the HTML and CSS provided by core for specific pages really "supported"?

Not sure if "supported" is the right word here. The wp-admin CSS is loaded on all plugins settings screens. It affects these screens, and many plugins reuse some parts to do things same way core does.

Right, this is a separate (complex/tough) conversation, but if fixes to WP can maintain backwards compatibility, like in this ticket, they definitely should.

This is incorrect. The #post-body ID is used in core as a container for the writing area

Hm, isn't this what I said above? "...used in core to identify the edit pages of posts, comments, menu items..."

Not exactly. The point here is that: "The #post-body ID is used in core as a container for the writing area, and is reused by many plugins for the same purpose."

the fact that the committed code reverts previous (long time ago) fix

I may have missed what this fix was. Asking genuinely, can you please clarify when you have a chance?

The already discussed "bug" introduced here that reverts to showing drop area borders when there's nothing to drop there, i.e. there is only one visible postbox. As already mentioned few times, it's an edge case for core. Now there's evidence that it affects some plugins too. It's not a "huge" deal, but will definitely cause some confusion to the users. Ideally this should not have been committed.

#67 in reply to: ↑ 65 @azaozz
4 years ago

Replying to joedolson:

It's worth having a discussion about how long we should maintain core CSS

Yes, completely agree. The WordPress "philosophy" on backwards compatibility has always been to maintain it as much as possible. This of course applies to CSS changes: if "breaking" back-compat can be avoided, it should. There are some efforts to better organize and review wp-admin CSS, hopefully this will be discussed there.

If we do remove core CSS that's used by thousands of plugins, it would need a dev note - preferably published sooner than RC1.

Right, exactly. Removal of deprecated code should at least happen in a separate ticket, with proper testing and communication.

Let's just restore those styles, and we can have a discussion about long-term maintenance of CSS separately.

Sounds good :)

#68 @azaozz
4 years ago

  • Keywords needs-patch added; has-patch removed

Looks like the TODO here is:

  • Restore the .inner-sidebar and .has-right-sidebar styling for now, then open a ticket, discuss if needed, and remove them with proper communications in 5.6.
  • Fix/revert changes in the use of #post-body. Seems these changes are not needed/the bugfixes on this ticket can be done without breaking back-compat. If that turns out to be very complex, revert these changes for now and fix this in 5.6.

This ticket was mentioned in Slack in #core by david.baumwald. View the logs.


4 years ago

#70 in reply to: ↑ 66 @afercia
4 years ago

Replying to azaozz:

Replying to afercia:

I'm all for reviewing better this change in the next release cycle if there are concerns for a broad impact on the plugins ecosystem.

Sorry but I'm not sure how to read this. I hope it doesn't mean "it is okay to potentially break up to 7,000 plugins installed on few millions of sites in 5.5, then "review" it and maybe fix it after 3-4 months. Releasing a WP version with known incompatibilities is... unthinkable?

Seems we're having some miscommunication... the whole sentence is:

I'm all for reviewing better this change in the next release cycle if there are concerns for a broad impact on the plugins ecosystem. Your call :) Of course, I have no objections to a revert.

I meant: I have no objections to a revert and I'm all for reviewing better in the next release cycle... :)

Looking into the other points now.

#71 follow-up: @afercia
4 years ago

@azaozz when you have a chance, I'd greatly appreciate your help to go through some thoughts here and see if I missed anything. Trying to summarize: I'm not sure the CSS part is the main backwards compatibility concern, not directly at least. The JS part comes first:

The JS part:

  • Previously, the empty-container class set via JS was used only on the Dashboard and in the #post-body layout only for the side-sortables, only in the 2 columns layout.
  • It is now used for all the sortables in the #post-body layout: this is necessary to make the whole thing work.
  • The reason is: there's the need to set a min-height on all the empty-container elements otherwise jQuery sortable won't work correctly.
  • Note: setting the min-height by default will produce some big empty spaces in the page. The best option in my opinion is to set the min-height only on the empty-container elements.
  • The lack of a min-height is the reason why it was not possible any longer to move a postbox into the advanced-sortables area when empty: this was a bug, see comment:7

What happens now:

  • The default style for the empty-container elements uses a fixed height of 250 pixels plus a dashed outline.
  • This CSS is now applied to _all_ the sortables also in the #post-body layout.
  • The new CSS does its best to compensate but it can't catch unusual usages or sortable areas with custom names.

Possible fix:

  • Restore the visible == 1 bit for now: this should be reviewed later because it doesn't make much sense now. Actually, on the Dashboard the minimum number of visible widgets is now 2: there's now the "WordPress Events and News" widget, which is visible to all users.
  • Turns out the visible == 1 bit should have been audited with the introduction of the new dashboard widget, but it was somehow missed.
  • Regardless, this visible == 1 has an impact also on the #post-body layout so I think the best option for now is to just restore it.
  • I don't think the other JS part related to the side variable needs to be restored, as now _all_ the sortables areas get the empty-container class.
  • Instead, we should review the default style for the empty-container elements: fixed height of 250 pixels and dashed outline.
  • It should be kept for the Dashboard.
  • It should be changed for the #post-body layout.
  • In the previous patch I tried to keep this styling only to cover an edge case illustrated in the screenshot below: when removing all the postboxes from side-sortables, a dashed "square" is supposed to be visible.

http://cldup.com/7Z5EZZoVfi.png

  • I'd say this is a very edge case.
  • I'd propose to just make the sortables areas in the #post-body layout not visible by default and reveal them only while dragging regardless whether it's the 1 or 2 columns layout.

Seems to me the best way to address backwards compatibility. If there's consensus, I can turn this into a patch.

#72 in reply to: ↑ 71 @azaozz
4 years ago

Replying to afercia:

I'm not sure the CSS part is the main backwards compatibility concern

Yes, looking more now, some of the js changes would need tweaking/fixing.

Previously, the empty-container class set via JS was used only on the Dashboard and in the #post-body layout only for the side-sortables, only in the 2 columns layout.
...
The lack of a min-height is the reason why it was not possible any longer to move a postbox into the advanced-sortables area when empty: this was a bug, see comment:7

Right. The drop areas on the old Edit Post screen are actually three. However the second and third are always shown together. If I remember correctly the reason the third is three is that postboxes added by plugins were put by default in it; at the bottom under the editor. Also think at some point (over 10 years ago) that area used to be shown separately, but now is always shown together with the second area. So when dragging of postboxes was added to that screen, the three areas were kept as back-compat, but the third was (mostly) disabled. The users could drag boxes out but there's no reason for them to add boxes back in it, as they would show and work in exactly the same way as when in the second area. In that terms the changes here can either maintain the previous behavior or make the third area "droppable" again. This won't change how the screen works.

  • Regardless, this visible == 1 has an impact also on the #post-body layout so I think the best option for now is to just restore it.

Sounds good. Yes, ideally it should have been implemented better.

Seems to me the best way to address backwards compatibility. If there's consensus, I can turn this into a patch.

Yes, I think all the outlined reasons and changes make sense (sorry I can't be of more help atm, having some health issues).

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

This ticket was mentioned in Slack in #core by peterwilsoncc. View the logs.


4 years ago

#74 @afercia
4 years ago

Thanks @azaozz. Get better soon!

@afercia
4 years ago

#75 @afercia
4 years ago

  • Keywords has-patch added; needs-patch removed

49288.9.diff does the following:

  • Restores the CSS rules removed in [48340] to ensure backwards compatibility, even it these rules aren't used in core since more than 8 years.
  • Restores the visible == 1 bit for now: this should then be reviewed in a new ticket, maybe #50699.
  • CSS: the fixed height of 250 pixels is now used only on the Dashboard and for #post-body.columns-2 #side-sortables, which is the sortable in the right column in the Classic Editor (no changes here).
  • CSS: _all_ the other elements with class .meta-box-sortables (so: not only the empty ones) have now a min-height of 1 pixel which is necessary to make jQuery UI Sortable work properly. This very small min-height also prevents the drop areas to take too much space.
  • CSS: this way, the CSS is also simplified: there's no need to target specific sortable areas and the CSS will target also sortable areas with custom names, see TablePress for example.
  • In short: in the so called #post-body layout, the sortables areas aren't visible by default: they get a dashed outline only while dragging.
  • JS: I added a call to the jQuery UI Sortable refreshPositions() method when the drag action starts: this improves the behavior as the placeholders get visible as soon as the mouse pointer overlaps a "drop area". Basically, the min-height changes from 1 pixel to 60 pixels and refreshPositions() makes jQuery Ui sortable take that into account.
  • Worth noting the refreshPositions() method is already used for the widgets, see [26366], the nav menus, see [14531], and the customizer widgets, see [30102].

Hopefully, this should address the backwards compatibility concerns. I'd greatly appreciate some testing and review :)

@TobiasBg mind testing with your plugin when you have a chance? I did test a bit already and didn't notice any issue.

#76 @TobiasBg
4 years ago

Hi @afercia!

Thanks for taking another deep look here!

I just tested the patch with TablePress and couldn't manage to "break" anything :-) That is, I can confirm that the empty drop zones are now invisible again, just like in earlier versions of WP, without any CSS changes in TablePress. Thanks a lot!

#77 @afercia
4 years ago

@TobiasBg thanks for the very quick and precious feedback!

@afercia
4 years ago

#78 @afercia
4 years ago

49288.10.diff adds a fix for the max-width: 850px media query, that is: when the post edit screen auto-switches to one-column layout under a viewport width of 850 pixels and before the responsive view kicks at 782 pixels.

Note: this is an edge case because the difference between these two viewport widths is a "range" of only 68 pixels. There are other cases in core and I think they originate from a time when the responsive view switch at 782 pixels wasn't implemented yet. These special cases, together with the admin menu "auto-fold" maybe don't make much sense any longer and maybe, for the future, it would be worth considering to remove them now that there's a responsive view.

Last edited 4 years ago by afercia (previous) (diff)

This ticket was mentioned in Slack in #accessibility by afercia. View the logs.


4 years ago

#80 @audrasjb
4 years ago

  • Keywords needs-testing removed

I think it's worth to consider committing 49288.10.diff before WP 5.5 RC2. I tested that patch and I didn't notice any issue on the edit post screen, on any viewport width.

Last edited 4 years ago by audrasjb (previous) (diff)

#81 @whyisjake
4 years ago

  • Keywords commit dev-feedback added

This ticket was mentioned in Slack in #core by afercia. View the logs.


4 years ago

#83 @SergeyBiryukov
4 years ago

  • Keywords dev-reviewed added; dev-feedback removed

49288.10.diff looks good to me.

#84 @afercia
4 years ago

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

In 48717:

Accessibility: Administration: Address backward compatibility concerns for the sortable postboxes areas after [48340].

Fixes #49288.

#85 @afercia
4 years ago

  • Keywords needs-backwards-compatibility removed
  • Resolution fixed deleted
  • Status changed from closed to reopened

Reopening for the 5.5 branch.

#86 @afercia
4 years ago

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

In 48718:

Accessibility: Administration: Address backward compatibility concerns for the sortable postboxes areas after [48340].

Merges [48717] to the 5.5 branch.
Fixes #49288 for 5.5.

#87 @afercia
4 years ago

  • Keywords fixed-major added
Note: See TracTickets for help on using tickets.