Improve usability of core compatibility ranges on available updates report

One extremely important thing this UI should make more clear is if a given release is compatible with the version of core you're currently running. That's definitely not obvious when skimming the output. Especially if your version of core is inside one of the printed ranges, it's going to take quite a bit of mental energy to parse the range and figure out if the given update would work on your site. We could compute that automatically and tell you by some more obvious mechanisms (icon with hidden text, explicit text, both, etc).

The other extremely useful bit of info that's currently rather buried is if the given release would be compatible with a recommended core update. This is a bit harder to quantify, since there might be multiple core updates recommended. E.g. if you're on 8.7.9 right now, update.module will tell you to update to 8.7.11 and mention that 8.8.1 also exists. But it'd be really slick if an available update for one of your contribs could say (approximately):

Address 8.x-1.7
This module is compatible with Drupal core 8.6.0 to 8.8.1, including your current version (8.7.9), recommended core (8.7.11) and latest core (8.8.1)

(more or less).

Thanks, @webchick!

Re:

They just simply would just not show you things that do not work for your version

Big ole' can-o-worms. :/ See #3076183: [Meta] Determine how available updates for modules should be handled if they are not compatible with the current version of Drupal core -- oh yeah, you commented there, too. ;)

I like the ideas of collapsing most of the details on this report. That sounds like a really good move, but seems a bit out of scope. I agree this report hasn't had much UX review in years (although it's not fair to say "never" we did try in the earlier days, including a fair bit of effort from bojan and yoroy). I was trying to be more specific with this issue and the details around the core compatibility ranges, not a full-scale "redesign the available updates report" issue.

That said, if we go this route to address the duplicate / verbose info that was just added, we'd still end up burying the important info about core compatibility. Or, we'd have to put this info "above the fold", and then we're not really any better off.

Or, we'd have to summarize the key compatibility stuff I mentioned in #2, put *that* above the fold, and put all the detailed version range stuff in the hidden details.

Or something. ;)

Wow, flurry of activity. Yay. ;)

@webchick re: #5:

Can you elaborate on why exactly you think this info is so critical and should be raised in visual importance? To me, the critical thing to answer is "Does this work with my version of core or not?" and IMO you could handle that in simpler, graphical terms (e.g. green checkmark next to active download link / red X next to disabled download link).

Exactly. That's what I'm trying to say in #2. I think the thing that was added in #3096078 as if it was important and useful info at the same visual importance as a bunch of other stuff is mostly completely irrelevant to site builders, and what they actually need to know are the 2 points I wrote:

1. One extremely important thing this UI should make more clear is if a given release is compatible with the version of core you're currently running...
2. The other extremely useful bit of info that's currently rather buried is if the given release would be compatible with a recommended core update...

...

To that end, @xjm's #16 (or something like it) is way better than what I put at the end of #2.

+1 to the top-2 user stories in #18. Those seem like the right things for this report to help site builders understand. The last non-case of "show me all possible core updates that a given release is compatible with" (what was actually added) seems mostly irrelevant and confusing. That's why I tried to stop #3096078 from being committed in the first place, and was in favor of reverting it, but I was both too late and not convincing enough. ;) Happy to see this now considered a critical follow-up to actually fix it.

Thanks,
-Derek

p.s. I understand this issue is now blocking a backport to 8.8.x, but there is no such info in the available updates report in 8.8.x at this time, any patches for this will have to target 8.9.x, be "tested" against 8.9.x, etc. Seems like 8.9.x is the more appropriate version for this issue, but ultimately I defer to @xjm on that.

p.p.s. The other unspoken elephant in this room is that even if a given contrib release says its compatible with a given version of core, that's but one piece of the whole dependency tree. E.g. maybe you're still on PHP 7.1 and a new contrib release now requires 7.3. Or perhaps the latest release of a contrib you have now adds a new dependency on some other contrib you don't have. Nothing on this report knows about any of that or can tell you about either of those (or other) cases.

So while we can make approximate recommendations, we can't guarantee that what we're recommending you do will actually work. I'm not really sure what to do about that. Writing all the text in a conditional style in that implies "we think this will work, but you'll have to try it and see" would be crappy. Yet, we can't actually state with certainty that the releases we recommend you upgrade to will all work, either individually, or collectively. Hence, why-are-we-trying-to-re-implement-composer-in-update.module? ;) But yeah, I understand we don't yet require everyone to use composer, so we have to have something to help site builders get by without it.

Even with composer in the mix, it's nice to have something tell you automatically that you're missing updates, which composer on its own does not do.

I'm in the middle of a very initial quick hack prototype of this. Stay tuned...

Super ugly. Totally un-styled. Probably not The Right Way(tm) to deal with the template changes (although I don't believe any of this has actually been released yet, so we should still be free to make such changes without BC worries). Probably not the right markup we want to end up with. Left some @todo for possible API changes to ProjectCoreCompatibility we might want to make.

That said, basically working prototype. ;)

