Closed Bug 1675721 Opened 3 years ago Closed 3 years ago

Improve the focus ring indicator of the "View Message Security Info" button in the message header

Categories

(MailNews Core :: Security: OpenPGP, defect)

defect

Tracking

(thunderbird_esr78 wontfix)

RESOLVED FIXED
86 Branch
Tracking Status
thunderbird_esr78 --- wontfix

People

(Reporter: KaiE, Assigned: aleca)

References

Details

(Keywords: regression)

Attachments

(1 file, 9 obsolete files)

14.49 KB, patch
aleca
: review+
aleca
: ui-review+
Details | Diff | Splinter Review

In the past, when looking at a received email, it was possible to access the message's security information using a menu commend, view, message security info.

This is gone in the latest Thunderbird 78.4.0

Alessandro, was this a deliberate change? Is there an alternative accessible way to open it (which doesn't require clicking an icon).

Flags: needinfo?(alessandro)

This was a deliberate change in the bug that implemented the button to pop out the information instead of a dialog: the experience was very strange when the menu action popped out something down on the page.

Why is that strange? If the user requests information to be shown, and it shows up, that seems fine to me. Seems better to me than removing a functionality.

Menus don't normally open something at the other side of the window. But there was also the issue of what to do when there it's not a message security: there's nothing to anchor the security popup in.

No functionality is really removed, only the specific menu to access it.

Alessandro, was this a deliberate change? Is there an alternative accessible way to open it (which doesn't require clicking an icon).

Deliberately done for discoverability and usability purposes.
We want to move away from menu items as much as possible when it makes sense, in favour of in-context actions that keep the user focus in the area he's interacting in the moment.

What you're experiencing now is a weird "missing feature" due to your muscle memory of clicking a message and then going in the top menu and clicking "view > message security info".
That's fundamentally a weird interaction since you need to remove your attention from the message area and search for something that might not be active or useful, and stored inside a submenu.

With the new message security UI, the encryption technology is clearly reported in the message header, keeping the user focused in the area he's currently interacting with. And seeing the security info is only one click away in the same context.
If no encryption technology is displayed, the user immediately knows that no "message security info" popup exists, therefore there's no need to go search for that menu item and getting distracted from the context.

Also, by removing the menu item, we removed the necessity of enabling/disabling every time, and the weirdness of clicking the menu item and having a popup panel appearing at the total opposite location of the clicked interaction.

Flags: needinfo?(alessandro)

Thanks for the explanation.

I still think there should be a way to bring up the security details using a mechanism that doesn't require a mouse, for accessibility reasons. Would you agree?

Without a mouse, you cannot discover there are more details available behind the openpgp status icons, right?

The encryption button is keyboard accessible in the focus ring of the message header.
Indeed, that's not as immediate and we should consider reserving a specific shortcut for quick access.

Severity: -- → S4
Version: unspecified → 78
Attached patch 1675721-info-shortcut.diff (obsolete) — Splinter Review

This patch improves the keyboard accessibility of the message header by implementing the following changes:

  • All the buttons and selectable labels are part of the TAB focus ring.
  • All the focusable elements will show a dotted outline when receiving focus.
  • The Message Security popup panel can be triggered with the SHIFT+S shortcut.
  • All the buttons inside the security popup panel are now part of the focus ring and show the dotted outline when focused.

I'm asking a ui-review specifically for these changes:

  • I changed the crypto button to a toolbarbutton in order to gain the tooltiptext attribute, as the html:button title attribute doesn't seem to work. I'm not sure if this change created alignment issues with labels and icons in the various OSs.
  • I implemented a CSS styling for focusable buttons.
Attachment #9189669 - Flags: ui-review?(richard.marti)
Attachment #9189669 - Flags: review?(mkmelin+mozilla)
Status: NEW → ASSIGNED
Comment on attachment 9189669 [details] [diff] [review]
1675721-info-shortcut.diff

Review of attachment 9189669 [details] [diff] [review]:
-----------------------------------------------------------------

Good idea, but:
The tabbing is a bit weird: the first tab goes to the buttons and I have to tab through all buttons (six times) until I'm on the from address. Couldn't we do only one tab stop on the buttons and navigate in them with the cursor (like FX is doing and we have in composer in the chevron)? Also for what is the tab stop at the date good? I can't copy or doing something else here.

In the security pane I have always an invisible tab-stop I don't know for what it is.

Not tested Linux, as you surely did it. Windows shows the focus-ring inside the button -> good. Mac needs a `outline-offset: -2px;` on the toolbarbuttons to show it there too.

The icons in the encryptionTechBtn are correctly aligned on Mac and Windows.

And to remember, we have bug 1553231 for the main toolbar.

::: mail/themes/shared/mail/messageHeader.css
@@ +768,5 @@
> +.msgHeaderView-button:focus:not(:hover),
> +.dateLabel:focus:not(:hover),
> +.button-focusable:focus:not(:hover) {
> +  outline: 1px dashed Highlight;
> +}

Instead of using :focus you should use focus-visible. Then the focus-ring is only set when the tabbing is used and not when the button is clicked by the mouse. On Windows click the `More` button and click outside TB or click the button again: we have the focus-ring.

With this code this works better:
.msgHeaderView-button:focus:not(:hover),
.dateLabel:focus:not(:hover),
.button-focusable:focus:not(:hover) {
  outline: none;
}

.msgHeaderView-button:focus-visible:not(:hover),
.dateLabel:focus-visible:not(:hover),
.button-focusable:focus-visible:not(:hover) {
  outline: 1px dashed Highlight;
}
Attachment #9189669 - Flags: ui-review?(richard.marti) → feedback+

Thanks for the review, and very good points on the way to improve the focus on those buttons.
I think I'll open a dedicated bug for the message header to only work on that section.
I'll remove the edits done and keep this bug only about the message security button.

In the security pane I have always an invisible tab-stop I don't know for what it is.

Ah, I didn't notice that. I'll fix it.

Attached patch 1675721-info-shortcut.diff (obsolete) — Splinter Review
Attachment #9189669 - Attachment is obsolete: true
Attachment #9189669 - Flags: review?(mkmelin+mozilla)
Attachment #9189754 - Flags: ui-review?(richard.marti)
Attachment #9189754 - Flags: review?(mkmelin+mozilla)
Attached patch 1675721-info-shortcut.diff (obsolete) — Splinter Review

The focus ring issues should be fixed now.

Attachment #9189754 - Attachment is obsolete: true
Attachment #9189754 - Flags: ui-review?(richard.marti)
Attachment #9189754 - Flags: review?(mkmelin+mozilla)
Attachment #9189786 - Flags: ui-review?(richard.marti)
Attachment #9189786 - Flags: review?(mkmelin+mozilla)
Comment on attachment 9189786 [details] [diff] [review]
1675721-info-shortcut.diff

Review of attachment 9189786 [details] [diff] [review]:
-----------------------------------------------------------------

Probably can't spare a key for this. As long as one can tab there I guess that's ok.

::: mail/locales/en-US/messenger/openpgp/msgReadStatus.ftl
@@ +4,5 @@
>  
> +header-action-view-security =
> +    .key = S
> +button-show-message-security =
> +    .tooltiptext = Show Message Security (Shift+S)

We don't write out keys like this. 
I don't think shift is an appropriate acccelrator to use either. This would mean one can't e.g. filter for something with S without the security popup triggering

@@ +6,5 @@
> +    .key = S
> +button-show-message-security =
> +    .tooltiptext = Show Message Security (Shift+S)
> +button-show-message-security2 =
> +    .value = OpenPGP

Never used?  And what about S/MIME?
Attachment #9189786 - Flags: review?(mkmelin+mozilla) → review-

Comment on attachment 9189786 [details] [diff] [review]
1675721-info-shortcut.diff

The buttons inside the popup have a 2px border. This looks a bit fat together with the normal button border. I think 1px would be better here. It would also be more consistent with the normal buttons. And Mac needs the normal focus "glow" around the buttons in the popup.

Attachment #9189786 - Flags: ui-review?(richard.marti)

You could restore the old menu command, that way you don't need a shortcut, and it can be reached in a clear way.

button-show-message-security =
.tooltiptext = Show Message Security (Shift+S)

We don't write out keys like this.

How do we write it? Is there a recommended approach used to highlight a shortcut inside a tooltip?

button-show-message-security2 =
.value = OpenPGP

Never used? And what about S/MIME?

Sorry, that 's irrelevant as it's a leftover I used for a quick test.

Probably can't spare a key for this. As long as one can tab there I guess that's ok.

I think it would be nice to have a shortcut dedicated for this panel since it's so far down the focus ring it takes many tabs to reach.

I guess it would be okay to not have a shortcut for it if we drastically improve the focus ring of the message header. eg: 3 tabs to jump from addressing area, buttons area, and security area, and then using the arrow keys to navigate the various elements.

(In reply to Alessandro Castellani (:aleca) from comment #15)

How do we write it? Is there a recommended approach used to highlight a shortcut inside a tooltip?

Not that I know of. Basically in the documentation, if there isn't a menu item that self-documents it.
But thinking about it again, maybe it wouldn't be so bad...

Firefox does it this way for some buttons (eg. reload page, bookmark), that's why I did it that way.
It seems that there's a slight more dynamic approach in FF with buttons associated to specific keys:
https://searchfox.org/mozilla-central/rev/0db73daa4b03ce7513a7dd5f31109143dc3b149e/browser/locales/en-US/chrome/browser/browser.properties#362
https://searchfox.org/mozilla-central/rev/0db73daa4b03ce7513a7dd5f31109143dc3b149e/browser/base/content/browser.js#6755

I think it's a good approach we should think about adopting as it improves a lot the discoverability of those shortcuts without solely relying on the menu bar.

Attached patch 1675721-info-shortcut.diff (obsolete) — Splinter Review

Thanks to Richard for updating the macOS variation.
I assigned the Ctrl+Alt+S as shortcut as it doesn't seem to conflict with anything else.

Temporarily, I'm writing the shortcut in the tooltiptext by grabbing the key from fluent, so l10n will respect whatever key is used.

I'd like to tackle in a follow up bug the implementation of a global approach to assign shortcuts in tooltips based on the actual key and modifiers defined in the <key> tag.

If we want to consider uplifting this to 78, we can temporarily exclude the new strings as this patch improves the focus ring and keyboard accessibility of the security pane, but I think having a tooltip is quite important.

Attachment #9189786 - Attachment is obsolete: true
Attachment #9189903 - Flags: ui-review?(richard.marti)
Attachment #9189903 - Flags: review?(mkmelin+mozilla)
Attached patch 1675721-info-shortcut.diff (obsolete) — Splinter Review

Forgot a qrefresh, sorry for the noise.

Attachment #9189903 - Attachment is obsolete: true
Attachment #9189903 - Flags: ui-review?(richard.marti)
Attachment #9189903 - Flags: review?(mkmelin+mozilla)
Attachment #9189905 - Flags: ui-review?(richard.marti)
Attachment #9189905 - Flags: review?(mkmelin+mozilla)

Comment on attachment 9189905 [details] [diff] [review]
1675721-info-shortcut.diff

All looks good except on Mac we have no ALT key. On Mac it should be ⌘ ⌥ without a plus sign.

Attachment #9189905 - Flags: ui-review?(richard.marti)
Comment on attachment 9189905 [details] [diff] [review]
1675721-info-shortcut.diff

Review of attachment 9189905 [details] [diff] [review]:
-----------------------------------------------------------------

Bitrotted, so didn't try it yet.

::: mail/locales/en-US/messenger/openpgp/msgReadStatus.ftl
@@ +7,5 @@
> +header-action-view-security =
> +    .key = S
> +
> +#   $type (String) - the shortcut key defined in the header-action-view-security
> +button-show-message-security =

AIUI, seems to be preferred to put the type last, like "message-security-button"

::: mailnews/extensions/smime/msgReadSMIMEOverlay.js
@@ +62,5 @@
>  
> +/**
> + * Trigger the message security info from a keyboard shortcut.
> + */
> +function cmd_showMessageSecurityInfo() {

Let's remove the cmd_
You can just do these checks in the showMessageReadSecurityInfo() function

@@ +598,5 @@
> +
> +  // Force the focus back on the date label above the crypto button to prevent
> +  // the focus ring from being visible when interacting with the mouse.
> +  let dateLabel = document.getElementById("dateLabel");
> +  if (dateLabel) {

dateLabel is always in the dom I think. But wouldn't you rather put the focus on the security button?
Attachment #9189905 - Flags: review?(mkmelin+mozilla)
Attached patch 1675721-info-shortcut.diff (obsolete) — Splinter Review

Thank you both for the review.
I updated the patch to respect the natural focus restoration of the panel.
That's very important to maintain as users might open the security popup with the shortcut when focused on another element, and we need to restore the original focus location to prevent user error.

I reduced the focused outline to 1px width to be more inline with the other elements.
The fact that the focus ring is visible after the panel is closed if the user clicked on a non focusable element (eg. the empty header area) is a minor UI annoyance we can ignore.
It's more important to maintain the natural focus accessibility and not hack it.

Attachment #9189905 - Attachment is obsolete: true
Attachment #9190066 - Flags: ui-review?(richard.marti)
Attachment #9190066 - Flags: review?(mkmelin+mozilla)

Comment on attachment 9190066 [details] [diff] [review]
1675721-info-shortcut.diff

Yeah, 1px fits better with the other focus rings in the header pane.

Attachment #9190066 - Flags: ui-review?(richard.marti) → ui-review+
Comment on attachment 9190066 [details] [diff] [review]
1675721-info-shortcut.diff

Review of attachment 9190066 [details] [diff] [review]:
-----------------------------------------------------------------

::: mailnews/extensions/smime/msgReadSMIMEOverlay.js
@@ +60,5 @@
>    }
>  }
>  
> +/**
> + * Reveal message security popup panel with udpated OpenPGP or S/MIME info.

typo: updated
Attachment #9190066 - Flags: review?(mkmelin+mozilla) → review-
Comment on attachment 9190066 [details] [diff] [review]
1675721-info-shortcut.diff

Review of attachment 9190066 [details] [diff] [review]:
-----------------------------------------------------------------

(repeating since bugzilla ate the main comment)

I don't like how this is adding more XULisms. Instead of adding a <key>, please just do it the webby way and hook up relevant eventlisteners for the key combinations you want, which also means you can get rid of inline event handler(s).

I don't like how this is adding more XULisms. Instead of adding a <key>, please just do it the webby way and hook up relevant eventlisteners for the key combinations you want, which also means you can get rid of inline event handler(s).

Good call, thanks.

Do you know if we have an html:button equivalent for the tooltiptext? I wish I could avoid using the toolbarbutton.

That's the title attribute, no?

I originally tried that but unfortunately the tooltip doesn't appear on hover.

Seems title only works when using top level <html>.

Attached patch 1675721-info-shortcut.diff (obsolete) — Splinter Review

What do you think about this solution?

I created an enableMessageHeaderShortcuts() method in the msgHdrView.js file which gets called once that area is loaded on startup.
We could later extend that method once we need to drop XUL entirely and port all the message header related shortcuts to JS.

Attachment #9190066 - Attachment is obsolete: true
Attachment #9191773 - Flags: review?(mkmelin+mozilla)
Comment on attachment 9191773 [details] [diff] [review]
1675721-info-shortcut.diff

Review of attachment 9191773 [details] [diff] [review]:
-----------------------------------------------------------------

I don't like that it creates translations that we won't ever need, since top level html is not far off.
Maybe we should just hold this off for a month or two and then we can fix it without adding more xul?
Attachment #9191773 - Flags: review?(mkmelin+mozilla)

Sounds good to me.
I'll update this patch to only tackle the focus ring improvements.
We can deal with the tooltip through the title tag once we get the top level html.

Summary: menu command "view / message security info" for received messages is gone → Improve the focus ring indicator of the "View Message Security Info" button in the message header

Or maybe we could land this string with the title so we automatically get it once the top level HTML is implemented?

Attached patch 1675721-info-shortcut.diff (obsolete) — Splinter Review

Magnus, what do you think about this?
We leave the html:button and implement the fluent string with the title attribute.
So once the top level HTML is implemented, we automatically inherit the tooltip from the title without needing any extra work.
More time for translators also to do their thing.

Attachment #9191773 - Attachment is obsolete: true
Attachment #9193604 - Flags: review?(mkmelin+mozilla)
Comment on attachment 9193604 [details] [diff] [review]
1675721-info-shortcut.diff

Review of attachment 9193604 [details] [diff] [review]:
-----------------------------------------------------------------

::: mail/base/content/msgHdrView.js
@@ +326,5 @@
>    );
>  
>    AddressBookListener.unregister();
>  
> +  document.removeEventListener("keypress", onShowMessageReadSecurityInfo);

normal event listeners do not really need to be removed, since they get removed when the window closes

@@ +3598,5 @@
> + * Trigger the Message Security Info popup panel from a keybaord shortcut.
> + *
> + * @param {Event} event - The DOM Event.
> + */
> +function onShowMessageReadSecurityInfo(event) {

.... which means these can be inlined and we don't need the global kSecurityShortcut either
Attachment #9193604 - Flags: review?(mkmelin+mozilla)
Attached patch 1675721-info-shortcut.diff (obsolete) — Splinter Review
Attachment #9193604 - Attachment is obsolete: true
Attachment #9193776 - Flags: review?(mkmelin+mozilla)
Comment on attachment 9193776 [details] [diff] [review]
1675721-info-shortcut.diff

Review of attachment 9193776 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good. r=mkmelin

::: mail/base/content/msgHdrView.inc.xhtml
@@ +442,2 @@
>                                                   onclick="showMessageReadSecurityInfo();"
> +                                                 onkeypress="if (event.key == 'Enter') { showMessageReadSecurityInfo(); }"

we should move this one too to be handled by the event handler

::: mail/base/content/msgHdrView.js
@@ +295,5 @@
>        menu.hidden = opensAreHidden;
>      }
>    }
>  
> +  enableMessageHeaderShortcuts();

since it's used only here, I would inline the function, and just have a comment instead that here we're adding the event listener for the message header. And that it's Ctrl+Alt+S /Cmd+Alt+S

::: mail/locales/en-US/messenger/openpgp/msgReadStatus.ftl
@@ +4,5 @@
>  
> +## Message Header Encryption Button
> +
> +message-header-show-security-info-action = S
> +

Maybe -key instead of -action? Otherwise might be unclear to localizers what it is.
Attachment #9193776 - Flags: review?(mkmelin+mozilla) → review+

Thanks for the review.
Patch updated and try-run launched: https://treeherder.mozilla.org/jobs?repo=try-comm-central&revision=a008f0f2b8360cb89b3b4f5a6e3367844ee4e7d4

It shouldn't affect any test, but since I updated the load method to async in order to be able to fetch the fluent string, I want to be sure I'm not causing any issue.

Attachment #9193776 - Attachment is obsolete: true
Attachment #9195363 - Flags: ui-review+
Attachment #9195363 - Flags: review+
Target Milestone: --- → 86 Branch

Pushed by geoff@darktrojan.net:
https://hg.mozilla.org/comm-central/rev/137fd38d8491
Implement tooltip and keyboard shortcut for message security info. r=mkmelin

Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: