List "Quick edit" before "Configure" in contextual links while in Edit mode

Added initial patch, Worked on JS to prioritize link on toggle of "EDIT", Works fine when we toggle "Edit" mode on page. But there's some issue when we enable "Edit" Mode and reload the page. At that time the "Quick Edit" link is not being set on top.

@webchick && @miteshmap I am not seeing the problem. Maybe I don't understand.
Are you seeing the contextual links actually switch order after you click "edit" in the toolbar? For the contextual links for each block?

I always see the contextual links in the same order with "quick edit" after.

@webchik: For me it's always "configure block" at the top. But yeah your last comment makes it clear that in any case "Quick Edit" should always be on top.

Here is a php attempt. It places all outside-inblock-configure links first by removing it and using array addition. I tried using #weight on the array but it doesn't look like the contextual link render respects it. I didn't dig too deep into that but it appears it could be a bug?

will look into this at drupalcondublin2016

tested this successfully but needed to slightly reroll patch

patch #12 works as expected

We should add a test for this?

I meant to assign to needs work for #14

Ok this adds a test that "Quick edit" shows up first.

Also 2 other fixes.

1. Added a period at the end of the comment
2. Changed to use short array syntax.

TEST_ONLY patch to prove it fails without change.

Just TEST_ONLY patched failed which was expected.

Patch no longer applies because of the c9dbe65 commit. Rerolling updated patch.

Can anyone upload screenshots of before and after for this patch? I do not see any difference in the order of any contextual links when I apply it...

Looks correct to me.

Before

After

Right, thanks @tkoleary!

This patch was already set to RTBC once, it has tests too, all looks good to me.

Please do not RTBC your own patch.

I guess this has tests, though, now.

ok, I'll RTBC it. :)

That's strange. Rerolling the same patch.

Yup, that's the same patch. Thanks!

