Incorrect behaviour for block page visibility

In addition, it should be explicitly documented that the behaviour is different from previous versions of Drupal.

For instance, if I want a block to only appear on mysite.com/node/1, then I should enter /node/1 in the block visibility field, with the leading slash.

Here's a patch that changes the message.

One question, though - is the behaviour the same in other places where this form is used, as well as blocks?

Hmm I thought 'Hide for the listed pages' might have changed behaviour in D8 but no. Not sure that the help text change will solve any potential usability issues here, sorry it seems a bit off topic - perhaps that could be moved to a new issue?

That also makes me confused.

I'm curious whether we could add some validation so people don't accidentally add URLs without leading slash.

adding related issue, #2843992: Block page visibility paths are not validated, using path a without leading forward slash does not work

For an empty list, "Show on the listed pages" should not show the block anywhere. And, "Hide on the listed pages", should have the block show up on every page, when the list is empty.

I disagree with the first statement, but agree with the second (and the latter part of the issue summary mostly echoes what I think).

For me, an empty list should have no effect, and no effect = 'Not restricted' (as per the vertical tab summary label) - so it makes sense for it to be visible everywhere (barring restrictions in other tabs). However it's very misleading to hide from all pages if the list is empty, especially when the summary label is still "Not restricted". (After all, why would you use that radio button to hide a block everywhere when you could just disable it?)

Example: I had a view block using a contextual filter and hook_views_pre_view() to pass the default value; on some previous occasion I must have decided to hide it from one or more pages but later removed the list, not realising I needed to change the radio button back to show - more than once trying to debug it today, I noticed how it was set, thought, "surely it can't be anything to do with that, there's nothing in the list...", and moved on. Eventually I changed it as a last resort, having added Logger statements to my hook.

If we're just changing descriptions (I think it's unwise to mess with the logic for existing sites - we'd have to heavily warn users before upgrading) - can we alter the vertical tab to say 'Hidden' or 'Hidden on all pages' rather than 'Not restricted' when is 'Hide' selected.

If that's too tricky, I suggest modify this patch so the description says:

"Leaving this blank will match all paths, or hide from all pages." or "…regardless of show/hide setting".

We definitely need to explicitly help people who've chosen 'Hide' and think the choice does nothing if they've not specified any URLs.

But often when someone places a new block, they won't even touch the Pages tab, and yet 'Show for the listed pages' is highlighted and the list is empty. The user hasn't actually actively chosen a combination, but you seem to be arguing we should treat it as though they have? What would you have happen in that situation? (Perhaps, if we were designing from scratch, we'd have a third, default, 'Do nothing' radio button).

Personally I feel requiring a user to enter a wildcard each time (or generating one with new blocks), just complicates things and is inconsistent with the other options (on the Pages tab, the block is visible on all pages unless they actively tick options, likewise Roles - an empty set of checkboxes is ignored.)

The main priority for me right now would be improving the messaging in the tabs and underneath the list, so people don't hide blocks by mistake (I think blocks being hidden when they shouldn't is a problem that's harder to fix / less obvious to spot than the other way around).

Would it be better to add a new radio button option for "Show on all pages", select this by default, hide the text area when this is selected, then validate that the box is not empty if either of the other two options are selected?

So I just spent a day trying to figure out why a block was missing. The most confusing part for me was that the vertical on the left still said "Not Restricted" even those the block wasn't showing up anywhere. I ended trying to look everywhere else because it said "Not Restricted" on the left.

Whichever route is decided upon for this the messaging needs to be consistent.

Closed #3051522: Block visibility not coherent as a duplicate so added credit here for nick.ev

So I just spent a day trying to figure out why a block was missing. The most confusing part for me was that the vertical on the left still said "Not Restricted" even those the block wasn't showing up anywhere. I ended trying to look everywhere else because it said "Not Restricted" on the left.

Whichever route is decided upon for this the messaging needs to be consistent.

ME TOO! And, I agree!!

...............
I think the "simplest fix" would be to "fix" the field help text (and the "not restricted" indicator, excellent point by a couple folks here), and leave the behavior as-is, it seems to me like changing the behavior would be very difficult to do in a backwards-compatible/non-breaking way.

Also, as confusing as many of us agree that the behavior and help text are, the lack of attention/outcry to this issue (for 6.5 years!) indicates to me that we're in the minority, so, a major change to the behavior like what we're talking about probably wouldn't be accepted.

My two cents :)

