940038 - Australis: Subviews don't show shortcuts (in the menu panel)

Status: RESOLVED FIXED

(Firefox :: Toolbars and Customization, defect)

Description

[Girish Sharma [:Optimizer]]

For example, the devtools subview is dynamically created out of tools > web developers. But instead of menu items, we use toolbarbuttons, due to which we can no longer show the shortcut of the items besides them.

FWIW : This is a regression from "App menu > web developers" popup. (may be in terms of accessibility ?)

Also, the very small width and large number of items make the rectangle look out of proportion to my eyes :)

Comment 1

[Tim Nguyen :ntim]

I'd suggest adding a new attribute called "shortcut" to the toolbar buttons, then use ::after to show the shortcuts :
.PanelUI-subview toolbarbutton[shortcut]:after {
content:attr(shortcut);
float:right; /* The -moz-box layout might prevent this. */
}

Comment 2

[Blake Winton (:bwinton) (:☕️)]

Attachment: The first version of the patch…

Well, this is a thing that seems to work on OS X.
What do you all think?

Comment 3

[Jared Wein [:jaws] (please needinfo? me)]

Comment on attachment 8392338 [details] [diff] [review]
The first version of the patch…

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

tentative r+ based on the question about the CSS rule applying being applied/unapplied.

::: browser/components/customizableui/src/CustomizableWidgets.jsm
@@ +291,5 @@
>          } else if (node.localName == "menuitem") {
>            item = doc.createElementNS(kNSXUL, "toolbarbutton");
>            item.setAttribute("class", "subviewbutton");
> +          let shortcutId = node.getAttribute("key");
> +          let shortcut = doc.getElementById(shortcutId);

We shouldn't pass an empty string to getElementById.

@@ +375,3 @@
>            item.classList.add("subviewbutton");
> +          let shortcutId = node.getAttribute("key");
> +          let shortcut = doc.getElementById(shortcutId);

Same here.

@@ +780,5 @@
> +        let shortcutId = item.getAttribute("key");
> +        let shortcut = doc.getElementById(shortcutId);
> +        if (shortcut) {
> +          elem.setAttribute("shortcut", ShortcutUtils.prettifyShortcut(shortcut));
> +        }

We shouldn't duplicate this for all of these widgets. Can you factor out these new lines? "function addShortcutIfPresent(aShortcutId, aElement)" ?

function addShortcutIfPresent(aShortcutId, aElement) {
  if (!aShortcutId)
    return;
  let shortcut = doc...
}

::: browser/themes/shared/customizableui/panelUIOverlay.inc.css
@@ +632,5 @@
>  }
>  
> +.PanelUI-subView .subviewbutton[shortcut]::after {
> +  content:attr(shortcut);
> +  float:right;

nit, spaces after colons here. i also tried float:left and it appeared the same to me, which i thought was weird.

@@ +633,5 @@
>  
> +.PanelUI-subView .subviewbutton[shortcut]::after {
> +  content:attr(shortcut);
> +  float:right;
> +  color: hsl(0,0%,50%);

this is probably more efficient than using opacity:.5 or something like that. given that we have hardcoded the background color this is probably fine.

@@ +636,5 @@
> +  float:right;
> +  color: hsl(0,0%,50%);
> +}
> +
> +.PanelUI-subView.cui-widget-panelview .subviewbutton[shortcut]::after {

How is this different than the above rule? when does this apply and the other doesn't?

Comment 4

[Tim Nguyen :ntim]

It's ok to show shortcuts in the widget panels since the panel's width expand to fit content, but I don't think it's gonna look good in subviews since their width is limited. The text will be overflowed everywhere. This is something we should take in consideration.

Comment 5

[Tim Nguyen :ntim]

(In reply to Tim Nguyen [:ntim] from comment #4)
> It's ok to show shortcuts in the widget panels since the panel's width
> expand to fit content, but I don't think it's gonna look good in subviews
> since their width is limited. The text will be overflowed everywhere. This
> is something we should take in consideration.

This is mostly for the History subview, which has long menu items.

Comment 6

[Tim Nguyen :ntim]

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #3)

> @@ +636,5 @@
> > +  float:right;
> > +  color: hsl(0,0%,50%);
> > +}
> > +
> > +.PanelUI-subView.cui-widget-panelview .subviewbutton[shortcut]::after {
> 
> How is this different than the above rule? when does this apply and the
> other doesn't?

I'm guessing one applies to both subviews and panels, and the other one to only panels.

Comment 7

[Jared Wein [:jaws] (please needinfo? me)]

Ah, thanks!

Comment 8

[Girish Sharma [:Optimizer]]

(In reply to Tim Nguyen [:ntim] from comment #5)
> (In reply to Tim Nguyen [:ntim] from comment #4)
> > It's ok to show shortcuts in the widget panels since the panel's width
> > expand to fit content, but I don't think it's gonna look good in subviews
> > since their width is limited. The text will be overflowed everywhere. This
> > is something we should take in consideration.
> 
> This is mostly for the History subview, which has long menu items.

Why will each history entry have a shortcut associated to it ?

Comment 9

[Tim Nguyen :ntim]

(In reply to Girish Sharma [:Optimizer] from comment #8)
> (In reply to Tim Nguyen [:ntim] from comment #5)
> > (In reply to Tim Nguyen [:ntim] from comment #4)
> > > It's ok to show shortcuts in the widget panels since the panel's width
> > > expand to fit content, but I don't think it's gonna look good in subviews
> > > since their width is limited. The text will be overflowed everywhere. This
> > > is something we should take in consideration.
> > 
> > This is mostly for the History subview, which has long menu items.
> 
> Why will each history entry have a shortcut associated to it ?

"Show history sidebar", "Restore Closed tabs", "Clear Recent History" have keyboard shortcuts, and the text is quite long to fit with those shortcuts.

Comment 10

[Blake Winton (:bwinton) (:☕️)]

Attachment: The second version of the patch.

Carrying forward r=jaws.
Nits fixed.

(In reply to Tim Nguyen [:ntim] from comment #9)
> "Show history sidebar", "Restore Closed tabs", "Clear Recent History" have
> keyboard shortcuts, and the text is quite long to fit with those shortcuts.

So, it turns out that those don't have keys, and so don't get shortcuts, at least in the current implementation.  I assert that if we add them later, we can adjust the spacing then.  ;)

Comment 11

[Stephen Horlander [:shorlander]]

Comment on attachment 8392993 [details] [diff] [review]
The second version of the patch.

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

Tested on Windows 8, Linux and OS X.

Looks good, thanks!

Comment 12

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/fx-team/rev/d1ec8097a399

Comment 13

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/d1ec8097a399

Comment 14

[Blake Winton (:bwinton) (:☕️)]

Comment on attachment 8392993 [details] [diff] [review]
The second version of the patch.

[Approval Request Comment]
Bug caused by (feature/regressing bug #): Australis.
User impact if declined: Some shortcuts don't show up.
Testing completed (on m-c, etc.): on m-c.
Risk to taking this patch (and alternatives if risky): Geez, I dunno.  I mean, it adds a generated element, and an attribute to each node, but it's pretty small.  But by the same token, we could probably live without it for a couple of releases.
String or IDL/UUID changes made by this patch: None.

Comment 15

[:Gijs (he/him)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/0eb4ef5d0347
https://hg.mozilla.org/releases/mozilla-beta/rev/889f6b66a670

Comment 16

[:Gijs (he/him)]

(should we file a followup for the history menu's "Show History Sidebar" and reopen (the last) recently closed tab shortcuts?)

Comment 17

[Blake Winton (:bwinton) (:☕️)]

Yeah, that seems reasonable, although that'll be a slightly trickier patch due to the spacing issues mentioned above…

Comment 18

[u428464]

(In reply to :Gijs Kruitbosch from comment #16)
> (should we file a followup for the history menu's "Show History Sidebar" and
> reopen (the last) recently closed tab shortcuts?)

There is also bug 947344. It may be used for that too (Clear History is missing too)

Comment 19

[u279076]

Does this code have tests?

Comment 20

[:Gijs (he/him)]

(In reply to Anthony Hughes, QA Mentor (:ashughes) from comment #19)
> Does this code have tests?

No, and I don't think we need specific tests for this.