782810 - Work - Go over metrofx context menus to make sure they conform to Win8 guidelines

Status: VERIFIED FIXED

(Firefox for Metro Graveyard :: Browser, defect)

Description

[Jim Mathies [:jimm]]

http://msdn.microsoft.com/en-us/library/windows/apps/hh465308.aspx

We hit on most of these, but we need to check some things like capitalization  and ordering.

Comment 1

[Jim Mathies [:jimm]]

Attachment: string updates

Comment 2

[Jim Mathies [:jimm]]

Attachment: string updates

found a couple more to remove

Comment 3

[Jim Mathies [:jimm]]

Attachment: Yuan's context menu proposal (pdf)

Comment 4

[Matt Brubeck (:mbrubeck)]

Comment on attachment 714801 [details] [diff] [review]
string updates

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

r=mbrubeck with one change noted below.

::: browser/metro/base/content/browser.xul
@@ -57,5 @@
>  
>    <commandset id="mainCommandSet">
>      <!-- basic navigation -->
> -    <command id="cmd_back" label="&back.label;" disabled="true" oncommand="CommandUpdater.doCommand(this.id);"/>
> -    <command id="cmd_forward" label="&forward.label;" disabled="true" oncommand="CommandUpdater.doCommand(this.id);" observes="bcast_urlbarState"/>

Some command labels in this section are actually used by toolbarbutton elements.  Can you move the labels to the toolbarbuttons where appropriate?  (Some of the buttons currently have non-internationalized labels, yikes!)  The labels aren't visible in the UI but they can be used for accessibility.

Comment 5

[Jim Mathies [:jimm]]

Here's an itemization of all the context menu strings we currently have:

Text clipboard functions
-----------------------------
context-copy: Copy
context-copy-all: Copy All
context-paste: Paste
context-paste-n-go: Paste & Go
context-select-all: Select All


Image functions
-----------------------------
context-save-image-lib: Save to Picture Library
context-copy-image: Copy Image
context-copy-image-loc: Copy Image location
context-open-image-tab: Open Image in New Tab


Link functions
-----------------------------
context-openinnewtab: Open Link in New Tab
context-bookmark-link: Bookmark Link
context-copy-link: Copy Link
context-copy-email: Copy Email Address
context-copy-phone: Copy Phone Number


Video functions:
-----------------------------
context-play-media: Play
context-pause-video: Pause
context-videotab: Open In New Tab
context-save-video: Save Video
context-save-video-to: Save Video To


App bar more button:
-----------------------------
context-findinpage: Find in Page
context-viewondesktop: View on Desktop

I can mesh up the text / link / image per yuan's proposal. I'll need a decision on what to do with video though.

Comment 6

[Jim Mathies [:jimm]]

Comment on attachment 714804 [details]
Yuan's context menu proposal (pdf)

Asa, yuan did up a really nice context menu spec that covers almost everything (except video). Curious how you would like to handle this. I can do a mass update here in this bug of all the strings so localizers can start working with these. If you would like though I can hold off until we have story bugs similar to the images one for other content.

Comment 7

[Asa Dotzler [:asa]]

Don't wait on me to get stories sorted out.

For video, the items should align well with the image context menus plus the addition of Play and Pause, so I think they should be:

Play
Pause
Save to video library
Open video in new tab

Please go ahead with that unless Yuan says otherwise before you start coding it up.

Comment 8

[Jim Mathies [:jimm]]

Attachment: patch v.1

lots going on here. This builds on top of the patches in bug 833165. I still need to write tests as well.

Comment 9

[Jim Mathies [:jimm]]

Attachment: patch v.2

Comment 10

[Jim Mathies [:jimm]]

Attachment: text and link tests

Comment 11

[Jim Mathies [:jimm]]

Attachment: patch v.3

This patch basically implements yuan's spec. I've also done some cleanup in ContextCommands.js.

Note full screen video functionality is disabled in this because it's not working (bug 842130).

I've also filled out toast notifications (we need a story and spec on these, see bug 783232).

There's one text based context menu test that's missing - context menus on selected text in content. I'll address this in the selection work I'm doing.

Comment 12

[Jim Mathies [:jimm]]

Attachment: text and link tests

I've run all the context menu tests multiple times and addressed a couple  potential random oranges I found.

Comment 13

[Jim Mathies [:jimm]]

