Make WordPress Core

Opened 7 years ago

Closed 7 years ago

Last modified 4 years ago

#40152 closed defect (bug) (fixed)

Crop Image button off-screen on mobile

Reported by: cybr's profile Cybr Owned by: swissspidy's profile swissspidy
Milestone: 4.7.4 Priority: normal
Severity: normal Version:
Component: Media Keywords: has-patch has-screenshots fixed-major
Focuses: ui, administration Cc:

Description

Tested on Safari 9 on an iPhone 6S+, confirmed on actual iPhone 7+.

The following CSS line requires a media query to become bigger than 33% (i.e. 100%?):

.media-modal-content .media-toolbar-primary.search-form {
    width: 33%;
}

http://i.imgur.com/zTa0ou3.png

Attachments (7)

40152.diff (565 bytes) - added by karinedo 7 years ago.
Tentative fix, moving skip button to the left
CropImage_button_offscreen_on_mobile_fixed.jpg (54.0 KB) - added by mehul0810 7 years ago.
Patch 40152.diff Works for me
40152.patch (1.2 KB) - added by sagarprajapati 7 years ago.
40152.2.diff (1.0 KB) - added by Cybr 7 years ago.
40152.2.patch (1.7 KB) - added by sagarprajapati 7 years ago.
40152.3.patch (1.7 KB) - added by sagarprajapati 7 years ago.
Remove unnecessary space from the code
40152.3.diff (1.6 KB) - added by swissspidy 7 years ago.

Download all attachments as: .zip

Change History (44)

#1 @adamsilverstein
7 years ago

  • Keywords good-first-bug added

@karinedo
7 years ago

Tentative fix, moving skip button to the left

#2 @swissspidy
7 years ago

This sounds like a duplicate of #40132, which in turn was marked as a duplicate of #30634. I'm not sure it's really the same issue though.

#3 @Cybr
7 years ago

#30634 is about moving/resizing the selection field.
#40132 seems to be the same as this ticket, but on a different device.

#4 @swissspidy
7 years ago

#40132 was marked as a duplicate.

#5 @swissspidy
7 years ago

  • Keywords has-patch added
  • Milestone changed from Awaiting Review to 4.7.4
  • Version trunk deleted

@mehul0810
7 years ago

Patch 40152.diff Works for me

#6 @mehul0810
7 years ago

  • Resolution set to worksforme
  • Status changed from new to closed

Checked Patch 40152.diff and it works for me. Please check attached screenshot (i.e. CropImage_button_offscreen_on_mobile_fixed.jpg)

#7 @ocean90
7 years ago

  • Resolution worksforme deleted
  • Status changed from closed to reopened

#8 follow-up: @Cybr
7 years ago

Setting it to 100% will cause all buttons to disappear if div with class media-toolbar-secondary is filled in with anything on all screens.

#9 in reply to: ↑ 8 ; follow-up: @adamsilverstein
7 years ago

Replying to Cybr:

Setting it to 100% will cause all buttons to disappear if div with class media-toolbar-secondary is filled in with anything on all screens.

@Cybr I don't quite understand your comment, what do you mean by " if div with class media-toolbar-secondary is filled in with anything", can you give an example or steps to reproduce?

#10 in reply to: ↑ 9 @Cybr
7 years ago

Replying to adamsilverstein:

@Cybr I don't quite understand your comment, what do you mean by " if div with class media-toolbar-secondary is filled in with anything", can you give an example or steps to reproduce?

I can't give you an example, because I don't really understand the correct structure for third party implementation on this part.

However, what I do know is that there are two div elements inside .media-frame-toolbar > .media-toolbar:

<div class="media-frame-toolbar">
	<div class="media-toolbar">
		<div class="media-toolbar-secondary"></div>
		<div class="media-toolbar-primary search-form">
			<button type="button" class="button media-button button-secondary button-large media-button-skip">Skip Cropping</button>
			<button type="button" class="button media-button button-primary button-large media-button-insert">Crop Image</button>
		</div>
	</div>
</div>

When you fill in .media-toolbar-secondary with any flow-forming content (like text), and set .media-toolbar-primary to 100%, then media-toolbar-primary will overflow.
This is because media-toolbar-secondary is output first (i.e. it should be the 67% of the left side) and there's no room for a second row.

Last edited 7 years ago by Cybr (previous) (diff)

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


7 years ago

#12 @swissspidy
7 years ago

  • Keywords good-first-bug removed
  • Owner set to swissspidy
  • Status changed from reopened to assigned

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


7 years ago

#14 @swissspidy
7 years ago

  • Keywords needs-patch added; has-patch removed

The patch works well as long as the buttons aren't too wide. When using de_DE, it looks like this:

https://cldup.com/Ixu2dQ1Xe7.png

https://cldup.com/MQh6e6iYdJ.png

I haven't tested RTL support just yet, but of course this needs to work there as well.

#15 @sagarprajapati
7 years ago

  • Keywords has-patch added; needs-patch removed

Hi @swissspidy and All

I have attached patch for the solution of above issue. Please check it.

Thanks

Last edited 7 years ago by sagarprajapati (previous) (diff)

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


7 years ago

#17 follow-up: @swissspidy
7 years ago

Thanks @sagarprajapati! This patch indeed resolves that issue. It's just unfortunate that the bottom bar is now 120px high even if the buttons are on one line. But that whole media CSS is a rabbit hole it seems…

#18 @sagarprajapati
7 years ago

Hi @swissspidy we should cut the long text using CSS and set ... After few later.

For example if text of the button is 'the button text is too long' then we should set it to 'the button..'.

#19 @swissspidy
7 years ago

