Closed Bug 782810 Opened 10 years ago Closed 10 years ago

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

Categories

(Firefox for Metro Graveyard :: Browser, defect)

x86_64
Windows 8.1
defect
Not set
normal

Tracking

(Not tracked)

VERIFIED FIXED

People

(Reporter: jimm, Assigned: jimm)

References

Details

(Whiteboard: feature=work)

Attachments

(3 files, 5 obsolete files)

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.
Product: Firefox → Firefox for Metro
Component: General → Browser
Whiteboard: [metro-mvp][LOE:1]
Whiteboard: [metro-mvp][LOE:1] → [metro-mvp][LOE:1] feature=work
Blocks: 831952, 831971, 833165
No longer blocks: metro-contextmenus
Summary: Go over metrofx context menus to make sure they conform to Win8 guidelines → Work - Go over metrofx context menus to make sure they conform to Win8 guidelines
Whiteboard: [metro-mvp][LOE:1] feature=work → feature=work
No longer blocks: 833165
Attached patch string updates (obsolete) — Splinter Review
Attachment #714798 - Flags: review?(mbrubeck)
Attached patch string updates (obsolete) — Splinter Review
found a couple more to remove
Attachment #714798 - Attachment is obsolete: true
Attachment #714798 - Flags: review?(mbrubeck)
Attachment #714801 - Flags: review?(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.
Attachment #714801 - Flags: review?(mbrubeck) → review+
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 &amp; 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 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.
Attachment #714804 - Flags: ui-review?(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.
Attachment #714804 - Flags: ui-review?(asa)
Attached patch patch v.1 (obsolete) — Splinter Review
lots going on here. This builds on top of the patches in bug 833165. I still need to write tests as well.
Assignee: nobody → jmathies
Attachment #714801 - Attachment is obsolete: true
Attachment #714933 - Attachment is patch: true
Depends on: 842361
Blocks: 833165
No longer blocks: metro-contextmenus
Attached patch patch v.2 (obsolete) — Splinter Review
Attachment #714933 - Attachment is obsolete: true
Attached patch text and link tests (obsolete) — Splinter Review
Attached patch patch v.3Splinter Review
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.
Attachment #715228 - Attachment is obsolete: true
Attachment #715452 - Flags: review?(mbrubeck)
I've run all the context menu tests multiple times and addressed a couple  potential random oranges I found.
Attachment #715230 - Attachment is obsolete: true
Attachment #715453 - Flags: review?(mbrubeck)
(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 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.
Attachment #715453 - Flags: review?(mbrubeck) → review+
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.
Attachment #715452 - Flags: review?(mbrubeck) → review+
(In reply to Matt Brubeck (:mbrubeck) from comment #15)
> for (command of this._commands.childNodes) {

I mean "for (let command..."
(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.
https://hg.mozilla.org/mozilla-central/rev/d3ee65f9f8c8
https://hg.mozilla.org/mozilla-central/rev/aae721e94e2c
Status: NEW → RESOLVED
Closed: 10 years ago
Flags: in-testsuite+
Resolution: --- → FIXED
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.
Status: RESOLVED → VERIFIED
Blocks: 843988
OS: Windows 8 Metro → Windows 8.1
You need to log in before you can comment on or make changes to this bug.