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.

CommentFileSizeAuthor
#166 interdiff-2056089-155-166.txt588 bytesTR
#166 core-js-module-list-2056089-166.patch31.91 KBTR
#156 Uninstall-drupal8.png268.22 KBManjit.Singh
#156 Extend-drupal8.png333.58 KBManjit.Singh
#155 interdiff-2056089-152-155.txt3.12 KBTR
#155 core-js-module-list-2056089-155.patch31.91 KBTR
#152 interdiff-2056089-148-152.txt17.08 KBTR
#152 core-js-module-list-2056089-152.patch29.5 KBTR
#149 interdiff-2056089-143-148.txt4.77 KBTR
#149 core-js-module-list-2056089-148.patch14.11 KBTR
#143 interdiff-2056089-141-143.txt2.11 KBTR
#143 core-js-module-list-2056089-143.patch12.35 KBTR
#141 interdiff-2056089-131-141.txt1.96 KBTR
#141 core-js-module-list-2056089-141.patch12.31 KBTR
#135 12-modules-or-4.png90.9 KBmgifford
#131 interdiff-2056089-128-131.txt975 bytesTR
#131 core-js-module-list-2056089-131.patch11.42 KBTR
#128 interdiff-2056089-121-128.txt1.42 KBTR
#128 core-js-module-list-2056089-128.patch11.12 KBTR
#123 interdiff-2056089-120-121.txt979 bytesDom.
#121 core-js-module-list-2056089-121.patch10.59 KBDom.
#120 core-js-module-list-2056089-120.patch10.62 KBmgifford
#103 Screen Shot 2014-12-16 at 2.52.15 PM.png193.88 KBmgifford
#103 Screen Shot 2014-12-16 at 2.50.38 PM.png37.07 KBmgifford
#102 interdiff-100-102.txt657 bytesBarisW
#102 core-js-module-list-2056089-102.patch9.82 KBBarisW
#100 core-js-module-list-2056089-100.patch9.82 KBBarisW
#98 core-js-module-list-2056089-98.patch9.46 KBBarisW
#91 core-js-module-list-2056089-91.patch9.97 KBmgifford
#89 core-js-module-list-2056089-89b.patch9.84 KBmgifford
#81 interdiff-68-81.txt1.88 KBBarisW
#81 core-js-module-list-2056089-81.patch9.64 KBBarisW
#81 modules-page.png78.29 KBBarisW
#81 modules.mov2.84 MBBarisW
#69 Screen Shot 2014-11-17 at 1.01.00 PM.png56.91 KBmgifford
#69 Screen Shot 2014-11-17 at 1.00.17 PM.png90.78 KBmgifford
#68 core-js-module-list-2056089-68.patch8.64 KBmgifford
#67 core-js-module-list-2056089-67.patch7.81 KBmgifford
#66 module-input.png36.28 KBBarisW
#60 core-js-module-list-2056089-60.patch7.84 KBBarisW
#60 interdiff-58-60.txt729 bytesBarisW
#58 interdiff-55-58.txt799 bytesBarisW
#58 core-js-module-list-2056089-58.patch7.35 KBBarisW
#56 core-js-module-list-2056089-55.patch7.33 KBBarisW
#53 uninstalling-modules.png14.19 KBBarisW
#53 2-modules-enabled.png19.7 KBBarisW
#53 1-module-enabled.png16.04 KBBarisW
#53 core-js-module-list-2056089-53.patch5.55 KBBarisW
#43 interdiff-2056089-42-43.txt2.32 KBrodvolpe
#43 core-js-module-list-2056089-43.patch4.08 KBrodvolpe
#42 core-js-module-list-2056089-42.patch2.75 KBnambisolo
#42 interdiff-2056089-23-42.txt1.56 KBnambisolo
#41 ui_problems_on_the-2056089-41.patch.patch2.31 KBrodvolpe
#38 core-js-module-list-2056089-38.patch1.79 KBkatewelling
#23 core-js-module-list-2056089-23.patch1.19 KBBarisW
#12 core-js-module-list-2056089-12.patch903 bytesmgifford
#10 core-js-module-list-2056089-10.patch786 bytesnod_
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

jhodgdon’s picture

Oh wait. "system_modules" is from a test module I had enabled. Ignore that. :)

