Make WordPress Core

Opened 3 years ago

Closed 3 years ago

#53833 closed defect (bug) (fixed)

'Replace image' Media Modal is missing select fields under filter media due to css issues

Reported by: mhuntdesign's profile mhuntdesign Owned by: sabernhardt's profile sabernhardt
Milestone: 5.8.1 Priority: normal
Severity: normal Version: 5.3
Component: Media Keywords: has-patch commit fixed-major
Focuses: css, administration Cc:

Description

When using the classic editor, after an image is added to the WYSIWYG, the image can be clicked on within the editor. The pencil icon opens up the 'Image details' Media Modal. There you can click on the Replace button, which opens up the 'Replace image' Media Modal. This modal has a Filter Media Option, but only the search bar shows. The select field is hidden from view due to incorrect CSS margins applied to both the 'h2.media-attachments-filter-heading' and 'select.attachment-filters'. Applying custom css to the backend corrects this issue.

.media-attachments-filter-heading {
    color: #3c434a  !important;
    margin: 5px 0 0 -15px  !important;
    font-size: 13px  !important;
}

.media-modal-content .media-frame select.attachment-filters {
	margin-top:18px !important;
}

Attachments (6)

Screen Shot 2021-07-29 at 3.54.06 PM.png (284.5 KB) - added by mhuntdesign 3 years ago.
Replace image media modal
Screen Shot 2021-07-29 at 3.53.56 PM.png (203.5 KB) - added by mhuntdesign 3 years ago.
Image details Media Modal
Screen Shot 2021-07-29 at 3.53.49 PM.png (151.6 KB) - added by mhuntdesign 3 years ago.
Edit Media Tooltip
53833.patch (399 bytes) - added by sabernhardt 3 years ago.
53833.more.patch (961 bytes) - added by sabernhardt 3 years ago.
also edits top value instead of using padding, removes auto width for date filter
53833.2.patch (420 bytes) - added by azaozz 3 years ago.

Download all attachments as: .zip

Change History (22)

@mhuntdesign
3 years ago

Replace image media modal

@mhuntdesign
3 years ago

Image details Media Modal

@mhuntdesign
3 years ago

Edit Media Tooltip

#1 @mhuntdesign
3 years ago

I should note that the margin values set for select.attachment-filters are correct on the 'Add Media' media modal, and making a change to the replace media modal does break the css set for add media, unless specificity is correctly set.

@sabernhardt
3 years ago

#2 @sabernhardt
3 years ago

  • Focuses css administration added
  • Keywords has-patch added
  • Milestone changed from Awaiting Review to 5.8.1
  • Owner set to sabernhardt
  • Severity changed from trivial to normal
  • Status changed from new to accepted

@mhuntdesign Hi and thanks for the report!

Changes to the h2 headings in r35427 and r46375 were made (mainly) for the first screen of the "Image details" modal, but "Replace image" is still inside that modal.

53833.patch makes the selector a bit more specific.

Last edited 3 years ago by sabernhardt (previous) (diff)

#3 @sabernhardt
3 years ago

  • Version changed from 5.8 to 5.3

#4 @mukesh27
3 years ago

  • Keywords needs-testing added

Thanks for the ticket.

53833.patch working fine for me.

needs-testing added for more testing hands.

This ticket was mentioned in Slack in #core-test by boniu91. View the logs.


3 years ago

#6 @Hareesh Pillai
3 years ago

The attached patch works well in the desktop screen size. However, it does not work for me in the responsive screen width.

#7 @sabernhardt
3 years ago

@hareesh-pillai Thanks for testing at multiple screen sizes! With the patch, is the problem one of the following issues, or have you found something else as well?

1) The top of the image focus outline and checkbox are cut off between 640 and 782 pixels (r51642 added top padding for 640px and narrower, or under 400px tall and any width, but changing top from 72px to 82px at a larger breakpoint might have been better).

2) In English, the Date filter drops below the type filter (out of sight) at about 320-350px or and 640-670px due to the auto width introduced in r44653. Other languages can break in wider viewports.

Last edited 3 years ago by sabernhardt (previous) (diff)

#8 @Hareesh Pillai
3 years ago

  • Keywords needs-refresh added; has-patch needs-testing removed

My bad! I should have been more specific.

Yes, @sabernhardt. I ran into the same issues mentioned by you.

With just a minor deviation - Issue 2 is seen at screen sizes <=462px as well.

@sabernhardt
3 years ago

also edits top value instead of using padding, removes auto width for date filter

#9 @sabernhardt
3 years ago

  • Keywords has-patch needs-testing added; needs-refresh removed

53833.more.patch tries to fix all three of these issues within the classic editor media modal. I would like at least the heading selector corrected in the minor release, as long as it does not make something else worse.

If the top value adjustment is better than top padding, that change would fit in 5.8.1 as well (before anyone experiences it differently).

The patch also involves reverting r44653, which goes back to WordPress 5.1. Setting automatic width on the last select element makes it too wide at various smaller screen sizes when there are two of them. The Core changeset was intended as a first step, but then the code was never removed from the block editor styles. So the image block's fix still operates in the block editor even with this patch.

Additional considerations (likely better to revisit for a major release):

  • I think the 82px height and top values belong at the 900px breakpoint, where the filter selects and search box increase in font size and height. In the 783-900 pixel range, these elements can touch the top of selected image checkboxes and the focus outline can be clipped slightly.
  • The date filter can be too wide in the block editor as well as classic, when setting a Featured Image or using the Classic block. Theoretically, specifying :first-of-type:last-of-type together would make sure it only affects a single filter, but that selector is not pretty.

This ticket was mentioned in Slack in #core-media by sabernhardt. View the logs.


3 years ago

#11 @azaozz
3 years ago

53833.patch seems okay, definitely safer than 53833.more.patch for a dot release :)

@sabernhardt wondering if limiting it to .media-embed might break some other h2, perhaps even in plugins. There seem to be several .image-details h2 in several places. Perhaps excluding h2.media-attachments-filter-heading would be safer? Could you check if 53833.2.patch works as expected?

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

@azaozz
3 years ago

#12 follow-up: @sabernhardt
3 years ago

  • Keywords commit added; needs-testing removed

The filter heading is the biggest problem here, and 53833.2.patch fixes that in the Replace image dialog for both Classic Editor and Classic blocks.

With that patch, the inline uploader still has the all-caps heading style within the same Replace dialog, but that is merely an inconsistency. The first patch would correct that and possibly another unidentified issue the earlier change created, yet we do not have time to guarantee it does not introduce a new problem.

Let's correct the filter heading now so at least the first filter is usable again (without tabbing to it). Selecting (all) Images can be a temporary workaround for #53765, which will not make this minor release.

Version 0, edited 3 years ago by sabernhardt (next)

#13 in reply to: ↑ 12 @azaozz
3 years ago

Replying to sabernhardt:

Let's correct the filter heading now...

Right, my thinking exactly.

#14 @azaozz
3 years ago

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

In 51703:

Media: fix showing of the "Filter Media" filds when replacing an image from the media modal.

Props mhuntdesign, sabernhardt, azaozz.
Fixes #53833.

#15 @azaozz
3 years ago

  • Keywords fixed-major added
  • Resolution fixed deleted
  • Status changed from closed to reopened

Reopen for 5.8.1.

#16 @desrosj
3 years ago

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

In 51708:

Media: fix showing of the "Filter Media" filds when replacing an image from the media modal.

Props mhuntdesign, sabernhardt, azaozz.
Merges [51703] to the 5.8 branch.
Fixes #53833.

Note: See TracTickets for help on using tickets.