With JS enabled, the entire Views table is rebuilt, and the cursor is moved to the top of the page.

Splitting off from #1830822-26: Split the Views UI listing into two tables for enabled and disabled.

I can imagine you might want to move the cursor along with the view you enabled because you are likely wanting to edit it further. When disabling a view you might want to stay in the enabled views list because you don't want to do anything further with the disabled view. That would mean that the cursor would always end up somewhere in the enabled views listing. Makes sense?

Why this should be an RC target

Without this patch, using Views will be more difficult as a screen reader users or a keyboard only users . Predictability is really important for blind users and having the cursor location move unexpectedly can be a major disruption.

RC phase evaluation

Reference: https://www.drupal.org/core/d8-allowed-changes
Issue category Bug because the expected behavior is to keep the focused element
Issue priority Normal because it only affects a really small percentage of users
Needed in RC The main goal of this issue is accessibility. (Reduces screen readers pain using the Drupal interface)
Disruption: Fixes a bug, blocks a contributed project, or should be backported to Drupal 7 Not disruptive as it takes care to do nothing in the case the focus were called by the ajax response on purpose.
Files: 

Comments

dawehner’s picture

tim.plunkett’s picture

Well, also because their cursor is inside a div that is completely wiped away by AJAX.

DuaelFr’s picture

Issue summary: View changes

To answer @mgifford from Twitter, this is still an issue for keyboard navigation today.
What would you think about adding a bit of JS to force to refocus the button after it has been moved?

mgifford’s picture

I think adding some JS here to refocus the navigation would be fine. Is this a generalizable problem?

DuaelFr’s picture

@mgifford I agree. See my comment in #1824636-14: Do not move the cursor to the top of the page on ajax calls about how to generalize.

mgifford’s picture

Issue tags: +keyboard focus

Ok, great. So where in core/modules/views_ui/admin.inc would we have to insert it? I assume that's where it would go when the Views table is rebuilt.

DuaelFr’s picture

Tested that issue with the lastest patch in #1824636: Do not move the cursor to the top of the page on ajax calls and it doesn't fix that particular case as the triggering element is a simple link.
I'm going to try to think about an other generic solution but we may have to develop a specific thing for that kind of pages.

milanpavic’s picture

Assigned: Unassigned » milanpavic
milanpavic’s picture

Status: Active » Needs review
FileSize
1.42 KB

Here's the patch. When click enable scroll goes up and edit button gets focused.

DuaelFr’s picture

Assigned: milanpavic » Unassigned
Category: Task » Bug report
Status: Needs review » Needs work

Thank you very mush for your work @milanpavic

However, I don't think it's the best approach in this case. core/misc/ajax is supposed to be generic and we should not have some Views related code in there. Your code should be in the views module and attached to that particular page only.

Moreover, your fix should stay in the scope of the issue. So, the only needed part is the one that focus the button. The automatic scroll is interesting but should be discussed in a follow-up issue.

milanpavic’s picture

Status: Needs work » Needs review
FileSize
1.34 KB

OK so i removed code from ajax file and put it inside module.
When u click on enable you stay where you are by default, seems that another issue solved this one.
But i left scrool cuz whit out it this task kinda loose its point.
So when click on enable, if there's need scrool gets up and edit button gets focused.

Denchev’s picture

Status: Needs review » Needs work

So a few things...

1. As DuaelFr suggested the scroll i kinda nice but out of scope of this issue so should be removed.

