Add a basic keyboard shortcuts overview dialog #11717

kituuu · 2024-03-02T16:25:14Z

#11711 [REVIEWs & CHANGEs]

The basic structure for this issue is ready.

TODO (For my convenience):

Write unit tests
Get maintainers' feedback for changes or improvements.

Check this

Do you think the implementation of the design from Figma is necessary? Because right now, the default styling and existing dialogue template do not fit that design. I will have to write separate CSS and a new dialogue box (Maybe we can make this new one default if it is better visually).
[Personally I feel this dialogue box design (figma one) is a bit better in distinguishing different sections : D ]

squash-labs · 2024-03-02T16:25:21Z

Manage this branch in Squash

Test this branch here: https://kituuukitukeyboard-shortcuts-fkfnh.squash.io

wagtail/admin/templates/wagtailadmin/shared/keyboard_shortcuts_dialog.html

wagtail/admin/templatetags/wagtailadmin_tags.py

kituuu · 2024-03-04T20:56:19Z

Hey @lb- , I have tried my best with the design. I used a custom scss file for styling. And also, instead of hard coding everything, I just made a dictionary of all the shortcuts so that in the future, addition and removal of shortcuts could be more accessible. I am not good in naming CSS classes LoL : D

Also, the figma design has the tables divided into 2 columns but as we have less width available in our current dialog, ig it's better to keep single column.

If everything is fine, please tell me more about writing unit tests for this PR.

lb- · 2024-03-04T21:03:36Z

Awesome I will take a look.

