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
#20 interdiff_18-20.txt1.09 KBmondrake
#20 2015-04-15 16_02_28.png13 KBmondrake
#18 2469929-18.patch1.59 KBmondrake
#18 2015-04-15 15_39_18.png12.59 KBmondrake
#9 Screen Shot 2015-04-14 at 11.35.02 AM.png73.69 KBBojan Zivkov
#9 Screen Shot 2015-04-14 at 11.33.52 AM.png77.48 KBBojan Zivkov
#4 2469929-4-added-classes.patch560 bytesBojan Zivkov
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

LewisNyman’s picture

LewisNyman
he/him
CreditAttribution: LewisNyman commented
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 Zivkov’s picture

Bojan Zivkov CreditAttribution: Bojan Zivkov commented

I will take it

Bojan Zivkov’s picture

Bojan Zivkov CreditAttribution: Bojan Zivkov commented
Assigned: Unassigned » Bojan Zivkov
Bojan Zivkov’s picture

Bojan Zivkov CreditAttribution: Bojan Zivkov commented
FileSize
2469929-4-added-classes.patch560 bytes

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

Bojan Zivkov’s picture

Bojan Zivkov CreditAttribution: Bojan Zivkov commented
Status: Active » Needs review
LewisNyman’s picture

LewisNyman
he/him
CreditAttribution: LewisNyman commented

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

Bojan Zivkov’s picture

Bojan Zivkov CreditAttribution: Bojan Zivkov commented

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

Bojan Zivkov’s picture

Bojan Zivkov CreditAttribution: Bojan Zivkov commented
Assigned: Bojan Zivkov » Unassigned
Bojan Zivkov’s picture

Bojan Zivkov CreditAttribution: Bojan Zivkov commented
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
Screen Shot 2015-04-14 at 11.33.52 AM.png77.48 KB
Screen Shot 2015-04-14 at 11.35.02 AM.png73.69 KB
LewisNyman’s picture

LewisNyman
he/him
CreditAttribution: LewisNyman commented
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

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott commented
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

mondrake
Location 🇮🇹
CreditAttribution: mondrake commented
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

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett at Acquia commented
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

alexpott
he/they
Primary language English
Location 🇪🇺🌍
CreditAttribution: alexpott commented
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

LewisNyman
he/him
CreditAttribution: LewisNyman commented
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

mondrake
Location 🇮🇹
CreditAttribution: mondrake commented
Issue summary: View changes
Status: Needs work » Needs review
FileSize
2015-04-15 15_39_18.png12.59 KB
2469929-18.patch1.59 KB

How about this?

Before:

After:

LewisNyman’s picture

LewisNyman
he/him
CreditAttribution: LewisNyman at Wunder commented
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

mondrake
Location 🇮🇹
CreditAttribution: mondrake commented
Related issues: +#2278715: Introduce a primary-danger link/button class
FileSize
2015-04-15 16_02_28.png13 KB
interdiff_18-20.txt1.09 KB
2469929-20.patch516 bytes

@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

mondrake
Location 🇮🇹
CreditAttribution: mondrake commented
Status: Needs work » Needs review
LewisNyman’s picture

LewisNyman
he/him
CreditAttribution: LewisNyman at Wunder commented
Status: Needs review » Reviewed & tested by the community

Nice, thanks :)

mondrake’s picture

mondrake
Location 🇮🇹
CreditAttribution: mondrake commented
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

roderik
Primary language Dutch
Location Amsterdam,NL / Budapest,HU
CreditAttribution: roderik commented
Issue summary: View changes

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

YesCT’s picture

YesCT CreditAttribution: YesCT commented
Issue summary: View changes
YesCT’s picture

YesCT CreditAttribution: YesCT commented
Issue summary: View changes
xjm’s picture

xjm
she/her
Primary language English
CreditAttribution: xjm at Acquia commented
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

tim.plunkett
he/him
Primary language English
Location Philadelphia
CreditAttribution: tim.plunkett at Acquia commented

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

Bojhan CreditAttribution: Bojhan as a volunteer commented
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

LewisNyman
he/him
CreditAttribution: LewisNyman at Wunder commented
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

veronicanerak CreditAttribution: veronicanerak as a volunteer commented

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

xjm
she/her
Primary language English
CreditAttribution: xjm at Acquia commented
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.

Elin Yordanov’s picture

Elin Yordanov CreditAttribution: Elin Yordanov commented
Version: 8.0.x-dev » 7.x-dev
Category: Bug report » Feature request
Issue tags: +Needs backport to D7

It would be nice if we had that also on Drupal 7.x.