WordPress.org

Make WordPress Core

Opened 20 months ago

Closed 11 days ago

Last modified 5 days ago

#21785 closed task (blessed) (fixed)

Add header image uploads with cropping to the customizer

Reported by: nacin Owned by:
Milestone: 3.9 Priority: normal
Severity: normal Version: 3.4
Component: Appearance Keywords: has-patch
Focuses: javascript, administration Cc:

Description

See #21355 for an explanation for why header image uploads (sans cropping) was removed from the 3.4 branch. This ticket is about adding it back, with a crop step, to ensure proper support.

Attachments (31)

21785-media-manager.diff (8.3 KB) - added by ehg 8 weeks ago.
21785-customizer-header.diff (68.7 KB) - added by ehg 8 weeks ago.
21785-tests.diff (162.0 KB) - added by ehg 8 weeks ago.
close-button.light.png (1.9 KB) - added by ehg 8 weeks ago.
21785-media-manager.2.diff (8.2 KB) - added by ehg 7 weeks ago.
21785-customize-header.2.diff (48.2 KB) - added by ehg 7 weeks ago.
21785-tests.2.diff (162.6 KB) - added by ehg 7 weeks ago.
21785-03.patch (219.0 KB) - added by gcorne 6 weeks ago.
21785-04.patch (219.2 KB) - added by gcorne 6 weeks ago.
21785-05.patch (221.7 KB) - added by DH-Shredder 6 weeks ago.
Code formatting cleanup; mostly brackets.
21785-06.patch (221.6 KB) - added by gcorne 6 weeks ago.
21785-move-query-logic.diff (7.9 KB) - added by mcsf 5 weeks ago.
21785-doCrop-move.diff (1.7 KB) - added by ehg 5 weeks ago.
21785-fix-randomize.diff (1.5 KB) - added by mcsf 4 weeks ago.
21785-doCrop-move.2.diff (2.7 KB) - added by ehg 3 weeks ago.
updated doCrop refactor
21785-dashicons-randomize.diff (1.6 KB) - added by mcsf 3 weeks ago.
21785-suggested-dimensions.diff (4.6 KB) - added by ehg 3 weeks ago.
21785-mm-crop-error-button.diff (1.5 KB) - added by ehg 3 weeks ago.
consolidated-query-logic-1.diff (2.0 KB) - added by mcsf 3 weeks ago.
consolidated-query-logic-2.diff (2.5 KB) - added by mcsf 3 weeks ago.
21785-mm-crop-error-overlay.diff (3.8 KB) - added by ehg 3 weeks ago.
overlay error on cropping fail
21785-accessibility.diff (3.8 KB) - added by ehg 2 weeks ago.
a11y-ui-improvements.diff (11.0 KB) - added by mcsf 2 weeks ago.
a11y-ui-improvements-2.diff (13.1 KB) - added by mcsf 2 weeks ago.
a11y-ui-improvements-3.diff (13.1 KB) - added by mcsf 2 weeks ago.
21785-suggested-dimensions.4.diff (4.2 KB) - added by ehg 2 weeks ago.
21785.suggested-dimensions.5.diff (4.3 KB) - added by nacin 2 weeks ago.
21785-a11y-ui-improvements.2.diff (2.8 KB) - added by ehg 2 weeks ago.
21785-doCrop-move.3.diff (3.0 KB) - added by gcorne 12 days ago.
21785.suggested-dimensions.6.diff (4.9 KB) - added by gcorne 12 days ago.
fix-new-image-ordering.diff (377 bytes) - added by mcsf 9 days ago.

Download all attachments as: .zip

Change History (119)

comment:2 webord19 months ago

  • Cc bordoni.dev@… added

comment:3 webord19 months ago

I was looking at the code on the Custom Header, the usage of steps on this page feels wrong;

We have the power to put all this stuff at the Media Box, in a way that the user have the same experience of cropping that he would have if he was inserting an image to the post.

Even if this is not the approach we take, at least the steps should have a POST then GET, at every step, on the second step you get a duplicate of your image if you reload the page, then in the third step you will get a duplicated crop.

I would love to see this page rebuild from scratch, maintaining backwards compatibility but still rebuilt.

Not saying that this should be on 3.5 but...

comment:4 goto1018 months ago

  • Cc dromsey@… added

comment:5 jazbek18 months ago

  • Cc j.yzbek@… added

comment:6 nacin18 months ago

  • Keywords 3.6-early added
  • Milestone changed from 3.5 to Future Release
  • Type changed from defect (bug) to task (blessed)

comment:7 kwight17 months ago

  • Cc kwight@… added

comment:8 lancewillett12 months ago

  • Cc lancewillett added

comment:9 stiofansisland11 months ago

  • Cc stiofansisland added

