Problem/Motivation

On the confirmation forms (example: /admin/structure/views/view/content/delete) the "cancel" option is styled as a link when it should be styled as a button.

before
There are three types of buttons supported in core and in the Seven style guide. Standard, primary, and danger.
It's important that the correct button types are used consistently throughout Drupal

Remaining tasks

Review and commit

User interface changes

The anchor tag in all entity deletion confirmation forms got one new class, button.

after

API changes

None.

Beta phase evaluation

Reference: https://www.drupal.org/core/beta-changes
Issue category Bug because usability standards
Issue priority Not critical because minor usability defect
Unfrozen changes Unfrozen because it only changes markup
Prioritized changes The main goal of this issue is usability
CommentFileSizeAuthor
#20 2469929-20.patch516 bytesmondrake
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 94,344 pass(es). View
#20 interdiff_18-20.txt1.09 KBmondrake
#20 2015-04-15 16_02_28.png13 KBmondrake
#18 2469929-18.patch1.59 KBmondrake
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,444 pass(es). View
#18 2015-04-15 15_39_18.png12.59 KBmondrake
#9 Screen Shot 2015-04-14 at 11.35.02 AM.png73.69 KBBojan Živkov
#9 Screen Shot 2015-04-14 at 11.33.52 AM.png77.48 KBBojan Živkov
#4 2469929-4-added-classes.patch560 bytesBojan Živkov
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,419 pass(es). View
Members fund testing for the Drupal project. Drupal Association Learn more

Comments

LewisNyman’s picture

Title: The delete view confirmation form's cancel link should be styled a a button » The delete view confirmation form's cancel link should be styled as a button
Bojan Živkov’s picture

I will take it

Bojan Živkov’s picture

Assigned: Unassigned » Bojan Živkov
Bojan Živkov’s picture

FileSize
560 bytes
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,419 pass(es). View

This is not views related, it's related to all entities.
So i added the "danger" button class to ConfirmFormHelper.

Bojan Živkov’s picture

Status: Active » Needs review
LewisNyman’s picture

@Bojan Živkov Is there a way of knowing which pages this will affect?

Bojan Živkov’s picture

@LewisNyman This will affect all "delete confirmation" pages where "cancel" button is displayed.

Bojan Živkov’s picture

Assigned: Bojan Živkov » Unassigned
Bojan Živkov’s picture

Title: The delete view confirmation form's cancel link should be styled as a button » The entity delete confirmation form's cancel link should be styled as a button
Issue summary: View changes
FileSize
77.48 KB
73.69 KB
LewisNyman’s picture

Issue summary: View changes
Status: Needs review » Reviewed & tested by the community

We discussed this at Drupal Dev Days and found that the scope of this issue is broader than just views. The title and issue summary has been updated and screenshots added.

alexpott’s picture

Status: Reviewed & tested by the community » Fixed

This issue is a normal bug fix, and doesn't include any disruptive changes, so it is allowed per https://www.drupal.org/core/beta-changes. Committed 168a859 and pushed to 8.0.x. Thanks!

  • alexpott committed 168a859 on 8.0.x
    Issue #2469929 by Bojan Živkov: The entity delete confirmation form's...
mondrake’s picture

Priority: Normal » Critical
Status: Fixed » Needs work

This does not look right to me. The element now in red is the 'cancel' link, which is usually meant to say 'abort the delete request and return to the entity edit form', i.e. it is not deleting anything. It's the 'Delete' button that should be styled for danger I think.

Also - the patch changed ConfirmFormHelper so now all the 'cancel' links in any confirmation form are red. Should the change be on EntityDeleteForm instead?

tim.plunkett’s picture

Priority: Critical » Normal
Status: Needs work » Fixed

Unless this needs to be urgently reverted, please open a new issue.
And the priority should be normal or major, not critical. Thanks!

  • alexpott committed c52ea87 on 8.0.x
    Revert "Issue #2469929 by Bojan Živkov: The entity delete confirmation...
alexpott’s picture

Status: Fixed » Needs review

Actually I agree with @mondrake - why is a cancel link styled as a danger button. Reverted since there is no conflict.

LewisNyman’s picture

Issue summary: View changes
Status: Needs review » Needs work

Ah sorry guys, too many issues! The cancel link should be styled as a button, I've updated the issue summary to reflect this.

mondrake’s picture

Issue summary: View changes
Status: Needs work » Needs review
FileSize
12.59 KB
1.59 KB
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 91,444 pass(es). View

How about this?

Before:

After:

LewisNyman’s picture

Status: Needs review » Needs work