Truncating text isn't really a11y-friendly and also confusing for users and translators. Fixing the layout is the better approach.

#20 in reply to: ↑ 17 @Cybr
7 years ago

Replying to swissspidy:

But that whole media CSS is a rabbit hole it seems…

This is where CSS3 flexbox comes in handy. But only if you wish to drop IE10 support, though.

Alternatively, use inline-block and/or tables; where there's a min-height with padding. But, no max-height nor default height.

Version 0, edited 7 years ago by Cybr (next)

@Cybr
7 years ago

#21 @Cybr
7 years ago

Well, I was introduced to the awful world of absolute alignment. This essentially means it has no room for changes.
Nevertheless, the attached diff makes it work on all screens as originally intended, with mobile and secondary toolbar support:

If it overflows (not shown in the images) then the secondary toolbar will be slightly moved upwards.
Unfortunately, because we're working with absolute alignment, the rest won't flow with it.

But, it's much better (and at the very least usable again) compared to before.

http://i.imgur.com/jg57Fv7.jpg
http://i.imgur.com/ksfDyv8.jpg
http://i.imgur.com/57XlGYU.jpg
http://i.imgur.com/AQ8exgf.jpg

Last edited 7 years ago by Cybr (previous) (diff)

#22 @sagarprajapati
7 years ago

Hi @Cybr

Patch work well when button is not too wide. when button is wide it will go up in the iPhone 5. Please check screenshot.

http://imgur.com/mzZf3FQ

http://imgur.com/a/BWgdA

Thanks

#23 @Cybr
7 years ago

@sagarprajapati Yes! Exactly what I meant with:

If it overflows (not shown in the images) then the secondary toolbar will be slightly moved upwards.

But it also affects the primary bar :)

There just isn't enough space.

And because of the absolute values, the bottom bar has to be expanded vertically and statically (media queries) on mobile to support the occasional i18n hick-up.
Alternatively, it requires a JS overflow listener to adjust the heights. Or, even a complete rewrite from absolute values to real flow.

I don't believe we can get closer to a resolution than this without doing any of the above.

Last edited 7 years ago by Cybr (previous) (diff)

#24 @sagarprajapati
7 years ago

Hi @Cybr and @swissspidy

I have updated the patch and now if the text of the button is larger then the first will move up with proper spacing.

http://imgur.com/a/IH3gy
http://imgur.com/a/aLhMs
http://imgur.com/a/DCzNz

Thanks

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


7 years ago

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


7 years ago

@sagarprajapati
7 years ago

Remove unnecessary space from the code

#27 @mayurk
7 years ago

  • Keywords has-screenshots added

Hi All,

I have tested latest patch on the following devices and browsers. I have also attached screenshot for review. Please check it.

1) Android Firefox
2) Android Chrome
3) Ipad Chrome
4) Ipad Firefox
5) Iphone6 Chrome
6) Iphone6 Firefox

Screenshots:

https://www.dropbox.com/s/xu0q0ybbwuv87ru/android%20chrome.png?dl=0
https://www.dropbox.com/s/w58c0umkmk3400d/android%20firefox.png?dl=0
https://www.dropbox.com/s/522a29ggwxqp5yi/2017-04-03.png?dl=0
https://www.dropbox.com/s/i0yoi4doh7lwhzf/2017-04-04.png?dl=0
https://www.dropbox.com/s/2pe7s4nmrlpbtjm/2017-04-06.png?dl=0
https://www.dropbox.com/s/qhf86800j9t8doh/2017-04-07.png?dl=0

Thanks

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


7 years ago

#29 @mayurk
7 years ago

  • Keywords has-patch has-screenshots removed

Hello,

I just checked latest patch on the browserstack tool & it's completely working fine. I am still ready for any suggestion and ready for any help.

#30 @mayurk
7 years ago

  • Keywords has-patch has-screenshots added

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


7 years ago

#32 @kirasong
7 years ago

40152.3.patch fixes the issue in Chrome.
Tested with 2012 and header image selection/crop in resized window.

@swissspidy
7 years ago

#33 @swissspidy
7 years ago

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

In 40428:

Media: Ensure Crop Image is always visible.

Previously, the crop button in the media modal after uploading header images or similar was hidden and the task could not be completed.

Props karinedo, sagarprajapati, Cybr, mayurk.
Fixes #40152.

#34 @swissspidy
7 years ago

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

#35 @swissspidy
7 years ago

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

In 40429:

Media: Ensure Crop Image is always visible.

Previously, the crop button in the media modal after uploading header images or similar was hidden and the task could not be completed.

Props karinedo, sagarprajapati, Cybr, mayurk.
Fixes #40152.

Merges [40428] to the 4.7 branch.

#36 @afercia
4 years ago

Noticed that [40428] added a height: auto; without removing a height: 60px; already existing in the same ruleset. The added property just overrides the previous one (which should have been removed).

P.S. Same for the bottom value:
https://core.trac.wordpress.org/browser/trunk/src/wp-includes/css/media-views.css?rev=46866&marks=745-754#L744

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

#37 @afercia
4 years ago

In 47266:

Media: Fix bottom spacing on various Media Modal elements for non-webkit browsers.

Implementation of bottom padding in overflow content differs across browsers. See https://github.com/w3c/csswg-drafts/issues/129. To make bottom spacing consistent across browsers there's the need for an alternate CSS method.

  • uses a CSS after pseudo element or simply a bottom margin to reserve some bottom spacing
  • removes a couple leftovers from [40428]
  • fixes an annoying visual glitch where the media modal content is visible behind the bottom toolbar border

Props sabernhardt, afercia.
See #40152.
Fixes #48378.

Note: See TracTickets for help on using tickets.