comment:10 dreamwhisper6 months ago

  • Cc dreamwhisper added

comment:11 westonruter6 months ago

  • Cc weston@… added

comment:12 celloexpressions3 months ago

  • Focuses ui added
  • Keywords 3.6-early removed

Perhaps the work around adding image editing to the media modal in 3.9 could be extended to fix this too. This bug has been around for far too long, and blocks #25571.

We should use the media modal for the entire upload process so that users can select existing images from the media library or upload a new file, before moving to a crop step, along the same lines as #21483.

comment:13 follow-up: helen3 months ago

  • Focuses javascript added; ui removed

Removing ui focus here, too; adding javascript. Yes, it affects UI, but that's not really the focus here.

comment:14 in reply to: ↑ 13 celloexpressions3 months ago

Replying to helen:

Removing ui focus here, too; adding javascript. Yes, it affects UI, but that's not really the focus here.

Actually, wouldn't both make sense, since we need to design a new UI and it'll be implemented with some JS wizardry (presumably in the media modal)?

Also, per something that came up in #21483, we might want to consider moving the "uploaded headers" section into the modal as well, if that's how this is implemented. Alternatively something inline could work, but given the width of the customizer controls that could be tricky. Ideally users could select images from the media library as well, but the cropping implementation approach will determine whether that gets fixed in this ticket.

Unfortunately, too much has changed since this was first opened for there to be a clear assumption of how the workflow for uploading/selecting and cropping headers in the customizer should work; if anyone has suggestions that might be a good first step toward finally making this happen.

comment:15 nacin2 months ago

I seem to recall someone doing work on this, perhaps a plugin somewhere?

comment:16 obenland2 months ago

@ehg and @miguelcsf are working on it in #26505. We just talked about the current status two weeks ago, as a matter of fact, and they will spend the coming week on getting the .com implementation ready for core (I've been told).

comment:17 follow-up: mcsf2 months ago

What Konstantin said. It currently works as a core plugin, although @ehg and I need to sort out the odd bug or two, as well as decouple it from our cropping extension of the Media Manager.

comment:18 in reply to: ↑ 17 nacin2 months ago

Replying to mcsf:

What Konstantin said. It currently works as a core plugin, although @ehg and I need to sort out the odd bug or two, as well as decouple it from our cropping extension of the Media Manager.

If it can be done in the next week or so, I think this would be fantastic to add to 3.9.

comment:19 ircbot8 weeks ago

This ticket was mentioned in IRC in #wordpress-dev by mcsf. View the logs.

ehg8 weeks ago

ehg8 weeks ago

ehg8 weeks ago

ehg8 weeks ago

comment:20 ehg8 weeks ago

#26505 was marked as a duplicate.

comment:21 mcsf8 weeks ago

These patches add cropping support to a newly developed Header Image control in the Customizer. This is our first pass at merging the plugin we developed into core, we’re sure there are things to be added/corrected/debated — feedback would be greatly appreciated.

— Brought to you by team @ehg & @mcsf

21785-media-manager.diff

  • Adds a Cropper controller and view to the Media Manager + L10n
  • Allows us to add HTML to the title/minimised menu item title

21785-customizer-header.diff

  • Replaces WP_Customize_Header_Image_Control from scratch using:
    • Underscore templates;
    • Extended backend logic to keep track of and remove recently used image.
  • Adds a wp.customize.HeaderControl, which leverages:
    • The new cropping feature in the Media Manager;
    • The existing imgAreaSelect plugin to select an area of the image to crop;
    • Backbone views, models and collections for header images.
  • Refactors wp-admin/custom-header.php and adds 3 AJAX endpoints to it for adding, removing and cropping images.
  • Adds a lib, jquery-slimscroll, which we use to scroll uploaded header images in a limited height container.
  • Adds relevant CSS and a PNG image (see below).

21785-tests.diff

  • Adds sinon.js for mocking/stubbing/spying for qunit js tests.
  • Adds tests & fixtures for the Backbone header models/collections.
  • Adds phpunit tests for header dimension calculations & extracted custom-header.php functions.

src/wp-admin/images/close-button.light.png

  • The remove image button for the header control.

ehg7 weeks ago

ehg7 weeks ago

comment:22 mcsf7 weeks ago

Updated patches 21785-media-manager.2.diff, 21785-customize-header.2.diff and 21785-tests.2.diff address feedback from @nacin and @ocean90, namely:

  • Replaced image (the "close" PNG) with Dashicon
  • Ditched slimScroll
  • AJAX: Added missing capabilities check
  • AJAX: action adds alternatively use $this and __CLASS__
  • L10n: Added missing l10n for "No image set" placeholder
  • CSS: Removed wpcom-ism (body:not(.blue))
  • JS: Moved Header views and models from wp-admin to wp-includes
  • JS: Ditched the template in wp.media.view.Cropper
  • PHP: In WP_Customize_Header_Image_Control::render_content: braces around if and else blocks.
  • PHP: Use wp_die over die
  • Tests: have test classes extend WP_UnitTestCase
  • Tests: Move JS test fixtures to index.html
  • Check that JSHint rules are followed
  • (not from feedback) Cleanup: Removed leftover parent attachment logic from previous feature

