Move title of confirm form to a backdrop message

[robertgarrigos]

Description of the need

I always found strange that the confirm form function sets the title of the page. This issue made me think about a practical solution to override this functionality. However, I would say it would be much better to change core.

Proposed solution

This is what happens now. The demonstration contrib module uses tabs:

These two tabs show the title normally, but the third one is a confirm form:

and the title gets changed. Of course, you can still see the context in the breadcrumb, but what happens if you don't use the breadcrumb? Context indeed gets lost.

Wouldn't it be much nicer this?:

Alternatives that have been considered

I don't see an easy way to accomplish this functionality by altering code with a contrib module.

Additional information

I will provide a PR

Draft of feature description for Press Release (1 paragraph at most)

Confirm forms now show the title as a warning backdrop message.

[robertgarrigos]

This is how a blank backdrop works with this pr:

Instead of:

[izmeez]

This is fantastic. Looks good, much better. PR works and the code is a one line change. Now, what bad things can happen? I don't know.

[izmeez]

The PR also fixes the issue for the demo module, https://github.com/backdrop-contrib/demo/issues/32. It will change behaviour contrib modules are used to and may be a problem? Or may be an overall improvement?

[izmeez]

This issue may be really ancient and extends back to Drupal 7. If you go to a Drupal 7 site with something like the backup_migrate module, look at saved backups and go to delete one. The confirm question message shows in place of the title.

[izmeez]

Here is how this core fix improves the current backdrop-contrib/backup_migrate delete saved backup display. Before patch:

After patch:

Both before and after show the wrong tab menu highlighted, so that's a separate issue for bam.

[izmeez]

Maybe, this can qualify as a bug fix?

[indigoxela]

I'm not really sure, whether this change isn't an A11Y regression.

Instead of providing the info in the page title AND the meta title for quick access, it's now just some text somewhere on the page. And the page title is the generic title of the content or item we're about to delete. No A11Y expert, though.

At the very minimum I'd expect the title to be something like "Confirm to delete [title of item]". Not sure if that would be sufficient.

[avpaderno]

What I find more confusing on these screenshots is the fact the tab/sub-tab is still showed as active.

When I deleted a node, the title is changed, but none of the visible tabs is shown as active.

Should not what done by Backdrop core when deleting a node be always done when a confirmation page is shown?

[avpaderno]

When deleting a view from the page showing all the existing views, the tabs disappear.

They disappear because the page changes from /admin/structure/views/list to /admin/structure/views/view/<view machine name>/delete and the latter does not have any tab. (In the case of nodes, the page changes from /node/<node ID>/edit to /node/<node ID>/delete.)

Even in this case, the title change is not confusing, since the page is clearly another one.

[indigoxela]

What I find more confusing on this screenshot is the fact the sub-tab is still showed as active.

@avpaderno is this something directly related to the change proposed here, or an independent (new) topic?

[robertgarrigos]

What @avpaderno is talking about has nothing to do with the changed proposed here. I just changed one backdrop_set_title() for a backdrop_set_message(). Tab functionality has not been changed.

[avpaderno]

@avpaderno is this something directly related to the change proposed here, or an independent (new) topic?

I am talking of the screenshots shown right after Proposed solution and in another comment (which shows the before-and-after changes), so yes, it is related to the change proposed here.

I am wondering if this is an issue with a contributed module because, as I shown in the screenshots attached to my previous comments, there is no issue with Backdrop core and the fact the page title is changed.

[robertgarrigos]

@avpaderno, yes, this is just how those contrib modules handle those tabs. Core's behavior is as you have shown, but again, this PR has nothing to do with how core or contrib modules handle the tabs, even I used them to explain the problem.

However, I got the point that I have to take a wider look at this issue, also, as @indigoxela pointed out, from an A11Y perspective. I agree that having just the same title is not clear enough. I'll try your suggestion of changing the title and printing the warning message as well.

[robertgarrigos]

Added the title keeping the warning:

[indigoxela]

Added the title keeping the warning:

Oh yes, better.

Of course, loads and loads of functional tests are failing, as they expect a different string for their checks. :wink: And phpcs also has something to nag - coding standards.

But let's see what people think re this UI change. Personally, I find this is an improvement, but I'm no UX specialist.

[robertgarrigos]

Fixed coding standards and failing tests

[avpaderno]

@avpaderno, yes, this is just how those contrib modules handle those tabs. Core's behavior is as you have shown, but again, this PR has nothing to do with how core or contrib modules handle the tabs, even I used them to explain the problem.

The issue is exactly how that contributed module handles tabs. It shows a confirmation form on the already rendered tab; that is why the page title changes.

It would not happen if the confirmation form would be rendered on a different page, which is what Backdrop core does with /node/<node ID>/delete.

With the provided PR, /node/<node ID>/delete is rendered as follows.

I prefer how Backdrop renders /node/<node ID>/delete.

[argiepiano]

The current PR assumes that confirm_form() is always used for deletion purposes (it hard-codes "Confirm deletion" as the title. This is not always the case. Confirm forms ARE used for other purposes.

For example, to confirm rebuilding permissions. Navigate to /admin/reports/status/rebuild and you'll see this erroneous and confusing page 👎🏽 :

confirm_form has a myriad of other uses in contrib, that are not deletions.

Hard-coding titles in a function that is supposed to be agnostic of context is a "no-no" for me. In that same vein, using an if statement to check if the passed $form is that of a node is very limiting use of code, and also a "no-no" for me.

On the other hand I do agree about the ugliness of the UI before the PR.

[robertgarrigos]

good point, @argiepiano, I agree. Let's find a different approach. The simplest solution would be to set a fixed, agnostic of context, title, but very clear about what we are doing:

No need of if statement either. Just two lines of code:

PR fixed with this approach.

[argiepiano]

OK, this is better, but keep in mind that system messages may be shown in a different spot of the layout (or even as a modal window that disappears after a certain amount of time, as is the case with some themes). So, we can't trust that the warning will always be shown in the "right" spot (below the title and above the button). This may confuse the user.

I feel like a system warning should not be used in this case - AFAIK system warnings are rarely used to prompt the user for some action (i.e. confirm something by clicking a button) - they are informational messages that are the results of an action.

Curious to hear others' feedback.

[robertgarrigos]

Another good point, @argiepiano, I agree. But I'll fix this ;-)

These are not system messages. Note that there is no cross to close them in the upper right corner. They are form elements with prefix to shows as warning message and will not disappear:

Just pushed a commit with this fix.

[klonos]

I like the sentiment here, but would really prefer to have the confirmations thrown by confirm_form() replaced by a confirm_dialog() instead, which pops up a dialog instead of taking the user to another page/path. In other words, I would like us to do #3769 / #3771. That would be an awesome feature for 1.30 I think.

The good thing with such a solution is that users are not "redirected" somewhere else - they are kept within the same context of what they were doing.

[robertgarrigos]

This sounds a better solution, indeed.