Closed Bug 940038 Opened 7 years ago Closed 7 years ago

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

Categories

(Firefox :: Toolbars and Customization, defect)

x86_64
Windows 7
defect
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 31
Tracking Status
firefox29 --- fixed
firefox30 --- fixed
firefox31 --- fixed

People

(Reporter: Optimizer, Assigned: bwinton)

References

(Blocks 2 open bugs)

Details

(Whiteboard: [Australis:P4])

Attachments

(1 file, 1 obsolete file)

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 :)
Depends on: 878546
Summary: Subviews should mimic the node type of the source. → Subviews lack shortcuts
Whiteboard: [Australis:P4]
Summary: Subviews lack shortcuts → Australis: Subviews don't show shortcuts (in the menu panel)
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. */
}
Blocks: 963951
Well, this is a thing that seems to work on OS X.
What do you all think?
Assignee: nobody → bwinton
Attachment #8392338 - Flags: ui-review?(shorlander)
Attachment #8392338 - Flags: review?(jaws)
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?
Attachment #8392338 - Flags: review?(jaws) → review+
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.
(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.
(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.
(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 ?
(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.
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.  ;)
Attachment #8392338 - Attachment is obsolete: true
Attachment #8392338 - Flags: ui-review?(shorlander)
Attachment #8392993 - Flags: ui-review?(shorlander)
Attachment #8392993 - Flags: review+
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!
Attachment #8392993 - Flags: ui-review?(shorlander) → ui-review+
Keywords: checkin-needed
https://hg.mozilla.org/integration/fx-team/rev/d1ec8097a399
Keywords: checkin-needed
Whiteboard: [Australis:P4] → [Australis:P4][fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/d1ec8097a399
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Whiteboard: [Australis:P4][fixed-in-fx-team] → [Australis:P4]
Target Milestone: --- → Firefox 31
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.
Attachment #8392993 - Flags: approval-mozilla-beta?
Attachment #8392993 - Flags: approval-mozilla-aurora?
Attachment #8392993 - Flags: approval-mozilla-beta?
Attachment #8392993 - Flags: approval-mozilla-beta+
Attachment #8392993 - Flags: approval-mozilla-aurora?
Attachment #8392993 - Flags: approval-mozilla-aurora+
Depends on: 986769
(should we file a followup for the history menu's "Show History Sidebar" and reopen (the last) recently closed tab shortcuts?)
Yeah, that seems reasonable, although that'll be a slightly trickier patch due to the spacing issues mentioned above…
(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)
Does this code have tests?
Flags: in-testsuite?
(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.
Flags: in-testsuite? → in-testsuite-
Flags: in-qa-testsuite?
Depends on: 1035164
No longer depends on: 1035164
You need to log in before you can comment on or make changes to this bug.