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
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. |
Comment | File | Size | Author |
---|---|---|---|
#87 | interdiff-1848940.txt | 1.21 KB | dawehner |
#87 | 1848940-87.patch | 3.93 KB | dawehner |
#84 | 1848940-84.patch | 3.45 KB | Lendude |
#84 | interdiff-1848940-82-84.txt | 1.5 KB | Lendude |
#82 | interdiff-1848940.txt | 1.67 KB | dawehner |
Comments
Comment #1
dawehnerThis seems to be related to #1824636: Do not move the cursor to the top of the page on ajax calls
Comment #2
tim.plunkettWell, also because their cursor is inside a div that is completely wiped away by AJAX.
Comment #3
DuaelFrTo 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?
Comment #4
mgiffordI think adding some JS here to refocus the navigation would be fine. Is this a generalizable problem?
Comment #5
DuaelFr@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.
Comment #6
mgiffordOk, 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.
Comment #7
DuaelFrTested 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.
Comment #8
milanpavic CreditAttribution: milanpavic commentedComment #9
milanpavic CreditAttribution: milanpavic commentedHere's the patch. When click enable scroll goes up and edit button gets focused.
Comment #10
DuaelFrThank 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.
Comment #11
milanpavic CreditAttribution: milanpavic commentedOK 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.
Comment #12
thenchev CreditAttribution: thenchev at MD Systems GmbH commentedSo a few things...
1. As DuaelFr suggested the scroll i kinda nice but out of scope of this issue so should be removed.
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
Comment #13
AjitSComment #14
slashrsm CreditAttribution: slashrsm at MD Systems GmbH commentedIt 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:
This file should only be included on listing, which means there is no need to check path.
Comment #15
slashrsm CreditAttribution: slashrsm at MD Systems GmbH commentedI 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.
Comment #16
milanpavic CreditAttribution: milanpavic commentedTnx 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?
Comment #17
pivica CreditAttribution: pivica at MD Systems GmbH commentedOk 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.
Space Between // and Enable, put dot on the end of comment. This goes for all comments in code.
Missing spaces for interval=setInterval and function().
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.
When comment is breaking 80char limit you should break comment into multiple lines.
Wrong spaces again remove extra spaces in if and add extra space before {, meaning it should look like
if($(this).attr('title') == title) {
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);
Missing space before 500.
Comment #18
milanpavic CreditAttribution: milanpavic commentedThis should be it. In interdiff file you can see style changes based on you comments.
Comment #19
thenchev CreditAttribution: thenchev at MD Systems GmbH commentednit, forgot dot at the end
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.
Comment #20
milanpavic CreditAttribution: milanpavic commentedUpdated. Comment have actually less than 80 characters, so i change it to one line.
Comment #21
milanpavic CreditAttribution: milanpavic commentedComment on 2 lines updated.
Comment #22
slashrsm CreditAttribution: slashrsm at MD Systems GmbH commentedLooks good. Thank you!
Comment #23
mgiffordComment #24
nod_I have a few comments. Will update when I get to my laptop.
Comment #25
nod_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.
Comment #26
nod_Sorry, stray console.log in previous patch
Comment #27
DuaelFrThanks! That looks promising!
Some comments, though.
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.
(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.
Comment #28
nod_Indeed :)
Comment #29
sdstyles CreditAttribution: sdstyles at FFW commentedChanged script to trigger focus also when a view is disabling.
Comment #30
DuaelFrCode 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.
Comment #31
DuaelFrI also checked eslint before RTBC'ing so everything's perfect!
Comment #33
xjmFrom 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.
Comment #34
falufalump CreditAttribution: falufalump commentedThe 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?
Comment #35
DuaelFrTriggered a retest on the last patch that should not fail as it only changes JS...
Comment #36
DuaelFrGreen again!
Comment #37
droplet CreditAttribution: droplet commentedmissing equal comparison.
---
any more elegant way to do this job ?
Comment #38
droplet CreditAttribution: droplet commentedComment #39
nod_You're right could be much simpler.
Added a new data-drupal-view-id attribute on the tr, makes the code much simpler.
Comment #40
DuaelFrIsn't
$('[data-drupal-view-id] .use-ajax', context)
better? (For my personal knowledge)That looks good, though :)
Comment #41
nod_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.
Comment #42
droplet CreditAttribution: droplet commented#41 is correct. You may interested:
https://github.com/jquery/jquery/blob/99e8ff1baa7ae341e94bb89c3e84570c7c...
Needed check_plain? Because looking at 2 lines before it's passing via @name, not !name.
Comment #44
DuaelFrI manually tested all cases under Firefox and Chrome.
That's now working as expected.
Thank you all for your work :)
Comment #45
dawehnerThank you for your manual testing!
Comment #46
alexpottI think we should add a javascript test base for this since ViewsUI testing is one of the main areas this can help us.
Comment #47
DuaelFrI'll try to work on these tests.
Comment #48
DuaelFrI 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.
Comment #49
DuaelFrComment #51
dawehnerThis is weird, as its sort of a global variable. Is this the way to do that in javascript?
Comment #52
nod_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.
Comment #53
DuaelFr#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()
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.
Comment #54
dawehner@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.
Comment #55
nod_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.
Comment #56
DuaelFrRelaunched the tests now #2753791: Add javascript testing for the Views listing page. has landed.
Comment #60
dawehnerHaha, I was about to say: damnit another random test failure until I realized. This one is totally legit.
Comment #61
DuaelFrI'm on it
Comment #62
DuaelFrFound it. Stupid mistake: once a view has been disabled we need to check for an "enable" action, not another "disable" one...
Comment #63
LendudeLooking good @DuaelFr, one thing:
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 :)
Comment #65
mradcliffeThe issue summary could use an update as it has a lot of information from last year.
Comment #69
helenasue CreditAttribution: helenasue at Lullabot commentedThis is re-roll of the work that @DuaelFR did, because it works well and has died in the queue for two years. Let's get this in and talk about future hypothetical issues when we come to them instead of leaving this dead.
https://github.com/helenasue/drupal/pull/1/files
Comment #70
helenasue CreditAttribution: helenasue at Lullabot commentedComment #71
justafishThanks @helenasue! 🙏 This patch will also need updating to adhere to the new JS coding standards, you can see the failures by running yarn lint:core-js-passing
Comment #72
helenasue CreditAttribution: helenasue at Lullabot commentedAh-ha, neat. This should do the trick. Thanks for the help with this, @justafish!
Comment #73
runeasgar CreditAttribution: runeasgar as a volunteer commentedI am at DrupalCon Nashville 2018 mentor sprint, and will review this now (in the hopes the patch passes tests!).
Comment #74
runeasgar CreditAttribution: runeasgar as a volunteer commentedApologies; I do not know how to test this. I thought maybe I could observe it, but I can't seem to do so.
Comment #75
runeasgar CreditAttribution: runeasgar as a volunteer commentedHelena was kind enough to stop by and show me how to use Voiceover to test this. Upon enabling or disabling a view, I am immediately presented with the option to perform the inverse operation. i.e., my "cursor" is on the newly available button that is active for that view's operations.
Marking as RTBC.
Comment #76
dawehnerIt is really nice to have test coverage for that!
@helenasue and myself had a more in depth look into that. We were wondering, why #1824636: Do not move the cursor to the top of the page on ajax calls did not just resolve the problem and whether it would be possible to use the same
mechanisms.
Why played around with it and got it working, but we aren't sure that is the best solution.
Comment #77
justafishComment #78
helenasue CreditAttribution: helenasue at Lullabot commentedThis adds the testing to @dawehner's patch.
Comment #79
helenasue CreditAttribution: helenasue at Lullabot commentedComment #81
helenasue CreditAttribution: helenasue at Lullabot commentedIt seems that the last implementation didn't pass testing. :(
Do we want to move ahead with #72 since that one was RTBC and working?
Comment #82
dawehner@helenasue
Given that the core committers will take their time to look at this patch anyway, I think we have time to experiment with alternative solutions.
Let me see whether adapting the test fixes things. Does that make sense?
Comment #84
LendudeHere we go, should be green now. The view-id getting added as a data attribute got taken out, so the test was looking for something that didn't exist anymore.
Comment #85
dawehnerIt feels like checking for the data-drupal-selector could still be useful?
Comment #86
helenasue CreditAttribution: helenasue at Lullabot commented@dawehner I totally agree, as long as we keep the issue going. My main concern about not putting something working in is that will die again and people will be stuck waiting for an accessibility solution because it died in debate in the queue. If we've got something that works or we're continuing to try to find something that works better, that's fine - my concern is to not leave people hanging for two years who need this for accessibility.
Comment #87
dawehnerNo worries. This is a bug, and as such it can totally be committed to 8.5.x and 8.6.x
I'm expanding the test here, so we actually test the enable link too.
Comment #88
LendudeExtra coverage++
We have a slick fix and nice coverage for the changes, ready to go I think.
Comment #89
catchCommitted b20aea6 and pushed to 8.6.x. Thanks!
s/the data-drupal-selector/on the data-drupal-selector/ fixed on commit.
Comment #91
andrewmacpherson CreditAttribution: andrewmacpherson as a volunteer and at Annertech commentedNice work! I think we'll need some follow-ups to fix similar issues elsewhere, as this only fixes the ViewsListBuilder. The block layout page is another place with a similar problem.
Comment #92
andrewmacpherson CreditAttribution: andrewmacpherson as a volunteer and at Annertech commentedI filed #2973114: When enabling or disabling a block, don't move the focus to the top of the page to get a similar behaviour on the Block UI.
Comment #93
andrewmacpherson CreditAttribution: andrewmacpherson as a volunteer and at Annertech commentedWhen a screen reader enables or disables a view, the screen content changes, and users need to be informed of this. Drupal.announce() will probably work. I filed a follow-up - #2973116: After enabling or disabling a view, convey changes to screen reader users..