NW for all sorts of things, including:

• Improve implementation, remove @todo's, etc
• Possibly updating existing tests.
• Adding new tests.
• Fix styling.

(added all those to the summary's remaining tasks list).

Sort of a PITA to actually test this. Ended up installing token 8.x-1.5 from tarball, hacked my local copy of core/lib/Drupal.php to claim I'm on 8.8.0, then manually forced a FALSE return from ProjectCoreCompatibility::isCoreCompatible() to see the incompatible cases happen.

Screenshots from Seven:

Compatible

Initial

Open

Incompatible

Initial

Open

Putting screenshots from #26 into summary, other updates / cleanups.

Here's what I meant with the @todo in the patch. Technically, this isn't "in scope", but it seems like a useful API cleanup given the in-scope changes we do need to make to this class to make this all work.

I didn't touch the tests, so this would still fail. No reason to have the bot churn on it until we get closer to what we want and fix the tests to match.

Also, I know we want this to backport into 8.8.x, but meanwhile, all the patches and testing are going to have to target 8.9.x, so I'm moving the version accordingly. See #20.

Finally, hiding all the screenshots from the file table. They're all linked or embedded in the comments, so we don't need them in the file table in the summary.

Thanks,
-Derek

@tedbow re: #30:

1. Great, glad you agree. Already fixed in #29.

2. Whoops! ;) compatibility^H^H^He is to blame. ;) Fixed.

I didn't know why this was stored in the $release array but I see we use in the twig template to not output the download link.

I guess we can't just unset($release['download_link']) here because of BC concerns if some overrode the template?

Yeah, I guess. :/ I've been deeply involved in a bunch of issues that try to touch core twig templates and I still don't really understand what we're allowed to change, when, and why. :( So I simply hacked this together, without caring too much about The Right Way(tm) to solve everything. I still consider this a prototype, and I'd rather focus on how we actually want this to look and behave first, then sort out how to implement it such that it can be committed.

For a UX review, I wonder if the details should go above (#26) or below the "Release notes" link. Even if we style this better, it still seems a bit jarring to have things in this order:

• Download
• [Compatible releases]
• Release notes

Maybe this is better:

• Download
• Release notes
• [Compatible releases]

And in the not compatible case:

• Release notes
• [Not compatible]

Added making a decision about this to remaining tasks.
Also re-ordered the remaining tasks a bit.

@AaronMcHale re: #28 -- sorry I missed this earlier...

If I remember correctly I think we agreed to not hold this issue up too much on specific wording and look at it in a follow-up, but my initial thoughts are that to avoid any confusion we should be a little more explicitly, for example instead of simply saying "Not compatible" we could say "This update has compatibility issues", that would just make sure there's no confusion about what "Not compatible" relates to. But again if we felt it's better to do that in a follow-up, happy to wait until then ;)

I think this is that follow-up. ;) Since this is (part of) what's blocking a bunch of update.module patches from being backported into 8.8.x, it seems we need to get this correct, here and now. We can't keep punting the wording to the future, or we'll never be able to actually commit anything to 8.8.x. I believe the idea is "let's not completely redesign the entire available updates report UI" here. That much is true. ;) That needs to happen separately. But anything related to this compatibility range stuff should be fixed as part of this issue.

Specifically re: "This update has compatibility issues" -- I think that's still too vague. If we want to be more explicit, we should say something like "This update is not compatible with your version of Drupal" or something. It's not just "compatibility issues". It's fully incompatible. ;)

Added making a decision on these details titles to the remaining tasks...

Thanks!
-Derek

Also added this to remaining tasks:

• Decide if we should also inform site admins about compatibility with recommended/available core update versions. See #2

@webchick: Thanks for the replies!

Re: #35: Yeah, absolutely. I kept trying to make it clear this is totally un-styled. The screenshots in #26 in no way represent what I (or anyone else) wants this to actually look like. ;) It's just proof-of-concept, fleshing out the required API changes to make it work, etc.

Re: #36: Good question. Thanks for the pointer to an existing pattern in core to try to re-use. All those styles live directly in core/modules/system/css/system.admin.css:

.system-modules details[open] {
  overflow: visible;
  height: auto;
  white-space: normal;
}
.system-modules details[open] summary .text {
  text-transform: none;
  -webkit-hyphens: auto;
  -moz-hyphens: auto;
  -ms-hyphens: auto;
  hyphens: auto;
}
.system-modules td details a {
  color: #5c5c5b;
  border: 0;
}
.system-modules td details {
  height: 20px;
  margin: 0;
  border: 0;
}
.system-modules td details summary {
  padding: 0;
  cursor: default;
  text-transform: none;
  font-weight: normal;
}

