Make WordPress Core

Opened 10 years ago

Closed 10 years ago

Last modified 10 years ago

#30725 closed defect (bug) (fixed)

Add media modal - different select width

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

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 10 years ago.
30725.diff (741 bytes) - added by valendesigns 10 years ago.
30725.2.patch (2.0 KB) - added by afercia 10 years ago.
30725.3.diff (361 bytes) - added by valendesigns 10 years ago.
Screen Shot 2015-01-16 at 10.15.42 AM.png (130.1 KB) - added by helen 10 years ago.
Screen Shot 2015-01-16 at 10.32.06 AM.png (13.2 KB) - added by helen 10 years ago.
30725.4.diff (416 bytes) - added by valendesigns 10 years ago.
30725.2.diff (1.9 KB) - added by helen 10 years ago.
30725.5.diff (5.5 KB) - added by obenland 10 years ago.
select_ie8.png (99.3 KB) - added by DrewAPicture 10 years ago.
30725.6.diff (9.9 KB) - added by afercia 10 years ago.

Download all attachments as: .zip

Change History (59)

#1 @pavelevap
10 years ago

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

#2 @afercia
10 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
10 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
10 years ago

  • Severity changed from normal to minor

@valendesigns
10 years ago

#5 @valendesigns
10 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
10 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
10 years ago

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


10 years ago

#8 @helen
10 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
10 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
10 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
10 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
10 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?

#13 @valendesigns
10 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
10 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 10 years ago by helen (previous) (diff)

#15 @helen
10 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, we would get this:

https://core.trac.wordpress.org/raw-attachment/ticket/30725/Screen%20Shot%202015-01-16%20at%2010.32.06%20AM.png

Version 1, edited 10 years ago by helen (previous) (next) (diff)

#16 @valendesigns
10 years ago

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

#17 @valendesigns
10 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
10 years ago

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

#19 @valendesigns
10 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
10 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
10 years ago

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

#22 @afercia
10 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.


10 years ago

#24 @azaozz
10 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 10 years ago by azaozz (previous) (diff)

#25 @helen
10 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
10 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
10 years ago

#27 @helen
10 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
10 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
10 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
10 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
10 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.


10 years ago

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


10 years ago

#34 @helen
10 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
10 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
10 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
10 years ago

In 31616:

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

See #30725.

#38 @helen
10 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
10 years ago

#39 follow-up: @obenland
10 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
10 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.


10 years ago

#43 @afercia
10 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
10 years ago

#44 @helen
10 years ago

In 32101:

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

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

#45 @DrewAPicture
10 years ago

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

#46 @azaozz
10 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
10 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
10 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.