+++ b/core/modules/views_ui/js/views_ui.listing.js
@@ -51,4 +51,31 @@
+  Drupal.behaviors.viewsEnableFix = {

2. viewsEnableFix is not descriptive enough for a name so maybe change it to viewsEnableFocus.

3. Also, there are some code styling errors. Take a look at https://www.drupal.org/node/172169 and try to find and fix them.

4. Also for the next patch create an interdiff. Here is a guide https://www.drupal.org/documentation/git/interdiff

AjitS’s picture

Status: Needs work » Needs review
FileSize
2.18 KB
1.32 KB
  1. Fixed
  2. Fixed some of the coding issues. Please let me know if I've missed any.
slashrsm’s picture

Status: Needs review » Needs work

As DuaelFr suggested the scroll i kinda nice but out of scope of this issue so should be removed.

It seems that the original problem that this issue addressed was already fixed (nor @milanpavic not myself managed to reproduce it with current codebase). We have two options in this case:

1. Close this issue and add scroll in a follow-up or
2. re-focus this issue and add scroll here.

I don't see any issues if we take route nr. 2.

And one other thing that I noticed in the patch:

+++ b/core/modules/views_ui/js/views_ui.listing.js
@@ -51,4 +51,31 @@
+      if(window.location.href.endsWith('/admin/structure/views')==true){

This file should only be included on listing, which means there is no need to check path.

slashrsm’s picture

I was wrong. This is still the issue. Let's do a scroll in a follow-up.

Let's fix the one remaining issue and this should be OK to go.

milanpavic’s picture

Assigned: Unassigned » milanpavic
FileSize
4.75 KB
1.97 KB

Tnx for suggestions, here's the new patch.
Scroll removed but it seems that javascript focus is sometimes pushing scroll at the top of the page. Thats for example if you enable archive.
There's some extra code in mine patch, not sure why but it gets generated like this. It looks like patch+ interdiff. Someone know why?

pivica’s picture

Ok first thing first, patch is weird because you did two diffs that should not be there, one for patch

diff --git a/1848940_enableFix.patch b/1848940_enableFix.patch
deleted file mode 100644

and another for interdiff

diff --git a/interdiff-1848940-11-15.txt b/interdiff-1848940-11-15.txt
deleted file mode 100644

You should probably only have this part

diff --git a/core/modules/views_ui/js/views_ui.listing.js b/core/modules/views_ui/js/views_ui.listing.js

I checked the patch it self, no idea about logic (don't have time to check that part) but here is some feedback about code style errors and some other small optimizations in code that you could do. Check https://www.drupal.org/coding-standards for more tips about Drupal coding standards.

  1. +++ b/core/modules/views_ui/js/views_ui.listing.js
    @@ -51,4 +51,22 @@
    +  //Enable button fix
    

    Space Between // and Enable, put dot on the end of comment. This goes for all comments in code.

  2. +++ b/core/modules/views_ui/js/views_ui.listing.js
    @@ -51,4 +51,22 @@
    +        var interval=setInterval(function() {
    

    Missing spaces for interval=setInterval and function().

  3. +++ b/core/modules/views_ui/js/views_ui.listing.js
    @@ -51,4 +51,22 @@
    +          var trs = $('tr.views-ui-list-enabled');
    +          trs.each(function(i) {
    

    It seems trs is used only to iterate on all selected elements, so no need to create local var for this, its enough to just do

    $('tr.views-ui-list-enabled').each(function() {...});

    Also i is not used in anonymous function so also you can skip defining it.

  4. +++ b/core/modules/views_ui/js/views_ui.listing.js
    @@ -51,4 +51,22 @@
    +            // If upper DOM gets updated get element(based on title) and focus it.
    

    When comment is breaking 80char limit you should break comment into multiple lines.

  5. +++ b/core/modules/views_ui/js/views_ui.listing.js
    @@ -51,4 +51,22 @@
    +            if( $(this).attr('title') == title ){
    

    Wrong spaces again remove extra spaces in if and add extra space before {, meaning it should look like

    if($(this).attr('title') == title) {

  6. +++ b/core/modules/views_ui/js/views_ui.listing.js
    @@ -51,4 +51,22 @@
    +              $(this).find('a').focus();
    

    Use of $(this) two times here, it make sense then to make local variable here first and speed up execution a little bit, something like

    var $this = $(this);

  7. +++ b/core/modules/views_ui/js/views_ui.listing.js
    @@ -51,4 +51,22 @@
    +        },500);
    

    Missing space before 500.

milanpavic’s picture

This should be it. In interdiff file you can see style changes based on you comments.

Denchev’s picture

+++ b/core/modules/views_ui/js/views_ui.listing.js
@@ -51,4 +51,26 @@
+        // Listen for changes on upper DOM lists

nit, forgot dot at the end

+++ b/core/modules/views_ui/js/views_ui.listing.js
@@ -51,4 +51,26 @@
+            // If upper DOM gets updated get element(based on title)
+            // and focus it.

Comments should be maximum 80 characters but don't split it too early use the maximum space. You might have space for a word or 2 more.

Also some additional information, patch naming is [project_name]-[short-description]-[issue-number]-[comment-number].patch
Not everyone adds all information but the most important part is issue number and comment number. With comment number you can easier find the patch in long discussions and can see what "version" it is.

And one more thing when you upload a patch, like you did with assigned in comments, change the status to needs review.

milanpavic’s picture

Status: Needs work » Needs review
FileSize
1.04 KB
993 bytes

Updated. Comment have actually less than 80 characters, so i change it to one line.

milanpavic’s picture

Comment on 2 lines updated.

slashrsm’s picture

Status: Needs review » Reviewed & tested by the community

Looks good. Thank you!

mgifford’s picture

Issue summary: View changes
Issue tags: +rc target triage
nod_’s picture

Status: Reviewed & tested by the community » Needs review

I have a few comments. Will update when I get to my laptop.

nod_’s picture

I was not a fan of the setInterval (or even setTimeout) solution so I went ahead and went with a new approach that make use of our existing API, and I added docs.

In essence I'm waiting on Drupal.attachBehaviors() to get called and change what I do in that new behavior based on the existance of a variable. That way we don't have to guess when the ajax call ended, the ajax api does it for us. Makes the code simpler.

Also took inspiration from the code above used to find rows to make the row finding more predictable.

Last thing I did was that I put in focus always on the first dropbutton link. The previous code was selecting a bunch of links (3 or 4 usually) and calling focus on all of them. Which one got the actual focus was not clear to me (looks like the last visible button gets the focus, but who knows what the actual rule is). Now you can always click "enable" and when you hit enter afterwards, you'll always go to the edit screen.

nod_’s picture

Sorry, stray console.log in previous patch

DuaelFr’s picture

Status: Needs review » Needs work

Thanks! That looks promising!
Some comments, though.

  1. +++ b/core/modules/views_ui/js/views_ui.listing.js
    @@ -51,4 +51,45 @@
    +      function focusRow(index, row) {
    ...
    +        $('tr.views-ui-list-enabled').each(focusRow);
    

    That function name is confusing as the function is called on each enabled views, not only on the one that needs to be focused. I think it's at least missing a few comments.

  2. +++ b/core/modules/views_ui/js/views_ui.listing.js
    @@ -51,4 +51,45 @@
    +        var textMatch = $source.text().indexOf(enabledView) !== -1;
    +        if (textMatch) {
    

    (minor) No need to create a variable here.

Also, your patch fixes the problem when enabling a view. That's really great but the same problem exists when disabling a view on that same page. I think we could apply the same behavior on this case because even if the user does not want to do something else with a disabled view, moving her anywhere else is going to be confusing. Plus, if she made a mistake disabling the view, she can still re-enable it quickly as the focus is not lost.

nod_’s picture

Issue tags: +Novice, +js-novice, +JavaScript

Indeed :)

sdstyles’s picture

Status: Needs work » Needs review
FileSize
2.22 KB
2.75 KB

Changed script to trigger focus also when a view is disabling.

DuaelFr’s picture

Code looks good to me.
I did the manual testing too and it works like a charm!
Thank you all for your help with this issue.

DuaelFr’s picture

Status: Needs review » Reviewed & tested by the community

I also checked eslint before RTBC'ing so everything's perfect!

Status: Reviewed & tested by the community » Needs work

The last submitted patch, 29: core-views-js-enable-focus-fix-1848940-29.patch, failed testing.

xjm’s picture

Issue tags: -rc target triage

From now until 8.0.0 we will not be committing anything except rc eligible patches or very severe bugfixes, so untagging for that reason. This is safe to fix in a patch release, though (unless the JS is breaking something I don't know about, which is possible because JavaScript). Thanks @DuaelFr for tagging it for triage.

falufalump’s picture

The patch applied clean, and I couldn't find any errors in it or the patched views_ui.listing.js. But, the error seems to be in \Drupal\locale\Tests\LocalePluralFormatTest.php,

lines 210 - 217 related to strings needing the translation function. Does anyone know if there are open tickets for that issue?

DuaelFr’s picture

Status: Needs work » Needs review

Triggered a retest on the last patch that should not fail as it only changes JS...

DuaelFr’s picture

Status: Needs review » Reviewed & tested by the community

Green again!

droplet’s picture

Status: Reviewed & tested by the community » Needs review
+++ b/core/modules/views_ui/js/views_ui.listing.js
@@ -51,4 +51,55 @@
+        if ($source.text().indexOf(changedView) !== -1) {

missing equal comparison.

---

any more elegant way to do this job ?

droplet’s picture

Status: Needs review » Needs work
nod_’s picture

Status: Needs work » Needs review
FileSize
2.37 KB

You're right could be much simpler.

Added a new data-drupal-view-id attribute on the tr, makes the code much simpler.

DuaelFr’s picture

+++ b/core/modules/views_ui/js/views_ui.listing.js
@@ -51,4 +51,37 @@
+      $(context).find('[data-drupal-view-id] .use-ajax').once('viewsUiListFocus')

Isn't $('[data-drupal-view-id] .use-ajax', context) better? (For my personal knowledge)
That looks good, though :)

nod_’s picture

using the context argument adds one step in the execution. In the end it is converted to .find() inside jQuery. So it's exactly the same, with .find() being more explicit and mapping more directly to what you'd do in vanillajs: document.querySelectorAll('[data-drupal-view-id] .use-ajax'). Once we get there it'll be easier to refactor.

droplet’s picture

Assigned: milanpavic » Unassigned

#41 is correct. You may interested:
https://github.com/jquery/jquery/blob/99e8ff1baa7ae341e94bb89c3e84570c7c...

+++ b/core/modules/views_ui/src/ViewListBuilder.php
@@ -122,6 +122,7 @@ public function buildRow(EntityInterface $view) {
       'title' => $this->t('Machine name: @name', array('@name' => $view->id())),
       'class' => array($view->status() ? 'views-ui-list-enabled' : 'views-ui-list-disabled'),
+      'data-drupal-view-id' => $view->id(),

Needed check_plain? Because looking at 2 lines before it's passing via @name, not !name.

Version: 8.0.x-dev » 8.1.x-dev

Drupal 8.0.6 was released on April 6 and is the final bugfix release for the Drupal 8.0.x series. Drupal 8.0.x will not receive any further development aside from security fixes. Drupal 8.1.0-rc1 is now available and sites should prepare to update to 8.1.0.

Bug reports should be targeted against the 8.1.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.2.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

DuaelFr’s picture

Status: Needs review » Reviewed & tested by the community
Issue tags: +DevDaysMilan

I manually tested all cases under Firefox and Chrome.
That's now working as expected.
Thank you all for your work :)

dawehner’s picture

Thank you for your manual testing!

alexpott’s picture

Status: Reviewed & tested by the community » Needs work
Issue tags: +Needs tests

I think we should add a javascript test base for this since ViewsUI testing is one of the main areas this can help us.

DuaelFr’s picture

I'll try to work on these tests.

DuaelFr’s picture

I made the test but it's dependant on #2753791: Add javascript testing for the Views listing page. so it's going to fail until that one gets commited.

DuaelFr’s picture

The last submitted patch, 48: core-views-js-enable-focus-fix-1848940-48.patch, failed testing.

dawehner’s picture

+++ b/core/modules/views_ui/js/views_ui.listing.js
@@ -51,4 +51,37 @@
+  // When a view is enabled/disabled, this variable will hold it's machine name.
+  // It is used in the behavior to focus on the first dropbutton link of this
+  // view's row.
+  var changedView = '';

This is weird, as its sort of a global variable. Is this the way to do that in javascript?

nod_’s picture

It's as much a global as a private property in a PHP class is. That script is loaded only on the page listing and it's handling the state of that listing. We don't have any info from the PHP/HTML of which view got enabled, and instead of saving the list for both enabled and disabled views and doing a diff we keep the id of the view clicked, which is simpler.

If you have a better way though, happy to hear it.

DuaelFr’s picture

#51 well, as seen at dev days, it's a very specific behavior we need for this page because it's not a form and it's entirely rebuilt by the ajax call.
We could replace the entire JS of this patch by something like the following in \Drupal\views_ui\Controller\ViewsUIController::ajaxOperation()

      if (in_array($op, ['enable', 'disable'])) {
        $response->addCommand(new InvokeCommand('tr[data-drupal-view-id="' . $view->id() . '"] .dropbutton li:first-child a', 'focus'));
      }

but I'm not sure it's the right way to do as it makes that code really related to the HTML structure and difficult to override if someone wants to change how that page looks.

We just need to wait for #2753791: Add javascript testing for the Views listing page. to get commited to be sure the test I added works as requested.

dawehner’s picture

@nod_
Yeah I would be happy to see a better solution, this is for sure! Maybe attaching a new "behaviour" to the main body or so could work.

nod_’s picture

Well that's the one I came up with so I don't have anything better. I think you're talking about something like in #21, but it makes thing brittle and complicated.

Not a fan of the PHP solution in #53 but it doesn't add any js so why not.

DuaelFr’s picture

The last submitted patch, 48: core-views-js-enable-focus-fix-1848940-48.patch, failed testing.

The last submitted patch, 48: core-views-js-enable-focus-fix-1848940-48.patch, failed testing.

The last submitted patch, 48: core-views-js-enable-focus-fix-1848940-48.patch, failed testing.

dawehner’s picture

Haha, I was about to say: damnit another random test failure until I realized. This one is totally legit.

DuaelFr’s picture

Assigned: Unassigned » DuaelFr

I'm on it

DuaelFr’s picture

Found it. Stupid mistake: once a view has been disabled we need to check for an "enable" action, not another "disable" one...

Lendude’s picture

Status: Needs review » Needs work

Looking good @DuaelFr, one thing:

+++ b/core/modules/views_ui/js/views_ui.listing.js
@@ -51,4 +51,37 @@
+      $(context).find('[data-drupal-view-id] .use-ajax').once('viewsUiListFocus')

This selector doesn't sound very 'enable/disable' specific. It seems to be for the moment, but the class is very aspecific. So I would advise to look for a more specific way to target this or to update all the docs around it to indicate it triggering on any ajax enabled dropbuttons (which currently is only the enable/disable one, but who knows what the future/contrib might bring).

edit: took the nitpick out :)

Version: 8.1.x-dev » 8.2.x-dev

Drupal 8.1.9 was released on September 7 and is the final bugfix release for the Drupal 8.1.x series. Drupal 8.1.x will not receive any further development aside from security fixes. Drupal 8.2.0-rc1 is now available and sites should prepare to upgrade to 8.2.0.

Bug reports should be targeted against the 8.2.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.3.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.

mradcliffe’s picture

The issue summary could use an update as it has a lot of information from last year.

Version: 8.2.x-dev » 8.3.x-dev

Drupal 8.2.6 was released on February 1, 2017 and is the final full bugfix release for the Drupal 8.2.x series. Drupal 8.2.x will not receive any further development aside from critical and security fixes. Sites should prepare to update to 8.3.0 on April 5, 2017. (Drupal 8.3.0-alpha1 is available for testing.)

Bug reports should be targeted against the 8.3.x-dev branch from now on, and new development or disruptive changes should be targeted against the 8.4.x-dev branch. For more information see the Drupal 8 minor version schedule and the Allowed changes during the Drupal 8 release cycle.