Two weeks ago the Bug Smash triage looked at block visibility issues and this was included in that review. The review suggested that this issue and #2451461: Hide/show block page visibility negate form element if pages form element is/is not empty need a Usability review to decide on what should or should not be done to clarify the 'Pages' section of block visibility. Then work will continue on their recommendations.

I've updated the IS including before and after screenshots to help that review. Adding tag.

I'll ask in #ux for a review.

Applied patch working fine after patch block page visibility description changed for ref sharing screenshot ...

@vikashsoni, thank you for looking into this issue. I think you missed that screenshots were added in the previous comment. There is no need to add another set. Therefor, credit has been removed per How is credit granted for Drupal core issues.

Usability review

We discussed this issue at #3239070: Drupal Usability Meeting 2021-10-01. That issue has a link to a recording of the meeting.

The consensus at the meeting was to follow the suggestion in #19 (also listed as Option 5 in the current issue summary).

Here is a brief summary of the discussion.

• Using * as a wildcard makes sense to programmers, but other users might not recognize it as such.
• Long help text (the solution in the current implementation) should only be used as a last resort. The longer it is, the less likely people are to read it!
• Leaving both radio buttons un-selected does not make it clear what is going to happen.

More specifically, the usability group recommends

1. Add a third radio button, selected by default: "Show on all pages".
2. By default, the text area is hidden.
3. When one of the other radio buttons is selected, the text area appears below the three radio buttons.

I am setting the issue status to NW to implement this solution. Remember to update the issue summary: list only the actual implementation under "Proposed reesolution" and update the screenshots. I am adding the issue tag for an issue summary update.

Deciding how to implement the feature is outside the scope of the usability group, but consider this comment from the related issue #2451461-5: Hide/show block page visibility negate form element if pages form element is/is not empty:

I think this should be part of the request path condition form always, not just when it is used in the context of blocks.

It might help to have a look at the patch attached to that issue. Or maybe the solutions here already follow that recommendation. I have not checked.

Another thing: I may be wrong, but I think any visible change to the admin UI needs a change record, so I am adding the issue tag for that.

I created a MR for this issue with the implementation suggested by the UX team. Maybe the checkbox label and the validation message should be improved...

I updated the IS with new images after the implementation.

Assuming my fix to the tests work, I support this being moved to RTBC.

