Add media modal - different select width

[pavelevap]

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.

[pavelevap]

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

[afercia]

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.

[nacin]

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.

[valendesigns]

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

[afercia]

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.

[helen]

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

[helen]

In 31197:

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

props valendesigns.
fixes #30725 for trunk.

[helen]

In 31198:

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

Merges [31197] to the 4.1 branch.

props valendesigns.
fixes #30725.

[afercia]

Please consider to remove or limit to modals that

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

which is currently causing this:

[valendesigns]

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]

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.

[helen]

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.

[helen]

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, see attached image below.

[valendesigns]

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

[valendesigns]

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.

[valendesigns]

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

[valendesigns]

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.

[afercia]

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:

Investigating on a complete different approach.

[valendesigns]

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

[afercia]

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.

[azaozz]

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.

[helen]

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.

[afercia]

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

[helen]

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.

[afercia]

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

[helen]

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?

[afercia]

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;
}

[helen]

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.

[helen]

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.

[helen]

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.

[helen]

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.

[wonderboymusic]

In 31616:

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

See #30725.

[helen]

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]

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.

[helen]

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.

[DrewAPicture]

See select_ie8.png​ for IE8 with 30725.5.diff​ applied.

[afercia]

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".

[helen]

In 32101:

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

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

[DrewAPicture]

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

[azaozz]

In 32120:

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

[pavelevap]

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?

[SergeyBiryukov]

Replying to pavelevap:

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

Please create a new one.