Created from https://www.drupal.org/project/drupal/issues/2809635#comment-12433583

Yoroy says we should only be "showing the warning on the install profile selection screen only on selecting the Umami profile".

So, the Umami profile has the same prominence as the other profiles, but when you select the radio button for it, you then see the warning. If you see the warning before then, it looks like it applies to all of the profiles.

CommentFileSizeAuthor
#74 umami-selector.gif226.23 KBwebchick
#61 2938185-61.patch7.84 KBtedbow
#61 interdiff-59-61.txt1.29 KBtedbow
#60 interdiff-6374f7.txt1.31 KBlarowlan
#59 Select_an_installation_profile__umami_selected_Drupal.png224.38 KBEli-T
#59 Select_an_installation_profile___Drupal.png224.3 KBEli-T
#59 interdiff_2938185_52_59.patch1.33 KBEli-T
#59 2938185_59_update_strings.patch7.13 KBEli-T
#53 2938185-52.patch7.21 KBacbramley
#53 interdiff-51-52.txt4.32 KBacbramley
#51 interdiff-50-51.txt1.64 KBacbramley
#51 2938185-51.patch2.89 KBacbramley
#50 2938185-50.patch2.92 KBnavneet0693
#50 interdiff-46-50.txt3.56 KBnavneet0693
#47 Demo profile - installation.mov2.61 MBnavneet0693
#46 interdiff-33-46.txt829 bytesnavneet0693
#46 2938185-46.patch2.47 KBnavneet0693
#33 2938185-33.patch2.49 KBtedbow
#33 interdiff-29-33.txt3.72 KBtedbow
#29 2938185-29.patch3.06 KBtedbow
#29 interdiff-22-29.txt3.26 KBtedbow
#22 interdiff-14-22.txt1.75 KBJayKandari
#22 2938185-22-show-warning-on-experimental-profile.patch2.63 KBJayKandari
#17 updated_warning_beginning.png129.24 KBtedbow
#17 current_warniing_end.png130.12 KBtedbow
#17 2938185-17-move_warning.patch1.1 KBtedbow
#14 interdiff-5-14.txt2.05 KBJayKandari
#14 2938185-14-show-warning-on-experimental-profile.patch2.27 KBJayKandari
#5 experimental-profile-poc.gif373.34 KBJayKandari
#5 2938185-5-show-warning-on-experimental-profile.patch2.32 KBJayKandari
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

markconroy created an issue. See original summary.

webchick’s picture

Priority: Normal » Major
Issue tags: -beta blocker +Umami beta blocker

Since this is a beta blocker, escalating to major.

andrewmacpherson’s picture

Issue tags: +Accessibility

We'll need to convey the warning message to assistive tech.

The current role="contentinfo" is definitely wrong, that's for info that relates to the whole page. We've got a bug about that for drupal warning messages in general.

I think we could give the Umami radio button a double IDREF for it's aria-describedby attribute. (Need to test that to see how it actually performs.) Failing that we could use drupal.announce() but that doesn't seem right for a message that is displayed visibly.

markconroy’s picture

Title: When installing Umami, only show warning when 'Demo Umami' radio button is selected » When installing Umami, only show warning if 'Demo Umami' radio button is selected
JayKandari’s picture

Hello,

This is a POC. Not sure how to do without changing the install_select_profile_formform itself. This form cannot be hook altered. Thus implemented a solution, which kind of works, plus if another installation profile has 'warning' value defined in their info.yml file. They are also shown as warning when selected.

Kindly review. :)

I've also attached a preview of how it looks like.

Thanks!

markconroy’s picture

This looks great.

We did mention on the weekly call that we’d try to solve it with CSS, but if this works (and from testing the patch it seems fine), I’d be happy to go with this.

I'll leave it as 'Needs review' for now and let one of the backenders check the code and set to RTBC

navneet0693’s picture