nod_’s picture

Issue tags: +JavaScript, +Usability, +d8ux

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.

jhodgdon’s picture

Issue tags: +Accessibility

RE #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.

nod_’s picture

tim.plunkett’s picture

Title: UI problems on the Modules/Extend page » Prevent find-as-you-type inputs from responding to the return key
Priority: Major » Normal

Find-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)

nod_’s picture

Status: Active » Postponed

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.

nod_’s picture

Status: Postponed » Needs work
tstoeckler’s picture

Title: Prevent find-as-you-type inputs from responding to the return key » UI problems on the Modules/Extend page Prevent find-as-you-type inputs from responding to the return key Priority: Major

Seems @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.

tstoeckler’s picture

Title: UI problems on the Modules/Extend page Prevent find-as-you-type inputs from responding to the return key Priority: Major » UI problems on the Modules/Extend page

Whoops that Ctrl+C picked up more than I intended.

nod_’s picture

Status: Needs work » Needs review
FileSize
786 bytes

Added code to prevent form from submitting is hitting enter key.

For a11y I don't know.

mgifford’s picture

This 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:

Drupal.announce(
  Drupal.t('The list of modules has been altered by a search.')
);

We should test that out, but it should do the trick.

mgifford’s picture

Re-rolling with Drupal.announce. Is this what we want it to say to a screen reader?

The list of modules has been altered by a search.

mgifford’s picture

Issue tags: +dcamsa11y
jhodgdon’s picture

I 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).

mgifford’s picture

The 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.

jhodgdon’s picture

The summary for this issue lists (a) - (f) problems. They all need to be retested, and then we can see whether they still need fixing.

mgifford’s picture

I can spin a) off into a separate issue like c) & e) were. We can make this a meta issue and create child elements.

jhodgdon’s picture

I've updated the issue summary with a coherent list of what still needs to be done.

mgifford’s picture

@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.

jhodgdon’s picture

Issue summary: View changes

@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.

mgifford’s picture

Issue summary: View changes

Good 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.

jhodgdon’s picture

Issue summary: View changes

You'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.

BarisW’s picture

The 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.

BarisW’s picture

Issue tags: +Amsterdam2014
mgifford’s picture

Thanks for fixing the JS error. It's looking better!

Désiré’s picture

As 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.

mgifford’s picture

@Désiré - Can you add that to the issue summary? It's a good point.

Désiré’s picture

Issue summary: View changes
Bojhan’s picture

Sorry, but we are not introducing new features here. Only bug fixing, new features -> new issue.

Désiré’s picture

Ok, 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?

Bojhan’s picture

@Desire that would be two separate issues I guess. I think we already search on the machine name, but I might be wrong.

jhodgdon’s picture

Bojhan / #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!

Désiré’s picture

BarisW’s picture

Great, thanks! Does that mean that this one is RTBC'ed?

Désiré’s picture

Issue summary: View changes
jhodgdon’s picture

I 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.

katewelling’s picture

I am looking to see if I can change the text on the search box from "Search" to "Filter Modules"

katewelling’s picture

Added a patch that will change the name on the Search box from "Search" to "Filter modules". Will continue to work on this issue.

Kate

katewelling’s picture

I 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

rodvolpe’s picture

I 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.

rodvolpe’s picture

Ok this is my first fix so not sure it is correct. Follow issue #4. Confusing message when you enable a module.

nambisolo’s picture

Addressed 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.

rodvolpe’s picture

This change addresses part 4 Confusing message when you enable a module.
This is 41 merged into 42.

BarisW’s picture

Thanks all! This looks pretty good to me! I'm not sure if we need to add a t() wrapped around the module names?

mgifford’s picture

@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.

BarisW’s picture

I 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!

The last submitted patch, 41: ui_problems_on_the-2056089-41.patch.patch, failed testing.

The last submitted patch, 42: core-js-module-list-2056089-42.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 43: core-js-module-list-2056089-43.patch, failed testing.

BarisW’s picture

Apparently, the tests need to be updated as well.

@Rod; will you take this on?

rodvolpe’s picture

I can do @barry. What do I need to do exactly please? I am going home tonight but I can do it tomorrow night.

jhodgdon’s picture

@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.

BarisW’s picture

