WordPress.org

Make WordPress Core

Opened 5 years ago

Closed 5 years ago

Last modified 5 years ago

#30725 closed defect (bug) (fixed)

Add media modal - different select width

Reported by: pavelevap Owned by: helen
Milestone: 4.2 Priority: normal
Severity: minor Version: 4.1
Component: Media Keywords: needs-patch
Focuses: ui Cc:
PR Number:

Description

When clicking "Add media", images are loading in modal window.
But there is different width when loading spinner appears.
When everything loaded, select field shrinks back and it is distracting.
Also, localized string is longer than English original and it is not visible the whole text.

See attached screenshot.

Attachments (11)

Loading_media_items.png (13.0 KB) - added by pavelevap 5 years ago.
30725.diff (741 bytes) - added by valendesigns 5 years ago.
30725.2.patch (2.0 KB) - added by afercia 5 years ago.
30725.3.diff (361 bytes) - added by valendesigns 5 years ago.
Screen Shot 2015-01-16 at 10.15.42 AM.png (130.1 KB) - added by helen 5 years ago.
Screen Shot 2015-01-16 at 10.32.06 AM.png (13.2 KB) - added by helen 5 years ago.
30725.4.diff (416 bytes) - added by valendesigns 5 years ago.
30725.2.diff (1.9 KB) - added by helen 5 years ago.
30725.5.diff (5.5 KB) - added by obenland 5 years ago.
select_ie8.png (99.3 KB) - added by DrewAPicture 5 years ago.
30725.6.diff (9.9 KB) - added by afercia 5 years ago.

Download all attachments as: .zip

Change History (59)

#1 @pavelevap
5 years ago

It worked without problems in 4.0.1 and date filter was not available here.

#2 @afercia
5 years ago

  • Focuses ui added

Looks like it's visible each time the spinner is shown. After r30813 those selects have a percentage max-width value, when the spinner appears, it will increase the width of their container so they will "grow" and then "shrink" when the spinner disappears.
Also, a max-width is not good for languages other than English that may translate the option values with longer strings.

#3 follow-up: @nacin
5 years ago

  • Milestone changed from Awaiting Review to 4.1.1

This is a pretty amusing bug.

max-width is OK, we don't really have a choice often to put a sane limit on things, but we're usually pretty good about giving translators at least 50% to 100% more space over what English requires.

#4 @nacin
5 years ago

  • Severity changed from normal to minor

@valendesigns
5 years ago

#5 @valendesigns
5 years ago

  • Keywords has-patch added

I've added a patch that fixes this issue. There were a few ways to fix this, but I chose to absolute position the spinner and add container padding to the right. We could have replaced the display attribute with visibility to maintain the flow of elements as a second option. However, the pure CSS option seems like the best because the alternative would require JS modifications. Please test and let me know if you have any suggestions.

Cheers,
Derek

#6 in reply to: ↑ 3 @afercia
5 years ago

Replying to nacin:

max-width is OK

I'd propose then to follow @valendesigns solution but using max-width on the select elements only when needed, within a media query. Updated patch uses max-width: 1120px as breakpoint, to be honest it's a bit arbitrary, please let me know if you have any suggestions about a better breakpoint. Tried also some CSS cleaning.

@afercia
5 years ago

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


5 years ago

#8 @helen
5 years ago

Patch looks good, just had to add one semi-esoteric rule so that options with long strings don't invisibly take up space. Committing.

#9 @helen
5 years ago

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

In 31197:

Media: Prevent filter selects from jiggling when the spinner shows.

props valendesigns.
fixes #30725 for trunk.

#10 @helen
5 years ago

In 31198:

Media: Prevent filter selects from jiggling when the spinner shows.

Merges [31197] to the 4.1 branch.

props valendesigns.
fixes #30725.

#11 follow-up: @afercia
5 years ago

  • Resolution fixed deleted
  • Status changed from closed to reopened

Please consider to remove or limit to modals that

width: 100%; /* prevents options from invisibly taking up horizontal space */ 

which is currently causing this:

https://cldup.com/BAD3DkU6_M.png

#12 in reply to: ↑ 11 ; follow-up: @valendesigns
5 years ago

  • Keywords needs-patch added; has-patch removed

Replying to afercia:

Please consider to remove or limit to modals that