This is an awesome solution! I am willing to issue RTBC, but still want any one from core team to review this.

  1. +++ b/core/lib/Drupal/Core/Installer/Form/SelectProfileForm.php
    @@ -72,6 +72,21 @@ public function buildForm(array $form, FormStateInterface $form_state, $install_
    +          '#states' => array(
    +            'visible' => array(
    

    Just a suggestion to use short array syntax.

  2. +++ b/core/profiles/demo_umami/demo_umami.info.yml
    @@ -1,7 +1,8 @@
    +warning: 'Warning: this is a sample website, you should not use it as the basis for your website.'
    

    What if we append 'Warning: ' text in the #markup itself in the SelectProfileForm ? So, that there can be no need of specially adding in info.yml ?

David_Rothstein’s picture

+      if (isset($profiles[$profile_name]['warning'])) {
+        $warning = '<div role="contentinfo" aria-label="Warning message" class="messages messages--warning">';
+        $warning .= $this->t($profiles[$profile_name]['warning']);

Shouldn't the aria-label be run through $this->t() also?

In general, this looks like a nice patch and a neat solution. However, I am not sure it is a good idea to expose a "warning" functionality to all install profiles, as a general feature.

  • First, adding a capability for other (experimental) profiles to do this seems to be more what #2822414: Redesign the 'install profile selection' installer screen to allow for experimental profiles and more information is about (or if not, then I'm not sure why this is a separate issue from that one)?
  • Second and more important, if Drupal exposes a "warning" functionality in the installer as a standard capability of install profiles, it seems likely that developers will... overuse the warnings. Choosing an install profile is supposed to be a simple, happy step that gets people started on their Drupal site-building journey. We shouldn't encourage developers to throw warnings in people's face on this screen.

If the goal is actually to introduce something generic here, my suggestion would be to think of this as more of an "informational" message, styled as such by default. That is primarily what is happening here; when people click on the radio button they get some extra information about the profile (beyond the main profile description) which appears on the screen. So as a result, I would also suggest that the .info.yml key for this be something more like extra_description. (And if the Umami profile really wants to keep its message as a warning, pending discussion of that in other issues, it could add its own HTML to that new .info.yml key to do so - similar to what it does now.)

andrewmacpherson’s picture

re: #8

First, adding a capability for other (experimental) profiles to do this seems to be more what #2822414: Redesign the 'install profile selection' installer screen to allow for experimental profiles and more information: is about (or if not, then I'm not sure why this is a separate issue from that one)?

I also think we should be focusing on that issue. The current warning was put into the Umami profile info as a quick fix for MVP purposes. The approach here is just making an Umami-specific workarounds more complicated.

Second and more important, if Drupal exposes a "warning" functionality in the installer as a standard capability of install profiles,

There are now several issues with patches around this functionality. Some kind of key in the profile info file will be needed for #2822414: Redesign the 'install profile selection' installer screen to allow for experimental profiles and more information and #2934374: Add permanent warning message for experimental profiles to avoid its usage on production sites. It's worth pausing to get these issues tackled in the right order.

andrewmacpherson’s picture

Status: Needs review » Needs work

Accessibility review:

1. Remove the role="contentinfo" and aria-label="Warning message". This role is intended for regions which contain information about the page as a whole. It's not appropriate for this warning message which applies to a particular selection among the radio buttons.

2. The warning message isn't conveyed to assistive technology. On page load, the warning message has CSS display:none from the States API :visible. So the information isn't conveyed to assistive tech. The warning needs to be conveyed to AT users when the Umami radio is selected.

Possible approaches:

  • Use an aria-describedby to programatically associate the Umami radio button with the warning message. This means the umami radio button will be described by TWO elements (the warning first, then the form #description) while the Standard and Minimal will be described by one element (the form #description).
  • Use role="alert" or aria-live="assertive" on the warning message (it must be on same element which has display:none toggled. This might not be a robust solution! In theory this should cause the message to be announced immediately when it becomes visible, but in practice some screen readers don't give priority to assertive ARIA live regions yet. The message may not be announced. Needs testing.
  • Use Drupal.announce() to convey the message when it appears. This also needs testing, there's no guarantee it will be spoken. I expect some screen readers will treat the radio button's <label> and aria-describedby as higher priority, and omit to annouce the warning.

My hunch is the first one will be the most reliable, but we'll see. I'd like to play around with these approaches with some screen readers.

markconroy’s picture

Hi Andrew,

We are going to look at the specific HTML and content in the follow up issue - https://www.drupal.org/project/drupal/issues/2938803 - "Finalise the wording about the Umami profile on the install screen".

For this issue we simply want to solve the issue of showing the warning if the Umami Profile is selected (and/or make it more generic so it can be used by future demos - if that scope doesn't hamper efforts here).

I'll add your comment above about editing the aria items to the follow up issue.

andrewmacpherson’s picture

I'm confused now. That issue's just about wording? The accessibility points are about dynamic behaviour, not wording.

Which issue did you mean as the follow-up?

I think my review from #10 should really go on the general purpose installer issue at #2822414: Redesign the 'install profile selection' installer screen to allow for experimental profiles and more information. I'm re-posting them there now.

markconroy’s picture

During our weekly OOTB call yesterday, we agreed to make the issues smaller, so this one would deal with the functionality of "If you select the Umami profile, make sure a warning is displayed (and worry about the content of that message later)".

The exact wording of the warning (which we took as the HTML that will be used to show display the warning), we said, we'd fix in a follow up issue. I guess we presumed we could put the aria labels and things like that in the HTML of the finalised content.

Sorry if I'm not making sense here. You know more about which issue the a11y items should go in.

JayKandari’s picture

Status: Needs work » Needs review
FileSize
2.27 KB
2.05 KB

Refined patch with latest recommendations.

Implemented #7

Re: #10
Removed 1.
For 2. Just used a role="alert" but again that's inside the element which is toggling show/hide. Needs work on this. Tried adding #attributes to item type, but that's not possible it seems. If we can get #attributes property working then we should be able to attach role attribute to the main element div.

Marking this as Needs review, as I think more eyes will be needed to finalize this functionality.
Thanks!

larowlan’s picture

Issue tags: +Needs tests
+++ b/core/lib/Drupal/Core/Installer/Form/SelectProfileForm.php
@@ -72,6 +72,21 @@ public function buildForm(array $form, FormStateInterface $form_state, $install_
       $form['profile'][$profile_name]['#description'] = isset($profiles[$profile_name]['description']) ? $this->t($profiles[$profile_name]['description']) : '';
+      // Show profile warning if available.
+      if (isset($profiles[$profile_name]['warning'])) {
+        $warning = '<div role="alert" class="messages messages--warning">';

I agree with #8 and #9 - I don't think we want to create a warning API based on info files.
That way, if later on we change how we implement it, we're not left supporting this functionality. If this patch goes in with support for a new info key, we have to support it until Drupal 9. I don't think that is something we want to do, especially when we're not sure how #2822414: Redesign the 'install profile selection' installer screen to allow for experimental profiles and more information will end up.

I note that we already have the hook_install_tasks_alter() API. Did we consider showing this message as an additional step in the installer? With a 'go back' link (which would just go back to the previous step). This would require the user to acknowledge the warning, as they'd need to click 'continue' on that step.

Also, we have installer tests, so we should be able to test this.

smaz’s picture

Did we consider showing this message as an additional step in the installer? With a 'go back' link (which would just go back to the previous step).

Yes, this was mocked up & considered:
https://www.drupal.org/project/drupal/issues/2809635#comment-12406088

However the general feedback was that having an extra step would be unnecessary/not the best idea, and that it's not really the installation of the demo that is the point of no return - it's when people want to start configuring their site / making changes, hence the toolbar warning.

tedbow’s picture

I found this issue because installing core again for testing.

I first saw it on simplytest.me and I actually thought this was something that they had altered recently to tell users that simlytest.me was demo.

I think because the warning placed at the end of the profile description and this profile last in this list makes it seem like the message is for the page in general and not the specific profile.

If we want a very quick fix here and tackle the general problem in #2822414: Redesign the 'install profile selection' installer screen to allow for experimental profiles and more information I think simply moving the warning div to the beginning of the profile description makes it visually very obvious that warning is just for the 1 profile

Currently

Moved

Since this will be in 8.5 I think we should get a obvious fix to the warning out as quickly as possible without creating and API outside of the experimental profile. Since this is experimental we can still change it later.

markconroy’s picture

Thanks for that Ted. I think it's a great idea to move the warning to just under the name. Well done.

andrewmacpherson’s picture

yes, I really like moving it that way too. The indentation gives it a clearer connection to that radio button

navneet0693’s picture

@tedbow I am 100% agreed ! This is really better implementation.

markconroy’s picture

Status: Needs review » Needs work

Setting this to needs work.

At a minimum, we need:

  1. the patch from 14
  2. Along with the tests required from 15
  3. and the patch from 17 (but with the aria attributes fixed up).
JayKandari’s picture

Rerolled 14 with #17 changes. Along with aria attributes fixed up (now the role='alert' is applied to the element toggling display css attribute.

This lacks Tests, Someone here can help writing Installer tests for this. I tried a bit but was spending too much time. Or any guide for writing Installer Test for this specific case, I see other installer tests inside system module, do this installer also needs to go inside system or inside demo_umami. Help required here!!

Kindly review. :) Thanks

andrewmacpherson’s picture

Needs manual testing with assistive technology. role=alert is just one of the approaches I suggested in #10, and it may not be reliable.

Status: Needs review » Needs work
tedbow’s picture

Title: When installing Umami, only show warning if 'Demo Umami' radio button is selected » On installation is unclear that the "sample website" warning only applies to the 'Demo Umami' profile

Re #21
I don't think we need at a minimum we need #14
and if we don't need that we don't need the tests.

Specifically because of #15

I don't think we want to create a warning API based on info files.
That way, if later on we change how we implement it, we're not left supporting this functionality.

+++ b/core/lib/Drupal/Core/Installer/Form/SelectProfileForm.php
@@ -72,6 +72,29 @@ public function buildForm(array $form, FormStateInterface $form_state, $install_
+      if (isset($profiles[$profile_name]['warning'])) {

This is specifically adding a new API for the 'warning' key .info.yml files.
like @larowlan said

If this patch goes in with support for a new info key, we have to support it until Drupal 9.

I am updating the title of this issue to
On installation is unclear that the "sample website" warning only applies to the 'Demo Umami' profile
because we should focus on solving that problem without creating a new API as quick fix and tackle the larger problem in #2822414: Redesign the 'install profile selection' installer screen to allow for experimental profiles and more information

Otherwise if we are going keep supporting the new warning key in this patch we should update the title and summary indicate this issue is creating a new API which I don't think we want to do.

In my opinion #17 solves the problem of conveying to the user that the warning is only for the Unami profile.

navneet0693’s picture

@markconroy I am agreed with @tedbow and @larowlan, we are not creating a new warning API, we just want to improve the context of warning. Patch in #17 is a better approach.

markconroy’s picture

There is a requirement to only show the warning message when the umami profile is selected. What we have in #17 does not do that. It simply moves the warning message to above the description.

I'm going to edit the Issue title back to what it was, which I believe better reflects what we are trying to do.

markconroy’s picture

Title: On installation is unclear that the "sample website" warning only applies to the 'Demo Umami' profile » When installing Umami, only show warning if 'Demo Umami' radio button is selected (and ensure that it is obvious that warning message only applies to the Umami profile)
tedbow’s picture

Status: Needs work » Needs review
FileSize
3.26 KB
3.06 KB

@markconroy sorry I have not been following this initiative closely.

My problem with the approach is not that it:

only show warning if 'Demo Umami' radio button is selected

But rather what it actually does is "Adds support of a warning key in profile .info.yml files to show a profile specific warning when that profile is selected"

This would allow any profile not in core to start using this warning key and core could then could never remove support for this until Drupal 9. I think this is what @larowlan was talking about in #15

Here is patch that keeps the functionality of #22 but removes the warning key from demo_umami.info.yml. Rather it hardcodes this warning in \Drupal\Core\Installer\Form\SelectProfileForm::buildForm for the demo_umami specifically.

Hardcoding it doesn't feel great but then this leaves us with the ability to have a different solution for this warning in #2822414: Redesign the 'install profile selection' installer screen to allow for experimental profiles and more information.
This also adds a @todo to remove this fix in #2822414

The other place this could be hardcoded in is in a new form alter system_install_select_profile_form_form_alter() because I think system module is the only module enabled at this point.

smaz’s picture

+
+function demo_umami_form_alter($form, FormStateInterface $form_state) {
+    $a = 'b';
+}

I don't think this is meant to be there in the latest patch?

You're right, we don't need the API for providing warnings - I think what we meant was needed from #14 was that the warning only showed when it was selected, not all the time as per #17.

I think hardcoding for now with a followup is ok. Another option is to follow along the lines of experimental modules - we're proposing to add (Experimental) to the profile name in #2938803: Remove the warning about the Umami profile on the install screen. ModulesListForm::buildModuleList checks for the package name being Core (Experimental), so we could check for the presence of (Experimental) in the profile name. Probably one for the followup issue though.

markconroy’s picture

Hi Ted,

You are correct, we don't want to introduce a new API. I hadn't read that comment properly. What we need is the functionality of show/hide depending on if the profile is chosen or not. Your latest patch seems to accomplish that (Yay!).

The specific wording in this patch will need fixing, but we have a follow up issue for that - so I'm happy for us to just focus on functionality here - #2938803: Remove the warning about the Umami profile on the install screen

As @smaz mentioned on Slack, you might have left some testing code in this patch.

markconroy’s picture

Status: Needs review » Needs work
tedbow’s picture

As @smaz mentioned on Slack, you might have left some testing code in this patch.

whoops!

I also changed to move the logic from \Drupal\Core\Installer\Form\SelectProfileForm::buildForm() to system_form_install_select_profile_form_alter().

I think this is much cleaner because then in #2822414: Redesign the 'install profile selection' installer screen to allow for experimental profiles and more information we could remove system_form_install_select_profile_form_alter() completely if needed tp rather than a block of code inside of SelectProfileForm::buildForm()

markconroy’s picture

Functionally, this seems good to me.

I'll hold off RTBC until we have a backender look at it as well. We also need to have Andrew or someone from the a11y team to make sure the approach is okay.

andrewmacpherson’s picture

Re: #15

Did we consider showing this message as an additional step in the installer? With a 'go back' link (which would just go back to the previous step). This would require the user to acknowledge the warning, as they'd need to click 'continue' on that step.

Yes, it is one of the ideas in #2822414: Redesign the 'install profile selection' installer screen to allow for experimental profiles and more information, either as an extra page, or a dialog. It's still an open question there IIRC. I'm in favour of it, because it's the same flow as we have for enabloing experimental modules.

Re: #15, #25, #29 (the new warning key in info files):

If we want to have a generic way to handle this for all experimental profiles, then we'll need some kind of way to identify them for #2822414: Redesign the 'install profile selection' installer screen to allow for experimental profiles and more information and #2934374: Add permanent warning message for experimental profiles to avoid its usage on production sites. Currently the modules page looks for a specific package name: "Core (experimental)", and that's what the POC patch in #2934374: Add permanent warning message for experimental profiles to avoid its usage on production sites checks for too.

I don't like the warning key idea, with a string, because every experimental profile would have to provide it's own string. Instead I'd suggest we have an optional boolean experimental: true entry in the info files of all extension types, and use generic messages for the toolbar warning and installer warning. This would allow:

  • Core experimental modules
  • Core experimental profiles
  • Distros with experimental profiles
  • Distros with experimental modules - package: Core (experimental) isn't appropriate for this.
  • Experimental themes? This use-case is a bit less clear at the moment.

Individual profiles could override the generic core messages with hook_form_alter() or hook_toolbar_alter() if they like, but many won't need to.

andrewmacpherson’s picture

Accessibility: I played around with the approaches which I suggested in #10. The aria-describedby approach turned out to be the only reliable one. I only tried ChromeVox and NVDA mind you.

When using an ARIA live region (both Drupal.announce() and role="alert") the warning message was never conveyed to a screen reader. ARIA live regions aren't guaranteed to be announced, if some other announcement is more important. In this case, focusing/selecting (same thing for radios) causes the radio button's label to be treated as the most important thing to announce, and the live region was never announced.

When associating the warning to the radio button with aria-describedby, the label is still the most important thing, but a screen reader would eventually announce the warning, assuming the user didn't go elsewhere on the page before the label finished. This approach is more robust because there's a programatically-determinable relationship between the radio button and the warning. It's persistent, and supplements the name of the control. The live region approaches fail because they involve timing and a conflict of priority.

I think the best pattern would be:

<label for="demo-umami">
<input type="radio" id="demo-umami" aria-describedby="demo-umami-warning demo-umami-description">
<div id="demo-umami-warning">this is a demo</div>
<div id="demo-umami-description">This is a food magazine.<div>

Note the umami warning DIV can be anywhere in the source code. It's the order of the IDREFs in aria-describedby which matters.

If it's too tricky to set up a double IDREF (see #14, then you could concatenate the warning with the FAPI #description.. UPDATE: - patch #33 tried this, it didn't work very well, see a11y reviews in #42.

tedbow’s picture

re

Individual profiles could override the generic core messages with hook_form_alter() or hook_toolbar_alter() if they like, but many won't need to.

I don't think profiles can actually use these hooks because at the point the profiles form is built none of the profiles are enabled so none of the hooks for in .profile files are fired. I think only the system module is enabled at this point so only it's hooks are fired.

andrewmacpherson’s picture

I think only the system module is enabled at this point so only it's hooks are fired.

During installation, form alter hooks get fired, no?

The toolbar hooks aren't needed by the installer, that's for the permanent message displayed after installation.

andrewmacpherson’s picture

My accessibility testing (#36) wasn't based on the latest patch (#33).

I was testing an earlier patch, and messing about in firefox dev tools. I'll test #33 now.

tedbow’s picture

@andrewmacpherson

During installation, form alter hooks get fired, no?

Unfortunately not on the form to set the profile. If you have empty database and you step through the installation at the point of picking the profile none of the profiles are "enabled" so there hooks don't fire.

After that step in the install process then yes the hooks for the profile that was selected(but none of the others) would be fired.

So demo_umami_form_install_configure_form_alter() would be invoked because at that point the demo_umami profile has been selected.

I just tested this manually to be sure and it makes sense and is probably the desired behavior. You won't want hooks to be invoked for profiles that won't be enabled.

Eli-T’s picture

+++ b/core/modules/system/system.module
@@ -1440,3 +1440,35 @@ function system_element_info_alter(&$type) {
+ * Show profile warning demo profile is selected.

Sorry for the bikeshedding, but does this comment make sense?

Should it be "Show profile warning *if* demo profile is selected." ?

andrewmacpherson’s picture

tl;dr: I tested patch #33 with various screen readers. Patch #33 follows actually follows a suggestion I made in #36, to concatenate the warning into the description. I recommend we remove the following line from the patch in #33:
+ 'role' => 'alert',

The alert role caused some undesirable and confusing behaviour in various screen readers. It looks to be a conflict of priorities between announcing the current radio button that has just been selected, versus the alert role.

The rest of the comment is my detailed test notes. The main problem is actually the opposite of what I experienced in #36, but there I was messing around changing markup directly in the browser dev toools, so I deserve all I get. This time I just applied the patch in #33 and avoided messing with dev tools.

IE11 + NVDA 2017.4 + Win7: For standard and minimal, it reads the label, then description. For Umami, it reads the alert region. That's all - the umami label and description are NOT read out. The alert region trumps the accessible name and description of the radio button entirely. Very bad (breaks normal announcement of radio button).

Firefox 52 ESR + NVDA 2017.4 + Win7: Same problem as IE11 + NVDA + Win 7. Very bad (breaks normal announcement of radio button).

Chrome 64 + NVDA 2017.4 + Win7: Same problem as IE11 + NVDA 2017.4 + Win7. Very bad (breaks normal announcement of radio button).

Note: experienced NVDA users may know that they can press NVDA+TAB to announce the name of the currently focused control. This works, but I don't think it's an acceptable mitigation.

Additionally, with Firefox 52 and Chrome 64 (but not IE11) I could get NVDA to announce the name and description of the Umami radio button if I switched to another window, then back again, but the warning message was not announced. This behaviour only happened when switching windows while the umami button was selected. This isn't an acceptable mitigation either, let's assume they stick to the same window during install.

Safari 11 + VoiceOver + macOS 10.13 High Sierra: For Standard and Minimal radio buttons, VoiceOver announces the name and description of the radio button.
For Umami, the warning is announced, then the umami description. The label of the radio button is not announced when it becomes selected. Very bad (breaks normal announcement of radio button).
Note: experienced Voiceover users may know that F3 or F4 keys will announce the name of the currently focused control. I don't think it's an acceptable mitigation.

Chrome 64 + VoiceOver + macOS 10.13 High Sierra: For Standard and Minimal radio buttons, VoiceOver announces the name and description of the radio button.
For Umami, the name and description "Install with the Umami food magazine..." are announced, but the warning message is NOT announced. Bad (doesn't convey warning).

Chrome 64 + ChromeVox + Win7: Announces label, followed by the warning, follwed by the umami description, followed by the warning again in a different tone of voice. This is because it's reading the entire description, followed by the alert region. This means the warning is said twice. Verbose but OK.

Chrome 64 + ChromeVox, Kubuntu 17.10 Linux: Same as ChromeVox + Win7. Verbose bu OK.

Android 7 + TalkBack + Chrome: The warning is never announced. The name + description is announced, but doesn't include the content of the DIV with the alert role. Bad (doesn't convey warning).

JayKandari’s picture

Re: #33
Wow !! This is awesome... I didn't realize the fact that only system module is enabled at this point. I tried doing hook_form_alter in demo_umami.profile. This clearly is a better approach than creating a new warning API. Looks Great to me from BE perspective.

+++ b/core/modules/system/system.module
@@ -1440,3 +1440,35 @@ function system_element_info_alter(&$type) {
\ No newline at end of file

a tiny nit pick, require an EOL. :)

This definitely requires a11y changes as mentioned by andrew :)

Should include #41 recommendations or something similar. Since code resides inside system.module "Warning to show when this profile is selected" seems a bit confusing. Something like "Warning to show if Demo Umami profile is selected."

Not changing status, as I was involved in previous patches in this issue... IMO, I'd give a green signal with above tiny changes...
require more eyes... :)

Great job @tedbow. thanks !!

markconroy’s picture

Issue summary: View changes

So it looks like we just need to get the a11y part of this right and then it's ready for RTBC

@andrew if we create the HTML you suggested in 36 implemented, are you okay to sign off?

<label for="demo-umami">
<input type="radio" id="demo-umami" aria-describedby="demo-umami-warning demo-umami-description">
<div id="demo-umami-warning">this is a demo</div>
<div id="demo-umami-description">This is a food magazine.<div>

If not, can you pop in the exact snippet of HTML you think would be okay and we can get a patch for that. We'll then be able to focus on the text of the message in #2938803: Remove the warning about the Umami profile on the install screen

andrewmacpherson’s picture

I want to do manual testing again before sign-off. The review in #42 was was very discouraging. This is quite an unusual visual UI to convey to AT. Having a separate step to confirm an experimental profile would be much easier.

navneet0693’s picture

Fixing comment and nit pick by @JayKandari

navneet0693’s picture

A small mov recored while testing the patch in #33

larowlan’s picture

+++ b/core/modules/system/system.module
@@ -1440,3 +1440,35 @@ function system_element_info_alter(&$type) {
+  if (isset($form['profile']['demo_umami'])) {
...
+    $description = $form['profile']['demo_umami']['#description'];

great work @tedbow, nice separation.

We can burn this later

We just need a test here

David_Rothstein’s picture

This patch looks like a nice temporary solution.

  1. +          'role' => 'alert',
    

    Once this is removed (which it sounds like it should be) then wouldn't any remaining accessibility problems be generic issues with the #states API? If so, seems like they should be dealt with in separate issues rather than trying to fix them here (there are already some at #1899836: Accessibility improvements for States API and #1966990: Forms API drupal_process_states() hidden content is not visible to screen readers).

  2. +        '#markup' => 'Warning: this is a sample website, you should not use it as the basis for your website.',
    

    Shouldn't this be translated?

  3. + * Implements hook_form_FORM_ID_alter() for\Drupal\Core\Installer\Form\SelectProfileForm.
    

    (minor) Needs a space after "for".

  4. I have to say I found it strange to see all this code in system module (it has nothing to do with the system module, and it's installer-specific, not runtime code). I think the patch that put it as temporary code in SelectProfileForm::buildForm() made more sense - but if this is temporary anyway, it doesn't matter that much.
navneet0693’s picture

Shall we wait for https://www.drupal.org/project/drupal/issues/2907728 to get committed, so that we can add an installer test and avoid re-work?

acbramley’s picture

Addressing 49.1, also moving the todo comment up to the part that's actually checking for the profile name, I think it makes more sense there.

acbramley’s picture

Assigned: Unassigned » acbramley

Taking a stab at the tests

acbramley’s picture

Assigned: acbramley » Unassigned
FileSize
4.32 KB
7.21 KB

Having a lot of issues getting installer tests running on my local set up so I did my best guess and we'll see what happens. Someone else will have to finesse this though :( Basically took the same approach as the InstallerTestBase from #2907728: Installer: Convert system functional tests to phpunit but using JTB so we can test the message displays via states api.

Status: Needs review » Needs work

The last submitted patch, 53: 2938185-52.patch, failed testing. View results

markconroy’s picture

This patch is almost there now.

Here's the wording we agreed at the last weekly call:
https://www.drupal.org/project/drupal/issues/2938803#comment-12456111

Title:
Demo: Umami Food Magazine (Experimental)

Warning:
This profile is intended for demonstration purposes only.

Description:
Install an example site that shows off some of Drupal's capabilities.

Does someone want to put that wording in here? It'll mean we've two issues closing when this issue is committed.

tedbow’s picture

Adding #2907728: Installer: Convert system functional tests to phpunit as a related issue because it would probably be easier to get these tests done after that.

I am not sure if we should postpone on that.

markconroy’s picture

@tedbow,

Let's remove this when we need to instead of postponing working on it.

If we postpone it, Umami cannot get into 8.5 and the world will lose out on it for another 6 months :-)
(the world can't live another 6 months without it!)

larowlan’s picture

I added the needs tests tag so we had something to be sure that the form alter kept working. I'd be happy with a non JavaScript smoke test that checked the text was present on the page, because that would prove it was being fired. Then the JavaScript version could come when that issue was resolved. Does that help? Do folks agree?

Eli-T’s picture

Patch to implement #52

This does not address #58 which was made as I was rolling this.

Initial selection screen with this patch:

Selection screen with umami selected:

EDIT - apologies for naming the interdiff .patch like a noob.

larowlan’s picture

FileSize
1.31 KB

The attached interdiff should get the test running. See comments in the interdiff for the why.

Leaving someone else to kick this along so I can be eligible to commit the issue when it reaches RTBC

tedbow’s picture

Status: Needs work » Needs review
FileSize
1.29 KB
7.84 KB

@larowlan thanks for figuring this out!

Here is the updated patch.

markconroy’s picture

Status: Needs review » Reviewed & tested by the community

Oh oh oh ... I'm excited about this one!

RTBC

The last submitted patch, 59: interdiff_2938185_52_59.patch, failed testing. View results

navneet0693’s picture

RTBC + 1

andrewmacpherson’s picture

Status: Reviewed & tested by the community » Needs review

Still needs a11y review, hopefully soon

The last submitted patch, 59: 2938185_59_update_strings.patch, failed testing. View results

ckrina’s picture

Just marked as postponed the issue about the wording addressed here #2938803: Remove the warning about the Umami profile on the install screen since it's being solved in this issue. Once this one is fixed we'll probably have to close the other one unless anything else need refinement.

Some comments there that ideally should be addressed here would be this last one from @David_Rothstein:

He proposed: This creates a site that is intended for demonstration purposes.

We are using: This profile is intended for demonstration purposes only.

is that I wonder if the second gives people the impression that this isn't a "real" profile (i.e., that the option on the screen is itself just there for demo purposes, and that clicking on the radio button won't actually do anything)?
I do think they'll figure it out eventually, but I would not be surprised if some people hesitate briefly out of confusion -so that is why I went with the "creates a site" text originally.

I personally think his suggestion makes sense, but to not bike-shedding and block this issue it could be solved in the other one if this one is committed.

andrewmacpherson’s picture

Accessibility-wise this one has been tricky.

On the whole, I prefer the idea of a separate "are you sure?" step like we use for experimental modules. No dynamic behaviour means no awkward hoops for screen readers.

There are still a few markup patterns we could try for the design used here - #36 hasn't been done yet, with two separate aria-describedby IDREFs, one of which has display:none toggling with "visible" States API.

andrewmacpherson’s picture

Let's not make the accessibility problem block an 8.5 backport. As long as we have a follow-up issue where we can keep trying for a more accessible experience, we'll get there.

andrewmacpherson’s picture

Issue tags: +Needs followup
markconroy’s picture

Status: Needs review » Reviewed & tested by the community

Moving to RTBC - ally was the last check that was needed.

Follow up issue here [#32942232]

andrewmacpherson’s picture

larowlan’s picture

Issue tags: -Needs tests, -Needs followup

Discussed this with @alexpott and we could live with the hard-coding in SelectProfileForm for the time-being, under the proviso that it will be removed by #2822414: Redesign the 'install profile selection' installer screen to allow for experimental profiles and more information

webchick’s picture

Status: Reviewed & tested by the community » Fixed
FileSize
226.23 KB

Tested this from a UX POV, and the behaviour is exactly what we discussed the other week at the UX meeting:

Initially all three profiles shown the same; subsequently the warning text appears.

Since the framework managers have discussed and resolved their concerns, the accessibility maintainer is comfortable addressing those issues in a follow-up, let's get this one in!

Committed and pushed to 8.6.x. ROCK!

  • webchick committed fa3e38e on 8.6.x
    Issue #2938185 by tedbow, JayKandari, navneet0693, acbramley, Eli-T,...

  • alexpott committed a0be792 on 8.5.x authored by webchick
    Issue #2938185 by tedbow, JayKandari, navneet0693, acbramley, Eli-T,...

Status: Fixed » Closed (fixed)

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