Status: Needs work » Needs review
FileSize
5.55 KB
16.04 KB
19.7 KB
14.19 KB

Here's a patch that updates the tests.

Other changes

Some screenshots

  • Uninstalling modules
  • Enabling 2 modules
  • Enabling 1 module
BarisW’s picture

Ignore the patch in 53. I forgot to roll in the JS changes above. Will provide a new patch.

Status: Needs review » Needs work

The last submitted patch, 53: core-js-module-list-2056089-53.patch, failed testing.

BarisW’s picture

Status: Needs work » Needs review
FileSize
7.33 KB

Status: Needs review » Needs work

The last submitted patch, 56: core-js-module-list-2056089-55.patch, failed testing.

BarisW’s picture

Status: Needs work » Needs review
FileSize
7.35 KB
799 bytes

Try again testbot.

Status: Needs review » Needs work

The last submitted patch, 58: core-js-module-list-2056089-58.patch, failed testing.

BarisW’s picture

Status: Needs work » Needs review
FileSize
729 bytes
7.84 KB

Apologies for the noise. Should be green now (forgot one test).

jhodgdon’s picture

Before 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!

BarisW’s picture

I think that all has been fixed now.

  1. Fixed. The screen reader now reads "The list of modules has been altered by a search."
  2. Fixed. The search box now has a title "Filter modules" and a description "Enter a part of the module name or description to filter by."
  3. Fixed. Enter is ignored. Nothing happens when hitting Enter.
  4. Fixed. It now says "2 modules have been enabled: Content Translation, Language." or "Module Language has been enabled."

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?

jhodgdon’s picture

Thanks! 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.

jhodgdon’s picture

Status: Needs review » Needs work

Forgot status, and I forgot to say everything else in the issue summary seems to be OK (except I didn't test accessibility). Thanks!

Bojhan’s picture

Screenshot?

BarisW’s picture

FileSize
36.28 KB

Module input

This is how it looks with the patch from #60. The other screenshots can be found in #53.

mgifford’s picture

Status: Needs work » Needs review
FileSize
7.81 KB

Rerolled 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.

mgifford’s picture

Actually 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.

mgifford’s picture

Screenshots of module install/uninstall pages.

Install

Uninstall

mgifford’s picture

Testing this with VoiceOver, I should hear this. At the moment though I'm not even seeing it in the source.

+          Drupal.announce(
+            Drupal.t('The list of modules has been altered by a search.')
+          );

Not sure if it's done incorrectly in core/modules/system/system.modules.js

jhodgdon’s picture

RE #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?)?

Bojhan’s picture

I 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").

jhodgdon’s picture

OK. 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.

Bojhan’s picture

@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 :)

jhodgdon’s picture

OK. 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?

Bojhan’s picture

Sounds 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.

BarisW’s picture

I'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.

jhodgdon’s picture

RE #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.

Bojhan’s picture

@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?

jhodgdon’s picture

Status: Needs review » Needs work

Perfectly! 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).

BarisW’s picture

Status: Needs work » Needs review
FileSize
2.84 MB
78.29 KB
9.64 KB
1.88 KB

Here's a patch that does exactly that:

  • Removed the description
  • Removed the field label by hiding it
  • Changed the placeholder text

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.

jhodgdon’s picture

Status: Needs review » Needs work

Looks 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.

jhodgdon’s picture

And 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

Bojhan’s picture

Assigned: Unassigned » Wim Leers

Hmm, I'd like WimLeers to chime in on the description - I thought there was reasons why we didnt do that.

BarisW’s picture

We could also simplify the placeholder with 'Filter module list' and don't make it explicit what is searched on? Would that work?

jhodgdon’s picture

The 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?

Wim Leers’s picture

