Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
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.
Comment | File | Size | Author |
---|---|---|---|
#74 | umami-selector.gif | 226.23 KB | webchick |
#61 | 2938185-61.patch | 7.84 KB | tedbow |
#61 | interdiff-59-61.txt | 1.29 KB | tedbow |
#60 | interdiff-6374f7.txt | 1.31 KB | larowlan |
#59 | Select_an_installation_profile__umami_selected_Drupal.png | 224.38 KB | Eli-T |
Comments
Comment #2
webchickSince this is a beta blocker, escalating to major.
Comment #3
andrewmacpherson CreditAttribution: andrewmacpherson as a volunteer and at Annertech commentedWe'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.
Comment #4
markconroy CreditAttribution: markconroy as a volunteer and at Annertech commentedComment #5
JayKandariHello,
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!
Comment #6
markconroy CreditAttribution: markconroy as a volunteer and at Annertech commentedThis 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
Comment #7
navneet0693 CreditAttribution: navneet0693 as a volunteer and at QED42 commentedThis is an awesome solution! I am willing to issue RTBC, but still want any one from core team to review this.
Just a suggestion to use short array syntax.
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 ?
Comment #8
David_Rothstein CreditAttribution: David_Rothstein as a volunteer commentedShouldn'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.
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.)Comment #9
andrewmacpherson CreditAttribution: andrewmacpherson as a volunteer and at Annertech commentedre: #8
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.
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.
Comment #10
andrewmacpherson CreditAttribution: andrewmacpherson as a volunteer and at Annertech commentedAccessibility review:
1. Remove the
role="contentinfo"
andaria-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:
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).role="alert"
oraria-live="assertive"
on the warning message (it must be on same element which hasdisplay: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.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>
andaria-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.
Comment #11
markconroy CreditAttribution: markconroy as a volunteer and at Annertech commentedHi 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.
Comment #12
andrewmacpherson CreditAttribution: andrewmacpherson as a volunteer and at Annertech commentedI'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.
Comment #13
markconroy CreditAttribution: markconroy as a volunteer and at Annertech commentedDuring 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.
Comment #14
JayKandariRefined 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!
Comment #15
larowlanI 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.
Comment #16
smazYes, 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.
Comment #17
tedbowI 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.
Comment #18
markconroy CreditAttribution: markconroy as a volunteer and at Annertech commentedThanks for that Ted. I think it's a great idea to move the warning to just under the name. Well done.
Comment #19
andrewmacpherson CreditAttribution: andrewmacpherson as a volunteer and at Annertech commentedyes, I really like moving it that way too. The indentation gives it a clearer connection to that radio button
Comment #20
navneet0693 CreditAttribution: navneet0693 as a volunteer and at QED42 commented@tedbow I am 100% agreed ! This is really better implementation.
Comment #21
markconroy CreditAttribution: markconroy as a volunteer and at Annertech commentedSetting this to needs work.
At a minimum, we need:
Comment #22
JayKandariRerolled 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
Comment #23
andrewmacpherson CreditAttribution: andrewmacpherson as a volunteer and at Annertech commentedNeeds manual testing with assistive technology. role=alert is just one of the approaches I suggested in #10, and it may not be reliable.
Comment #25
tedbowRe #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
This is specifically adding a new API for the 'warning' key .info.yml files.
like @larowlan said
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.
Comment #26
navneet0693 CreditAttribution: navneet0693 as a volunteer and at QED42 commented@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.
Comment #27
markconroy CreditAttribution: markconroy as a volunteer and at Annertech commentedThere 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.
Comment #28
markconroy CreditAttribution: markconroy as a volunteer and at Annertech commentedComment #29
tedbow@markconroy sorry I have not been following this initiative closely.
My problem with the approach is not that it:
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 #15Here 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 thedemo_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.Comment #30
smazI 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.
Comment #31
markconroy CreditAttribution: markconroy as a volunteer and at Annertech commentedHi 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.
Comment #32
markconroy CreditAttribution: markconroy as a volunteer and at Annertech commentedComment #33
tedbowwhoops!
I also changed to move the logic from
\Drupal\Core\Installer\Form\SelectProfileForm::buildForm()
tosystem_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 ofSelectProfileForm::buildForm()
Comment #34
markconroy CreditAttribution: markconroy as a volunteer and at Annertech commentedFunctionally, 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.
Comment #35
andrewmacpherson CreditAttribution: andrewmacpherson as a volunteer and at Annertech commentedRe: #15
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:package: Core (experimental)
isn't appropriate for this.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.
Comment #36
andrewmacpherson CreditAttribution: andrewmacpherson as a volunteer and at Annertech commentedAccessibility: 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()
androle="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:
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. UPDATE: - patch #33 tried this, it didn't work very well, see a11y reviews in #42.#description
.Comment #37
tedbowre
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.Comment #38
andrewmacpherson CreditAttribution: andrewmacpherson as a volunteer and at Annertech commentedDuring 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.
Comment #39
andrewmacpherson CreditAttribution: andrewmacpherson as a volunteer and at Annertech commentedMy 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.
Comment #40
tedbow@andrewmacpherson
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.
Comment #41
Eli-TSorry for the bikeshedding, but does this comment make sense?
Should it be "Show profile warning *if* demo profile is selected." ?
Comment #42
andrewmacpherson CreditAttribution: andrewmacpherson as a volunteer and at Annertech commentedtl;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).
Comment #43
JayKandariRe: #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.
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 !!
Comment #44
markconroy CreditAttribution: markconroy as a volunteer and at Annertech commentedSo 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?
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
Comment #45
andrewmacpherson CreditAttribution: andrewmacpherson as a volunteer and at Annertech commentedI 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.
Comment #46
navneet0693 CreditAttribution: navneet0693 as a volunteer and at QED42 commentedFixing comment and nit pick by @JayKandari
Comment #47
navneet0693 CreditAttribution: navneet0693 as a volunteer and at QED42 commentedA small mov recored while testing the patch in #33
Comment #48
larowlangreat work @tedbow, nice separation.
We can burn this later
We just need a test here
Comment #49
David_Rothstein CreditAttribution: David_Rothstein as a volunteer commentedThis patch looks like a nice temporary solution.
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).
Shouldn't this be translated?
(minor) Needs a space after "for".
Comment #50
navneet0693 CreditAttribution: navneet0693 as a volunteer and at QED42 commentedShall 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?
Comment #51
acbramley CreditAttribution: acbramley at PreviousNext for Transport for NSW commentedAddressing 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.
Comment #52
acbramley CreditAttribution: acbramley at PreviousNext for Transport for NSW commentedTaking a stab at the tests
Comment #53
acbramley CreditAttribution: acbramley at PreviousNext commentedHaving 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.
Comment #55
markconroy CreditAttribution: markconroy as a volunteer and at Annertech commentedThis 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.
Comment #56
tedbowAdding #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.
Comment #57
markconroy CreditAttribution: markconroy as a volunteer and at Annertech commented@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!)
Comment #58
larowlanI 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?
Comment #59
Eli-TPatch 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.
Comment #60
larowlanThe 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
Comment #61
tedbow@larowlan thanks for figuring this out!
Here is the updated patch.
Comment #62
markconroy CreditAttribution: markconroy as a volunteer and at Annertech commentedOh oh oh ... I'm excited about this one!
RTBC
Comment #64
navneet0693 CreditAttribution: navneet0693 as a volunteer and at QED42 commentedRTBC + 1
Comment #65
andrewmacpherson CreditAttribution: andrewmacpherson as a volunteer and at Annertech commentedStill needs a11y review, hopefully soon
Comment #67
ckrinaJust 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.
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.
Comment #68
andrewmacpherson CreditAttribution: andrewmacpherson as a volunteer and at Annertech commentedAccessibility-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.
Comment #69
andrewmacpherson CreditAttribution: andrewmacpherson as a volunteer and at Annertech commentedLet'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.
Comment #70
andrewmacpherson CreditAttribution: andrewmacpherson as a volunteer and at Annertech commentedComment #71
markconroy CreditAttribution: markconroy as a volunteer and at Annertech commentedMoving to RTBC - ally was the last check that was needed.
Follow up issue here [#32942232]
Comment #72
andrewmacpherson CreditAttribution: andrewmacpherson as a volunteer and at Annertech commentedComment #73
larowlanDiscussed 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
Comment #74
webchickTested this from a UX POV, and the behaviour is exactly what we discussed the other week at the UX meeting:
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!