Option to open link in new window

Here's a patch. Not sure yet how to add automated tests; but manual testing suggests this works.

Note its update hook will conflict with the one added in #3077459: Integration with chosen or select2 js; this cannot be avoided until either this issue or 3077459 is committed.

This patch adds some Functional [PHP] tests to verify the HTML output.

I wasn't able to add Functional JavaScript tests for this feature, because the version of the Selenium driver that Drupal core/testbot uses doesn't appear to properly support multiple windows... so the tests failed. Although I turned some of that work into #3163621: Add a FunctionalJavascript test.

Apparently the test error is because Testbot couldn't clone Drupal. I daresay that's not an issue with the code in the patch. I'll try to get Testbot to retry that later.

***

In the meantime, I need to apply both #3077459-9: Integration with chosen or select2 js and this patch, so I'm adding another, 3101397-5--new-window--on-3077459-9.do-not-test.patch patch here which assumes that patch has already been applied. Note the "do-not-test" so it doesn't trigger Testbot. Also note that since this is the same code as #4 (just rebased onto something different), there is no interdiff.

Most of this looks good.

+++ b/js/views-jump-menu.js
@@ -9,7 +9,16 @@
+          window.open(destination, '_blank');

I wouldn't say that this is a critical security issue because it's the same origin, but given the document relationship vulnerability, it would probably be a good idea to update this anyway as "there's still some mischief that can be done" (Use window.open but block use of window.opener).

Here's an updated patch and interdiff; and also another patch on #3077459-12: Integration with chosen or select2 js.

Reviews welcome!

Looks good to me. Thanks!

Here are new patches, rerolled upon the changes in 8a2cc17e.

I had to make a manual merge conflict resolution; see the interdiff.

Okay, here's a patch that should fix the failed test; and updates the code comment to better describe what is being tested.

Reviews are welcome.

The breakage was caused because the change in commit 8a2cc17e made the test a bit more fragile.

The change in commit 8a2cc17e looks like (I've reordered the lines slightly and added an extra newline for clarity)...

-    $testUserOption = $this->xpath('//option[@data-url="' . $testUserViewUrlString . '"]');
+    $testUserOption = $this->xpath('//option[@data-url]');


-    $this->assertGreaterThan(0, count($testUserOption), 'Ensure there is an option with the data-url ' . $testUserViewUrlString);
+    $this->assertEqual($testUserOption[2]->getAttribute('data-url'), $testUserViewUrlString, 'Ensure there is an option with the data-url ' . $testUserViewUrlString);

The old code asserted that there was at least one option with the correct data URL. The new code asserts that the 3rd option in the array of options (i.e.: [2] is the third item after [1] and [0]) has the correct data URL.

This is fragile, because if any of the test's ancestor classes (ViewTestBase, BrowserTestBase, etc.) change the number of users they add in their fixture functions (setUpBeforeClass(), setUp(), or assertPreConditions()), or another test in StyleJumpMenuTest happens to add a user and also run before this test, then the option with the correct URL will switch places, which will break the test again.

The patch in this issue refactored StyleJumpMenuTest, so that instead of both methods adding a user, the setUp function added one user instead, so the user with the data URL we wanted to test went from position 3 to position 2.

I've changed the test in this patch to look at [1], but it might be worth going back to the original test logic (i.e.: that there is at least one option with the correct data URL), and testing the order of the items in the jump menu in a separate class so that (a) you can have tighter control over the order that users are created in, (b) you aren't introducing subtle dependencies into existing test logic, and (c) you can be more explicit about what you're testing, which will make debugging failures in the new class easier.

This is looking good to me. Sorry for taking so long to look at it! Merged in dev. I agree the tests could perhaps be improved in general, but your changes make sense to get them working here.

Thanks very much @bmcclure!