Assigned: Wim Leers » Unassigned
  1. I looked it up in the original issue (#1848064: Allow to filter modules by arbitrary search strings on the Modules page) and AFAICT there was no reason to not search the description. The only reason it wasn't done then is because it went beyond the MVP.
  2. Searching != filtering. This is filtering. So +1 to jhodgdon's remarks about that.
    +++ b/core/modules/system/src/Form/ModulesListForm.php
    @@ -169,10 +169,10 @@
    +      '#title' => $this->t('Search'),
    ...
    +      '#placeholder' => $this->t('Filter by module name'),
    

    These are inconsistent.

  3. +++ b/core/modules/system/src/Form/ModulesListForm.php
    @@ -169,10 +169,10 @@
    +      '#placeholder' => $this->t('Filter by module name'),
    
    +++ b/core/modules/system/system.modules.js
    @@ -30,7 +30,7 @@
    -          var $sources = $row.find('.table-filter-text-source');
    +          var $sources = $row.find('.module-name, .module-description');
    

    These are contradicting.

jhodgdon’s picture

OK, 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.

mgifford’s picture

I 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.

mgifford’s picture

Status: Needs work » Needs review
mgifford’s picture

@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.

The last submitted patch, 89: core-js-module-list-2056089-89b.patch, failed testing.

Status: Needs review » Needs work

The last submitted patch, 91: core-js-module-list-2056089-91.patch, failed testing.

Status: Needs work » Needs review

Status: Needs review » Needs work

The last submitted patch, 91: core-js-module-list-2056089-91.patch, failed testing.

BarisW’s picture

It 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 :(

jhodgdon’s picture

It also looks like more UI changes are being discussed on #2056089: UI problems on the Modules/Extend page. We should probably combine these issues?

BarisW’s picture

Status: Needs work » Needs review
FileSize
9.46 KB

Here's a re-roll to get the tests green again.

Status: Needs review » Needs work

The last submitted patch, 98: core-js-module-list-2056089-98.patch, failed testing.

BarisW’s picture

Status: Needs work » Needs review
FileSize
9.82 KB

I think I missed a bit.

Status: Needs review » Needs work

The last submitted patch, 100: core-js-module-list-2056089-100.patch, failed testing.

BarisW’s picture

Status: Needs work » Needs review
FileSize
9.82 KB
657 bytes

Forgot one (important) line. Sorry for the noise. Try again, testbot.

mgifford’s picture

@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.

Hover over Install

Uninstall

jhodgdon’s picture

Oh 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.

AdamEvertsson’s picture

On 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.

idebr’s picture

@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:

Long story short, from a screen reader user point of view let's close this issue as won't fix...

Wim Leers’s picture

I 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.

mgifford’s picture

@Wim Leers - I understand.. Not sure how to resolve this though. It just causes a lot of problems for assistive technology.

jhodgdon’s picture

To 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:

Form field focus

While it can seem convenient to give a form field the initial focus on a page, this should only be done if that form field is or enables the primary purpose of the page. For example, on a Search This Site page, giving focus to a Search Keywords field would probably be appropriate, because searching is the primary purpose of the page. But on a Modules List page, giving focus to a Filter Keywords field would be inappropriate, because filtering the module list is far from being the page's primary purpose.

The main reason for this guideline is that a side effect of giving the focus to a form field is that screen readers will start reading the page at that field, skipping the top of the page. Users of screen readers will therefore miss out on the context provided by starting at the top, and have difficulty understanding what the page is about, since (unlike users without vision difficulties) they cannot see the entire page at a glance to understand it.

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.

mgifford’s picture

Issue tags: +atag

@jhodgdon agreed about the documentation. It would help meet ATAG 2.0 compliance too.

jhodgdon’s picture

OK, where should we put the documetnation in #109 then?

Wim Leers’s picture

I 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.

jhodgdon’s picture

Let'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.

jhodgdon’s picture

And 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].

BarisW’s picture

So, can we RTBC this then?

jhodgdon’s picture

BarisW: 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.)

TR’s picture

Status: Needs review » Needs work

Patch doesn't apply anymore...

The last submitted patch, 102: core-js-module-list-2056089-102.patch, failed testing.

mgifford’s picture

Status: Needs work » Needs review
FileSize
10.62 KB

Just a re-roll of 120.

Dom.’s picture

Hi,
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.

jhodgdon’s picture

@Dom. : When you upload a new patch, please make an interdiff file to show what changed since the last patch.

Dom.’s picture

FileSize
979 bytes

@jhodgdon : sorry, still in the learning process !

jhodgdon’s picture

Thanks 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.

mgifford’s picture

I 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..

mgifford’s picture

Status: Needs review » Needs work

TR’s picture

Status: Needs work » Needs review
FileSize
11.12 KB
1.42 KB