Verified and tested merge request !1311 - (https://git.drupalcode.org/project/drupal/-/merge_requests/1311.patch)
Patch applied successfully and looks good to me.

Testing Steps:
# Goto: Appearance -> Apply Seven theme
# Goto: admin/structure/block/manage/seven_page_title?destination=/admin/structure/block
# Click on Pages
# Observe the results.

Expected Results:
# After applying merge request !1311, the "Show on all pages" checkbox appearing.

Actual Results:
# "Show on all pages" checkbox not appearing currently.

Please refer attached screenshots for the same.
Looks good to me.
Can be a move to RTBC.

Thanks to everyone for moving this older issue along. Just a few more things need to be done.

@chetanbharambe, thanks for the testing. To be RTBC there are still a few things to do here. There is more information about the steps in the Core gates documentation.

Setting to NW for a change record. I don't see a code review here either, adding that to the IS. The issue summary also needs an update, Specifically it contains a TBD in the User interface changes section which appears needs to be removed.

Then I took a look at the test. The only change here is to set show_on_all_pages to FALSE. The test here needs to set show_on_all_pages both TRUE and FALSE and check the changes to the displayed text on the Pages tab. Therefor adding 'needs tests' tag.

Hi everyone,
i read comment 30 again and the approach in MR is wrong because i created a checkbox instead of a third radio button...

I think it will be a little more complicated to implement because those options are created in BlockForm.php and not in the condition class RequestPath.php.

All changes and validation will need to be moved to the BlockForm.php file.

Updated the MR to add a third radio button instead of a new checkbox.
I updated the issue summary as well with new images.

@paulocs, thanks for re-reading the UX review. I looked at the screenshots and it does look much nicer than the previous implementation. Thanks for placing the latest screenshots in the IS.

I think the issue summary update and adding a change record tasks portion of this is suitable for a novice. I added these to the remaining tasks.

Updating issue summary...

I'll write some tests for it.

Added tests to it.

I also created a change record for this issue.

I did a code review, as you can see above. Note that I did not actually run the code.

Apart from the comments/suggestions above, I must say that it all feels a bit messy to me. While I like the change from a UX point of view, it doesn't look quite right to me on the code side. What used to be a boolean negate option (Negate? Yes / No) is being hijacked to add the "show_on_all_pages" option, even though "Show on all pages" is not an answer to the question "Should we negate these paths?". As a result, we have to cast values to integer and accept that we are violating our own schema definition. Is that really how we want our code to work?

Yes, I hijacked it but i did not save it with the wrong schema value.
I'll try to provide a new solution without hijacking it and fix the unresolved threads... Thanks for the review :)

Updated the MR with the changes suggested and change the target branch to 9.4.x.
Lets see if tests will pass as I removed the 'negate' key from the request_path block condition.

I put a few comments on the MR. Setting back to NW.

I created a new branch because i massed up the branches after 9.4.x branch was released.
I basically addressed what @danflanagan8 pointed except the new test he suggested because i need some clarification on what exactly he wants to be tested.

Setting to NW for the failing tests and for the test suggestions in an unresolved thread.

This is not merely unintuitive/confusing, if this problem occurs, the UX is telling people they do not even need to look at this (as noted by geekygnr in #24 and alison in #26).

1. Leave "Pages" blank.
2. Have the "Show for the listed pages" and "Hide for the listed pages" radio buttons set to "Hide for the listed pages"
3. The summary of Visibility will tell you Pages are "Not restricted" when in fact it is restricted so severely it will not show on any page.

Given that fixing this ought to change the behavior in the edge cases of blank lists, we should make that change, and so will also need an update hook to do so.

Even if we do not change stored configuration we should have an update that provides a link to the change record informing people the user interface behavior has changed (whether anything is updated or not). The update hook should further specifically alert people that the configuration they have may be leading to an unexpected result of hiding their block from every page, if they have 'negate' ('hide_on_pages' => 'Hide on specific pages') checked and the Pages textarea blank.

If we don't want to allow such a confusing case (the default of displaying everywhere is fixed by the 'Show on all pages' radio button in the patch) then we need a fourth radio button 'Hide on all pages' or we need to update such blocks to be disabled. (The latter is better in my opinion, provided we let people know which blocks were in such a state. But i'm not sure how that would interact with other visibility display settings, let alone more complex modules like Block Visibility Groups.)

Where does the logic to invert the standard behavior for a blank list actually happen? I know it's sort of common in Drupal but whenever it is also paired with 'Negate the condition' the double negative (negate an empty list that is interpreted as everywhere == hide everywhere) is bound to be counterintuitive anywhere it comes up. I cannot find it in core/modules/system/src/Plugin/Condition/RequestPath.php or core/lib/Drupal/Core/Condition/ConditionPluginBase.php (which provides negate as a default option) nor in core/lib/Drupal/Core/Path/PathMatcher.php.

Block visibility based on Vocabulary (using core/lib/Drupal/Core/Entity/Plugin/Condition/EntityBundle.php i think) has the same odd behavior (you can select nothing at all, choose negate the condition, and then the block is hidden everywhere) but at least it doesn't set any expectations at all— no help text at all and no summary in the vertical tabs!

Some or all of my comments are probably for follow-up issues. A lot of work has been put into this, especially by paulocs; it would be wonderful to get this across the finish line.

MR has conflicts even with 9.x (but also needs to be redone against 11.x) -- anyway, adding "needs reroll" tag (right?)

I don't think I did it particularly well but I have started a re-roll to 11.x

Testing the UI I think it is working as expected but there is probably still a lot of fixing to do.

I cherry picked the commit but the merge conflicts were difficult to resolve. It will need additional work.