For reference, we're doing development at github:ehg/wordpress-develop; use /commits/dev?author=AUTHOR with AUTHOR in (ehg|mcsf) for granular commits.

comment:23 ircbot7 weeks ago

This ticket was mentioned in IRC in #wordpress-dev by ocean90. View the logs.

gcorne6 weeks ago

comment:24 gcorne6 weeks ago

I spent some time playing around with this and looking at the code and fixed up the following small issues in 21785-03.patch.

  • Unbound the resize event handler when the Cropper view is removed
  • Extend WP_Customize_Image_Control instead of WP_Customize_Control so properties such as get_url are declared before the class is instantiated.
  • Switched the "Skip Cropping" to using the secondary button style instead of primary.
  • Cleaned up some whitespace issues.

For fun, I also pushed these changes to a branch on Github: https://github.com/gcorne/develop.wordpress/tree/trac-21785-03

One thing that I didn't fix that I think we should address is how the new ajax handlers work. Instead of sending a string when an error occurs, we should be using wp_json_send_error and wp_json_send_success. Right now, if there is an error when cropping, JSON.parse() throws an error and the modal is stuck until the user closes it.

I also wonder if we should change the terminology used for the buttons in the customizer. "Hide" and "Add new" don't feel quite right. I was thinking maybe it should be "Add" when there isn't a header image set, "Remove" and "Replace" when there is a single image set, and "Remove" and "Add" when the header image is set to randomize.

comment:25 nacin6 weeks ago

  • Milestone changed from Future Release to 3.9

comment:26 ircbot6 weeks ago

This ticket was mentioned in IRC in #wordpress-dev by nacin. View the logs.

comment:27 ircbot6 weeks ago

This ticket was mentioned in IRC in #wordpress-dev by nacin. View the logs.

gcorne6 weeks ago

comment:28 gcorne6 weeks ago

21785-04.patch refreshes the patch.

While testing, I realized that it would be nice to cancel the cropping step and go back to the browser to select a different image. Add some sort of cancel, might be something worth adding in a future iteration.

comment:29 ehg6 weeks ago

@gcorne thanks for this, and sorry for the delay!

We were actually looking into wp_json_send_*, but were unsure on how to present an error to the user. Any ideas?

Also, we originally had “Remove” in place of “Hide”, but we got some beta user feedback that “Remove” could be mistaken for “Remove image from media manager”. As for “Replace/Add” — we haven’t encountered any feedback that implies confusion over our existing “Add new” button so far, with just under 2 million cropped images since December — but we could be wrong. ;)

DH-Shredder6 weeks ago

Code formatting cleanup; mostly brackets.

comment:30 DH-Shredder6 weeks ago

  • Focuses administration added
  • Keywords has-patch added

This looks good! With testing so far, functions as advertised.

As I was reading through, went ahead and added a bit of code cleanup (in the patch attached) -- mostly brackets -- to what was added, and a couple of the functions changes were made in.

Before release, also could use hook docs.

gcorne6 weeks ago

comment:32 nacin6 weeks ago

In 27497:

Add header image uploads with cropping to the customizer.

props mcsf, ehg, gcorne.
see #21785.

comment:33 nacin6 weeks ago

In 27498:

JS linting for [27497].

see #21785.

comment:34 j-falk5 weeks ago

I love this new feature, exactly the easy workflow with a already set aspect ratio set and the user just postion and resize how they want it to be cropped.

But I really think it would be even better to also use it with featured images on pages. Then it would be so simple to get them cropped correctly which today is a bit complicated.

Maybe that is already possible to implement, maybe there are already plugins for this and maybe this is totally the wrong forum for this kind of comment...

comment:35 nacin5 weeks ago