I 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.

TR’s picture

Oh, 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.

nod_’s picture

Status: Needs review » Needs work

eslint complains.

core/modules/system/system.modules.js
  60:14  error  Unexpected token ;

✖ 1 problem (1 error, 0 warnings)

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.

TR’s picture

Status: Needs work » Needs review
FileSize
11.42 KB
975 bytes

I'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.

Also not too happy about the selector used to get the list of names.

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.")

nod_’s picture

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

<label id="module-simpletest" for="edit-modules-core-simpletest-enable" class="module-name table-filter-text-source">Testing<span class="hidden"> (simpletest)</span></label>

Or adding the ID string to the text search, either would address the "uc_" use case without adding false-positives in search results.

TR’s picture

Search for "tes" and get "Aggregator, Internal page cache, RDF, Testing, Update Manager, Interface Translation" as a result, before only "Testing" was a match.

This 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.

TR’s picture

And 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.

mgifford’s picture

Issue summary: View changes
FileSize
90.9 KB

It 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.

firebug screenshot

katewelling’s picture

I 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.

jhodgdon’s picture

Oh, 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.

jhodgdon’s picture

Besides 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.

Status: Needs review » Needs work

The last submitted patch, 131: core-js-module-list-2056089-131.patch, failed testing.

TR’s picture

Status: Needs work » Needs review
FileSize
12.31 KB
1.96 KB

Here'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).

BarisW’s picture

+++ b/core/modules/system/js/system.modules.js
@@ -80,7 +82,7 @@
-          event.preventDefault();
+          event.defaultPrevent();

What's this? defaultPrevent()?
Not sure if this is intended (never heard of that). Otherwise: great improvements!

TR’s picture

Mozilla 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.

jhodgdon’s picture

Status: Needs review » Needs work

Thanks 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.

jhodgdon’s picture

Oh 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.

TR’s picture

I'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:

... 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.

From #144:

- 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.

(Note: "the text that you currently get when you hover" is "Enter a part of the module name or description to filter by.")

TR’s picture

Also, #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.

jhodgdon’s picture

I'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...

TR’s picture

Status: Needs work » Needs review
FileSize
14.11 KB
4.77 KB

OK, 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.

Status: Needs review » Needs work

The last submitted patch, 149: core-js-module-list-2056089-148.patch, failed testing.

TR’s picture

Changing 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.

TR’s picture

Status: Needs work » Needs review
FileSize
29.5 KB
17.08 KB

Only difference from #148 is that some tests had to be changed to check for "Install" rather than "Save configuration".

Status: Needs review » Needs work

The last submitted patch, 152: core-js-module-list-2056089-152.patch, failed testing.

TR’s picture

The 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.

TR’s picture

Status: Needs work » Needs review
FileSize
31.91 KB
3.12 KB

Differences 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).

Manjit.Singh’s picture

FileSize
333.58 KB
268.22 KB

@TR @jhodgdon I am installing color module then message appears

Module Color has been enabled.

And when i am uninstalling color module, message appears

The selected modules have been uninstalled.

Why is it so. I think uninstall message should be

The color modules have been uninstalled.

Please check screenshots for the same.

alt

alt

TR’s picture

Well, 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.

mgifford’s picture

Seems like a good idea @TR - Any further concerns should be dealt with as a new issue.

jhodgdon’s picture

#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.

Manjit.Singh’s picture

@jhodgdon @mgifford Are we creating a new issue what i have suggested in #156 ?

jhodgdon’s picture

@Manjit.Singh: I haven't done anything... please file an issue. :)

Manjit.Singh’s picture

TR’s picture

Re: @jhodgdon in #159:

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.

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.:

  <div aria-busy="false" aria-live="polite" class="visually-hidden" id="drupal-live-announce">
      4 modules are available in the modified list.
  </div>

The patch in #155 still applies, and without any offsets ...

mgifford’s picture

Status: Needs review » Reviewed & tested by the community
Related issues: +#2030659: Disable 'enter' submission on extend page

I tested this with NVDA and it seems to work fine. I'm happy to mark this RTBC.

Wim Leers’s picture

