Problem/Motivation
There are several usability/accessibility problems on the Extend/Modules page [Note: See the "original report" section below for the original (a), (b), etc. designations of these problems, referenced in comments 1-17. This issue summary has been updated to have a cleaner list of the problems.]:
1. Accessibility of filter box: When you start typing in the box, the list of modules changes. This is not announced to users, so someone with visual limitations would not understand what is happening. It should use the JS Drupal.Announce to do this. [originally this was part of (a)]
2. Usability of filter box: The input field is labeled Search (when it should say Filter modules), and the placeholder text just says "Enter module name". It is only if you happen to hover over the box with your mouse that you get the help text "Enter a part of the module name or description to filter by." [Note: This is actually incorrect anyway, because you can only actually filter based on the module name, not words in the description.]
Using a title like this over an input isn't a common pattern. This help text should be below the input field instead of being a hover tool-tip, and ... the wording is fairly awkward as well. [originally this was part of (a) and also (e)].
3. Behavior when you hit Enter: If I hit return/enter in the search box, it causes a page reload. But when it comes back:
- The search word is not in the box (It still says "Enter module name").
- My search has not executed (everything is still displayed).
- I think rather than searching it is actually submitting the modules form below. That doesn't seem right.
[originally this was (b)]
4. Confusing message when you enable a module: If I check a box to enable a module and save, the message I get is "The configuration options have been saved. " ... um? Shouldn't it tell me my module(s) were installed? That is all you can do on this screen, so it doesn't even have to figure out what you submitted. [originally this was (d).] The messages should be consistent with the language used for disabling a module,
"The selected modules have been uninstalled." - so let's make it "The selected modules have been installed".
Suggested replacement message: The selected modules have been installed.
Originally reported items that are now fixed:
(c) ==> #2060573: Standardized filter/search behavior; surface filter matches regardless of their containing box's open state; and/or #2053285: Module listing doesn't hide empty package grouping fieldsets when filtering.
Proposed resolution
Fix these issues, either with a patch here or on sub-issues.
The patch in #12 may fix items (1) and (3).
Remaining tasks
Items 1-4 in the Problem section still need to be fixed.
User interface changes
Improved usability and accessibility of the Modules/Extend page.
API changes
None.
Original report by @jhodgdon
I really like the idea of the new design of the Extend/Modules page. However, it is not really working all that well for me, and I think these things should be fixed before release, so I've marked this "major" even though they are not really all that major of issues or hard to fix probably.... And by the way I am not using the Overlay. This is directly in my browser window (Ubuntu/Firefox fairly recently updated).
a) I do not think it is OK from an accessibility perspective to filter as I type. It is also quite different from what you see on the Content page or other Drupal Core admin pages, which I thought it was a little jarring. And there was no help or field description telling me that this is what it was doing. Actually it wasn't working for me for a while, and I only just now figured out that is what it was supposed to do. I'll have to start a new browsing session and see if it works then... something wasn't right at first.
b) If I hit return in the search box, it causes a page reload. But when it comes back:
- The search word is not in the box (It still says "Enter module name").
- My search has not executed (everything is still displayed).
- There's a message on the screen that just says: "system_modules". What does that mean? Hmmm... I think the filtering box is part of the main form on the page so it is submitting my modules enabled list. That is probably wrong.
c) If the filtering as I type is working, and I have collapsed my module categories, I get no feedback that my filtering is working at all. I can't see the module list being reduced, and there is just no way to see what it might be doing.
d) If I make a change (enable/disable a module) and save, the message I get is not very friendly. It says: "Configuration updated. system_modules". What does that mean?
e) The filtering box says "Enter module name" and I don't see the information that it accepts only part of the module name unless I happen to hover my mouse over the field. Maybe it should say something like "Start typing and your list will be filtered" or ... well something getting that point across?
f) Most web sites that have help text right in the box, the help text vanishes as soon as you click. This help text vanishes only after I stop typing, which is ... weird.
Comment | File | Size | Author |
---|---|---|---|
#166 | interdiff-2056089-155-166.txt | 588 bytes | TR |
#166 | core-js-module-list-2056089-166.patch | 31.91 KB | TR |
| |||
#156 | Uninstall-drupal8.png | 268.22 KB | Manjit.Singh |
#156 | Extend-drupal8.png | 333.58 KB | Manjit.Singh |
#155 | interdiff-2056089-152-155.txt | 3.12 KB | TR |
Comments
Comment #1
jhodgdonOh wait. "system_modules" is from a test module I had enabled. Ignore that. :)
Comment #2
nod_a) is it confusing or is it just not working? or both?
b) Shouldn't be able to submit from the text field, needs fixing.
c) Search doesn't work with collapsed details, needs fixing.
d) Ignoring per #1
e) Sure, but it doesn't take long to get it, no? need input from UX team I guess. Also #1848064-40: Allow to filter modules by arbitrary search strings on the Modules page.
f) That's a bug somewhere else, the field is using the browser built-in placeholder attribute for text fields.
Comment #3
jhodgdonRE #2...
a) When I first went to the Modules/Extend page, it wasn't working (I think), but I was unable to reproduce the problem later. Once I saw it working, it was merely a bit confusing, but I think someone who couldn't see would find it totally incomprehensible, since there is no help telling you that is what it will do, and I don't think that content that appears/disappears as you type is in any way obvious to people using screen readers.
e) I don't normally hover my mouse over text input fields. It was totally by accident that my mouse happened to be there. This is not a good way to provide help -- it should be in a description field under the text field, or in the text you see in the field before you start typing.
Comment #4
nod_c) #2060573: Standardized filter/search behavior; surface filter matches regardless of their containing box's open state;
Comment #5
tim.plunkettFind-as-you-type is a pattern we've chosen to use. I don't think it needs further indication that's how it works...
c) is definitely a bug, but it has an issue. Rescoping this for b)
Comment #6
nod_if that's the case, postponing on #2060573: Standardized filter/search behavior; surface filter matches regardless of their containing box's open state;, I don't want to reroll a patch here needlessly.
Comment #7
nod_Comment #8
tstoecklerSeems @jhodgdon's accessibility concern's have gotten lost through the re-titling. I don't care in which issue which thing gets fixed ultimately, but I do care that things that people have brought up do not get lost. In other words, I don't mind if this gets re-re-titled but only if a separate issue for the accessibility concerns is opened.
Comment #9
tstoecklerWhoops that Ctrl+C picked up more than I intended.
Comment #10
nod_Added code to prevent form from submitting is hitting enter key.
For a11y I don't know.
Comment #11
mgiffordThis is just the kind of situation that Drupal.announce was built for #2124415: Use Drupal.announce to give a screen reader user a succinct summary of how changes to a View's definition affected the preview
It's definitely a problem that the DOM is changing without alerting the user.
More documentation is here:
We should test that out, but it should do the trick.
Comment #12
mgiffordRe-rolling with Drupal.announce. Is this what we want it to say to a screen reader?
Comment #13
mgiffordComment #14
jhodgdonI think we need an issue summary here. Since I originally filed this issue, some of the UI stuff has been fixed I think... someone needs to go through the summary, test this again, and see which issues remain (probably marking the rest with a DEL tag so that they are still there for reference in earlier comments but are crossed out).
Until then it is kind of hard to evaluate whether the patch fixes the issue(s).
Comment #15
mgiffordThe accessibility stuff in a) hasn't been fixed. However, that's what I attempted to do in my last patch in #12.
@nod_'s patch which I was building on was all about preventing the form from submitting with hitting the enter key. I don't know if that's in your summary, but perhaps.
Comment #16
jhodgdonThe summary for this issue lists (a) - (f) problems. They all need to be retested, and then we can see whether they still need fixing.
Comment #17
mgiffordI can spin a) off into a separate issue like c) & e) were. We can make this a meta issue and create child elements.
Comment #18
jhodgdonI've updated the issue summary with a coherent list of what still needs to be done.
Comment #19
mgifford@jhodgdon - I wrote a much longer post but lost it somehow when I hit save.. Very annoying... Sorry for the terse response.
Thanks so much for doing this!
1) In #12 I think. Needs testing.
2) Ya, this needs work. The title in the input isn't a very common pattern. Text like the title should be in a description (although the UX folks are eliminating descriptions so maybe it should just be removed) The label should be "Filter Modules" and not Search.
3) In #12 I think. Needs testing.
4) It should be consistent with the language for disabling a module,
The selected modules have been uninstalled." - so let's make it "The selected modules have been installed".
Mind you, if only one module is selected, that's not quite right. It would be consistent though.
Comment #20
jhodgdon@mgifford, it is useful when making a reply like this to update the issue summary as well.
Regarding item 2, I don't think we can entirely remove that help text, as it explains you are searching on the name or description (is that even true actually?). As it is, the placeholder text says "Enter module name", which implies the whole module name must be entered, that you need to hit "Enter", and only the name not description is searched.
Comment #21
mgiffordGood point.. That certainly makes it easier to keep track of the discussion. I added my 2 points.
You're only filtering for the module name. Very true though that you don't have to enter the whole module name. It's actually a module fragment as any section of the text. Filtering for 'act' gives you Actions, Activity Tracker & Contact.
In the patch, the module doesn't filter as as you type.
Comment #22
jhodgdonYou're right, you're only filtering on the module name. I'll add that to the summary, because the hover text says you can filter by description and that is simply incorrect. Oh, I see you added that but I missed it when I skimmed the summary; clarifying a bit.
Comment #23
BarisW CreditAttribution: BarisW commentedThe patch in #12 didn't work; I got a JS error. Here's the same logic, but then with the Drupal.announce moved to the
filterModuleList()
function.Comment #24
BarisW CreditAttribution: BarisW commentedComment #25
mgiffordThanks for fixing the JS error. It's looking better!
Comment #26
Désiré CreditAttribution: Désiré commentedAs an addition, I think the modules machine name should be also displayed somewhere, since they can be different from the real name (eg.: locale vs. 'Interface Translation' in the core). It also would be great, if the search field can filter for the machine name too.
Comment #27
mgifford@Désiré - Can you add that to the issue summary? It's a good point.
Comment #28
Désiré CreditAttribution: Désiré commentedComment #29
Bojhan CreditAttribution: Bojhan commentedSorry, but we are not introducing new features here. Only bug fixing, new features -> new issue.
Comment #30
Désiré CreditAttribution: Désiré commentedOk, the search filter for machine name is a new feature, I'll create a new issue for it. What about displaying it amongst the module info?
Comment #31
Bojhan CreditAttribution: Bojhan commented@Desire that would be two separate issues I guess. I think we already search on the machine name, but I might be wrong.
Comment #32
jhodgdonBojhan / #31 - I just tested on a fresh D8 install. You cannot type "locale" in the search box, so no, we do not already search on the machine name, just the displayed name of the module.
So Désiré / #30 : I think we should file one new issue to display the machine names of modules somewhere on the Extend page, and another one to be able to filter by the machine names in the search box. Thanks!
Comment #33
Désiré CreditAttribution: Désiré commentedHere are the two new issues:
#2347543: Filter for module machine name with the module filter on extend page
#2347533: Display module machine names on the extend page
Comment #34
BarisW CreditAttribution: BarisW commentedGreat, thanks! Does that mean that this one is RTBC'ed?
Comment #35
Désiré CreditAttribution: Désiré commentedComment #36
jhodgdonI applied the patch in #23 and tested it, though I do not know how to test the accessibility so I'm hoping someone else will be able to address that.
So the items in this issue (see issue summary for details) are:
1. Accessibility - I did not test.
2. Usability/labels - Still broken. This patch does not fix those issues at all.
3. Behavior on enter - fixed - this patch makes it so hitting Enter in the search field does nothing, which is much better than having it submit the form on the page.
4. Message when you enable a module - Not addressed by this patch.
So... We need to test whether this patch fixes #1. Then we also could use a fix for #2 and #4 -- these are small fixes to messages/labels and would not make the patch much bigger.
Comment #37
katewelling CreditAttribution: katewelling commentedI am looking to see if I can change the text on the search box from "Search" to "Filter Modules"
Comment #38
katewelling CreditAttribution: katewelling commentedAdded a patch that will change the name on the Search box from "Search" to "Filter modules". Will continue to work on this issue.
Kate
Comment #39
katewelling CreditAttribution: katewelling commentedI have now stopped looking at this issue. We were trying to work on Point 4 between us here, and have been unsuccessful in being able to add any Drupal 8 Contrib modules via the UI using each of the different methods available. e.g. hyperlink to zip file, or downloading and uploading the tar.gz file having downloading it directly. Also not able to use drush to dl install any modules. Tried this on 3 different Macs with various different modules. Did anyone else have this issue?
Thanks,
Kate
Comment #40
rodvolpe CreditAttribution: rodvolpe commentedI have sorted out the message on the first list module form page but I realised that the message needs fixing when you enable a module that has a dependency. I am working on it.
Comment #41
rodvolpe CreditAttribution: rodvolpe commentedOk this is my first fix so not sure it is correct. Follow issue #4. Confusing message when you enable a module.
Comment #42
nambisolo CreditAttribution: nambisolo commentedAddressed second part of 2 by changing text within box to 'Start typing'. Also added description below field.
This patch submission also includes changes from #23 and #38.
Rod will roll his changes in #41 into this shortly.
Comment #43
rodvolpe CreditAttribution: rodvolpe commentedThis change addresses part 4 Confusing message when you enable a module.
This is 41 merged into 42.
Comment #44
BarisW CreditAttribution: BarisW commentedThanks all! This looks pretty good to me! I'm not sure if we need to add a t() wrapped around the module names?
Comment #45
mgifford@BarisW I'm having trouble finding the t() around the module names in the last patch. What line is it on?
Agreed though that that doesn't make sense. The 'Views' module will never be the t('Views') module as far as I'm aware.
Comment #46
BarisW CreditAttribution: BarisW commentedI meant that we might want to add it, it's missing now. But I agree with you, it does not make sense to translate module names.
So +1 from me!
Comment #50
BarisW CreditAttribution: BarisW commentedApparently, the tests need to be updated as well.
@Rod; will you take this on?
Comment #51
rodvolpe CreditAttribution: rodvolpe commentedI can do @barry. What do I need to do exactly please? I am going home tonight but I can do it tomorrow night.
Comment #52
jhodgdon@rodvolpe: What you need to do is click "View" under the patch you uploaded, to look at the results from the test bot. If you expand the failing tests, it will show you which specific lines of which test classes had test assertions that failed. Presumably, they were looking for the old status message, and they need to be updated to look for the new status message from this patch.
Comment #53
BarisW CreditAttribution: BarisW commentedHere's a patch that updates the tests.
Other changes
$before = $this->moduleHandler->getModuleList();
Some screenshots
Comment #54
BarisW CreditAttribution: BarisW commentedIgnore the patch in 53. I forgot to roll in the JS changes above. Will provide a new patch.
Comment #56
BarisW CreditAttribution: BarisW commentedComment #58
BarisW CreditAttribution: BarisW commentedTry again testbot.
Comment #60
BarisW CreditAttribution: BarisW commentedApologies for the noise. Should be green now (forgot one test).
Comment #61
jhodgdonBefore I go test this... Which of the problems in the Issue Summary, specifically, is this patch supposed to be addressing? Last few times I tested, not everything was fixed by the patch, and I don't really want to waste time testing yet another incomplete patch. Thanks!
Comment #62
BarisW CreditAttribution: BarisW commentedI think that all has been fixed now.
The only thing I find really confusing is when I enable a module and submit the form, I see a nice message, but that message isn't been read out by the screenreader. Would be nice if all Drupal messages are exposed with
Drupal.accounce()
too, but we might want to create a follow-up for that?Comment #63
jhodgdonThanks! I have tested this on simplytest.me. The following are still problems:
a) The description below the filter field is incorrect. I cannot type text from the description and have it filter. For instance, the Aggregator module includes the word "Atom" in its description, but if I type this no modules are shown. The field description says "Enter a part of the module name or description to filter by."... so this is incorrect. It is also ... I think slightly confusing because of the grammar/wording. And we don't normally end these in .
So I suggest changing it to:
Enter all or part of a module name
(We already know it is a filter, so I don't think we need to say that again.
b) Do we need a follow-up to make exactly the same UI text changes on the Uninstall page, or can it be done here because it is the exact same change? This page has the same UI problems as the Install page, as far as UI text. The messages and enter behavior seem to be fine on the uninstall page though.
c) I did not test the accessibility features.
Comment #64
jhodgdonForgot status, and I forgot to say everything else in the issue summary seems to be OK (except I didn't test accessibility). Thanks!
Comment #65
Bojhan CreditAttribution: Bojhan commentedScreenshot?
Comment #66
BarisW CreditAttribution: BarisW commentedThis is how it looks with the patch from #60. The other screenshots can be found in #53.
Comment #67
mgiffordRerolled the patch to deal with @jhodgdon point in #63. The description doesn't seem to filter still.
I do think we can have follow-up issues for accessibility & the uninstall page.
This patch did not introduce the accessibility problem with the filter box.
Comment #68
mgiffordActually Drupal.announce is already included (but not tested) and it seems the change to the uninstall text is pretty trivial. Let's just try to deal with this here.
Comment #69
mgiffordScreenshots of module install/uninstall pages.
Comment #70
mgiffordTesting this with VoiceOver, I should hear this. At the moment though I'm not even seeing it in the source.
Not sure if it's done incorrectly in core/modules/system/system.modules.js
Comment #71
jhodgdonRE #70, you mean you are not seeing those lines in the source in the system.modules.js file, which must be attached to the page (because otherwise the filtering would not work, right?)?
Comment #72
Bojhan CreditAttribution: Bojhan commentedI am not very thrilled about adding a help text description below the field. We added this in a tooltip on purpose and checked this with @mgifford before it got in.
I don't think you can compare this behaviour with anything else in core, since we simply don't have a filter pattern. I want to keep it very minimal from a visual perspective, as that is a pattern that we can scale. It's always tempting to go down Drupal's route of adding a help text for everything. But frankly this is a type of pattern that is used across the web, mobile devices, etc. Having a design with vey minimal design impact (label and field) would severely increase our ability to apply it in other places with a nice visual balance.
Obviously we need to fix the a11y issues (I don't see how that was originally missed). But I'd like to consider the idea of thinking a little bit more outside the box (hah) when it comes to the help text. Can we for example use a more helpful HTML5 placeholder message (e.g. "Enter a module name").
Comment #73
jhodgdonOK. But "Enter a module name" is not accurate. You do not have to "enter" anything (in the sense of hitting "enter") and you don't have to put in an entire module name. All you have to do is start typing part of a module name and the filtering starts.
Regarding the tooltip, as you can see from my original report, as a regular sighted user I didn't notice the tooltip, and with the text saying "Enter a module name" I had no idea what it was going to do. Having changed the field name at least to say "filter" it is a lot better than it was... So for me, if I cannot get the information without hovering my mouse over the field (the only way I would see the tooltip, and something I'm ***really*** unlikely to do on this page), then this information might as well not be there. IMO it should be either placeholder in the field or help below the field.
I'll take your word for it right now that this type of filtering functionality doesn't exist elsewhere in Core although ... doesn't Views use it? It did in 7.x I think. Well who knows.
Comment #74
Bojhan CreditAttribution: Bojhan commented@Jhodgdon So lets figure out a placeholder that accurately captures the interaction (I adamantly agree that using a tooltip is not discoverable). I ought using the word "enter" by not deviating too much from the previous conversation, but I am fine with using the word filter too. Does anyone have ideas?
Sadly, Views is not really a example of good usability. Attempts to improve that during the D8 cycle, have largely ended up being moot. They do use it - but I think you might cry a little, if you see how unintuitive that is :)
Comment #75
jhodgdonOK. With Standard install profile, I looked through admin pages and found a few places where this type of Ajax filtering is being used, which filters on all or part of a "something" name:
a) admin/structure/block - right column (Place Blocks)
- There is no field label
- There is no description text below
- The placeholder says "Filter by block name"
b) admin/structure/views (Views list)
- The field label is Search
- There is no description text below
- The placeholder says "Enter view name"
[My guess is that this UI is where the Modules page UI came from? Looks pretty similar]
c) admin/modules (this issue - Modules list). Without current patch:
- The field label is Search
- There is no description text below
- The placeholder says "Enter module name"
d) admin/modules/uninstall (now part of this issue - modules uninstall page). Same as (c).
So... I think (a) has the best UI and maybe we should change (b), (c), and (d) to be the same? So they would all lose the field label and the placeholder would say in all cases:
Filter by [whatever] name
Thoughts?
Comment #76
Bojhan CreditAttribution: Bojhan commentedSounds great, although in some cases its title - not name, right? Do you think that saying that suffices?
In views you also have it on the "field list" thats what I was referring to.
Comment #77
BarisW CreditAttribution: BarisW commentedI'd like to work on this tomorrow. There's a code sprint in Amsterdam where I'm mentoring and this issue is a nice one to finish for a novice.
Comment #78
jhodgdonRE #76, in which case(s) do you think it should it be "Filter by [whatever] title" not "Filter by [whatever] name"? I think for modules, we refer to them as "module names", right? For blocks it is I think the block's name and not the title (the title is what would be displayed when the block is displayed, as opposed to the name, which is displayed administratively), right? And same for Views?
As far as whether that suffices... If you only want placeholder text and not a field description underneath, I don't think we have room for anything but something like that -- in particular, I don't think we have room to explain that it will start filtering as soon as you start typing or that it filters for anywhere in any word of the [whatever] name. Personally I think having that explanation under the field is helpful and provides useful information, but if as a larger Usability effort we're trying to get rid of them... well. It's a tradeoff between having the information and keeping the UI clean. I'm not the usability expert to decide which factor is more important here.
Comment #79
Bojhan CreditAttribution: Bojhan commented@jhodgdon Good point, I think doing "Filter by [whatever] name" suffices than. I have rarely seen users struggle with a pattern like this, I've seen people use it in mobile apps, applications and websites. The idea of automatically filtering something, has gotten more and more common. I think anyones gut instinct is to provide more text, to clarify how to use a more sophisticated interaction.
However having more explanation doesn't always make it more usable. For example with drag-and-drop we created quite a few issues with the description (observed in various tests). Because it made people over-think a interaction, that they could use with little trouble after experiencing it (initial use age). My experience so far, tells me that its likely the same user behaviour will apply here - adding help text will be of limited value for actually making it easier to use, and likely will cause people to over think a interaction - that they should/will be able to use effectively after trying it.
I think part of the problem, was the fact that on the module page our text elements (tooltip, placeholder, label) were all incorrect. If we use this pattern consistently, than we create a certain expectation on how it works. At large we are trying to significantly reduce the amount of help text, but it is difficult - as it feels a little counterintuitive.
Does this help elaborate my point of view?
Comment #80
jhodgdonPerfectly! That's really great feedback, and illustrates why you're on the Usability team and I'm not. I agree that removing the incorrect information and replacing it with something relatively simple (which is then similar to what's on other admin pages) is the way to go.
So... It sounds like the plan is for the Modules install/uninstall pages, on top of the patch we already have:
- Remove the description
- Remove the field label
- Use placeholder text saying "Filter by module name"
@BarisW: That should be a good novice project for the sprint. :)
We also need to figure out the accessibility problem, see #70 - apparently the "announce" functionality in this patch is not actually announcing anything (it apparently also didn't make it into the Ajax/HTML/JS) (so it was probably done wrong).
Comment #81
BarisW CreditAttribution: BarisW commentedHere's a patch that does exactly that:
Also: I changed the javascript so that the description is used to filter on as well.
The announce function is working fine, see screen recording.
Comment #82
jhodgdonLooks pretty close! Nice demo movie on the accessibility stuff.
A few things:
a) The visually hidden label on admin/structure/block is "Filter". This patch still has "Search" on the Modules page. I think "Filter" would be better?
b) Great that you can filter now on the module name as well as description, which had been the original intent of the UI! But the placeholder text is now incorrect, because it still says "Filter by module name". I think it would be confusing for people as it is.
c) The uninstall page admin/modules/uninstall didn't get the latest updates.
Comment #83
jhodgdonAnd by the way I just filed a related issue for fixing up the filter on admin/structure/views in the same way:
#2380117: Fix UI of filter on Views list page
Comment #84
Bojhan CreditAttribution: Bojhan commentedHmm, I'd like WimLeers to chime in on the description - I thought there was reasons why we didnt do that.
Comment #85
BarisW CreditAttribution: BarisW commentedWe could also simplify the placeholder with 'Filter module list' and don't make it explicit what is searched on? Would that work?
Comment #86
jhodgdonThe Views page has the same problem - you can currently filter on the view name or description text, so we need to do the same thing on #2380117: Fix UI of filter on Views list page as we decide here.
Right now I am not sure what makes the most sense. I don't know that "Filter module list" and "Filter view list" would make it clear to users who didn't already know that the description text as well as the name is being used in the filtering. They might just wonder why these other modules/views are showing up in the list? I really don't know.
Can we fit in "Filter by module name/description" as the placeholder, or perhaps make the field wider so we can?
Comment #87
Wim LeersThese are inconsistent.
These are contradicting.
Comment #88
jhodgdonOK, so I think what we need to do on this patch is:
a) Change the hidden field title to Filter instead of Search.
b) Change the placeholder text from "Filter by module name" to "Filter by name/description", maybe? Would that work? I think we don't actually need the word "module" in there, since it should be obvious we're on the module page, right?
c) Make the same UI updates on the Uninstall page.
Comment #89
mgiffordI think this patch deals with #88-a
Although I think part of the question is, are we fixing the text to adjust for the behavior, or fixing the behavior so that we are searching by name and description. Ideally it would be the latter.
Let's see how this patch does. I think if it works as expected we'll need to reroll to deal with #87-3.
Comment #90
mgiffordComment #91
mgifford@BarisW I don't have an opinion between 'Filter module list' and 'Filter by name or description' - yours is shorter which is good.
This now addresses the discrepancies in #87-3
It also makes it consistent with the uninstall page in #88c.
Comment #96
BarisW CreditAttribution: BarisW commentedIt seems that the whole module install process has been changed in #2324055: Split up the module manager into runtime information and extension information.. That's why the tests fail :(
Comment #97
jhodgdonIt also looks like more UI changes are being discussed on #2056089: UI problems on the Modules/Extend page. We should probably combine these issues?
Comment #98
BarisW CreditAttribution: BarisW commentedHere's a re-roll to get the tests green again.
Comment #100
BarisW CreditAttribution: BarisW commentedI think I missed a bit.
Comment #102
BarisW CreditAttribution: BarisW commentedForgot one (important) line. Sorry for the noise. Try again, testbot.
Comment #103
mgifford@jhodgdon - I think you used this node id in #97 and not the one you had intended. Here's a recent screenshot. We've got to be really, close if not RTBC at this point.
Comment #104
jhodgdonOh sorry. The other issue is:
#2035079: [PP-3] Figure out what to do with the install/uninstall modules page
I think this issue can be worked on independently though. That other one is maybe going to change how the checkboxes for installing, and uninstalling work, and it may also remove the uninstall page entirely. This one is now mostly about filtering, I think, so there's no real conflict.
Comment #105
AdamEvertsson CreditAttribution: AdamEvertsson commentedOn a not so related note, but since this ticket is about the UI of the modules page: Could we add that when you visit the page, the Search box is pre-selected so that you can start typing at once, saving you a couple of seconds and a motion with the mouse.
Thanks.
Adam E.
Comment #106
idebr CreditAttribution: idebr commented@AdamEvertsson The idea for autofocus was discussed and then discarded in issue #2096347: Add "autofocus" attribute to the module search. The synopsis for the issue was:
Comment #107
Wim LeersI really wish we could bring back autofocus to the modules page search field. It's incredibly, incredibly annoying that it's not autofocused. It trips me up every single time, because I expect every single time that it is autofocused.
Comment #108
mgifford@Wim Leers - I understand.. Not sure how to resolve this though. It just causes a lot of problems for assistive technology.
Comment #109
jhodgdonTo make the short story in #106 a little longer, see #2096347-15: Add "autofocus" attribute to the module search, which has an eloquent description of the confusion faced by blind users when the focus starts in a form field, on a page where using that form field is not the primary purpose.
For example, if you go to google.com and the Search box gets the focus, that makes sense, even if you cannot see the page, because the primary purpose of that page is to search the web.
But if you go to the Modules page in a Drupal site, and the Filter box gets the focus, that doesn't make sense to someone who cannot see the whole page at a glance, because the primary purpose of that page is to view/enable/see information about modules, and filtering is just one small tool on the page. So they'd be skipped past the information that would let them understand what the page is all about, and have to navigate backwards to try and understand it.
I wonder if we should put a note about this somewhere in our Accessibility and/or Usability guidelines pages. I for one had never considered this before seeing this part of this issue thread... It seems like we should state the guideline something like this:
I'm not sure exactly where to put it, but it seems like something like this would be good to add to our docs somewhere? And maybe we should also add information on what else would be appropriate to do if you do add focus, like adding the Aria description stuff to the field, or ... I'm not sure, but that other issue has some information on it.
Comment #110
mgifford@jhodgdon agreed about the documentation. It would help meet ATAG 2.0 compliance too.
Comment #111
jhodgdonOK, where should we put the documetnation in #109 then?
Comment #112
Wim LeersI wonder if we can fix this with heuristics + a cookie? The heuristic would be: a cookie signaling "focusing form fields is okay" could be set if the currently logged in user has used the mouse at all.
Comment #113
jhodgdonLet's open a separate issue to discuss all of this. We're getting sidetracked from the Modules page; the issue of form field focus affects many other pages.
Comment #114
jhodgdonAnd on #109, I added something about this to https://www.drupal.org/node/465106 and https://www.drupal.org/node/1637990 [sorry for keeping the off-topic thread going].
Comment #115
BarisW CreditAttribution: BarisW at LimoenGroen commentedSo, can we RTBC this then?
Comment #116
jhodgdonBarisW: If someone has tested the latest patch manually, and it fixes all the problems listed in the issue summary, then we can RTBC it. (Test bot cannot test these things, they need manual testing.)
Comment #117
TR CreditAttribution: TR commentedPatch doesn't apply anymore...
Comment #120
mgiffordJust a re-roll of 120.
Comment #121
Dom. CreditAttribution: Dom. commentedHi,
I add a manual try at patch #120 regarding issue description :
1) Nothing announce me that the list is going to change, but I don't know what is Drupal.Announce so not sure how to validate.
2) Input box wording say "Filter by name or description" and so it does : this point is validated.
3) If I type something in the input field and hit "enter" nothing happens, actually as expected.
4) Activating one module give as per result : "Module modulename has been enabled." and in the case of multiple modules : "x modules have been enabled: module1, module2, ..., moduleX" So this point is actually validated.
In definitive, the patch solves #2, #3 and #4. As per #1, I don't really know how to test Drupal.Announce, but the overall user experience is now confortable. So RTBC+1 for me as per this.
Other notices about the patch:
- This patch also change the wording of filter input in uninstall module page to be coherent with install page, fixing the various point of this issue for uninstall page too. Just adding this notice for scope of the patch.
- Also the patch introduce two Drupal coding standards issues : space characters in empty lines 523 and 529 of ModulesListForm.php.
Adding a patch here where the diff is only these lines removed. Thus I don't change the tag to RTBC only for the purpose of simpletesting this new patch. Otherwise #120 is RTBC+1 for me.
Comment #122
jhodgdon@Dom. : When you upload a new patch, please make an interdiff file to show what changed since the last patch.
Comment #123
Dom. CreditAttribution: Dom. commented@jhodgdon : sorry, still in the learning process !
Comment #124
jhodgdonThanks for the interdiff! Your changes are fine.
So it looks like @Dom. has tested most of the patch, but we still need testing for the accessibility question.
Comment #125
mgiffordI talked with Lucy Greco about this and it's coming close on accessibility, but it needs to say how it has changed.
After typing in "agg", it should say something like "2 modules are available in the modified list."
Typing "user" in the box should give you. "13 modules are available in the modified list."
Lucia Greco said "saying 2 items or 0 items is the information that is missing, telling me it has changed is meanningless without letting me know how it has changed. You would not need more then the number of items you don't even need to say changed to just some thing like 4 files in the list 2 files in the list or even 0 search results."
I think we can get this in here without much more work..
Comment #126
mgiffordComment #128
TR CreditAttribution: TR commentedI reviewed the patch in #121 for items 2), 3), and 4), and found the patch behaved as desired.
I then modified the patch to account for the comments in #125, using the suggested text.
One other change I made: The patch in #121 doesn't search machine names of modules. This is a regression, as before this patch machine names *were* searched. It is important to search machine names in the case of sets of modules like Ubercart or Commerce which have machine names beginning with uc_ and commerce_ respectively and which don't necessarily have "Ubercart" or "Commerce" in either the module name or the module description. I added that feature back in.
Comment #129
TR CreditAttribution: TR commentedOh, and a hint for those trying to review this patch without a screen reader ...
Just edit the JavaScript (core/modules/system/system.modules.js in this case) to use alert() instead of Drupal.announce() - you will then see a pop-up dialog box instead of hearing the announcement.
Comment #130
nod_eslint complains.
Also not too happy about the selector used to get the list of names. If getting the list is a requirement I would rather drop show()/hide() altogether and rely on a class to do the display:none, that way we can filter module list using a class, which does not involve a (slow) sizzle-specific selector. Also the core module page doesn't have sticky headers so that part of the selector can be dropped.
For debugging A11y stuff we have https://www.drupal.org/project/devel_a11y but it's probably outdated by now.
Comment #131
TR CreditAttribution: TR commentedI've attached a new patch with the lint error fixed and the sticky-headers selector removed. The sticky-headers code was not introduced by this patch, it has been in there for years, so this constitutes scope creep on this issue but it's a minor fix.
I didn't recommend devel_a11y because it is horribly broken and the owner hasn't patched it in more than a year. Using alert() is a quick and easy alternative to finding and installing a screen reader and other infrastructure needed to properly test this patch.
Again, that's really old code that has not been changed by any of the proposed patches in this issue. Because this is a rather bigger change, I think it belongs in its own issue and shouldn't be added on to the already too long list of things we're trying to accomplish here. (That code was added 8 Feb 2013 in "Issue #1848064 by sun, Wim Leers, bleen18, nod_, tstoeckler: Added Allow to filter modules by arbitrary search strings on the Modules page.")
Comment #132
nod_Thanks for simplifying that. JS code is RTBC for me.
I'm not complaining about your patch, I know the code is old and I'm probably the one to blame for not cleaning it up earlier when we redesigned the module page. I'm grateful you're working on it :)
I'd rather the change to the search selector be dropped but I can live with it. The search is much less accurate now. Search for "tes" and get "Aggregator, Internal page cache, RDF, Testing, Update Manager, Interface Translation" as a result, before only "Testing" was a match. I'd prefer to have a HTML change and keep the JS as is, something like:
$row['name']['#markup'] = $module->info['name'] . '<span class="hidden"> (' . $module->getName() . ')</span>';
Which gives
Or adding the ID string to the text search, either would address the "uc_" use case without adding false-positives in search results.
Comment #133
TR CreditAttribution: TR commentedThis is because the module *description* is now being included in the search as of the patch in #81. It's not clear to me from the issue summary whether the intention in this issue is to a) allow the description to be searched, or b) just change the help text which mistakenly says the description is being searched when it is not. I've interpreted it as implying that we should be searching the description, and apparently the author of #81 thought the same.
Searching the machine name won't have the same result of over-selecting, as in the majority of cases the machine name and the module name are almost identical. It's just for unusual cases like Testing/Simpletest, or Locale/Interface Translation where it becomes important, and changing the HTML will certainly suffice if we're just concerned with the machine name. But if the consensus is to be able to filter on the description, doing it in .js is less intrusive than adding a lot of hidden description text.
I personally like being able to search the description, I would rather have the filter be too broad than too narrow because then I can always type one more letter to get fewer results - "test" returns just "Testing" and none of those other modules, and "test" also returns "PHPUnit example" from the examples module, which is obviously relevant but would not be found unless the description was being searched.
Comment #134
TR CreditAttribution: TR commentedAnd another aside, I submitted a patch to fix devel_a11y last week when I was working on this issue, so anyone interested in testing the accessibility portion of this patch can try the patch in #2346217: Update code to reflect latest D8 changes or use the suggestion in #129.
Comment #135
mgiffordIt is sounding a lot better. One problem though I ran into was that if you search for "act" you actually get two messages. "4 modules are available in the modified list. 4 modules are available in the modified list."
I don't know what this would be duplicated. I just installed an instance on SimplyTest.me, but haven't evaluated this in another environment.
Comment #136
katewelling CreditAttribution: katewelling as a volunteer commentedI receive the same issue as you mgifford. This may be deliberate however, but I agree it would probably be better if it only searched strings beginning with the letters you entered.
Comment #137
jhodgdonOh, I hadn't realized before from the previous comments that this was the objection!
I totally agree, it should not be searching based on the middle of words in the description. That seems really confusing. Description words searching is probably OK, but only at the start of words.
Comment #138
jhodgdonBesides which, only on things that are visible or that at least become visible as you type. I think the description is normally hidden behind a closed fieldset, and if you start typing and get what looks like totally unrelated stuff up there, that cannot be good UI.
Comment #141
TR CreditAttribution: TR commentedHere's a new patch, which differs from the patch in #131 in the following ways:
1) Matches beginnings of words only (#136, #137). This also fixes the overselection described in #132.
2) Input debounced so that Drupal.announce isn't called for every keystroke (#135, #136).
3) Re-rolled to apply to current HEAD (#140).
Comment #142
BarisW CreditAttribution: BarisW at LimoenGroen commentedWhat's this? defaultPrevent()?
Not sure if this is intended (never heard of that). Otherwise: great improvements!
Comment #143
TR CreditAttribution: TR commentedMozilla was complaining that preventDefault() was deprecated, so I was playing around with that line but I had intended to put it back to preventDefault().
Here's a re-roll with preventDefault() restored. And while I was at it, I removed a couple of mid-string line breaks that were introduced in #53.
Comment #144
jhodgdonThanks for the patches and reviews!
I tested the patch from #143 on simplytest.me, and compared what it is doing to the items in the issue summary that this issue is supposed to address:
1. Accessibility of filter box: not tested by me.
2. Usability of filter box: partially fixed:
- Placeholder text is good.
- Label for the field saying "Search" is gone, good I guess.
- The hover problem is still there. We should not have hover.
- The description under the filter box is wrong. It still says "Enter all or part of a module name.". Probably the text that you currently get when you hover should be put in that description instead.
3. Behavior when you hit Enter - fixed
4. Confusing message when you enable a module - fixed... although I wonder if the "Save configuration" button at the bottom of the form should also be changed to say "Install" ? The button on the uninstall page says "Uninstall".
Anyway, I think we still need to address item 2 a bit. Should be very easy to fix in the patch. Note that the same problem is also on the Uninstall page.
Comment #145
jhodgdonOh and I think that the functionality of the filter box is good. It seems only to be filtering on text that I can actually see.
Comment #146
TR CreditAttribution: TR commentedI'm willing to make changes to the patch, and I know that this issue has morphed a bit, so can you please clarify this apparent contradiction?
From #63:
From #144:
(Note: "the text that you currently get when you hover" is "Enter a part of the module name or description to filter by.")
Comment #147
TR CreditAttribution: TR commentedAlso, #68 introduced changes to the ModulesUninstallForm, which have been retained by all the patches since then, including my most recent patch.
But isn't the uninstall page outside the scope of this issue? If this issue is dealing with the uninstall page, then the title and summary should be fixed. More than that, filtering doesn't work on the uninstall page either before or after this patch - the page structure is different enough from the install page that the JavaScript doesn't work. So including the uninstall page in this issue is opening up more work.
Comment #148
jhodgdonI'm unsure as to whether we need a description on these fields or not... yeah, probably not.
But do we need a field title/label (for accessibility?).
And if we do have a description, it should be right and currently it's misleading/incorrect. Also I dont' think we want hover text, for reasons discussed earlier (such as tablets and phones). Plus we probably just don't need it.
We do have a philosophy of minimizing UI text...
Comment #149
TR CreditAttribution: TR commentedOK, so here's my best guess at resolving the issues raised in #144, along with some improvements in the patch:
1) Changed \Drupal::translation()->formatPlural() to $this->formatPlural() in two places.
2) Changed t() to $this->t() in two places.
3) Changed "Save configuration" button to "Install".
4) Removed hover text from both install and uninstall pages.
5) Changed textfield description from "Enter all or part of a module name." to "Enter a part of the module name or description" on both install and uninstall pages.
I made one additional change not previously discussed in this issue, namely at the top of the page where the instructions tell the user to "Enable the Update Manager module ..." I linked the text to the anchor for enabling the Update Manager module. I can take that out, but I think it's a usability concern - we shouldn't tell users to do something without providing a link to do it.
Note, I did not try to make the uninstall page filtering work in this patch.
Comment #151
TR CreditAttribution: TR commentedChanging the "Save configuration" button to "Install" broke 388 core tests across a large number of core modules. That was unexpectedly disruptive. I'll fix up those tests and post a new patch soon.
Comment #152
TR CreditAttribution: TR commentedOnly difference from #148 is that some tests had to be changed to check for "Install" rather than "Save configuration".
Comment #154
TR CreditAttribution: TR commentedThe uninstall page issue I mentioned in #149 is being addressed in #2512106: Inline templates are XSS filtered incorrectly - when that is fixed then the filtering in this patch should work on the uninstall page.
Comment #155
TR CreditAttribution: TR commentedDifferences from #152:
1) Two more tests had to be changed to check for "Install" rather than "Save configuration".
2) Slight modification to the markup on the uninstall page so that the filtering JavaScript would work properly on both pages.
Note I didn't try to address #2512106: Inline templates are XSS filtered incorrectly here, so filtering by module name on the uninstall page is still broken. But filtering by description will now work with this patch (#155).
Comment #156
Manjit.Singh@TR @jhodgdon I am installing color module then message appears
And when i am uninstalling color module, message appears
Why is it so. I think uninstall message should be
Please check screenshots for the same.
Comment #157
TR CreditAttribution: TR commentedWell, I can change that message on the uninstall page, but it's not part of what we originally set out to do. (See the issue summary). We could go on forever adding "just one more" improvement, but I'd rather not have to keep rolling a new patch every day just to add one more little thing. This issue is already trying to tackle way too many different problems at one time. I think the patch already does everything it's supposed to do, as per the issue summary.
So is there anything else? The patch is RTBC otherwise? Please test it against all the items in the issue summary now, because I don't want to keep chasing after a moving target.
Comment #158
mgiffordSeems like a good idea @TR - Any further concerns should be dealt with as a new issue.
Comment #159
jhodgdon#156 does seem out of scope for this issue. I noticed that too -- the messages are not the same -- but at least both of them say something about installing/uninstalling modules and not "the configuration has changed".
So. I reviewed the code in the latest patch, and it seems to be fine to me.
I also ran the patch through simplytest.me and reviewed what is actually in the issue summary. I believe all of it has been fixed... that's assuming the accessibility announcement is valid.
That was last tested in comment #81. The code has changed significantly since then... I think maybe we should test it again before setting this to RTBC? Aside from that, I think it's ready.
Comment #160
Manjit.Singh@jhodgdon @mgifford Are we creating a new issue what i have suggested in #156 ?
Comment #161
jhodgdon@Manjit.Singh: I haven't done anything... please file an issue. :)
Comment #162
Manjit.Singh@jhodgdon created a new issue. Please check #2532652: Make the install and uninstall message the same
Comment #163
TR CreditAttribution: TR commentedRe: @jhodgdon in #159:
Yes, it would be very nice if @BarisW or someone else could make another screencast to demonstrate this for those who can't test it themselves, but the announcement functionality has been tested several times since #81.
A good evaluation was given in #125, and the patch in #128 accounted for that feedback.
Further feedback on the announce function was given in #135 and #136, and the patch in #141 resolved those issues.
I described a way to test the announce function in #129, and I also submitted a large patch to make devel_a11y work in the current version of D8 - that patch was committed so you can now use devel_a11y to test as well. OR, you could just look at your browser's dev console and see what the #drupal-live-announce div contains and how it changes when you type in the filter box, e.g.:
The patch in #155 still applies, and without any offsets ...
Comment #164
mgiffordI tested this with NVDA and it seems to work fine. I'm happy to mark this RTBC.
Comment #165
Wim LeersNit: 80 cols.
Also, why exactly is this necessary? This now uses regular expressions, rather than just using
toLowerCase()
, so it's likely much slower.Why not just
Drupal.announce(Drupal.t(
on one line?Why isn't this comparison necessary anymore?
Yay! :)
Comment #166
TR CreditAttribution: TR commentedFixed in attached patch. Do we really need to go back to "Needs review" and delay this for another month or two just because of a couple of extra characters in a comment?
As the above too-long comment states, the requirement is to match the filter pattern only at the beginnings of words. (See #136, #137). The regexp is a clear, efficient, expressive, understandable, and compact way to accomplish that. Do you know a better way? Previously, toLowerCase().indexOf() was used to match the query characters anywhere in the target string, but that's not what we want anymore. As for "slower", slower than what alternative? Propose another way to accomplish the same task then we can compare based on its merits as well as speed, but you can't compare speed to the old way that doesn't have the same functionality. Yeah, it takes more time to do more.
The indentation has been like that for a year, since #12. Numerous people have looked at it since. I didn't write that, but personally I find it easier to read/understand that way. It's well within our coding standards to indent code for readability so I don't see a reason to change it. It's going to get aggregated/compressed anyway so shaving a couple of bytes off doesn't help, but leaving them in makes the code clearer.
@nod_ ran eslint against that exact piece of code back in #130 - that error didn't show up. Since then, I have run eslint locally on every patch I have submitted here and I have not seen that error either. So you must have a newer/older version of eslint. Regardless, until we have an "official" version of eslint running in the testbot, the fact that it passes eslint for @nod_ should be enough.
That code has been there since #53. The old way of installing modules just displays "The configuration options have been saved" after installing. It did that by getting a list of module machine names (stored in $before), enabling them, then getting a new list and comparing it to $before. If there was any difference the page displayed that message.
One of the main tasks of this patch it to change the message to something meaningful, which means (among other things) listing the names of the modules that have been enabled - this has been raised as an issue repeatedly for more than 8 years (See #134849: Display informative messages upon module install / uninstall for example). That comparison to $before doesn't tell us what changed and doesn't give us actual module names (not machine names), so it's not needed. And in fact the $before variable has been removed entirely by the patch.
Comment #167
jhodgdonI think Wim's comments were addressed. Thanks!
Comment #168
alexpottI've manually tested the patch and works as expected. Nice to see some progress on tidying up the install page after the removal of the disable option.
Committed 37c1ee8 and pushed to 8.0.x. Thanks!
I get the same eslint errors and since I run eslint on all comitted JS I'm going to fix this on commit. And it matches the style we already have eg:
Comment #171
mgifford