WordPress.org

Make WordPress Core

Opened 2 years ago

Closed 23 months ago

#34816 closed defect (bug) (maybelater)

Theme Delete Button: (Prevent this page from creating additional dialogs)

Reported by: emiluzelac Owned by:
Milestone: Priority: normal
Severity: normal Version: 4.3.1
Component: Themes Keywords: has-screenshots
Focuses: ui, accessibility, javascript Cc:

Description

Hey all,

Not sure if this is something you guys are willing to improve, but it might be worth looking at it.

So here's the story.

When you delete more than one theme in a row, some browsers will offer you this alert box, where you can check "Prevent this page from creating additional dialogs".

Once this is checked, a theme delete button no longer works.

After talking to @swissspidy we realized that this is a browser feature and from what I could gather online, it affects Chrome and Firefox as well.

Attaching screenshots.

Attachments (6)

Screen Shot on 2015-11-29 at 14-36-00.png (31.5 KB) - added by emiluzelac 2 years ago.
Prevent this page from creating additional dialogs
Screen Shot 2015-11-29 at 14.07.10.png (47.9 KB) - added by emiluzelac 2 years ago.
Delete button
34816.diff (1.5 KB) - added by vtieu 23 months ago.
Displays dialog when attempting to delete a theme.
34816.2.diff (2.4 KB) - added by swissspidy 23 months ago.
notification-dialog.png (163.5 KB) - added by swissspidy 23 months ago.
34816.3.diff (4.8 KB) - added by swissspidy 23 months ago.

Download all attachments as: .zip

Change History (20)

@emiluzelac
2 years ago

Prevent this page from creating additional dialogs

#1 @afercia
2 years ago

