Support from Acquia helps fund testing for Drupal Acquia logo

Comments

ksenzee’s picture

Status: Active » Needs review
FileSize
5.54 KB

I'm attaching a patch that should take care of this. The JavaScript is a little more indirect than you would expect. The server-side code knows that it's thrown an error, and that the tab it provides needs to be active on the next page load. It can pass that information to the client via Drupal.settings. But the jQuery UI Tabs library isn't really set up to select a tab via a setting; it wants the URL hash to be the title of the tab that should be preselected as active. We can't do that. So we have to use the ugly backdoor API, which is specifying the integer index of the tab to select (i.e. "select the second tab in the list"). Hence the somewhat ugly JS.

Dave Reid’s picture

Hrm, wondering what happens if you submit the 'Web' tab with an error, then then instead of fixing it, go to another tab 'Upload' and submit it without selecting a file? What tab does it go to then?

ksenzee’s picture

Well. Two problems there:

1. I forgot to update the 'Upload' plugin in my 2.x version of the patch, so it wasn't setting itself to active after an error. (Yes, I'm still writing 1.x patches first. :/ )

2. I didn't realize form_get_errors() was as stupid (i.e. global) as it is. Apparently we need a helper function that iterates over the form to check every field for errors.

ksenzee’s picture

Here's a patch for 1.x if that's useful at this point.

Status: Needs review » Needs work

The last submitted patch, 1307596-4.browser-validation-behavior-1.x.patch, failed testing.

jarrodirwin’s picture

Status: Needs work » Needs review
idflood’s picture

Status: Needs review » Needs work
FileSize
7.05 KB

Here is a reroll of patch in #3, one function was in a different file. I've tested it a little and it worked nicely.

idflood’s picture

Status: Needs work » Needs review
Dave Reid’s picture

Issue tags: +needs backport to 1.x
dddave’s picture

Status: Needs review » Needs work
Issue tags: +needs backport to 1.x

The last submitted patch, 1307596-6-browser-validation-behavior-2.x.patch, failed testing.

katbailey’s picture

Here's a reroll of the 1.x version

Dave Reid’s picture

Issue tags: +sprint, +Media Initiative
raytiley’s picture

Here is a patch for the 2.x version that fixes the tab selection on validation errors only changing the javascript, not touching the form api or anything.

raytiley’s picture

Status: Needs work » Needs review
raytiley’s picture

Fixing for coding standards in above patch.

raytiley’s picture

This patch also applies cleanly to 1.1 and fixes the validation tab selection problem.

Dave Reid’s picture

I'm happy with just simple JavaScript for this for now. I'll keep this open as I'd like to be able to set the default tab programatically, or a better solution either in media.browser.inc or a potential MediaBrowserFormPlugin() that could automatically do the backend-processing we're seeing in this issue, but on a more generic basis.

Ray and I also discussed that there's an edge-case if you cause errors on multiple tabs (one after the other). And that it would also possibly be nice if the Tabs themselves were marked with an error star to indicate an error was visible in that tab. We can leave those as followups.

Dave Reid’s picture

Status: Needs review » Needs work

I committed #16 to both 7.x-2.x and 7.x-1.x. Thanks everyone for the great work so far!
http://drupalcode.org/project/media.git/commit/4730421
http://drupalcode.org/project/media.git/commit/536c100

gbernier’s picture

Hey Guys,

Noticing with modules which have their own Media Browser plugins and search forms, such as YouTube, when you do the search or the pagination it doesn't take you page to the proper tab either. This seems to be related to the same issue, we are using 7.x-2.0-unstable6+9-dev Media currently

mrfelton’s picture

mrfelton’s picture

This no longer works. Error messages are rendered at the top of the media browser page as opposed to within the individual tabs, so trying to work out which tab the errors relate to based om their position in the DOM doesn't work.

mrfelton’s picture

Status: Needs work » Needs review
FileSize
1.84 KB

Actually, there were 2 parts to my problem.

1) I had the clientside_validation module enabled, it was causing an empty div with the class .error to be injected into every tab of the media browser. This screwed up the js from the previous patches, because it would interpret this an error en every tab, and thus switch to one of them when the form was submitted, or an ajax widget was used (like the file upload button on the upload tab).

2) There is a bug in the media_internet_sources code that sets a form error on the wrong element. It sets the error on an element called url but there is no such element. The form item is actually called embed_code. This means that when there was an error using the internet sources tab, the offending field woud not be given the .error class, causing the js here to fail to work out which tab the error was in.

Attached patch resolves the issue in 2), correct the form_set_error() calls so that they set the error on the right element.

To fix 1), I ended up just excluding media/browser from clientside_validation. But I thought this may also be fixable by changing:

var errorTabID = $('#media-browser-tabset')
    .find('.error')
    .parents('.media-browser-tab')
    .attr('id');

to something like:

var errorTabID = $('#media-browser-tabset')
    .find('.error:visible')
    .parents('.media-browser-tab')
    .attr('id');
ParisLiakos’s picture

Status: Needs review » Needs work

Thanks, i tested it manually and works.
Committed the patch:)
http://drupalcode.org/project/media.git/commit/5977414
Back to needs work according to #18

Also committed to 1.x since there was the same issue there as well
http://drupalcode.org/project/media.git/commit/f8bfe9a

JBodkin’s picture

Same here, I had clientside validation enabled, I added "media/*" to the exclude path in the module configuration and it seems to be working correctly.

Devin Carlson’s picture

Status: Needs work » Fixed

This should be fixed with #2108647: Array.indexOf doesn't exist!.

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.