This is great. Really, I loved it, and it was so well done, too. In [27497], I made a number of changes beyond gcorne's latest patch. Most of them leave things to do:

  • The "Randomize" and "Randomizing" strings were not translatable, as they substituted words in. We always need to be translating full sentences. Additionally, it didn't handle plurals properly, and we don't have the ability to do plurals in JS, so it was removed for the moment. Finally, TODO: we shouldn't show "Randomize" or "Randomizing" when there is only one image — you can't "randomize" one image.
  • I added a final to the WP_Customize_Header_Image_Control class. This class changed significant shape and I wanted anyone who was extending it to get an immediate fatal error in 3.9. TODO: Investigate usage and backwards compatibility concerns. It's possible we just create a new class and empty this one, making it essentially an alias for WP_Customize_Image_Control, which it extends.
  • "Suggested dimensions" was removed as it was in the wrong place. This should be placed elsewhere, probably above "Attachment Details". TODO: Restore "Suggested dimensions", remove Title/Caption/etc. Consider removing "Edit Image", which doesn't make sense here.
  • I changed the XHR stuff to use wp.ajax.post and wp_send_json_*. TODO: All XHR requests need some kind of error handling, ideally. (There are existing tickets about this; we don't handle errors very well in the customizer currently, but there's a lot more things that can fail here.) I also removed the privilege-checking method from the main class, as it was an over-abstraction and hid important security checks under the rug.
  • I just noticed "this string must not be empty" and I'm not sure what's going on there. TODO: Most of that logic should not be in the customizer control, but in the custom header class, so we'll have to figure that out.
  • The dice just isn't clear that it's a dice, not at the font size size I see it in, anyway. (It looks like a broken unicode character.) TODO: We should use a dashicon or nothing here. (It also wasn't rotating for me — is it supposed to?)
  • TODO: doCrop et. al should be moved to the models, and out of the views.

comment:36 ircbot5 weeks ago

This ticket was mentioned in IRC in #wordpress-dev by nacin. View the logs.

comment:37 mcsf5 weeks ago

Nice. ehg and I will work on some of those items. Meanwhile:

TODO: we shouldn't show "Randomize" or "Randomizing" when there is only one image — you can't "randomize" one image.

This has been raised in wpcom and is intentional. The idea was to make the feature self-discoverable—or, rather, self-advertising. It has also been suggested we change the label on the button to something like “Add more images to randomize”, but that would be i18n hell and introduce needless complexity.

I just noticed "this string must not be empty" [...] I'm not sure what's going on there

This is because of #23268, which we found through the Codex: http://codex.wordpress.org/Class_Reference/WP_Query#Custom_Field_Parameters

The dice just isn't [...]

It’s just supposed to gradually change color when you hover over the button. ;) Agreed, though, that we should use a Dashicon. Making it roll would be cool!

mcsf5 weeks ago

ehg5 weeks ago

comment:38 follow-up: ehg5 weeks ago

TODO: Investigate usage and backwards compatibility concerns of WP_Customize_Header_Image_Control class.

I’ll get back to you soon on this - preliminary findings indicate that no one is using the class, but we want to search all plugins and themes for any usage of WP_Customize_Header_Image_Control. I’m currently checking out the plugins and themes repos to search for any mention of the class - opengrok for these repos would be useful, as the repos are rather large!

TODO: Restore "Suggested dimensions", remove Title/Caption/etc. Consider removing "Edit Image", which doesn't make sense here.

Placing the suggested dimensions in the Attachment Details sidebar feels a bit wrong, as the suggested dimensions aren’t particular to a specific image (see https://cloudup.com/iTMNezhnAI6). The idea behind putting them in the Media Manager’s title was to show the dimensions no matter what context you’re in - the suggested dimensions can be seen when you’re uploading *and* selecting an image (see https://cloudup.com/iDXX9n9Busq). An alternative would be to just not show the dimensions in the Media Manager at all, and rely on them being noticed in the Header section.

TODO: All XHR requests need some kind of error handling, ideally.

We’d like to make the case for a better, broader solution to error handling that would go beyond this ticket: adding a Customizer error handler that would display error messages by binding to customize(‘error’) events - see https://cloudup.com/ihRG2lmDTkC . We could also use the proposed error handling in the Customizer to show ‘crop failed’ events from the Media Manager.

TODO: Most of that logic should not be in the customizer control, but in the custom header class, so we'll have to figure that out.

See attachment:21785-move-query-logic.diff. @mcsf “took some liberty” and made sure process_default_headers could only run once. If this is wrong, let us know.

TODO: We should use a dashicon or nothing here.

@melchoyce is on the case :)

TODO: doCrop et. al should be moved to the models, and out of the views.

See attachment:21785-doCrop-move.diff

Aside:
@nacin - was there any reason why the unit tests weren’t committed? Do they need more work?

comment:39 nacin4 weeks ago

In 27620:

More translation cleanups.