Testing this, after I checked "Prevent this page..." in the JS alert, the "Delete" button still worked pretty well for me the first time I clicked on it: the theme got deleted without any warning :(

After that, the "Delete" button stopped working. Though preventing JS alert is a temporary browser setting that only lasts for the current browser session (at least in Chrome, close the browser tab, reopen, go in the same page and alerts work again), the unintentional theme deletion should be definitely avoided.

#2 @Kopepasah
2 years ago

  • Summary changed from Theme Delete Button: (Prevent this page from creating additional dialogs) to Theme Delete Button: (Prevent this page from creating additional dialogs

My findings:

Safari
When clicking Delete, the alert window appears and, one the second alert window, I am able to check the "Don't show more alerts from this webpage" checkbox. Once checked, I am no longer able to Delete the same theme; however, I am still able to Delete other themes without a problem (as the alert box shows).

Chrome
Similar to above, but I am not able to delete any theme once I have checked the "Prevent this page from creating additional dialogs" checkbox.

---

My suggestion, remove the alert window and use a different method for verifying a users intentions (e.g. an overlay dialog instead of a window alert).

#3 @Kopepasah
2 years ago

  • Summary changed from Theme Delete Button: (Prevent this page from creating additional dialogs to Theme Delete Button: (Prevent this page from creating additional dialogs)

#4 @swissspidy
23 months ago

  • Component changed from General to Themes
  • Focuses ui added
  • Keywords needs-patch good-first-bug added
  • Milestone changed from Awaiting Review to Future Release

My suggestion, remove the alert window and use a different method for verifying a user's intentions (e.g. an overlay dialog instead of a window alert).

Perhaps we can leverage the code used for the post lock dialog so we can use these dialogs in more situations where needed.

@vtieu
23 months ago

Displays dialog when attempting to delete a theme.

#5 follow-up: @vtieu
23 months ago

Great idea @swissspidy

Here's my first attempt at resolving the bug using the post lock dialog as suggested. Let me know what I can improve.

@swissspidy
23 months ago

#6 in reply to: ↑ 5 @swissspidy
23 months ago

  • Keywords has-patch added; needs-patch removed
  • Milestone changed from Future Release to 4.5

Replying to vtieu:

Great idea @swissspidy

Here's my first attempt at resolving the bug using the post lock dialog as suggested. Let me know what I can improve.

Awesome! Easier than I thought :)

I slightly updated the patch to include the translated text in the PHP instead of inserting it with JavaScript. Also, I changed the HTML slightly (h3 and paragraphs).

#7 @swissspidy
23 months ago

  • Keywords has-screenshots added

#8 @swissspidy
23 months ago

@afercia Got time to look at this from an a11y perspective?

#9 follow-up: @afercia
23 months ago

  • Focuses accessibility javascript added

I'd suggest to have a look at what was done for the wp-embed-share-dialog, see the aria role "dialog", the aria-label attribute used on the modal container, focus handling and constraining tabbing.

  • when opening the modal, focus should be moved to the modal: probably making the container itself focusable since this modal doesn't contain just form elements but also text that needs to be made available to assistive technologies
  • maybe add a Close button on the top right, in this case this should be the first element to receive focus
  • when opening the modal, a CSS class modal-open should be added to the body and then removed when closing the modal
  • pressing Escape should close the modal
  • when closing the modal, focus should be moved back to the "Delete" button
  • tabbing should be constrained within the modal
  • the "Cancel" link without a href attribute is not focusable, should be a button with a type="button" attribute
  • I'd consider to place the primary button on the left
  • maybe event should be explicitly passed to deleteTheme
  • when pressing OK to confirm the deletion, focus should be moved back to the most logical place (not sure where actually, maybe making the "Theme deleted" notice focusable)
  • I'm probably missing something :)

There are several modal dialog used in the admin, each one with its own implementation, duplicated code sometimes, just a few modals have a sufficient level of accessibility. Sooner or later we should probably standardize all the modal dialogs an have a sort of mini "Modal API".

@swissspidy
23 months ago

#10 in reply to: ↑ 9 @swissspidy
23 months ago

  • Keywords needs-refresh added

Replying to afercia:

I'd suggest to have a look at what was done for the wp-embed-share-dialog, see the aria role "dialog", the aria-label attribute used on the modal container, focus handling and constraining tabbing.

  • when opening the modal, focus should be moved to the modal: probably making the container itself focusable since this modal doesn't contain just form elements but also text that needs to be made available to assistive technologies

Done in the latest patch.

  • maybe add a Close button on the top right, in this case this should be the first element to receive focus

I'd rather like do that when settling for some kind of Modal API as you suggested.

  • when opening the modal, a CSS class modal-open should be added to the body and then removed when closing the modal

This class is already present because of the theme overlay (which itself is a modal).

  • pressing Escape should close the modal

Done in the latest patch.

  • when closing the modal, focus should be moved back to the "Delete" button
  • tabbing should be constrained within the modal

Both still to be done. Anyone wanna chime in?

  • the "Cancel" link without a href attribute is not focusable, should be a button with a type="button" attribute
  • I'd consider to place the primary button on the left
  • maybe event should be explicitly passed to deleteTheme

Done in the latest patch.

  • when pressing OK to confirm the deletion, focus should be moved back to the most logical place (not sure where actually, maybe making the "Theme deleted" notice focusable)

Still to be done.

  • I'm probably missing something :)

There are several modal dialog used in the admin, each one with its own implementation, duplicated code sometimes, just a few modals have a sufficient level of accessibility. Sooner or later we should probably standardize all the modal dialogs an have a sort of mini "Modal API".

+1. If this ticket turns out to be more complicated than it seems, perhaps we should first start this standardization.

#11 @obenland
23 months ago

Themes are not the only instance where we use confirm(), it's used in ~24 other places in core.

I'm not sure the price we're paying with another API to circumvent this browser feature in all of the admin is worth the benefit. How often do users really delete more than one theme at a time, AND get annoyed enough by the AYS message, to prevent it from popping up? I'd like to think users can differentiate between a necessary AYS that is worth keeping around and a site just randomly opening dialogs.

#12 follow-up: @swissspidy
23 months ago

  • Keywords close added; good-first-bug has-patch needs-refresh removed
  • Milestone changed from 4.5 to Awaiting Review

After considering @obenland's wise words for a bit, this really sounds like a wontfix or maybelater. Having something custom for such dialogs would be nice to have, but that doesn't need to be tied to a release. It's really not worth the effort, it already took me a while to get the current patch together.

#13 in reply to: ↑ 12 @afercia
23 months ago

Replying to swissspidy:

it already took me a while to get the current patch together.

Yep, making modal dialogs accessible it's pretty time consuming :) Thinking WordPress should provide an easy way to do that, something like a simple function call to invoke a modal dialog and pass content. A standardized component. Just a thought, there's not even a plan about it, hopefully something to consider for the next future.
I'd vote for "maybelater" :)

#14 @swissspidy
23 months ago

  • Keywords close removed
  • Milestone Awaiting Review deleted
  • Resolution set to maybelater
  • Status changed from new to closed
Note: See TracTickets for help on using tickets.