(In reply to Jim Mathies [:jimm] from comment #12)
> Created attachment 715453 [details] [diff] [review]
> text and link tests
> 
> I've run all the context menu tests multiple times and addressed a couple 
> potential random oranges I found.

Please ignore the prefs changes in this.

Comment 14

[Matt Brubeck (:mbrubeck)]

Comment on attachment 715453 [details] [diff] [review]
text and link tests

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

::: browser/metro/base/tests/browser_context_menu_tests.js
@@ +388,5 @@
>      ok(tabPromise && !(tabPromise instanceof Error), "promise error");
>  
>      purgeEventQueue();
>  
>      let imagetab = Browser.getTabAtIndex(2);

I just noticed you could make this more robust by doing:

let imagetab = Browser.getTabFromChrome(tabPromise.target);

::: browser/metro/profile/metro.js
@@ -20,3 @@
>  pref("metro.debug.selection.displayRanges", false);
>  pref("metro.debug.selection.dumpRanges", false);
> -pref("metro.debug.selection.dumpEvents", false);

Reminder to revert these changes.

Comment 15

[Matt Brubeck (:mbrubeck)]

Comment on attachment 715452 [details] [diff] [review]
patch v.3

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

Looks great!  Just some minor stylistic nits below.

::: browser/metro/base/content/ContextCommands.js
@@ +107,5 @@
> +      defaultName = defaultPB.getComplexValue("browser.search.defaultenginename", nsIPLS).data;
> +      let defaultEngine = Services.search.getEngineByName(defaultName);
> +      defaultURI = defaultEngine.getSubmission(aSearchString).uri.spec;
> +    } catch (ex) {
> +      Util.dumpLn(ex.message);

If we are shipping this error-reporting code, I wonder if we should use Cu.reportError instead so that it's easier to access in-product.

::: browser/metro/base/content/browser.xul
@@ -57,5 @@
>  
>    <commandset id="mainCommandSet">
>      <!-- basic navigation -->
> -    <command id="cmd_back" label="&back.label;" disabled="true" oncommand="CommandUpdater.doCommand(this.id);"/>
> -    <command id="cmd_forward" label="&forward.label;" disabled="true" oncommand="CommandUpdater.doCommand(this.id);" observes="bcast_urlbarState"/>

As noted above: Please move the labels from this section to the corresponding toolbarbuttons when possible.

::: browser/metro/base/content/helperui/MenuUI.js
@@ +140,5 @@
> +    // links are displayed.
> +    let multipleMediaTypes = false;
> +    if ((contentTypes.indexOf("image") != -1 && contentTypes.indexOf("link") != -1) ||
> +        (contentTypes.indexOf("video") != -1 && contentTypes.indexOf("link") != -1) ||
> +        (contentTypes.indexOf("selected-text") != -1 && contentTypes.indexOf("link") != -1))

For conciseness, could you write this like "if (link && (image || video || selected))"?

(That's pseudo-code just to show the order; you'd still need the indexOf tests.)

@@ +144,5 @@
> +        (contentTypes.indexOf("selected-text") != -1 && contentTypes.indexOf("link") != -1))
> +      multipleMediaTypes = true;
> +
> +    for (let i = 0; i < this._commands.childElementCount; i++) {
> +      let command = this._commands.childNodes[i];

for (command of this._commands.childNodes) {

@@ +153,5 @@
>      for (let i = 0; i < this._commands.childElementCount; i++) {
>        let command = this._commands.childNodes[i];
> +      let types = command.getAttribute("type").split(",");
> +      let lowPriority = (command.hasAttribute("priority") &&
> +        command.getAttribute("priority") == "low") ? true : false;

You can remove the "hasAttribute" guard, and the ?: operator:

let lowPriority = (command.getAttribute("priority") === "low");

@@ +154,5 @@
>        let command = this._commands.childNodes[i];
> +      let types = command.getAttribute("type").split(",");
> +      let lowPriority = (command.hasAttribute("priority") &&
> +        command.getAttribute("priority") == "low") ? true : false;
> +      let searchTextItem = (command.id == "context-search" ? true : false);

Again, remove the "? true : false".

::: browser/metro/locales/en-US/chrome/browser.dtd
@@ -7,5 @@
>  
> -<!ENTITY back.label            "Back">
> -<!ENTITY forward.label         "Forward">
> -<!ENTITY reload.label          "Reload">
> -<!ENTITY stop.label            "Stop">

Some of these should stay; see comment on browser.xul.

Comment 16

[Matt Brubeck (:mbrubeck)]

(In reply to Matt Brubeck (:mbrubeck) from comment #15)
> for (command of this._commands.childNodes) {

I mean "for (let command..."

Comment 17

[Jim Mathies [:jimm]]

(In reply to Matt Brubeck (:mbrubeck) from comment #14)
> Comment on attachment 715453 [details] [diff] [review]
> text and link tests
> 
> Review of attachment 715453 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> >      let imagetab = Browser.getTabAtIndex(2);
> 
> I just noticed you could make this more robust by doing:
> 
> let imagetab = Browser.getTabFromChrome(tabPromise.target);


let event = yield tabPromise;
let imagetab = Browser.getTabFromChrome(event .target)

yield is so innocent looking, it's hard to wrap your head around what it does.

Comment 18

[Jim Mathies [:jimm]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/d3ee65f9f8c8

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/d3ee65f9f8c8
https://hg.mozilla.org/mozilla-central/rev/aae721e94e2c

Comment 20

[Asa Dotzler [:asa]]

Verified with nightly from http://hg.mozilla.org/mozilla-central/rev/702d2814efbf  We will have a couple of change stories but the basics are working here other than the pending content selection work at bug 831952.