width: 100%; /* prevents options from invisibly taking up horizontal space */ 

It wasn't in the original patch. We probably should have done more testing if the CSS was going to change. Helen, I know you have an inline comment, but can you go into more detail about what was happening to the UI that required adding that line?

@valendesigns
5 years ago

#13 @valendesigns
5 years ago

  • Keywords has-patch added; needs-patch removed

After testing 31198 it's not immediately obvious what width: 100% is meant to fix, because it doesn't change the UI for the affected modal and spinner jiggling. It probably shouldn't have ever made it into the commit. In any case, I've added patch 30725.3.diff that removes it and everything goes back to normal.

#14 in reply to: ↑ 12 @helen
5 years ago

Replying to valendesigns:

can you go into more detail about what was happening to the UI that required adding that line?

See attached image above. I'll look at this some more.

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

#15 @helen
5 years ago

The max-width on the select elements in the media library doesn't account for the button being there. Might need to rethink that approach for this instance (it's fine in the media modal).

With the original patch, the spinner is all the way to the side at smaller widths, see attached image below.

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

@valendesigns
5 years ago

#16 @valendesigns
5 years ago

Sorry! That makes sense now. My bad. Here's an updated patch with better scoping.

#17 @valendesigns
5 years ago

  • Keywords needs-patch added; has-patch removed

Never mind. Patch is not complete. I didn't see your comments just now. The scoping works for modals but doesn't work for the media library. Back to the drawing board.

#18 @valendesigns
5 years ago

However, If I remember correctly, the spinner was all the way to the right in mobile even before the patch.

#19 @valendesigns
5 years ago

  • Keywords has-patch added; needs-patch removed

I just did a double check and 4.1 has the spinner all the way right in mobile. So realistically, nothing changed with the spinner position and 30725.4.diff should be the correct course of action to fix long titles and properly scope the .media-modal.

#20 @afercia
5 years ago

Quickly discussed on Slack with @helen, seems floats without an explicit width set cause some issues in IE 8 which should still be supported, see screenshot below:

https://cldup.com/msidHefAGE.png

Investigating on a complete different approach.

#21 @valendesigns
5 years ago

We could then scope for .ie8 and set the width to auto or something of that nature.

#22 @afercia
5 years ago

Was thinking to switch to an inline formatting model, let browsers do their job, and don't think about this issue ever again :) using the text-align: justify technique could work in IE 7 too and, in the future, maybe using CSS flexbox.

About the spinner, what about to display it as an overlay above the UI controls? See examples below (backgrounds and opacity are there just for testing purposes while I was doing some in-browser editing). Would appreciate any feedback.

https://cldup.com/PcpPZgY-oF.png

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


5 years ago

#24 @azaozz
5 years ago

A relatively easy fix would be to keep the old CSS, make the spinner display: inline-block; visibility: hidden; (or no display at all) and toggle visibility from JS. In fact all spinners would work better if we adopt that everywhere.

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

#25 @helen
5 years ago

  • Keywords needs-patch added; has-patch removed

I knew I was forgetting something - azaozz is right about the bigger picture, which we had discussed two years ago on #22839 :) Let's fix it for this instance in 4.1.1, and look to standardize across the board in 4.2 on that ticket.

#26 @afercia
5 years ago

Would like to try to patch this but can't do without patching also #29897 please let me know how to proceed.

@helen
5 years ago

#27 @helen
5 years ago

  • Keywords has-patch added; needs-patch removed

30725.2.diff should do it for this instance and reverts the previous commit - way simpler fix in the end to do it the right way. Please test.

#28 follow-up: @afercia
5 years ago

Tested, noticed some minor issues in responsive view and big issues in IE 8.
Please consider that percentage based calculation can't reserve enough space for the spinner when max-width kicks in. Say the max-width: 66% computes to 300px. The width taken by the two selects is:
1px + 47% + 2% + 1px + 47% + 2% = 296px

https://cldup.com/dK5A7RIPFn.png

https://cldup.com/1zN9jgRLfK.png

https://cldup.com/KGXYq2GTfj.png

https://cldup.com/pRYB1Uv5Z6.png

#29 in reply to: ↑ 28 ; follow-up: @helen
5 years ago

Replying to afercia:

Tested, noticed some minor issues in responsive view and big issues in IE 8.