Just a reminder to review the failing CI items when you get the chance, some may be just formatting (see formatting guidelines https://docs.wagtail.org/en/stable/contributing/python_guidelines.html#python-coding-guidelines - make format should be all that's needed though).

As for unit tests, it looks like we have lots of tests that are globally checking for td/tr in responses, and the addition of this table is breaking those tests. I might have a look at these and see if there is a simple solution to suggest.


======================================================================
FAIL: test_simple (wagtail.admin.tests.viewsets.test_model_viewset.TestUsageView)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/runner/work/wagtail/wagtail/wagtail/admin/tests/viewsets/test_model_viewset.py", line 1088, in test_simple
    self.assertEqual(len(tds), 3)
AssertionError: 19 != 3

======================================================================
FAIL: test_usage_with_describe_on_delete (wagtail.admin.tests.viewsets.test_model_viewset.TestUsageView)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/runner/work/wagtail/wagtail/wagtail/admin/tests/viewsets/test_model_viewset.py", line 1158, in test_usage_with_describe_on_delete
    self.assertEqual(len(tds), 3)
AssertionError: 19 != 3

======================================================================
FAIL: test_usage_without_permission_on_referrer (wagtail.admin.tests.viewsets.test_model_viewset.TestUsageView)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/runner/work/wagtail/wagtail/wagtail/admin/tests/viewsets/test_model_viewset.py", line 1136, in test_usage_without_permission_on_referrer
    self.assertEqual(len(tds), 3)
AssertionError: 19 != 3

======================================================================
FAIL: test_disable_preview_on_revisions_list (wagtail.snippets.tests.test_preview.TestDisablePreviewWithEmptyModes)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/runner/work/wagtail/wagtail/wagtail/snippets/tests/test_preview.py", line 474, in test_disable_preview_on_revisions_list
    self.assertNotContains(response, "Preview")
  File "/opt/hostedtoolcache/Python/3.10.13/x64/lib/python3.10/site-packages/django/test/testcases.py", line 676, in assertNotContains
    self.assertEqual(
AssertionError: 1 != 0 : Response should not contain 'Preview'

======================================================================
FAIL: test_disable_preview_on_revisions_list (wagtail.snippets.tests.test_preview.TestDisablePreviewWithoutMixin)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/runner/work/wagtail/wagtail/wagtail/snippets/tests/test_preview.py", line 474, in test_disable_preview_on_revisions_list
    self.assertNotContains(response, "Preview")
  File "/opt/hostedtoolcache/Python/3.10.13/x64/lib/python3.10/site-packages/django/test/testcases.py", line 676, in assertNotContains
    self.assertEqual(
AssertionError: 1 != 0 : Response should not contain 'Preview'

======================================================================
FAIL: test_custom_columns (wagtail.snippets.tests.test_viewset.TestListViewWithCustomColumns)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/home/runner/work/wagtail/wagtail/wagtail/snippets/tests/test_viewset.py", line 721, in test_custom_columns
    self.assertTagInHTML("<th>", html, count=7, allow_extra_attrs=True)
  File "/home/runner/work/wagtail/wagtail/wagtail/test/utils/wagtail_tests.py", line 208, in assertTagInHTML
    self.assertEqual(
AssertionError: 30 != 7 : Found 30 instances of '<th>' in response (expected 7)

----------------------------------------------------------------------

lb- · 2024-03-04T21:31:57Z

@kituuu for the tests that have the missing counts, you can rebase / merge in the changes from this PR to your PR #11724

For the preview link tests, here's a PR that will hopefully make those tests not fail with your PR including the word 'preview' in the dialog #11725

kituuu · 2024-03-05T15:29:06Z

I guess some test fails are still there even after running make format and merging those 2 PRs.

wagtail/admin/templatetags/wagtailadmin_tags.py

kituuu · 2024-03-06T21:57:46Z

@lb- Can you suggest something for this PR too? So that I can modify some tests to support this PR. : D
I tried some things locally, but mostly. Fixing one thing breaks something else.

lb-

@kituuu this is awesome progress so far, thank you!

I have added a bunch of comments inline, there will probably be a bit to do after that (we may want to tweak styles, review tests to add) but overall this is on the right track.

Items to action from this feedback;

Update classes to use the w-prefixed-kebab-convention and avoid using &___some-selector and instead re-write the full name in each block.
Use theme variables for any sizing, font weight, widths/padding/margins etc, if a variable does not exist, use the closest to your design goal. This ensures consistency through the UI and that we are using dark/light theme variations correctly + rems consistently.
Translations; every single string needs to be translated, except the actual shortcuts, this is in the template HTML and the template tags.
Remove the duplicate column header for the table
For the template tag, use a tuple as a key with an identifier (simple kebab-string) and the translated label.
Use the same variable name as the value passed in the context from the template tag for code readability
Move the 'common actions' above 'actions' & rename 'actions' to 'Page editing actions'
Move the position, in the code, of the new register_keyboard_shortcuts_menu_item to a more intuitive position in the code

One column for this layout is fine I think we do not want to have to redesign the whole dialogs as part of this initial work.

I have not looked at the tests sorry but encourage you to have a full read of the CI output to try to dig into what the cause could be.

If you can, I would suggest a rebase/squash on main after these updates are done, it will help the next round of reviews.

Thank you again! this is great to see.

@thibaudcolas - please take a look if you get the chance also. Thanks @laymonage for your input.

client/scss/components/_keyboard-shortcuts.scss

wagtail/admin/templates/wagtailadmin/shared/keyboard_shortcuts_dialog.html

wagtail/admin/templatetags/wagtailadmin_tags.py

wagtail/admin/wagtail_hooks.py

kituuu · 2024-03-06T22:16:40Z

@kituuu this is awesome progress so far, thank you!

I have added a bunch of comments inline, there will probably be a bit to do after that (we may want to tweak styles, review tests to add) but overall this is on the right track.

Items to action from this feedback;
1. [ ]  Update classes to use the `w-prefixed-kebab-convention` and avoid using `&___some-selector` and instead re-write the full name in each block.

2. [ ]  Use `theme` variables for any sizing, font weight, widths/padding/margins etc, if a variable does not exist, use the closest to your design goal. This ensures consistency through the UI and that we are using dark/light theme variations correctly + rems consistently.

3. [ ]  Translations; every single string needs to be translated, except the actual shortcuts, this is in the template HTML and the template tags.

4. [ ]  Remove the duplicate column header for the table

5. [ ]  For the template tag, use a tuple as a key with an identifier (simple kebab-string) and the translated label.

6. [ ]  Use the same variable name as the value passed in the context from the template tag for code readability

7. [ ]  Move the 'common actions' above 'actions' & rename 'actions' to 'Page editing actions'

8. [ ]  Move the position, in the code, of the new `register_keyboard_shortcuts_menu_item` to a more intuitive position in the code
One column for this layout is fine I think we do not want to have to redesign the whole dialogs as part of this initial work.

I have not looked at the tests sorry but encourage you to have a full read of the CI output to try to dig into what the cause could be.

If you can, I would suggest a rebase/squash on main after these updates are done, it will help the next round of reviews.

Thank you again! this is great to see.

@thibaudcolas - please take a look if you get the chance also. Thanks @laymonage for your input.

@lb- It's my pleasure to work with you guys. I will make all the changes as soon as possible. 🙃

kituuu · 2024-03-08T17:50:46Z

Hey, @lb-, I have made all the changes you asked for. I will now try to figure out the failed test cases.

lb- · 2024-03-12T10:23:27Z

@kituuu I know that debugging unit tests can be a bit tricky but I will walk you through one.

* File "/home/runner/work/wagtail/wagtail/wagtail/admin/tests/viewsets/test_model_viewset.py", line 1052, in test_empty self.assertIsNone(table)

The error is saying 'table....' is not none.

The unit test is in the file test_model_viewset.py on line 1052

wagtail/wagtail/admin/tests/viewsets/test_model_viewset.py

Lines 1043 to 1052 in 75af2f2

    
           def test_empty(self): 
        
               ModelLogEntry.objects.all().delete() 
        
               response = self.client.get(self.url) 
        
               soup = self.get_soup(response.content) 
        
               results = soup.select_one("#listing-results") 
        
               table = soup.select_one("table") 
        
               self.assertEqual(response.status_code, 200) 
        
               self.assertIsNotNone(results) 
        
               self.assertEqual(results.text.strip(), "There are no log entries to display.") 
        
               self.assertIsNone(table)

In that test we can see table = soup.select_one("table") is trying to find any element with type table, however, due to your change there will now be a new table, the one for the keyboard shortcuts.
This soup.select_one is the Soup library https://beautiful-soup-4.readthedocs.io/en/latest/ and select_one accepts a query selector.
So what we need to do is ensure this test only looks for the correct table and finds that it does not exist, in our case we could update this line to table = soup.select_one("#listing-results table").
This means it will look through the HTML and only find the table within the element with ID `listing-results.
Once you get this test passing, you will find that wagtail.admin.tests.viewsets.test_model_viewset.TestHistoryView.test_filtered_no_results, wagtail.admin.tests.viewsets.test_model_viewset.TestUsageView.test_empty will be very similar.

    def test_empty(self):
        ModelLogEntry.objects.all().delete()
        response = self.client.get(self.url)
        soup = self.get_soup(response.content)
        results = soup.select_one("#listing-results")
        table = soup.select_one("#listing-results table")
        self.assertEqual(response.status_code, 200)
        self.assertIsNotNone(results)
        self.assertEqual(results.text.strip(), "There are no log entries to display.")
        self.assertIsNone(table)

Further tests

For wagtail.admin.tests.viewsets.test_model_viewset.TestHistoryView.test_filters -> this is very similar, notice there is a count of rows using a selector for ALL rows in any table. rows = soup.select("tbody tr").
wagtail.snippets.tests.test_preview.TestDisablePreviewWithEmptyModes.test_disable_preview_on_revisions_list & wagtail.snippets.tests.test_preview.TestDisablePreviewWithoutMixin.test_disable_preview_on_revisions_list -> These look like the same problem where the word Preview is being searched for in the HTML response content but the new keyboard shortcut content is returning the word Preview. Maybe remove the check self.assertNotContains(response, "Preview") or refine that text label to be more specific.

kituuu · 2024-03-12T19:27:08Z

Hey @lb-, only one following test is left. Can you say something regarding this.

lb- · 2024-03-12T21:13:24Z

@kituuu you can ignore that current single failing test for now. This is our test against Django's main branch to flag anything that could go wrong in a future Django release. It's not related to your changes so it's ok to ignore.

wagtail/admin/templates/wagtailadmin/shared/keyboard_shortcuts_dialog.html

lb- · 2024-03-12T21:59:38Z

Here's a starting point for Python unit tests. @kituuu

File: wagtail/admin/tests/tests.py - add the new test block at the end.

class TestKeyboardShortcutsDialog(WagtailTestUtils, TestCase):
    def setUp(self):
        self.login()

    def test_keyboard_shortcuts_trigger_in_sidebar(self):
        response = self.client.get(reverse("wagtailadmin_home"))
        self.assertEqual(response.status_code, 200)

        sidebar_data = (
            self.get_soup(response.content)
            .select_one("#wagtail-sidebar-props")
            .contents[0]
        )

        self.assertIn(
            json.dumps(
                {
                    "role": "button",
                    "data-a11y-dialog-show": "keyboard-shortcuts-dialog",
                    "data-action": "w-action#noop:prevent:stop",
                    "data-controller": "w-action",
                }
            ),
            sidebar_data,
        )

    def test_keyboard_shortcuts_dialog(self):
        response = self.client.get(reverse("wagtailadmin_home"))

        self.assertEqual(response.status_code, 200)
        self.assertTemplateUsed(
            response, "wagtailadmin/shared/keyboard_shortcuts_dialog.html"
        )

        soup = self.get_soup(response.content)

        # Check that the keyboard shortcuts dialog is present
        shortcuts_dialog = soup.select_one("#keyboard-shortcuts-dialog")
        self.assertIsNotNone(shortcuts_dialog)

        # Check that the keyboard shortcuts dialog has basic accessible content
        self.assertIn(
            "All keyboard shortcuts", shortcuts_dialog.find("caption").prettify()
        )
        self.assertIn("Keyboard shortcut", shortcuts_dialog.find("thead").prettify())

Note that we have to use prettify to get the text content, this is due to the text being within a template element which BeautifulSoup will correctly ignore.

This unit test set should be updated to also check a few other parts of the shortcuts, and you will also need to test that the cmd / control key is correctly identified based on user agent. The default test will use Ctrl, so you will need to add a separate test that emulates MacOS for the different keyboard cmd symbol.

See https://docs.djangoproject.com/en/5.0/topics/testing/tools/#making-requests for how to go about this. Something like...

# In the test where we want to test the non-default behaviour we will not use `self.client` instead create a new client that has a different user agent.
client = Client(headers={"user-agent": "something-that-will-be-like-macos"})
client.get(reverse("wagtailadmin_home")

Good luck, hopefully that helps you with the next step.

kituuu · 2024-03-13T20:07:11Z

Hey, @lb- So, firstly, I added the two tests you gave me. Apart from this, I am writing some tests to simulate the behaviour of Mac & windows machines to test the shortcuts.

My initial plan was to create a new Client object with some header content resembling the Marcos
client = Client( headers={ "user-agent": "Mozilla/5.0 (Macintosh; Intel Mac OS X 10_15_7) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/107.0.0.0 Safari/537.36" } )
Then, get the table and kbd and check whether the correct keys are present in kbds.
But with this, I faced one issue.
When I ran response = client, get (reverse("wagtailadmin_home")), I did not get the desired result (which we got in the above two tests). Instead, I got some response with "URL":/admin/login/?next=/admin/ through which I understand that in our class in setup, we ran self.login() I guess which login the test user from WagtailTestUtils but unfortunately our new Client is out of the scope of WagtailTestUtils, so every time I use the client. Get, I got a 302 status code.

Things I tried to fix this:

I used the client. Login and client.forced_login methods (I thought it would do something, but it doesn't).
I tried assigning

self.client =  Client(
            headers={
                "user-agent": "Mozilla/5.0 (Macintosh; Intel Mac OS X 10_15_7) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/107.0.0.0 Safari/537.36"
            }
        )
Self. login

But something else is needed.
3. Then I changed my plan. I thought of checking directly the user_agent and is_mac = re.search(r"Mac|iPod|iPhone|iPad", user_agent) because ultimately our template is decided based on user_agent & is_mac variable... I tried extracting the user_agent from the request of response = client, get (reverse("wagtailadmin_home")), but it doesn't have the user-agent header... I tried logging in using a post request once, and the request post request had this user-agent header. So, I used that in my unit tests.

If you have a better way of doing this, could you tell me? If I have to write more unit tests, tell me.

EDIT1: Ignore my VScode theme pls

lb- · 2024-03-13T20:35:11Z

Ahh I see, yeah, I did not thank of that, great digging. @kituuu

One option would be to just change the self.client to your client within the test and login.

    def test_mac_behavior(self):
        user = self.create_user()
        self.client = Client(...)
        self.login()

        response = #...

As an aside, it's much easier to work with pasted text from your code in comments and not a screenshot, saves me having to re-write what you have done so far.

kituuu · 2024-03-13T21:26:17Z

@lb- So I tried the way you suggested.

    def test_mac_behavior(self):
        macuser = self.create_user(username='macuser',email="macuser@test.com", password='macuser')
        self.client = Client(
            headers={
                "user-agent": "Mozilla/5.0 (Macintosh; Intel Mac OS X 10_15_7) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/107.0.0.0 Safari/537.36"
            }
        )
        self.login(user=macuser, username='macuser', password='macuser')
        response = self.client.get(reverse("wagtailadmin_home"))
        print(response)

On running this, I got
<HttpResponseRedirect status_code=302, "text/html; charset=utf-8", url="/admin/login/?next=/admin/">
(i.e. the response, which means this is also not working)...

But with this, I just got the idea of trying it in the setUp function itself. So I created a new class.

class TestMacKeyboardShortcutsDialog(WagtailTestUtils, TestCase):
    def setUp(self):
        # Creates a client with a Mac user agent
        self.client = Client(
            headers={
                "user-agent": "Mozilla/5.0 (Macintosh; Intel Mac OS X 10_15_7) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/107.0.0.0 Safari/537.36"
            }
        )
        self.login()

    def test_mac_useragent_and_behavior(self):
        response = self.client.get(reverse("wagtailadmin_home"))

        # Check that the user agent is a Mac
        user_agent = response.context["request"].headers.get("User-Agent", "")
        is_mac = re.search(r"Mac|iPod|iPhone|iPad", user_agent)

        # Add assertions based on expected Mac behavior
        self.assertTrue(is_mac)

        # Check that the keyboard shortcuts dialog has Mac-specific content
        soup = self.get_soup(response.content)
        shortcuts_dialog = soup.select_one("#keyboard-shortcuts-dialog")
        all_shortcuts = shortcuts_dialog.select("kbd")
        for shortcut in all_shortcuts:
            # All shortcuts should have the ⌘ symbol
            self.assertIn("⌘", shortcut.prettify())

And this second one is working as expected.
I don't know why it isn't working in the first case.
I have pushed the code.

EDIT1: One won't send any screenshots from now on 🙃.

lb- · 2024-03-13T22:01:14Z

@kituuu great, that's fine to do the second class approach.

However, may I suggest you move BOTH test classes to their own file instead.

So instead of putting the tests in wagtail/admin/tests/tests.py, please create a new file wagtail/admin/tests/test_keyboard_shortcuts.py and put both there.

It's likely we will add to this keyboard shortcuts system over time, including the ability to support a hooks or some other mechanism for registering shortcuts so having its own file makes sense. This feature will be a similar scale to dismissables which also has its own test file wagtail/admin/tests/test_dismissibles.py

Great investigating here, I have learned a lot from you on this also.

kituuu · 2024-03-13T23:05:37Z

Done.
It sure took a longer time than expected. I thought of working on more issues these days to increase my contributions. But ig it's fine. I really enjoyed this one. I learned a lot of things and unit tests also.

lb- · 2024-03-14T07:43:49Z

Thanks. I'll do a full review and see if there is anything missing.

Don't worry about quantity of contributions, this is awesome work and will be a great improvement for all Wagtail users.

allcaps · 2024-03-14T09:39:14Z

wagtail/admin/templatetags/wagtailadmin_tags.py

+                "shortcut": f"{modifier} + shift + v",
+            },
+            {"label": _("Undo"), "shortcut": f"{modifier} + z"},
+            {"label": _("Redo"), "shortcut": f"{modifier} + y"},


The default Redo on Mac is SHIFT + ⌘ + Z, this one opens up history in Chrome. ;)
Mac also does't spell out shift but uses a arrow up icon.

Not sure how close we want to stay to OS style icons/wording.

If we use words for modifier keys like SHIFT, we need to check if these words need translation.
So maybe it is better to stick with icons?

EN: Press the shift key.
FR: Appuyez sur la touche Maj.

I already checked that with the translations channel, they advised against this.

allcaps · 2024-03-14T09:45:00Z

wagtail/admin/templatetags/wagtailadmin_tags.py

+            {"label": _("Cut"), "shortcut": f"{modifier} + x"},
+            {"label": _("Paste"), "shortcut": f"{modifier} + v"},
+            {
+                "label": _("Paste without formatting"),


The mac wording for this label is "Paste and Match Style".

Not sure how close we want to stay to OS style icons/wording.

How about Paste text only?

I think Windows-speak is "Paste without formatting" and on Mac it is "Paste and Match Style".
The feature seems the same: Whatever snippet is in the clipboard, drop the styling, but match the current style in the text editor.

I assume (don't have Windows here), that if you use "Paste without formatting" into a heading element, the text still gets the heading elements style applied???

My suggestion is, to align shortcuts and labels (wording) with the OS.

Something like:

mac = "mac" win = "windows" os = mac if re.search(r"Mac|iPod|iPhone|iPad", user_agent) else win shortcuts = { ("common-actions", _("Common Actions")): [ ..., { "label": _("Paste and Match Style") if os == mac else _("Paste without formatting"), "shortcut": f"{modifier} + shift + v", }, # You'd need something similar for the other shortcut. { "label": _("Redo"), "shortcut": f"Shift + {modifier} + z" if os == mac else f"{modifier} + y", }, ..., ] }

yeah, that's fair.

@kituuu are you up for this kind of change, happy if we still keep the is_mac variable but the approach suggested above is nice.

main thing is to be able to support the adjusted wording for the paste without formatting, paste and match style.

This ^ can become quite ugly code. So we can also do something like this:

# Windows is the default. shortcuts = [ # Win/Mac are the same. { "label": "Some label", "shortcut": "Some shortcut", }, # Mac has other label { "label": "Some label", "label_mac": "Some Mac label", "shortcut": "Some shortcut", }, # Mac has other shortcut { "label": "Some label", "shortcut": "Some shortcut", "shortcut_mac": "Some Mac shortcut", }, # Mac has other label and shortcut { "label": "Some label", "label_mac": "Some Mac label", "shortcut": "Some shortcut", "shortcut_mac": "Some Mac shortcut", }, ] # Update the labels and shortcuts with the Mac variant if we are on OSX or iOS. if re.search(r"Mac|iPod|iPhone|iPad", user_agent): for item in shortcuts: item["label"] = item.pop("label_mac", item["label"]) item["shortcut"] = item.pop("shortcut_mac", item["shortcut"]) print(shortcuts)

@lb- @allcaps I understand the required changes, but do we have any list or thing for these changes? For example, what should the label name be on Mac and Windows? Right now, I only know that "Paste and Match Style" and "Paste without formatting" need to be changed, but I don't know about other shortcuts.

I have made changes to this specific "Paste and Match Style". Are there any more labels that need to be changed based on OS?

Also, there should be some changes in unit tests to check this change. Should I update the unit tests in this PR itself? Or can we ignore it for now?

allcaps · 2024-03-14T09:57:12Z

wagtail/admin/templatetags/wagtailadmin_tags.py

+            {"label": _("Redo"), "shortcut": f"{modifier} + y"},
+        ],
+        ("page-editing-actions", _("Page editing actions")): [
+            {"label": _("Save"), "shortcut": f"{modifier} + s"},


I didn't know about ⌘ + s 👏

Models (snippets) are saved, but pages Save draft. Maybe the label should be Save (draft)?

This shortcut overrides the default browser behaviour. And in this specific case, that seems okay. Saving HTML is not a common thing to do. Overriding OS/browser shortcuts is something we should consider when adding more shortcuts.

This shortcut doesn't work when the cursor is inside a text-area. It will use the default, trigger the browser to save the HTML. -- Fixing this might be outside the scope of this PR.

Great feedback - thanks!

The behaviour for keyboard shortcuts to not listen inside input fields is intentional. However, I see how this could be confusing for the CMD + S scenario.

I'll look at that for a different PR.

Generally I agree that we should not override keyboard shortcuts in the browser but this has been in the code for a long time. Be sure to have a read of #3949 for the big picture plan for keyboard shortcuts, I've already raised the same concerns there.

wagtail/admin/wagtail_hooks.py

lb- · 2024-03-14T10:06:07Z

wagtail/admin/templatetags/wagtailadmin_tags.py

+            {"label": _("Redo"), "shortcut": f"{modifier} + y"},
+        ],
+        ("page-editing-actions", _("Page editing actions")): [
+            {"label": _("Save"), "shortcut": f"{modifier} + s"},


Suggested change

{"label": _("Save"), "shortcut": f"{modifier} + s"},

{"label": _("Save changes"), "shortcut": f"{modifier} + s"},

How about this @allcaps - keeps it generic across usage.

This is really a nitpick. For the first iteration we can go with "Save".

Just to explain myself: "Save changes" or "Save" do not clarify that pages don't have a "Save" option. Pages have a workflow with "Save draft" and "Publish". Therefore, "Save" on a page will always be result in a "Save draft". I think some users expect saving a already publish page will publish their changes. But that won't happen. They just get a new draft.

The modal with shortcuts is like a cheatsheet. And it is common for cheatsheets to have additional explanation. Maybe help texts? "A page will be saved as draft."

Nice to have. Not for now.

Yep. Let's keep this simple for now, I anticipate we would have a way for the label/shortcut to be registered somehow at a View or Form level.

Or we would drive this from the JS side and pull out the label from the element itself.

As this is static for this first version I think we'll have to compromise with something generic.

But if you have ideas on how we could provide a way for this to be registered - I'm super keen to hear them. Add a comment to #3949

wagtail/admin/templatetags/wagtailadmin_tags.py

Instead of checking for the word Preview NOT showing, only check for the exact URL. Instead of checking for all `tr` in the templates, only check for those within the correct scope.

lb-

Thank you @kituuu and also thanks for the extra cross-check @allcaps .

I have reviewed the shortcuts and designs and will make some changes when merging to get this finalised.

For the styling, we just needed some extra refinement to get this closer to the design. Font sizes/weights, colours of headings, background on the kbd. I have also simplified the custom classes accordingly. Your work here is great but just wanted to get that last bit of polish in.

I have a few other recommendations but these are quite small and nitpicky so I will just apply these when merging. Tuples vs dicts, adding Docstrings and comments. I have renamed 'page editing actions' to just 'Actions' again, as this can be used for Snippets and potentially other places also.

Finally, I have opted to include a {% block shortcuts %} within the template. We do not want to officially document this but at least there is an indirect way to customise these shortcuts (simply) when this goes live.

Here's the final state (comparing to the designs).

It's not exactly what you envisioned @benenright but as close as we can get without redesigning parts of the dialog structure and without introducing more complex HTML for the keyboard values (e.g. you have bold for the letters in the shortcuts).

I have tested in Chrome 124, Safari 17.1 and Firefox 123 - looking good in all.

I will make the changes discussed, force push with a changelog and look to merge this in.

lb- · 2024-03-16T06:22:35Z

wagtail/admin/templates/wagtailadmin/shared/keyboard_shortcuts_dialog.html

@@ -0,0 +1,28 @@
+{% load wagtailadmin_tags i18n %}
+{% dialog icon_name='regex' id='keyboard-shortcuts-dialog' title=_("Keyboard shortcuts") %}


I suggest we put the root classname="w-keyboard-shortcuts" on the dialog here, instead of on the table. Will make it easier to style the content as a whole.

lb- · 2024-03-16T06:23:03Z

wagtail/admin/templates/wagtailadmin/shared/keyboard_shortcuts_dialog.html

@@ -0,0 +1,28 @@
+{% load wagtailadmin_tags i18n %}
+{% dialog icon_name='regex' id='keyboard-shortcuts-dialog' title=_("Keyboard shortcuts") %}
+    <table class="w-keyboard-shortcuts">


Suggested change

<table class="w-keyboard-shortcuts">

<table class="w-w-full">

Probably simpler to have the full width as a tailwind class, with the root class moved to the dialog.

lb- · 2024-03-16T06:25:24Z

wagtail/admin/templatetags/wagtailadmin_tags.py

+    user_agent = context["request"].headers.get("User-Agent", "")
+    is_mac = re.search(r"Mac|iPod|iPhone|iPad", user_agent)
+    modifier = "⌘" if is_mac else "Ctrl"
+    shortcuts = {


Sorry for the back and forth on structure here but a few changes I will do when merging but wanted to explain.

Let's use tuples for the shortcuts -> {"label": _("Copy"), "shortcut": f"{modifier} + c"} would be (_("Copy"), f"{modifier} + c"). This is a bit easier to read when looking at a lot of these.

Let's stick to the same casing for Shift not a mix of Shift and shift.

Let's stick to the same casing for titles and labels, e.g. Common actions not Common Actions and Paste and match style not Paste and Match Style. Leaning towards 'Sentence case' not 'Title Case'.

Might be nice to add a simple docstring on this class for explanation.

""" Renders the keyboard shortcuts dialog content with the appropriate shortcuts for the user's platform. Note: Shortcut keys are intentionally not translated. """

lb- · 2024-03-16T06:25:59Z

wagtail/admin/wagtail_hooks.py

@@ -998,6 +998,23 @@ def register_editors_guide_menu_item():
    )


+@hooks.register("register_help_menu_item")
+def register_keyboard_shortcuts_menu_item():


Let's add a simple docstring here, helps for future context. I can add this when merging.

Suggested change

def register_keyboard_shortcuts_menu_item():

def register_keyboard_shortcuts_menu_item():

"""

Triggers the keyboard shortcuts dialog to open when clicked

while preventing the default link click action.

"""

Include the ability to trigger the dialog from the sidebar Include base styling and unit tests Fixes #wagtail#11711 Relates to larger work for keyboard shortcuts in wagtail#3949

lb- · 2024-03-16T06:59:14Z

#11767 created for the next bit to do (add an icon), for anyone who is keen.

lb- self-requested a review March 2, 2024 21:24

lb- reviewed Mar 2, 2024

View reviewed changes

wagtail/admin/templates/wagtailadmin/shared/keyboard_shortcuts_dialog.html Show resolved Hide resolved

lb- reviewed Mar 3, 2024

View reviewed changes

wagtail/admin/templatetags/wagtailadmin_tags.py Outdated Show resolved Hide resolved

lb- self-requested a review March 4, 2024 21:00

This was referenced Mar 4, 2024

Update universal listings & UI table unit tests to be more robust #11724

Merged

Ensure unit tests for preview links are more robust #11725

Merged

kituuu force-pushed the kitu/keyboard-shortcuts branch from 5133c6b to 18b3b4e Compare March 5, 2024 05:17

laymonage reviewed Mar 6, 2024

View reviewed changes

wagtail/admin/templatetags/wagtailadmin_tags.py Outdated Show resolved Hide resolved

lb- requested changes Mar 6, 2024

View reviewed changes

kituuu force-pushed the kitu/keyboard-shortcuts branch 3 times, most recently from fc59b7c to becc9dd Compare March 8, 2024 17:42

lb- added the Accessibility label Mar 12, 2024

kituuu force-pushed the kitu/keyboard-shortcuts branch 2 times, most recently from 93bcc09 to 5626d4a Compare March 12, 2024 19:06

lb- reviewed Mar 12, 2024

View reviewed changes

wagtail/admin/templates/wagtailadmin/shared/keyboard_shortcuts_dialog.html Outdated Show resolved Hide resolved

lb- reviewed Mar 12, 2024

View reviewed changes

wagtail/admin/templates/wagtailadmin/shared/keyboard_shortcuts_dialog.html Outdated Show resolved Hide resolved

kituuu force-pushed the kitu/keyboard-shortcuts branch from 13772a7 to 56e5d1b Compare March 13, 2024 21:21

kituuu force-pushed the kitu/keyboard-shortcuts branch 2 times, most recently from fbe1bb2 to 4e11ac4 Compare March 13, 2024 23:04

kituuu marked this pull request as ready for review March 13, 2024 23:29

lb- self-requested a review March 14, 2024 07:43

allcaps reviewed Mar 14, 2024

View reviewed changes

wagtail/admin/wagtail_hooks.py Show resolved Hide resolved

lb- reviewed Mar 14, 2024

View reviewed changes

wagtail/admin/templatetags/wagtailadmin_tags.py Outdated Show resolved Hide resolved

kituuu force-pushed the kitu/keyboard-shortcuts branch from efedef4 to bd80e7e Compare March 14, 2024 20:54

Refine unit tests to correctly scope checks for 'Preview' & table

d4844d5

Instead of checking for the word Preview NOT showing, only check for the exact URL. Instead of checking for all `tr` in the templates, only check for those within the correct scope.

lb- approved these changes Mar 16, 2024

View reviewed changes

lb- added a commit to kituuu/wagtail that referenced this pull request Mar 16, 2024

Add changelog for wagtail#11717

11c79df

lb- force-pushed the kitu/keyboard-shortcuts branch from bd80e7e to 11c79df Compare March 16, 2024 06:42

kituuu and others added 2 commits March 16, 2024 16:45

Add basic keyboard shortcuts dialog

9e5847b

Include the ability to trigger the dialog from the sidebar Include base styling and unit tests Fixes #wagtail#11711 Relates to larger work for keyboard shortcuts in wagtail#3949

Add changelog for wagtail#11717

48f5a64

lb- force-pushed the kitu/keyboard-shortcuts branch from 11c79df to 48f5a64 Compare March 16, 2024 06:45

lb- mentioned this pull request Mar 16, 2024

Update keyboard shortcuts menu button & dialog icon to a new 'keyboard' icon #11767

Closed

lb- merged commit 24f5a4a into wagtail:main Mar 16, 2024
16 of 17 checks passed

lb- linked an issue Mar 16, 2024 that may be closed by this pull request

Add a basic keyboard shortcuts overview dialog #11711

Closed

lb- mentioned this pull request Mar 16, 2024

Add a basic keyboard shortcuts overview dialog #11711

Closed

	{"label": _("Save"), "shortcut": f"{modifier} + s"},
	{"label": _("Save changes"), "shortcut": f"{modifier} + s"},

		@@ -0,0 +1,28 @@
		{% load wagtailadmin_tags i18n %}
		{% dialog icon_name='regex' id='keyboard-shortcuts-dialog' title=_("Keyboard shortcuts") %}

	<table class="w-keyboard-shortcuts">
	<table class="w-w-full">

Add a basic keyboard shortcuts overview dialog #11717

Add a basic keyboard shortcuts overview dialog #11717

Conversation

kituuu commented Mar 2, 2024 • edited

squash-labs bot commented Mar 2, 2024

Manage this branch in Squash

kituuu commented Mar 4, 2024

lb- commented Mar 4, 2024

lb- commented Mar 4, 2024

kituuu commented Mar 5, 2024

kituuu commented Mar 6, 2024 • edited

lb- left a comment • edited

Choose a reason for hiding this comment

kituuu commented Mar 6, 2024

kituuu commented Mar 8, 2024

lb- commented Mar 12, 2024

kituuu commented Mar 12, 2024 • edited

lb- commented Mar 12, 2024

lb- commented Mar 12, 2024

kituuu commented Mar 13, 2024 • edited

lb- commented Mar 13, 2024

kituuu commented Mar 13, 2024 • edited

lb- commented Mar 13, 2024

kituuu commented Mar 13, 2024 • edited

lb- commented Mar 14, 2024

allcaps Mar 14, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lb- Mar 14, 2024 • edited

Choose a reason for hiding this comment

allcaps Mar 14, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lb- left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lb- commented Mar 16, 2024

kituuu commented Mar 2, 2024 •

edited

kituuu commented Mar 6, 2024 •

edited

lb- left a comment •

edited

kituuu commented Mar 12, 2024 •

edited

kituuu commented Mar 13, 2024 •

edited

kituuu commented Mar 13, 2024 •

edited

kituuu commented Mar 13, 2024 •

edited

allcaps Mar 14, 2024 •

edited

lb- Mar 14, 2024 •

edited

allcaps Mar 14, 2024 •

edited

lb- left a comment •

edited