So, either we get to:
A) Duplicate those styles into core/modules/update/css/update.admin.theme.css in such a way they'll take precedent over the default details styles in Seven and Claro.
B) Add specific styles to Seven + Claro for this.
C) Do some hacky crap to add system-modules class and a <td> around the available updates report so those styles kick in (ugh).

I vote A, I guess, but the folks who really know about The Right Way(tm) to deal with markup/CSS changes in core should state their preference.

Re: #37:
- Position (4 - hope we don't renumber those): Great. I think the bottom of the pile is better, too.

- Title "Not compatible" (5) - Right, less is more. :) Note: there's no data about "existing wording" because this feature has never been released. ;)

- Compatibility with recommended updates (comment #2, currently 6 in remaining tasks): I was imagining that would be buried inside the message inside the details. So I'm less concerned about adding more text there, since someone already clicked on the "tell me more..." link. But sure, I guess we could punt that to yet another follow-up. I'm just already concerned we have a huge list of issues and patches that are interdependent that we're trying to resolve and backport all the way to 8.8.x, and every new "let's do that in a follow-up" adds to that complication.

Thanks again!
-Derek

Re: #39 Righto. ;)

I guess if we're going to point those things out, I would recommend we do them in one place, like either at the top of the page, or in the "Drupal core"'s row. Otherwise you're going to repeat yourself over and over and over again on each "expando-thingy" on what the various current/recommended/latest versions are (these will not change row-to-row), which adds a lot of extra noise each time and makes the "meat" of the message harder to parse.

We can't put compatibility with the latest recommended core release up only once, since in general, each project on this page could specify different core_version_requirement constraints, and therefore, not all of them will be compatible with any given core release (either currently installed or recommended). That's why this stuff is already duplicated for each row. In practice, a huge number of your projects will all have the same compatibility, so all these messages are going to be duplicate, which is why we want to hide them in a collapsed <details> in the first place.

My point from comment #2 was that if we're going to say this over and over:

This module is compatible with Drupal core: 8.8.0 to 8.8.2

as that list grows, and potentially includes multiple ranges of compatibility, and it gets harder and harder to parse the list and figure out WTF, it'd be nice if it simplified things for you to tell you "is compatible with what you're currently running" (yay, we're already doing that with the title of the details link) and "would be compatible with the version of core we're recommending you upgrade to". But yes, this last part is complicated to get right (since there might be multiple versions of core being recommended in the core section) and adds clutter to an already cluttered list of stuff for mere mortals to read through and parse. So probably won't fix or punt to Yet Another Followup(tm) and not let it hold up this critical issue.

But, I'll leave that open for more feedback before officially crossing it off the remaining tasks as a final decision. I am not a UI expert. ;)

Thanks,
-Derek

@AaronMcHale re: #41: Thanks, all great feedback.

Does "Not compatible" do that, well technically yes, but it does still have an ambiguity about it, the user might ask "Not compatible with what...", of course they can just expand the detail element to find out, but is it obvious enough that by expanding the detail element that's what they will find out?

As @webchick pointed out in #39, this is all just speculation at this point, since none of this has been released, we're not a few years into releases that are making these compatibility ranges/lists more verbose/complex, etc. We just have to make our best guesses, ship it, and see what happens. ;) Until we have any real evidence that "Not compatible" is actually confusing, Occam's razor says we should keep short.

Another reason to keep the details title at just "Not compatible" is that we might want to expand this stuff someday as part of #3076183: [Meta] Determine how available updates for modules should be handled if they are not compatible with the current version of Drupal core. The modules page already does a bunch of checks for if you can enable a given module or not, including core compatibility, missing dependencies, PHP version, and more. Perhaps we'll actually have a list of compatibility problems for you under "Not compatible", and it'd be a string break and UI change for site admins if these titles change later for that.

- - -

From the summary, is it safe yet to actually make these decisions:

4. Decide if the compatibility details area should go above or below: Below
5. Decide the correct text to use for the details titles: "Not compatible" and "Compatible releases"
6. Decide if we should also inform site admins about compatibility with recommended/available core update versions: Punt-to-followup for (non-critical, maybe not even backportable) discussion.

?

and for implementation details:

#38.A: Duplicate/custom <details> styles in update.admin.theme.css so the DETAILS AREN'T SHOUTING? ;)

Thanks,
-Derek

Ugh, hate to ask, but Occam's razor has me thinking... if the update is compatible with your current version of core, does any site builder actually care to see the list of all possible compatible versions? I'd be shocked and amazed if anyone is going to see the Available update report, remember their recommended core release, go through 1-by-1 to click "Compatible releases" for every project, and parse the list of version #s in each one to see if they're ready to upgrade to a later core or not, etc. If we intend to give site builders the info they need to make good decisions about what version of core to upgrade to and when, I think we need to do what I propose in #2. Otherwise, I suspect we're fooling ourselves that anyone is actually going to read all this and process it themselves.

So... maybe we only add the details to a project for the "Not compatible" case, and avoid cluttering things at all in the compatible case?

To move this along a bit, let's decide we're putting the release notes above the compatibility details in all cases.

p.s. I also noticed a bug in core/themes/claro/templates/admin/update-version.html.twig where I left a duplicate download link in place.

p.p.s. @tedbow, re: #30.2: "we can't just unset($release['download_link']) here..." because none of the core templates are written such that they check if download_link has a value or not. They all assume there's a value. And yeah, changing all that is more BC-breaking than the changes in this patch. *shrug*

Okay, here's a stab at styling. Currently only dealing with Seven. Claro is still @todo. But this is getting closer (I hope)...

Compatible

Initial

Open

Not compatible

Initial

Open

Notes on #45:

1. I don't love that it says "Compatible releases" for the compatible case. Maybe just "Compatibility" is better? "Releases" seems a bit weird here.

2. Still wonder if "Compatible releases" should just completely go away (see #43).

3. Maybe "Not compatible" needs to be red with an error icon or something?

4. Inside the message for "Not compatible", I wonder if the text should say more (I know, bad idea). But either add the word "only" or perhaps even add another sentence: "Your currently installed version ($version) is not supported." or something...

- Adds colors
- Changes summary text to just "Compatible" for the default case.

Slightly confused by @AaronMcHale in #48:

#46 (me), #47 (webchick) and #17 (xjm) all seem to be leaning towards removing all the compatibility details entirely for a compatible release. No new words / clutter. Just the same old download link. But in #48 you're saying to remove the details but leave the word "Compatible"? That seems a bit odd. I'd say we should either leave "Compatible" as a working details that shows the details, or remove it entirely. But adding a word (that's going to repeat a lot on this report) that seems fairly unnecessary, seems like a not-so-great-UI change...

Re: visual scanning -- Isn't scanning for "Download" (blue) vs. "Not compatible" (red) going to be sufficient? Do we really want/need to add green check boxes to the Download links?

I tried for a bit to get the error X icon on the summary, but I apparently don't have enough CSS-fu to make that work. ;) Any help on that welcome. If, indeed, we want an X icon next to "Not compatible" at all. I'm also worried a bit about icon overload, since we already have icons on each row about "up to date" (green check) vs. "update available" (yellow warning) vs. "security update missing" (red error).

Compatible

Initial

Open

Not compatible

Initial

Open

whoops, uploaded 1 wrong file... here it is.

Minor CSS cleanup, fixes (e.g. to make it work right when you expand the details so nothing crashes into anything else) and syncing the CSS between core/modules/update/css and core/themes/stable/css. End result looks the same for the purpose of the screenshots, so not uploading anything new for those.

I started looking at Claro, but it's very opinionated about <details> styles, so it's going to be more work to override all that and get it looking similarly there. @todo. ;)

Added these to remaining tasks:

• Decide #48: "Not compatible..." (with ellipsis)
• Decide if we want icons next to 'Not compatible' and perhaps download link (#6, #16, #48, #49, ...)
• Fix compatibility message to be aware of project type (not hard-code 'module' into the text)
• Fix styling:
  • Seven: Mostly done
  • Claro: Todo

Re: Fix compatibility message to be aware of project type (not hard-code 'module' into the text)
-- or maybe better yet, just say "This release is compatible ..." instead of 'module' or 'theme'.

Yeah, "release" is more accurate, anyway. Way simpler fix / smaller interdiff.

• Decide if the not-compatible message should say the currently installed core version, and if so, above or below the ranges of compatible core releases. See #46.4, #48, #55

p.s.: Again, to get these screenshots, I have to hack a FALSE return for isCoreCompatible() given the current state of releases. So ignore that it thinks 8.8.0 is incompatible with this stated range...

@AaronMcHale re: #56 thanks! Adding this to the remaining tasks:

• Decide if the release notes should go above the download link in the compatible case (#56)

Personally, I'd vote no. In practice, in the compatible case, "everyone" just wants to download and "no one" reads release notes. Swapping this order seems to put the less likely task first (even if we wish it was more likely). Also seems like scope creep (although I admit it's hard to tell what's in and out of scope in this issue).

If anything, to address the inconsistency, I'd reconsider the decision to move compatibility below release notes. If we remove "Compatible" entirely, and only show the details for "Not compatible", I think it'd be fine to have:

[> Not compatible]
Release notes

Cuz in that case, if it's not compatible with core, that's even more important for you to know than whatever the release notes say (which may or may not even tell you about the core compatibility, even though that could be happening automatically on d.o release nodes -- #NeedsFollowup?)

Thanks for the meetings, decisions and updates!

Harvesting all the action items from #58 and #60 into remaining tasks.

I'll take a stab at this now, stay tuned...

1. Add helper method for creating the details element
2. Open details by default for incompatible case.
3. Add sentence at top explaining compatibility summary in both cases.

- Fix text for compatibility range message.

(whoops, reversed which interdiff I attached to which comment... sorry about that).

- Remove red vs. green colors, always use grey.

- Restore download link for incompatible case.

@todo: Maybe we don't need to pass the $release['core_compatible'] bool to the update-version.twig.html anymore? Maybe people want to be able to tweak this?

Found some more CSS woes with Seven styles where the hover and focus for the details were causing it to remain blue. Trying to keep it grey, but use a bottom border (since text-decoration doesn't seem to work) for focus and hover...

Not sure I love this, but I'm trying to be consistent with the other Seven focus styles. /shrug

Release notes with focus

Compatibility details with focus

(Also fixing screenshot embeds in the summary from #68)

Main TODO remaining from the summary:

1. Decide if we should add a twig template for the compatibility details or let it all be hard-coded markup.
2. Decide if we should continue to pass core_compatible to the update-version.twig.html templates or not.
3. Claro styles
4. Fix existing tests
5. Add new tests?

Gotta stop for tonight. If anyone else wants to pick this up and run, feel free. ;)

Thanks!
-Derek

Adding this to the remaining tasks:

• Decide what the hover / focus styles should look like
  • Seven
  • Claro

Other minor updates to remaining if anyone else wants to work on anything.

@AaronMcHale re: #71: I agree that red for the not compatible summary would be better. However, for scanning, not compatible details are now open by default (#60.3 decision, #65.2 patch), which should help a lot. Also, neither of us were on the call where the decision to drop the colors was decided. Alas. ;) Not sure if it’s worth trying to keep redesigning this or just get something done enough to commit.

@webchick: re: #72 👍

@xjm re: #73 + #76:
I've been trying to save bot cycles and churn when we know the tests still fail.
I've been uploading (nearly) everything with the "Do not test" option selected. Apparently I missed on 1 or two. ;)
I can start adding do-not-test suffix to be extra clear (and to prevent accidents while uploading).
I don't want to fix the tests until we converge on what the actual output will be.

@AaronMcHale re: #74 👍

@testbot re: #76: Coding standards fixes -- well at least that much is useful feedback. 😜

@all: We're back up to 3 "Decide" tasks:

1. Decide if we should add a twig template for the compatibility details or let it all be hard-coded markup.
2. Decide if we should continue to pass core_compatible to the update-version.twig.html templates or not.
3. Decide what the hover / focus styles for these <details> elements should look like:
   • Seven
   • Claro

Thanks everyone,
-Derek

Some other (hopefully) quick questions:

A) Can y'all confirm this is what you want the text of the compatibility details to look like? The meeting summary from @tedbow in #60 says that the compatibility range itself is supposed to say:

Requires Drupal core: 8.8.1 to 8.9.2

Earlier, in #58 @benjifisher said:

Yes, in both cases start with a full sentence that explains what "Compatible" or "Not compatible" means.

But I'm not sure on the exact wording y'all wanted for that. Here's what's in the latest patch/screenshots:

Compatible

Your version of Drupal core (8.8.0) is compatible with this update.
Requires Drupal core: 8.8.0 to 8.8.2

Not compatible

Your version of Drupal core (8.7.11) is not compatible with this update.
Requires Drupal core: 8.8.0 to 8.8.2

Is that cool?

B) Do we want not compatible stronger:

Your version of Drupal core (8.7.11) is not compatible with this update.
Requires Drupal core: 8.8.0 to 8.8.2

?

C) Should it say "... with this release" per #53 and later? I originally put "with this update" in the 1st sentence to not repeat ourselves with "This release is compatible ..." in the 2nd. But now the 2nd has been simplified to just "Requires Drupal Core: [ranges]".

Thoughts?

Thanks!
-Derek

p.s. This patch just fixes the CS bug mentioned in #76.

p.p.s. Edited example text in here to make sense. The screenshots are a hack where I force incompatible for a compatible release, since I don't know of any modules that have releases in the wild yet that would trigger the incompatible cases...

This "fixes" the unit tests. Sort of. ;) We don't completely assert everything about the new <details>, so at least the unit tests do not cover ::createCoreCompatibilityDetails(). Doing a direct assertSame() on an expected array vs. a computed render array is not going to be fun, unless we love having expected code like this in our unit tests:

      'core_compatibility_details' => Array &2 (
            '#type' => 'details'
            'compatibility_range' => Array &3 (
                '#markup' => 'Requires Drupal core: 8.9.0, 8.9.2'
            )
            '#title' => Drupal\Core\StringTranslation\TranslatableMarkup Object &0000000052c097a1000000005256a49c (
                'translatedMarkup' => null
                'options' => Array &4 ()
                'stringTranslation' => Mock_TranslationInterface_02ce4cbd Object &0000000052c097e2000000005256a49c (
                    '__phpunit_invocationMocker' => PHPUnit\Framework\MockObject\InvocationMocker Object &0000000052c097e3000000005256a49c (
                        'matchers' => Array &5 (
                            0 => PHPUnit\Framework\MockObject\Matcher Object &0000000052c097db000000005256a49c (
                                'invocationMatcher' => PHPUnit\Framework\MockObject\Matcher\AnyInvokedCount Object &0000000052c097e4000000005256a49c (
                                    'invocations' => Array &6 ()
                                )
                                'afterMatchBuilderId' => null
                                'afterMatchBuilderIsInvoked' => false
                                'methodNameMatcher' => PHPUnit\Framework\MockObject\Matcher\MethodName Object &0000000052c097dd000000005256a49c (
                                    'constraint' => PHPUnit\Framework\Constraint\IsEqual Object &0000000052c097dc000000005256a49c (
                                        'value' => 'translate'
                                        'delta' => 0
                                        'maxDepth' => 10
                                        'canonicalize' => false
                                        'ignoreCase' => true
                                        'exporter' => SebastianBergmann\Exporter\Exporter Object &0000000052c097df000000005256a49c ()
                                    )
                                )
                                'parametersMatcher' => null
...

However, a) ::createCoreCompatibilityDetails() might go away in favor of a twig template and b) I'm assuming we'll test that in the functional tests, once we get those working.

Anyway, patch includes a hacked drupalci.yml to only run Update module tests so we can start to see how this behaves...

This patch fixes the assertions in the existing functional tests to work with the new layout/markup. So, all existing tests are now passing. Crossing that off remaining tasks.

Also fixed the CS bug I introduced in #79. ;)

Fleshed out the point on adding new tests:

• Add new tests?
  • Make sure details are closed vs. open vs. by default for compatible vs. not compatible releases?
  • Assert the intro text is how we expect it to be for compatible vs. not compatible?
  • Any other new behavior in here we need test coverage for?

Do we need any/all of that?

Thanks,
-Derek

Re: #83: Thanks for the feedback, and you're welcome. :)

I'd still love UX review / feedback on #78, but I guess I can keep pinging the #ux channel in Slack until that's decided.

Removing the ?s from the new tests point in remaining tasks, since that's now a definite TODO. I don't think there's anything for "Any other new behavior in here we need test coverage for?" -- it's either already in the (now updated) existing test coverage, or covered by the other two points in #81. So I deleted that one.

I was imagining "needs frontend framework manager review" should wait until we're done hacking Claro's styles, too. That's what "Markup and CSS" review point was a placeholder for. I thought it was premature to try to get @lauriii to spend any cycles on this just yet. But if they have time to look now, they could hopefully help answer some of the pending "Decide" tasks, and provide feedback on the approach for Seven styles (which would probably inform / improve how we go about trying to get Claro's styles working, too).

Re: manual testing, it's kind of a mess to see this work locally on a real site. :( So I added all this to remaining tasks:

• Manual testing. Steps to see this in action:
  1. Install a clean 8.9.x site.
  2. Apply latest patch.
  3. Download an older version of a module with a newer release that specifies core_version_requirement. E.g. token 8.x-1.5 (since 8.x-1.6 is out, and that defines the core compatibility in the new format that triggers all this).
  4. Modify your copy of core/lib/Drupal.php and set const VERSION = '8.8.0'; 8.0.0 (see patch in #85).
  5. Visit /admin/reports/updates and check for updates and inspect the "compatible" case.
  6. To see what would happen for an incompatible release, hack your copy of core/modules/update/src/ProjectCoreCompatibility.php so that isCoreCompatible() always returns FALSE (see patch in #85). Ignore the fact that the message will say something like "Your version of Drupal (8.8.0) is not compatible. Requires Drupal core: 8.8.0 to 8.8.2" (or whatever).

Thanks,
-Derek

Sorry, typo in the support patch and instructions. Fixing.

Woot! I asked in Slack if anyone had an official release of a contrib module that already requires 8.8.0. @berdir linked me to redis which is exactly what we need. This definitely simplifies manual testing! Updating the testing steps in the summary accordingly. Also update the screenshots, since now the "Not compatible" case makes sense.

Re: #88 -- that all makes sense and is fine with me. Would definitely simplify the code, and remove one of the items that #NeedsTests. ;) I've just been struggling to keep track of who's deciding what, and didn't want to completely ignore @benjifisher's input on this point. Can we consider that a final decision and proceed to rip it back out?

Thanks,
-Derek

Thanks @andrewmacpherson and @rainbreaw for the info! Very helpful.

I do think red would improve the UX here, and would not be an accessibility problem. We already have a very dark red (#a51b00) that we're using on this page for high contrast with the light red (#fcf4f2 - 7.02:1) or light yellow (#fdf8ed - 7.19:1) backgrounds where these <details> might appear. I'd like to restore red for the "Not compatible" summary / details, assuming there are no objections.

But first, here's a patch and screenshots for #88.

p.s. With permission, pasting @rainbreaw's Slack comments for posterity / education:

dww: I understand it's an accessibility problem to rely on color to convey any information. However, it seems like it shouldn't be a problem to use color as a supplemental way to express something, right? I'm working on https://www.drupal.org/project/drupal/issues/3102724 and earlier iterations of the patch used red for the <details> summary text that says "Not compatible". Later reviews have requested we remove the color for fear of introducing an accessibility problem. I don't care that strongly, and am happy to go along with it, but it seems like a bit of superstition / misplaced worry. Can anyone confirm / deny my hunch on this?

rain: I didn’t read through the issue in order to get the context of concern about using color as a supplemental indicator, but you are correct. Color may be used as an indicator as long as 1) it’s not the only way to get the information that the color conveys, 2) it has sufficient contrast (4.5:1 under most conditions for level AA, 7:1 under most conditions for level AAA), and 3) it’s meaning and legibility has been carefully evaluated so that you are confident it is clear. For some, red can be a problematic color when used for text on screens because it may take more for them to focus.

rain: When done well, using color as another source of meaning can be a huge positive, as some users may have difficulty understanding the meaning of the text, and others may not be sure of the meaning of icons. (edited)

rain: Keep in mind that color has different meaning to different people as well

... Then @andrewmacpherson chimed in, but they already posted above, so I won't re-paste the Slack thread that continued.

Thanks,
-Derek

Restoring the dark red for "Not compatible". Leaving the screenshots of all grey from #92 in the summary for comparison.

Minor summary cleanups, and officially crediting @andrewmacpherson, too.

Adding #2980304: Seven theme's details/summary focus style is broken/missing in some browsers. as related. In general, Seven's focus styles for <details> are inconsistent and broken on many browsers. :( Including on the admin/modules page that's the inspiration for these styles -- there's no visible focus indication *at all* on Chrome or Safari.

I hacked on this for a while, and discovered that although some docs suggest it works, using text-decoration on the <summary> is very poorly supported by most browsers. In my testing, only works on Firefox, but not Safari nor Chrome. I don't have access to other browsers to play with. Ugh.

So I tried using a bottom-border on hover. That mostly works, but doesn't look very nice. That's basically the screenshot from patch #69.

Then I tried making the summary display: inline-block so that the border is only as wide as the text. That looks way better on Safari and Chrome. But that ends up completely removing the show/hide triangle icon on Firefox. 🤦‍♂️

So then I read more crap online about hiding the default triangle and defining your own, I found #2405059: Update Drupal's default menu icons to use SVG and harvested some triangle SVG icons and CSS hackery, twiddled with it some more, and came up with this. Ouch. But it's working "well", and looking basically right on FF, Chrome and Safari:

(These screenshots are all from Chrome, but they look nearly identical in Safari and FF):

Initial state

Focus: Release notes

Focus: 'Not compatible' details summary

(Hit tab once after above screenshot):

Focus: 'Not compatible' details summary, collapsed

(Hit space once after above screenshot):

🎉🤦‍♂️🤞

I'm really not sure I'm the right person to try to get this working in Claro. If anyone else wants to run with that, please do! 🙏

Thanks,
-Derek

Now that I actually read all of #2980304: Seven theme's details/summary focus style is broken/missing in some browsers. and saw the solution in there, that's *WAY* better than #96. So much simpler. Tested #94 in here in combo with the latest patch over there and got this working with way less treachery and weirdness. Bumped that to RTBC. If we can land that one ASAP (a major bug in its own right), that'll simplify life in here considerably.

Thanks!
-Derek

While clicking around on my local test site for this issue, I noticed:
#3113992: The 'Update' page has no idea that some updates are incompatible
:( That's *definitely* out of scope for this issue, but it's at least major, perhaps another critical blocker...

Sorry,
-Derek

@benjifisher: Thanks for opening that follow-up, and for the updates.

Re:

Another point where the consensus went against me: as I recall, we agreed to remove the Download link in the Incompatible case. I am setting the status to NW for that.

Ugh. :/ A) that "decision" wasn't indicated at all in #60 or later. We've gone back and forth on this a few times already. I really don't want to change it again. If we have to, okay, but I'd really rather not.

Thanks,
-Derek

Re: #101 / #103.1: Okay, re-removed. New screenshots for the summary.

I'm interdiffing vs. #94 since I'm backing out the hacks in #96 for now in the hopes that we solve that via #2980304: Seven theme's details/summary focus style is broken/missing in some browsers. instead.

Re: #103.2: I've tried extremely hard to keep the summary of this monster issue up to date. I don't know what more I can do to make it easier to follow. Multiple folks in earlier iterations agreed that color would be nice. The UX team meeting had a superstitious fear of using color by not understanding the accessibility implications and dreading possible delay for an actual review. I spent the effort to research it myself, then ask the experts, then report the findings here. I also verified that the color (which we're already using on this page) has >7:1 contrast with both possible backgrounds. There's a lot to consider, but we've considered it, and it's already implemented and screenshot'ed for everyone to compare and confirm it's easier to scan than the all-grey alternative.

This is already a follow-up to a critical issue that was (IMHO) rushed in prematurely. I don't want to add yet-more-follow-ups for things that are already done, researched, and working. Let's get this as good as we can (short of a complete redesign of the report) and then we can be done, instead of making the issue spaghetti even more complex.

Finally, no one is saying it's an "accessibility issue if we don't use color", that's a totally unfair red herring of an argument. We're saying that using color to supplement the meaning of "Not compatible" helps the UI, and perhaps helps some users understand something's not right with a given update if they don't fully parse and digest all the text. Please don't put words in my mouth.

Thanks,
-Derek

Whee, fun! ;) Now 4 tests are failing because they can't find their download link. :/ Can we reconsider this decision and just go back to #94? Or do we really want to sprinkle checks and knowledge about core compatibility and download links throughout all of our tests?

Re: #108-#110:

Okay, I'm clearly out numbered. ;) Following the follow-up-follow-up. Still a bit sad the color improvements aren't going to make it into the same releases as the other UI fixes here. I thought the scope here was "Improve usability of core compatibility ranges on available updates report", which thoughtful use of color aids.

Meanwhile, we need new screenshots. Based on prior experience, I suspect #110 didn't solve the color problem and necessarily get us ready to commit, but instead re-introduced the weirdness where the summary is blue in some cases and grey in others, like it was in #92:

https://www.drupal.org/files/issues/2020-02-15/3102724-92.seven-initial-...

I'm clearly getting burned out trying to land this issue, so I'll probably step back and let y'all run with it for a while.

Thanks/sorry,
-Derek

Instead of #114, this is both way simpler and the tests now mostly pass. Interdiffs relative to both #112 and #114.

However, I still get 1 fail for this:

1) Drupal\Tests\update\Functional\UpdateContribTest::testSecurityUpdateAvailability with data set "8.x-1.0, 8.x-1.2 8.x-2.2" ('8.x-1.0', array('8.x-1.2'), 'SECURITY_UPDATE_REQUIRED', 'sec.8.x-1.2_8.x-2.2')
Release http://example.com/aaa_update_test-8-x-1-2.tar.gz is a security download link.
Failed asserting that false is true.

/.../drupal-8_9/core/tests/Drupal/TestTools/PhpUnitCompatibility/PhpUnit6/TestCompatibilityTrait.php:18
/.../drupal-8_9/core/modules/update/tests/src/Functional/UpdateTestBase.php:158
/.../drupal-8_9/core/modules/update/tests/src/Functional/UpdateContribTest.php:613

Unfortunately, aaa_update_test.sec.8.x-1.2_8.x-2.2.xml is a tangled web of <core_compatibility> definitions, and given the lack of comments on the fixtures, I'm not sure that changing them is okay without breaking something else. I'll leave this one for @tedbow to sort out. But in short:

• The test case says currently-installed core is 8.0.0.
• The test is expecting to see a 8.x-2.2 security release with a download link.
• In that fixture, the 8.x-2.2 release says: <core_compatibility>^8.1.1</core_compatibility>.

Something's gotta give. ;)

Sorry, I missed #116 (x-post)

Re: #116.2: See remaining tasks for the point on "Add new tests". ;) We need more than "a test that link doesn't show up for incompatible releases"...

Generally, keeping such TODO items in the summary is way easier to track than having them sprinkled throughout all the comments. To that end, updated the Seven styles TODO items again after #110.

Thanks,
-Derek

Oh right, core/modules/update/tests/src/Unit/ProjectCoreCompatibilityTest.php
I still don't love how that test works, trying to do assertSame() as it does on entire arrays. But instead of adding the new flag to each expected case, we can just ignore this flag, too (since we'll have real tests for the presence/absence of the download links elsewhere).

Based on UX meeting today, we agreed to move getting Claro styles perfect to a follow-up: #3114910: Improve Claro styles for core compatibility details on available updates report
Screenshots in #122 are Good Enough(tm) to ship this one.
Removing more things from remaining tasks. :) Yay!

Cheers,
-Derek

Re: CSS styles and how they work...

#3113992-10: The 'Update' page has no idea that some updates are incompatible shows we're going to want to re-use these custom <details> styles on the /admin/reports/updates/update form, too.

We should probably refactor the CSS and inject a special class directly into the details element so they can apply in both cases. Or something. ;) Awaiting feedback from @lauriii and/or others who are more how-core-handles-markup savvy than I am.

Thanks,
-Derek