Is that caused by the patch? Is it new in 4.1?

#30 in reply to: ↑ 29 @afercia
5 years ago

Replying to helen:

Is that caused by the patch? Is it new in 4.1?

Already existing in 4.1, except for the new spinner thing. Please notice the spinner inherits float: right so it will behave like block not like inline-block. Maybe should be reset to float: none ? but still there wouldn't be enough space for the spinner in responsive view.
About IE 8, this could work:

.ie8 .media-toolbar-primary {
	width: 33%;
}

.ie8 .media-toolbar-secondary {
	width: 66%;
}

.ie8 .media-toolbar-primary #media-search-input {
	float: right;
}

#31 @helen
5 years ago

Since we're looking at 4.1.1 here, what here needs to be fixed for a point release? Is this perhaps minor enough to do in 4.2 instead? I tend to think the latter at this point, especially if we've got more wide-reaching issues.

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


5 years ago

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


5 years ago

#34 @helen
5 years ago

  • Keywords needs-patch added; has-patch removed
  • Milestone changed from 4.1.1 to 4.2

I haven't seen other reports of the IE8 issue so it doesn't seem like an urgent fix, and the jiggling inputs are certainly also a bug that should be fixed, but it is minor and kind of funny. I'm going to revert the commit from the 4.1 branch and move this to 4.2, where we should fix it more holistically in terms of spinner visibility (#22839) and the percentage widths that don't account for things like the bulk select button or spinner.

#35 @helen
5 years ago

In 31468:

Revert [31198] from the 4.1 branch, as it is an incomplete fix that introduces more problems than the tiny issue it was attempting to solve.

see #30725.

#36 @helen
5 years ago

I should note that we can always move this back to 4.1.2 should one exist and the end fix be relatively self-contained.

#37 @wonderboymusic
5 years ago

In 31616:

[31468] reverted this in the 4.1 branch, also reverting in trunk.

See #30725.

#38 @helen
5 years ago

In 31996:

Spinners: Toggle a class instead of show/hide.

Toggling spinners also now uses visibility instead of display, so that the space is always reserved and nothing moves around unexpectedly.

props cdog, MikeHansenMe, valendesigns.
fixes #22839. see #31875, #30725.

@obenland
5 years ago

#39 follow-up: @obenland
5 years ago

I have no idea what I'm doing in 30725.5.diff, but it brings back the spinner for me in wp-admin/upload.php. It's currently missing.

#40 in reply to: ↑ 39 @helen
5 years ago

Replying to obenland:

It's a good start - we need to alter the relative width of the selects here, though, as the current percentages don't account for a spinner. Also needs an IE8 audit, I believe.

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


5 years ago

#43 @afercia
5 years ago

Far from being perfect, attached refreshed patch tries to fix the selects and the spinner position. Very, very, tricky:

  • there's a mix of elements with fixed widths and percentage widths
  • even using calc() we can't predict the width of some elements, for example the "Bulk select" button width will vary depending on the translation

Maybe the most sane things to do is to reserve additional space for longer translations, as much as possible, and accept the fact elements will flow in 2 or 3 lines on small screens.
There are still many edge cases, things may break. I'd consider this just an attempt to patch a layout that should really be rebuilt from scratch. We probably need to iterate on this. @helen please check carefully, any change and improvements more than welcome.
Note: url.js needs some attention, the spinner isn't revealed when using "Insert from URL".

@afercia
5 years ago

#44 @helen
5 years ago

In 32101:

Media: Bring back spinners, now without bouncing select elements.

props afercia for the initial patch.
see #22839, #30725.

#45 @DrewAPicture
5 years ago

@helen: What do you need to move this ticket and #31908 forward?

#46 @azaozz
5 years ago

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

In 32120:

Make sure the spinner in the media modal is visible on narrow screens (without affecting the media grid).
Fixes #30725.

#47 follow-up: @pavelevap
5 years ago

A part of this ticked was missed:

"Also, localized string is longer than English original and it is not visible the whole text."

There is enough space to show the whole string... Should I reopen this ticket?

#48 in reply to: ↑ 47 @SergeyBiryukov
5 years ago

Replying to pavelevap:

There is enough space to show the whole string... Should I reopen this ticket?

Please create a new one.

Note: See TracTickets for help on using tickets.