1. +++ b/core/modules/outside_in/tests/src/FunctionalJavascript/OutsideInBlockFormTest.php
   @@ -74,6 +74,7 @@ public function testBlocks() {
   +    $this->checkQuickEditLinks(array_column($blocks, 'id'));
   
   @@ -173,4 +174,22 @@ protected function openBlockForm($block_selector) {
   +  protected function checkQuickEditLinks($block_ids) {
   

   What is the likelihood of reusing this method? Also, it seems like it has some overlap with the code that follws it in testBlocks(). Rather than looping twice over the blocks, why not add these assertions in the existing loop?

2. +++ b/core/modules/outside_in/tests/src/FunctionalJavascript/OutsideInBlockFormTest.php
   @@ -173,4 +174,22 @@ protected function openBlockForm($block_selector) {
   +      self::assertTrue(count($links) > 1, "Block with id $block_id does not have more than 1 contextual link.");
   

   Doesn't the message here contradict what the assertion is actually asserting?

Thanks!

Postponing this issue until we have fixed #2830485: \Drupal\Tests\outside_in\FunctionalJavascript\OutsideInBlockFormTest fails randomly which marks Drupal\Tests\outside_in\FunctionalJavascript\OutsideInBlockFormTest::testBlocks() as skipped. Until then, this patch cannot be reliably tested.

In the mean time, here is an updated patch that tries to address @xjm's comments in #28.

Instead of postponing this issue and all others involving this test why don't we mark the test as incomplete? \PHPUnit_Framework_Assert::markTestIncomplete allows us to do this.

We can move the new asserts to the top of the loop and then skip over all others. Since our new asserts don't involve clicks or press they shouldn't have the random fails.

I added todo's remove these when #2830485 is fixed

Whoops my bad. fixed patch

1. +++ b/core/modules/outside_in/tests/src/FunctionalJavascript/OutsideInBlockFormTest.php
   @@ -80,6 +80,16 @@ public function testBlocks() {
   +      /** @var \Behat\Mink\Element\NodeElement[] $links */
   

   Should this be commented using the // format? I realise this is ok, but discouraged according to https://www.drupal.org/node/1354#inline

2. +++ b/core/modules/outside_in/tests/src/FunctionalJavascript/OutsideInBlockFormTest.php
   @@ -80,6 +80,16 @@ public function testBlocks() {
   +        self::assertEquals('Quick edit', $links[0]->getText(), "Expected 'Quick edit' to be the first contextrual link of the block with id {$block['id']}, but it wasn't.");
   

   Typo s/contextrual/contextual

@kiwimind thanks for the review!
re 1.
I think this is standard for comments that are type hinting a var using @var. If you search for "/** @var" in core it is pretty universal.

2. Fixed

Confirmed 1

1. +++ b/core/modules/outside_in/outside_in.module
   @@ -34,6 +34,12 @@ function outside_in_help($route_name, RouteMatchInterface $route_match) {
   +    // Place outside in link first.
   

   The module name should either be Settings Tray or outside_in, I think.

2. +++ b/core/modules/outside_in/tests/src/FunctionalJavascript/OutsideInBlockFormTest.php
   @@ -80,6 +80,16 @@ public function testBlocks() {
   +      // @todo: Remove if wait is not needed after https://www.drupal.org/node/2830485 is resolved.
   ...
   +      // @todo: Skip rest of test. Remove when https://www.drupal.org/node/2830485 is resolved.
   

   These don't follow our coding standards. The comments need to wrap at 80 characters. There also should not be a colon after @todo.

3. +++ b/core/modules/outside_in/tests/src/FunctionalJavascript/OutsideInBlockFormTest.php
   @@ -80,6 +80,16 @@ public function testBlocks() {
   +      continue;
   
   @@ -136,6 +147,8 @@ public function testBlocks() {
   +    $this->markTestIncomplete('Test incomplete due to random failures in DrupalCI, see https://www.drupal.org/node/2830485');
   

   Isn't the test already skipped earlier, so this test is still not running? We would need to remove the hunk marking it as skipped also, no?

   Can we re-upload a test-only patch to prove the coverage?

   Also, super-nitpick, but this message contains a comma splice, and I think we can remove the words "in DrupalCI".

Thanks!

@xjm thanks
1. fixed
2. fixed
3. You are right. My intention in #31 was to remove skip and replace with markTestIncomplete. The idea is we don't need to skip the entire test.
I forgot remove the skip.

Uploaded a TEST_ONLY
"super-nitpick". Super fixed

Aaand.. the blocking issue #2830485: \Drupal\Tests\outside_in\FunctionalJavascript\OutsideInBlockFormTest fails randomly is in. @tedbow, we can move back to simpler patch now, I guess.

@20th thanks for reviving this issue!

Yes here is patch that is just re-roll of your patch from #30
It also includes

• fix for #38.1 change to machine name(the rest of #38 review doesn't apply because not skipping test anymore.
• fix for #35.2 spelling fix

Also uploading a test only patch.
Since this was RTBC at #37 I think we should be good.

Actually I hadn't committed the machine name fix for #38.1. Fixed

@tebbow
Maybe it would be better to remove the top blank line added between the 'if' and the comment by:

  */
 function outside_in_contextual_links_view_alter(&$element, $items) {
   if (isset($element['#links']['outside-inblock-configure'])) {
+
+    // Place outside_in link first.
+    $outside_in_link = $element['#links']['outside-inblock-configure'];
+    unset($element['#links']['outside-inblock-configure']);
+    $element['#links'] = ['outside-inblock-configure' => $outside_in_link] + $element['#links'];
+
     $element['#links']['outside-inblock-configure']['attributes'] = [
       'class' => ['use-ajax'],
       'data-dialog-type' => 'dialog',

It is not needed there, I think.

And before someone else pointed this, can you reupload both patch and test-only patch in the same comment to make it easier to find the correct one?

Otherwise, the patch looks good, but because I have worked on it as well, I shouldn't be changing it to RTBC.

OK removed the empty line. also uploaded TEST_ONLY patch

This came up again at the dev days in Seville while reviewing #2784569-7: Settings Tray Accessibility: Improve tabbing.
I consider the approach in this issue actually as a quick fix, we could make the edit mode even more accessible (see linked comment).
I was only able to test this after a re-roll. Patch attached.

Woop, it works!

To be able to test this you need to clear you session/local storage after applying the patch.

All previously open issues with this patch are resolved.
Since the current strategy has already been RTBC-ed before and it still works for 8.4.x we can safely RTBC this again.

@dmsmidt Thanks for the patch. I tested it. And it is working for me. changing status to RTBC.

+++ b/core/modules/outside_in/tests/src/FunctionalJavascript/OutsideInBlockFormTest.php
@@ -73,6 +73,11 @@ public function testBlocks($block_id, $new_page_text, $element_selector, $label_
+    if (count($links) > 1) {

This isn't really needed and could cause the test to break in future

Deleted if (count($links) > 1) { } as per #51

+++ b/core/modules/outside_in/tests/src/FunctionalJavascript/OutsideInBlockFormTest.php
@@ -73,6 +73,8 @@ public function testBlocks($block_id, $new_page_text, $element_selector, $label_
+    /** @var \Behat\Mink\Element\NodeElement[] $links */
+    $links = $this->getSession()->getPage()->findAll('css', "{$block_selector} .contextual-links li a");

This doesn't actually test anything if the assert calls is removed.

Changing the test check that "Quick Edit" is the first link and it point to the correct href(to not be confused with block_content link in the future)
Had to update providerTestBlocks() to send the real block id so the href is easier to test.

Forgot the href won't be exact match because of DrupalCI sub directory

Test fix because a switch statement in \Drupal\Tests\outside_in\FunctionalJavascript\OutsideInBlockFormTest::testBlocks was not updated for the new value of the $block_id parameter.

So, no more CI errors?

Looks good in overall. Thanks for adding the test coverage for this. Committed 0707a70 and pushed to 8.4.x.

@lauriii thanks for committing this! It seems it is big accessibility improvement re: #48

Also I think this can be backported to 8.3.x because this an experimental module we have been able to keep the module exactly the same for 8.3.x and 8.4.x.

Good point @tedbow. Marking for review from another committer.

@lauriii alright thanks

I started #57 testing against 8.3.x so we can confirm it applies and passes.

Yup, let's commit to 8.3.x as well since it's an experimental module :)

It looks to me like DrupalCI marks this as Needs Work because #57 doesn't apply to 8.4.x which it was already committed to.
#57 was also tested against 8.3.x so marking as RTBC again.

Cherry picked 6d1b62d and pushed to 8.3.x.

@lauriii Thanks!

Changing to new settings_tray.module component. @drpal thanks for script help! :)