The change the the cancel element looks good, but we should leave the confirm button as a primary button for now, there is an issue open to add styling for elements that are primary and danger #2278715: Introduce a primary-danger link/button class

+++ b/core/lib/Drupal/Core/Form/ConfirmFormHelper.php
--- a/core/themes/seven/css/components/buttons.css
+++ b/core/themes/seven/css/components/buttons.css

+++ b/core/themes/seven/css/components/buttons.css
+++ b/core/themes/seven/css/components/buttons.css
@@ -188,6 +188,7 @@

@@ -188,6 +188,7 @@
   color: #c72100;
   font-weight: 400;
   text-decoration: underline;
+  text-shadow: none;

Do we need this change to the CSS? Maybe if we need this should add it in a separate issue

mondrake’s picture

Related issues: +#2278715: Introduce a primary-danger link/button class
FileSize
13 KB
1.09 KB
516 bytes
PASSED: [[SimpleTest]]: [PHP 5.4 MySQL] 94,344 pass(es). View

@LewisNyman ah ok did not know, makes sense. See new patch and screenshots.

The text-shadow: none; in previous patch was needed to avoid inheriting text-shadow attributes from primary button. But I guess it will all have to be sorted in the other issue.

After:

mondrake’s picture

Status: Needs work » Needs review
LewisNyman’s picture

Status: Needs review » Reviewed & tested by the community

Nice, thanks :)

mondrake’s picture

Title: The entity delete confirmation form's cancel link should be styled as a button » The confirmation forms' cancel link should be styled as a button
Issue summary: View changes

Note that this applies to all confirmation forms - ContentEntityConfirmFormBase, EntityConfirmFormBase, ConfirmFormBase - not just entity delete. Updated title and IS.

roderik’s picture

Issue summary: View changes

(updated screenshots - the original links in the issue summary were apparently dead links)

YesCT’s picture

Issue summary: View changes
YesCT’s picture

Issue summary: View changes
xjm’s picture

Status: Reviewed & tested by the community » Needs review
Issue tags: +Needs usability review
--- a/core/lib/Drupal/Core/Form/ConfirmFormHelper.php
+++ b/core/lib/Drupal/Core/Form/ConfirmFormHelper.php

So this change is being made according to the Seven style guide, per the summary, but it's not just changing Seven; it's changing all confirm forms, everywhere.

I'm fairly certain there was a reason these cancel links and other secondary operations were made links instead of buttons previously and this is reverting that. Setting Needs Review for more discussion. Please add references to the summary for other issues/documentation related to this, as well.

tim.plunkett’s picture

I recall the same as @xjm, but I guess it was for other cancel links and not just confirm forms, because confirm forms have ALWAYS been links.

#922634: Odd form actions in confirm_form() switched it from using #markup => l() to #type => link, and that was in 2010.
#29465: New drupal forms api. added confirm_form, using l()

Bojhan’s picture

Issue tags: -Needs usability review

I think that the web has evolved. We used to style it as a cancel link, because we wanted to make a clear visual distinction. However we now have primary buttons implemented very consistently. This should help us get to the same user behaviour.

With the new style guide we are pursuing a more common web standard for these primary/secondary buttons. We now only (intend to) use the link style for deletions, since that has to really stand out.

So I think its just a change in our approach, from 2009 :)

LewisNyman’s picture

Status: Needs review » Reviewed & tested by the community

So this change is being made according to the Seven style guide, per the summary, but it's not just changing Seven; it's changing all confirm forms, everywhere.

This is true, and consistent with how we add the other button classes. We want to maintain the design intent in the admin UI, and allow other admin themes to 'reskin' the UI instead of re-making the design decisions. See Changes to Drupal 8 that affect admin theme maintainers for more info.

veronicanerak’s picture

I tested the 2469929-20.patch and it works fine as seen in the issue summary

  • xjm committed 1fbb134 on 8.0.x
    Issue #2469929 by mondrake, Bojan Živkov, LewisNyman, tim.plunkett,...
xjm’s picture

Status: Reviewed & tested by the community » Fixed

Excellent, thanks @tim.plunkett, @Bojhan, and @LewisNyman for the additional information.

This issue mostly only changes markup (and render arrays), and is a usability and themeability improvement per @Bojhan and @LewisNyman, so it's fairly unfrozen and a prioritized change per https://www.drupal.org/core/beta-changes and can be completed any time during the Drupal 8 beta phase. Committed and pushed to 8.0.x. Thanks for adding the beta evaluation for to the issue summary, and also for the screenshots and manual testing illustrating the change.

Status: Fixed » Closed (fixed)

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