Affects widgets (see #27112), custom headers (see #21785), theme installer (see #27055, reverts [27614]), and some media stuff. Untranslates some complicated strings that need additional study.

see #27453.

mcsf4 weeks ago

comment:40 in reply to: ↑ description mcsf4 weeks ago

Included attachment:21785-fix-randomize.diff which fixes an issue introduced in [27497] where:

  • The labels for the "Randomize" buttons (as well as the "Randomizing" label in the Current Header preview) would not show, thus making the color-changing dice a very lonely one
  • The overlay close button on the corner of each user-uploaded image would not show

I believe that, following some translation-related fixes, nacin wanted to remove the "nImage" property instead of "type" in wp.customize.HeaderTool.ChoiceView.prototype.extendedModel. A delete-typo, if you will.

comment:41 in reply to: ↑ 38 ; follow-up: nacin4 weeks ago

Replying to ehg:

Placing the suggested dimensions in the Attachment Details sidebar feels a bit wrong, as the suggested dimensions aren’t particular to a specific image (see https://cloudup.com/iTMNezhnAI6). The idea behind putting them in the Media Manager’s title was to show the dimensions no matter what context you’re in - the suggested dimensions can be seen when you’re uploading *and* selecting an image (see https://cloudup.com/iDXX9n9Busq). An alternative would be to just not show the dimensions in the Media Manager at all, and rely on them being noticed in the Header section.

I get why you'd want them to be shown during upload — at that point, perhaps it would be appropriate to include it actually in the uploader instructions. As in, near "Maximum upload file size:", where allowed file types would be listed, etc.

My point was that the suggested dimensions should appear *above* the Attachment Details sidebar, such as how gallery settings are rendered. Likewise, we don't want/need captions or other fields here.


We’d like to make the case for a better, broader solution to error handling that would go beyond this ticket: adding a Customizer error handler that would display error messages by binding to customize(‘error’) events - see https://cloudup.com/ihRG2lmDTkC . We could also use the proposed error handling in the Customizer to show ‘crop failed’ events from the Media Manager.

Yeah, I think that makes sense. For the moment, we just need to make sure that if/when an XHR fails, things don't go completely haywire. (No JS failures, etc.)


was there any reason why the unit tests weren’t committed? Do they need more work?

Omitting them wasn't intentional. I'll get them in. Was there a decision on sinon?

comment:42 nacin4 weeks ago

In 27688:

Customizer header images: Fix randomizing headers.

props mcsf.
see #21785.

comment:43 ocean903 weeks ago

In 27705:

Update Dashicons.

  • Includes new Media icons, see #26650.
  • Includes icons for clipboard, megaphone, schedule, tickets, nametag, universal-access, microphone and <3.
  • Includes icon for randomize, see #21785.
  • Updates Google+ icon, fixes #26851.

props melchoyce, empireoflight.
see #26936.

ehg3 weeks ago

updated doCrop refactor

comment:44 mcsf3 weeks ago

attachment:21785-dashicons-randomize.diff makes use of the new Randomize dashicon (added in [27705])

comment:45 ehg3 weeks ago

attachment:21785-doCrop-move.2.diff - updated doCrop -> Attachment model refactor as per gcorne and nacin's feedback.

Also, I was wondering whether there's a better way to get the lastState's selection, rather than doing

this.set('selection', new media.model.Selection(this.frame._selection.single));
Last edited 3 weeks ago by ehg (previous) (diff)

comment:46 ehg3 weeks ago

attachment:21785-suggested-dimensions.diff is a WIP patch that replaces the sidebar attachment details with a theme suggested dimensions view.

I'm still not sure if it's worth doing this, as it's a significant patch for what I perceive as little value, with a bit of a weird UX. However, I could be Doing It Wrong™ :)

In the patch:

  • New media.view.Attachment.SuggestedDimensions that renders a new crop-suggested-dimensions template, which is based on a cut down attachment-details template.
  • Render the SuggestedDimensions view if crop options are supplied to wp.media(). I'm setting the cropOptions in the attachment model here, which feels wrong.

comment:47 in reply to: ↑ 41 ehg3 weeks ago

Replying to nacin:

We’d like to make the case for a better, broader solution to error handling that would go beyond this ticket: adding a Customizer error handler that would display error messages by binding to customize(‘error’) events - see https://cloudup.com/ihRG2lmDTkC . We could also use the proposed error handling in the Customizer to show ‘crop failed’ events from the Media Manager.

Yeah, I think that makes sense. For the moment, we just need to make sure that if/when an XHR fails, things don't go completely haywire. (No JS failures, etc.)

Apart from the Media Manager's "Cropping..." button being stuck on an error, the two other XHR requests (header_add/header_remove) will just silently fail on errors.

attachment:21785-mm-crop-error-button.diff (depends on attachment:21785-doCrop-move.2.diff) changes the "Crop" button to "Error? Retry" on an error - pic: https://cloudup.com/cb4uy8jfehO

The button in its error state is clickable, so users can retry. I'm assuming most errors will be network/server related - so this could be a reasonable approach.

Last edited 3 weeks ago by ehg (previous) (diff)

comment:48 ircbot3 weeks ago

This ticket was mentioned in IRC in #wordpress-dev by ehg. View the logs.

comment:49 nacin3 weeks ago

In 27722:

Use dashicons in header images in the customizer.

props mcsf.
see #21785.

comment:50 ircbot3 weeks ago

This ticket was mentioned in IRC in #wordpress-dev by nacin. View the logs.

comment:51 nacin3 weeks ago

In 27846:

JavaScript Unit Tests: Add Sinon (sinonjs.org) for test mocks, stubs, etc. see #21785.

comment:52 nacin3 weeks ago

In 27847:

Add PHP and JS unit tests for custom headers.

props mcsf, ehg.
see #21785.

comment:53 nacin3 weeks ago

In 27849:

Custom Headers: Simplify and consolidate the querying of custom headers for the customizer.

props mcsf.
see #21785.

comment:54 follow-ups: nacin3 weeks ago

  • 21785-doCrop-move.2.diff — Leaving this uncommitted pending a review from gcorne. He indicated to me it would make sense if doCrop then refreshed the model's attributes; there is also a question in comment:45 about referencing a selection.
  • 21785-suggested-dimensions.diff — I agree, mhg, that this is not ideal. Sorry for sending you on that goose chase. One of the reasons the original suggested dimensions didn't get committed is that it had this pretty weird hack with regards to the media manager title. I'm fine with the design, but I'd want to inject that view in another way.
  • 21785-mm-crop-error-button.diff — Nice! Chances are a crop isn't a transient failure, so I'm not sure how much a retry will help, but we have very little recourse here. Asking others to weigh in to see if anyone else can think of an existing paradigm we should otherwise copy.
  • I'm curious about whether get_default_header_images() needs to do all of that work. A default image is not allowed to be selected on Appearance > Header unless it is also a default header. I don't want to condone the practice of registering a default image *and* other default headers *and* not specifying the former with the latter. It doesn't work now, I'd like to leave it that way. It would still appear as the "Current Header" but wouldn't show up in "Suggested". If the issue is there is no "Restore Original" button, then OK, I can see that.
  • I have been using the UI more and more over the last few days and I am going to try to bring back the tabs and see how they work. I discussed this a bit in IRC mostly as it relates to "Randomize" and "Randomizing" being too dark and not really feeling like buttons (like one of them is) or a current state (like the other is). That's not a big deal, but I'm also concerned about all of the jumping around that occurs. It's very jarring. If you click "Randomize" things jump up top. If you click a suggested header when you already have a suggested header, the one you clicked suddenly vanishes and another takes its place. Sometimes you get jumped up (on purpose), but that's not a good experience either. I'm going to try to re-implement the tabs with some kind of a checkbox or link for Randomize to see if it feels better.

comment:55 in reply to: ↑ 54 ; follow-up: ocean903 weeks ago

Replying to nacin:

  • 21785-mm-crop-error-button.diff — Nice! Chances are a crop isn't a transient failure, so I'm not sure how much a retry will help, but we have very little recourse here. Asking others to weigh in to see if anyone else can think of an existing paradigm we should otherwise copy.

I think it should be more like these errors: https://cloudup.com/c4LTYurtj7z
The image should be replaced by the error message with a link to retry. The Crop button should be disabled when an error exists.

comment:56 nacin3 weeks ago

The error ocean90 references can also appear in the sidebar, if you're on the library view.

ehg3 weeks ago

overlay error on cropping fail

comment:57 in reply to: ↑ 55 ehg3 weeks ago

Replying to ocean90:

Replying to nacin:

  • 21785-mm-crop-error-button.diff — Nice! Chances are a crop isn't a transient failure, so I'm not sure how much a retry will help, but we have very little recourse here. Asking others to weigh in to see if anyone else can think of an existing paradigm we should otherwise copy.

I think it should be more like these errors: https://cloudup.com/c4LTYurtj7z
The image should be replaced by the error message with a link to retry. The Crop button should be disabled when an error exists.

attachment:21785-mm-crop-error-overlay.diff overlays the error message on top of the image. I avoided a 'retry' link, to keep the patch minimal.

Pic: https://cloudup.com/cb02wO5skg0

It:

  • Uses a template for Cropper view, rather having it as an img element, so we can add more elements to it in a clean way.
  • Re-uses media.view.UploaderStatusError to overlay cropping errors onto the image
  • Fixes an issue where imgAreaSelect was referencing the wrong parent element - which lead to weirdness with z-indexes in the crop-content template.
Last edited 3 weeks ago by ehg (previous) (diff)

ehg2 weeks ago

comment:58 ehg2 weeks ago

attachment:21785-accessibility.diff implements accessibility suggestions from #27598

  • Convert Add/Hide <a> buttons to <button> buttons
  • Rename "Hide" to "Hide image" and "Add new" to "Add new image"
  • Convert image <a>s into <button>s and add screen reader text
  • Use alternate text meta data for uploaded image alt text
  • Use suggested header names for suggested image alt text

mcsf2 weeks ago

comment:59 follow-ups: mcsf2 weeks ago

attachment:a11y-ui-improvements.diff includes ehgy's accessibility changes (see above), as well as UI improvements as discussed in IRC, namely:

  • Prefer a lighter UI.
  • Make the UI more coherent with the rest of the Customizer (button style, borders).
  • Don't hide the selected header, but rather highlight it. Adjust logic accordingly.
  • Stop the Customizer controls from jumping around when selecting a header.

Plus:

  • Fix ordering of headers by defaulting timestamps to 0.
  • Force height for Randomize buttons instead of inferring it from other Header views.
  • Some removal of code.

comment:60 in reply to: ↑ 59 davidakennedy2 weeks ago

Thanks for the quick patches ehg and mcsf! I'll test this and anything else added from an accessibility perspective tomorrow, and provide any feedback.

comment:61 in reply to: ↑ 54 ehg2 weeks ago

Replying to nacin:

  • 21785-suggested-dimensions.diff — I agree, mhg, that this is not ideal. Sorry for sending you on that goose chase. One of the reasons the original suggested dimensions didn't get committed is that it had this pretty weird hack with regards to the media manager title. I'm fine with the design, but I'd want to inject that view in another way.

Yep, the hack was necessary because the title isn't easily as easily editable as the rest of the MM views, such as the toolbar and content. We could turn the titlebar itself into a region, to give us cleaner code and more flexibility in the future - but this would involve significant changes, which probably aren't appropriate at this time in the release cycle.

Should we just rely on the suggested dimensions being in the Header control for now, and revisit this for a later release?

comment:62 ircbot2 weeks ago

This ticket was mentioned in IRC in #wordpress-ui by davidakennedy. View the logs.

comment:63 ircbot2 weeks ago

This ticket was mentioned in IRC in #wordpress-dev by ehg. View the logs.

comment:64 in reply to: ↑ 59 davidakennedy2 weeks ago

Replying to mcsf:

attachment:a11y-ui-improvements.diff includes ehgy's accessibility changes (see above), as well as UI improvements as discussed in IRC, namely:

  • Prefer a lighter UI.
  • Make the UI more coherent with the rest of the Customizer (button style, borders).
  • Don't hide the selected header, but rather highlight it. Adjust logic accordingly.
  • Stop the Customizer controls from jumping around when selecting a header.

Plus:

  • Fix ordering of headers by defaulting timestamps to 0.
  • Force height for Randomize buttons instead of inferring it from other Header views.
  • Some removal of code.

See #27598.

Regarding first issue: The current buttons in the new header image area of the theme customizer are not actual buttons... This works perfectly now and provides much more context to assistive technologies.

Second issue: This type of pattern is even more problematic in the "Randomize uploaded headers" button... This works perfectly now and provides much more context to assistive technologies. However, it looks like there's something off visually for me. This may be an on my end... See the button-like style happening behind each randomized button: https://www.dropbox.com/lightbox/home/Public/WPTicket-27598

Third issue: Also the markup for an uploaded header image that can be clicked to be set needs more context as well... This also works perfectly now and provides much more context to assistive technologies.

Fourth issue: If a user explicitly sets alternative text in the Media Library, that attribute doesn't seem to be displayed in the Theme Customizer Header Image area... The Header Image for Current image does not seem to be getting an alternative text attribute. The other two (Previously uploaded and Suggested) do, and that's great! The Current image header does need one in order for screen reader users to know which image is there. This would be a must have.

Great work ehg, mcsf and others! This also sets us up well for future enhancements here, especially around WAI-ARIA.

Let me know if there are any questions!

mcsf2 weeks ago

comment:65 mcsf2 weeks ago

attachment:a11y-ui-improvements-2.diff is a refresh of attachment:a11y-ui-improvements.diff that addresses some of davidakennedy's latest feedback (thank you!):

  • Add image caption/description to current image's alt text
  • Add some tabindices to possibly help screenreader navigation

It also includes UI tweaks as discussed on irc:

  • Don't bump headers on select
  • Place border over selected header without shrinking it

Props ehg

mcsf2 weeks ago

comment:66 mcsf2 weeks ago

attachment:a11y-ui-improvements-3.diff is a refresh of attachment:a11y-ui-improvements-2.diff that fixes an outline issue within the Randomize button.

comment:67 ircbot2 weeks ago

This ticket was mentioned in IRC in #wordpress-dev by ehg. View the logs.

comment:68 ehg2 weeks ago

attachment:21785-suggested-dimensions.4.diff adds suggested image dimensions to the attachments browser and uploader views.

In the patch:

  • Remove old suggested code
  • Add a crop object to the frame options, which contains the suggested dimensions as well as the imgAreaSelect options
  • Change references to imgSelectOptions
  • Display the suggested dimensions if crop is defined in the frame options
  • Add dimensions to uploader view

Pics:

https://cloudup.com/c5M3XF8YtE6 & https://cloudup.com/cKxy1a0UDJC

comment:69 nacin2 weeks ago

In 27946:

Header images: Handle cropping failures.

props ehg.
see #21785.

comment:70 nacin2 weeks ago

In 27947:

Header images: Accessibility and style improvements. Headers no longer jump around when chosen.

props mcsf, ehg.
see #21785.

comment:71 follow-up: nacin2 weeks ago

21785.suggested-dimensions.5.diff seems fine. I want gcorne to look at this as it changes media API.

What's left here? Anything that still needs to be committed? Anything that still needs to be done?

comment:72 ircbot2 weeks ago

This ticket was mentioned in IRC in #wordpress-dev by nacin. View the logs.

comment:73 ehg2 weeks ago

attachment:21785-a11y-ui-improvements.2.diff fixes the outline issue with the Randomize buttons. We're now directly styling the button.

comment:74 in reply to: ↑ 71 ehg2 weeks ago

Replying to nacin:

21785.suggested-dimensions.5.diff seems fine. I want gcorne to look at this as it changes media API.

What's left here? Anything that still needs to be committed? Anything that still needs to be done?

To be committed:

I don't think there's anything else outstanding.

comment:75 davidakennedy2 weeks ago

I performed a quick retest, and things hold up well. A few points of feedback:

  • The tabindex attributes added here are not needed: https://github.com/ehg/wordpress-develop/commit/87c993635cff79e237732a61b76c06697d620251#commitcomment-5916062 Is there a particular reason why they might be needed I'm missing? The source order here is very logical and everything can be tabbed to naturally so no need for the tabindex.
  • Everything still performs nicely with a screen reader.
  • The buttons that are images do not have the best visual indicator on focus. We've made some improvements with #27173. Can we add some kind of visual indicator here when those elements receive focus?

comment:76 ocean9012 days ago

In 27970:

Header images: Improve accessibility of Randomize buttons.

props ehg.
see #27598, #21785.

comment:77 ocean9012 days ago

In 27971:

Header images: Revert unnecessary tabindex="0" from [27947].

see #21785.

gcorne12 days ago

comment:78 gcorne12 days ago

21785-doCrop-move.3.diff has one small change from 21785-doCrop-move.2.diff. There was one place in createCropperContent where .first() instead of .single() was being called. I think that this latest patch is ready to be committed.

comment:79 gcorne12 days ago

I spent some time looking over 21785.suggested-dimensions.5.diff and started to wonder if it made sense to make suggestedWidth and suggestedHeight a bit more general. I also felt like for at the very least consistency's sake that it made sense for the data to be kept with the state instead of the media frame. This thought process led to 21785.suggested-dimensions.6.diff, which also adds a space between "Suggested image dimensions:" and the actual dimensions in the attachment browser view.

comment:80 ircbot11 days ago

This ticket was mentioned in IRC in #wordpress-dev by nacin. View the logs.

comment:81 nacin11 days ago

In 28030:

Header Images: Add suggested dimensions to the media workflow.

props ehg, gcorne.
see #21785.

comment:82 nacin11 days ago

In 28031:

Header Images: Add suggested dimensions to the media workflow.

This updates [28030] to the latest patch.

props gcorne.
see #21785.

comment:83 nacin11 days ago

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

Wow! Done! Please open new tickets for new issues/improvements.

I'm excited about this. This was a lot of work and it's really paid off — this is a great feature we're shipping! Thank you so much mcsf and ehg for your big contribution. :-)


I went through 21785-doCrop-move.2.diff with gcorne in IRC and we decided not to make the change. Feel free to re-open or comment if you disagree (or agree).

comment:84 nacin11 days ago

In 28039:

Media Templates: More escaping rather than interpolation. see #21785.

mcsf9 days ago

comment:85 ircbot9 days ago

This ticket was mentioned in IRC in #wordpress-dev by mcsf. View the logs.

comment:86 nacin7 days ago

In 28082:

Translate custom header customize control strings. Remove final added before beta to see what breakage we might cause by refactoring this class.

see #21785. fixes #27738.

comment:87 ircbot5 days ago

This ticket was mentioned in IRC in #wordpress-dev by ehg. View the logs.

comment:88 nacin5 days ago

In 28102:

Custom header: Fix image ordering.

props ehg.
see #21785. fixes #27791.

Note: See TracTickets for help on using tickets.