Status: Reviewed & tested by the community » Needs work
  1. +++ b/core/modules/system/js/system.modules.js
    @@ -30,17 +30,19 @@
    +        // Case insensitive search expression to find query at the beginning of a word.
    

    Nit: 80 cols.

  2. +++ b/core/modules/system/js/system.modules.js
    @@ -30,17 +30,19 @@
    +        var re = new RegExp('\\b' + query, 'i');
    

    Also, why exactly is this necessary? This now uses regular expressions, rather than just using toLowerCase(), so it's likely much slower.

  3. +++ b/core/modules/system/js/system.modules.js
    @@ -59,6 +61,13 @@
    +          Drupal.announce(
    +            Drupal.t(
    

    Why not just Drupal.announce(Drupal.t( on one line?

  4. +++ b/core/modules/system/js/system.modules.js
    @@ -59,6 +61,13 @@
    +              { '!modules': $rowsAndDetails.find('tbody tr:visible').length }
    
    $ eslint core/modules/system/js/system.modules.js
    
    core/modules/system/js/system.modules.js
      68:14  error  There should be no space after '{'   space-in-brackets
      68:76  error  There should be no space before '}'  space-in-brackets
    
    ✖ 2 problems (2 errors, 0 warnings)
    
  5. +++ b/core/modules/system/src/Form/ModulesListConfirmForm.php
    @@ -184,12 +181,12 @@ public function submitForm(array &$form, FormStateInterface $form_state) {
    -    if ($before != $this->moduleHandler->getModuleList()) {
    

    Why isn't this comparison necessary anymore?

  6. +++ b/core/modules/system/src/Form/ModulesListForm.php
    @@ -183,14 +183,15 @@ public function buildForm(array $form, FormStateInterface $form_state) {
    -      '#placeholder' => $this->t('Enter module name'),
    +      '#placeholder' => $this->t('Filter by name or description'),
    +      '#description' => $this->t('Enter a part of the module name or description'),
    

    Yay! :)

TR’s picture

Status: Needs work » Needs review
FileSize
31.91 KB
588 bytes
  1.     +++ b/core/modules/system/js/system.modules.js
        @@ -30,17 +30,19 @@
        +        // Case insensitive search expression to find query at the beginning of a word.

    Nit: 80 cols.

    Fixed 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?

  2.     +++ b/core/modules/system/js/system.modules.js
        @@ -30,17 +30,19 @@
        +        var re = new RegExp('\\b' + query, 'i');

    Also, why exactly is this necessary? This now uses regular expressions, rather than just using toLowerCase(), so it's likely much slower.

    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.

  3.     +++ b/core/modules/system/js/system.modules.js
        @@ -59,6 +61,13 @@
        +          Drupal.announce(
        +            Drupal.t(

    Why not just Drupal.announce(Drupal.t( on one line?

    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.

  4.     +++ b/core/modules/system/js/system.modules.js
        @@ -59,6 +61,13 @@
        +              { '!modules': $rowsAndDetails.find('tbody tr:visible').length }
        $ eslint core/modules/system/js/system.modules.js
    
        core/modules/system/js/system.modules.js
          68:14  error  There should be no space after '{'   space-in-brackets
          68:76  error  There should be no space before '}'  space-in-brackets
    
        ✖ 2 problems (2 errors, 0 warnings)

    @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.

  5.     +++ b/core/modules/system/src/Form/ModulesListConfirmForm.php
        @@ -184,12 +181,12 @@ public function submitForm(array &$form, FormStateInterface $form_state) {
        -    if ($before != $this->moduleHandler->getModuleList()) {

    Why isn't this comparison necessary anymore?

    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.

jhodgdon’s picture

Status: Needs review » Reviewed & tested by the community

I think Wim's comments were addressed. Thanks!

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

I'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!

core/modules/system/js/system.modules.js
  68:14  error  There should be no space after '{'   space-in-brackets
  68:76  error  There should be no space before '}'  space-in-brackets

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:

readyStateText = xmlhttp.status === 0 ? ("\n" + Drupal.t("ReadyState: !readyState", {'!readyState': xmlhttp.readyState})) : "";

  • alexpott committed 37c1ee8 on 8.0.x
    Issue #2056089 by BarisW, TR, mgifford, rodvolpe, Dom., nambisolo, nod_...

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.

mgifford’s picture

Issue tags: +keyboard focus