Implement softkey feature on the simple phone

RESOLVED WONTFIX

Status

Firefox OS
Gaia
RESOLVED WONTFIX
3 years ago
3 months ago

People

(Reporter: lchang, Unassigned)

Tracking

unspecified
ARM
Gonk (Firefox OS)

Firefox Tracking Flags

(feature-b2g:2.2r+)

Details

(Whiteboard: [red square][ETA = 11/6])

Attachments

(4 attachments, 2 obsolete attachments)

(Reporter)

Description

3 years ago
On simple phone, there are several (two, usually) softkeys on the screen. Sometimes a drop menu will show when clicking one of the softkeys. The functionality and caption of these softkeys may vary under different apps but should have the same look across the system. We may need an interface which can let each app easily customize their own softkeys without being aware of how they are rendered.

Updated

3 years ago
Group: tai-confidential
Blocks: 1142877
(Reporter)

Comment 1

3 years ago
The proposal is documented on Wiki [1]. Please refer to it for further discussion.

[1] https://wiki.mozilla.org/WebAPI/Softkey_through_context_menu

Comment 2

3 years ago
open it since there is nothing confidential.
Group: tai-confidential
No longer blocks: 1175422
Blocks: 1008862
No longer blocks: 1142877

Comment 3

3 years ago
Hi Luke,
I think it's very GOOD idea to use contextmenu to implement softkeys for simple phones :)

I would like to know one thing about that wiki.
> If an app doesn't define its own context menu, the default set of softkeys will be involved in system app.

Did you have any ideas about the situation "the defaults set of softkeys"?
Actually, I'm not having a idea that situation.
Then, if you have a idea, i'd like to discuss about it.

Comment 4

3 years ago
(In reply to Yusuke Yamamoto from comment #3)
> > If an app doesn't define its own context menu, the default set of softkeys will be involved in system app.
> 
> Did you have any ideas about the situation "the defaults set of softkeys"?

Hi Yusuke,

from my point of view the "default set of softkeys" shall be empty, leading to hiding of SK panel completely. Thus the presence of softkeys will be defined strictly according to every application needs.

Comment 5

3 years ago
(In reply to Egor.Levichev from comment #4)
> from my point of view the "default set of softkeys" shall be empty, leading
> to hiding of SK panel completely. Thus the presence of softkeys will be
> defined strictly according to every application needs.
Hi Egor,

That makes sense, that's right.
It jus occurred to me, we have to decide the specification to modify easily for each OEMs / mobile operators.  

As also said Luke, there are several (two, usually) softkeys on the screen that depends on OEMs / mobile operators.
# e.g. in Japan, we have a 2 or 4 softkeys on simple phone.
  It depends on mobile operators specifications.
(Reporter)

Comment 6

3 years ago
Hi Yusuke,

Sorry for my late reply due to I've been on vacation for weeks. 

I agree with Egor to hide the softkeys if there's no default set defined.
feature-b2g: --- → 2.2r+
feature-b2g: 2.2r+ → 2.2r?
Blocks: 1178464
No longer blocks: 1008862

Comment 7

3 years ago
To implement Softkey support below mentioned approaches can be discussed

1- Drawing by individual application  

A new div tag will be developed which will be used by application to draw the softkey navigation bar and respective menu.
As user pressed LSK/RSK, Gecko will send a event to Gaia System app, which will then pass to individual application via IPC
 
2- Introducing a new event
 
A new mozbrowser event will be introduced at the Gecko layer
Drawing will be handle by system.


Second approach is clean, as drawing control will be with System.
feature-b2g: 2.2r? → 2.2r+
Whiteboard: [red-tai]

Updated

3 years ago
Flags: needinfo?(dflanagan)
Praveen: what sort of info do you need? I'm guessing you just want to draw my attention to Ashish's comment?   

Ashish: in general, in order to get my attention you need to set a needinfo flag (or a review or feedback request when you have actual code). I'm cc'ed on too many bugs to follow all the comments in all of them. I hope that work on this feature has not been stalled waiting for my input since September 16th. If my input is needed, it is always okay to ping me on bugzilla or by email until I respond.

(In reply to Ashish Garg from comment #7)
> To implement Softkey support below mentioned approaches can be discussed
> 
> 1- Drawing by individual application  
> 
> A new div tag will be developed which will be used by application to draw
> the softkey navigation bar and respective menu.
> As user pressed LSK/RSK, Gecko will send a event to Gaia System app, which
> will then pass to individual application via IPC

Apps will probably be able to just listen for LSK and RSK key events directly. There is no need for any special IPC in gecko; key events are already dispatched correctly (though the Gaia system prevents some of them from being forwarded to apps)

>  
> 2- Introducing a new event
>  
> A new mozbrowser event will be introduced at the Gecko layer
> Drawing will be handle by system.
> 
> Second approach is clean, as drawing control will be with System.

The problem is that this depends on gecko changes that (to my knowledge) no one is working on.

There is something appealing about having the System app handle the soft key label drawing, especially if the that bar will be appearing and disappearing and causing resize events. (I think that whether the bar should appear and disappear is a UX question, and is related to the question of whether the same bar can be used for T9 input or whether we might sometimes need two bars. I'm not sure if we have UX answers for how the bar should behave, but we can worry about that a little later.)  

Let's put aside the implementation question right now and think about what we want for app authors. 
I'd like to avoid each app having to handle the soft key labels manually. But I'd be satisfied with a solution that was just a gaia-level drop-in library to enable soft keys. As long as it is easy for app authors I don't feel that this needs to be in gecko or in the system app, at least not at this point.

There are various possible implementation approaches we can take, and I think that the way we will want to proceed is to start with a Gaia-based prototype where each app (or any website) can just include a javascript utility library to automatically handle soft keys.  If we are successful with that, we'll probably just ship it for RedSquare but we can also consider eventually putting it on a standards track and trying to implement it natively in gecko (and the system app).

So what I'd like to see first is a concrete proposal for how an app or website that uses our softkey utility library will define those softkey labels and actions.  The discussion here has proposed using HTML's contextmenu as a starting point, and I think that is a good idea, but not exactly right. The problem is that in mouse and touch-based UIs, context menus are only for infrequently-used options. The main UI options are exposed with things like buttons. We don't want to ask content authors to turn their buttons into context menus in order to port their apps and web sites to work on flip phones. Ideally, we want a solution where an app can put its main functionality on a button with an onclick callback. When that app runs on a smartphone, the user should just see the button.  But when the app runs on a flip phone, the user will not see the button, but will see the button's label appear above the LSK or the RSK.

So, I'd propose using the HTML accesskey attribute. Let's say that soft keys are function keys (LSK = F1, RSK = F2). Then any button with accesskey=f1 will automatically be mapped to the left soft key.  Our soft key library would copy the label of that button and display it over the LSK. It would hide the button, but generate a click event on on that hidden button it if the user presses the LSK key.

If a button has an accesskey attribute that maps to a soft key and it has a contextmenu attribute, then pressing that soft key would display the context menu instead of generating a click event.

Implementing something like this in a JS utility library might be a little tricky, but I think it is doable. Here are some of my implementation thoughts:

- we'd probably need a Mutation Observer to watch for changes to accesskey attributes, and for apps that have multiple page views, we'd have to be able to automatically determine which view is currently displayed and only do softkeys for that view, not the hidden ones.

- I'm not sure if we have a way to trigger a gecko context menu from JavaScript. So we'd either have to add that ability with a gecko change, or we'd have to copy code from the system app into our utility library so that apps could display their own context menus.


Egor, Ashish and Praveen: Is this fleshed out enough to start prototyping, or do we need more design discussions?

Luke and Evelyn: what do you think of my accesskey modification of the contextmenu proposal from comment 1? Does this sound okay to you?  Does the Smart TV project have any code for anything like this?
Flags: needinfo?(praveen.pandey20)
Flags: needinfo?(lchang)
Flags: needinfo?(ehung)
Flags: needinfo?(dflanagan)
Flags: needinfo?(ashish.garg)
Flags: needinfo?(Egor.Levichev)

Comment 9

3 years ago
Thanks David...I had just set the flag to 'needinfo' yesterday to highlight Ashish's comment to you. I will follow the same process going ahead as you suggested...

some prototyping we already started and Ashish would be sharing more information on this.
Flags: needinfo?(praveen.pandey20)

Comment 10

3 years ago
Created attachment 8668402 [details]
Softkey prototype

Thanks David for the detailed explanation, on the same line we have implemented a prototype attaching the same for your feedback.

How to Use
-----------
1- App need to include below mentioned css and javascript (available in attachment)
  
<!-- Shared Components-->
    <link rel="stylesheet" type="text/css" href="shared/style/action_menu-rs.css">
    <link rel="stylesheet" type="text/css" href="shared/style/option_menu-rs.css">
    <script type="text/javascript" src="shared/js/option_menu-rs.js" defer</script>
<!---->

2- Initialize the library

var contextNavigationMenu = (function () {
        var options = new ContextNavigationMenu();
        var showMenus = {
            showContextMenu: function (akeys) {
                options.createContextMenu(akeys);
                options.showContextMenu();
            },
            showOptionsMenu: function (aoptions) {
                options.createOptionsMenu(aoptions);
                options.showOptionsMenu();
            }
        };

        return {
            showMenus: showMenus
        }
    })();

3- Handle the click 

contextNavigationMenu.showMenus.showContextMenu({
                lskKey: {
                    name: 'Back',
                    l10nId: 'Back',
                    method: function () {
                        window.close();
                    }
                },
                cskKey: {
                    name: 'OK',
                    l10nId: 'ok',
                    method: function (name) {
                        alert('CSK Pressed:- ' + name);
                    }
                },
                rskKey: {
                    name: 'Options',
                    l10nId: 'Options',
                    method: function () {
                        contextNavigationMenu.showMenus.showOptionsMenu({
                            items: [{
                                name: 'Option1',
                                method: function () { }
                            }, {
                                name: 'Option2',
                                method: function () { alert(this) }
                            }, { // Last item is the Cancel button
                                name: 'Option3',
                                method: function () { alert(this) }
                            }, { // Last item is the Cancel button
                                name: 'Option4',
                                icon: 'img/icons/icon48x48.png',
                                method: function () { alert(this) }
                            }, { // Last item is the Cancel button
                                name: 'Option5',
                                icon: 'img/icons/icon48x48.png',
                                method: function () { alert(this) }
                            }, { 
                                name: 'Option6',
                                icon: 'img/icons/icon48x48.png',
                                method: function () { alert(this) }
                            }],
                            header: "Header Comes here"
                        });
                    }
                }
            });


 
Currently LSK, CSK and RSK mapped with keyboard 'a', 's' and 'd' respectively. 
A video is available in zip file, for demo purpose.
Flags: needinfo?(ashish.garg)
Thanks for attaching this, Ashish. I have not had time yet to unzip the zip file and look at it carefully. I hope to do that tomorrow. For now, I'm just responding to the usage example in comment #10.

My main comment is that I don't want to ask app authors to define the soft keys and context menus with JavaScript objects and arrays. The point of the original proposal in comment #1 and my modification of that proposal is that we should leverage the declarative powers of HTML markup to define softkeys.  There shouldn't need to be an API that the app author uses... they should be able to just set things up in their HTML file and have them work.  Ideally, we can do something clever enough that the same HTML will work on a desktop or on a smartphone and also (with the addition of one JS utility library) work on a flip phone with soft keys.

The code you've got in the zip file may be a good implemenation for achieving that on a flip phone, but I'd like to keep the API completely private if we can. As I said, I'll try to actually look at the code tomorrow. The only thing I have to add now is that the "initialize the library" step you show above seems kind of complicated, which says to me that the API of ContextNavigationMenu is too complicated. It seems to me that internal showContextMenu() and showOptionsMenu() methods ought to take an optional argument, and when that is passed they do the menu creation step automatically.

Updated

3 years ago
Attachment #8668402 - Attachment mime type: video/file → application/zip
Attachment #8668402 - Flags: feedback?(dflanagan)
Setting the feedback? flag on the attachment, so it appears in my queue of code to look at. 

In general, Ashish, you should set this feedback? or r? for any attachments that you want me to look at. Enter :djf in the field and it will autocomplete to my email address.
Flags: needinfo?(ashish.garg)

Updated

3 years ago
Blocks: 1209000

Comment 13

3 years ago
(In reply to David Flanagan [:djf] from comment #8)
I like this approach, to have seamless solution which will adapt existing solutions to became usable on non touch device.
To achieve this we probably will need some way to disable our JS utility library in case if application launched on touch device, or to optionally include/exclude this library into the device image depending on build time options.

> If a button has an accesskey attribute that maps to a soft key and it has a
> contextmenu attribute, then pressing that soft key would display the context
> menu instead of generating a click event.
Regarding assigning of possible actions to the context menu items: we will need to decide way of prioritizing elements in the menu according to some additional attribute.
Flags: needinfo?(Egor.Levichev)
No longer blocks: 1178464

Comment 14

3 years ago
(In reply to David Flanagan [:djf] from comment #11)
> Thanks for attaching this, Ashish. I have not had time yet to unzip the zip
> file and look at it carefully. I hope to do that tomorrow. For now, I'm just
> responding to the usage example in comment #10.
> 
> My main comment is that I don't want to ask app authors to define the soft
> keys and context menus with JavaScript objects and arrays. The point of the
> original proposal in comment #1 and my modification of that proposal is that
> we should leverage the declarative powers of HTML markup to define softkeys.
> There shouldn't need to be an API that the app author uses... they should be
> able to just set things up in their HTML file and have them work.  Ideally,
> we can do something clever enough that the same HTML will work on a desktop
> or on a smartphone and also (with the addition of one JS utility library)
> work on a flip phone with soft keys.
> 
> The code you've got in the zip file may be a good implemenation for
> achieving that on a flip phone, but I'd like to keep the API completely
> private if we can. As I said, I'll try to actually look at the code
> tomorrow. The only thing I have to add now is that the "initialize the
> library" step you show above seems kind of complicated, which says to me
> that the API of ContextNavigationMenu is too complicated. It seems to me
> that internal showContextMenu() and showOptionsMenu() methods ought to take
> an optional argument, and when that is passed they do the menu creation step
> automatically.

Hi David,

We made some changes and now there is no need to initialize the library. Developers just need to include the attached js/css files and write the code as mentioned below:-

//contextNavigationMenu will be a global variable, only available if library is included

contextNavigationMenu.showContextMenu({
                lskKey: {
                    name: 'Back', // Display text
                    l10nId: 'back', 
                    method: function () {
                        window.close(); // Method will be called on click
                    }
                },
                cskKey: {
                    name: 'OK',
                    l10nId: 'ok',
                    method: function (name) {
                        alert('CSK Pressed:- ' + name);
                    }
                },
                rskKey: {
                    name: 'Options', // Text can be "Options" or "Menu" depending upon requirement
                    l10nId: 'options',
                    method: function () {
                        contextNavigationMenu.showOptionsMenu({ // Create options menu
                            items: [{
                                name: 'Option1',
                                l10nId: 'option1',
                                method: function () { }
                            }, {
                                name: 'Option2',
                                method: function () { alert(this) }
                            }, { 
                                name: 'Option3',
                                method: function () { alert(this) }
                            }, { 
                                name: 'Option4',
                                icon: 'img/icons/icon48x48.png', // icon if required
                                method: function () { alert(this) }
                            }, { 
                                name: 'Option5',
                                icon: 'img/icons/icon48x48.png',
                                method: function () { alert(this) }
                            }, { 
                                name: 'Option6',
                                icon: 'img/icons/icon48x48.png',
                                method: function () { alert(this) }
                            }],
                            header: "Header Comes here"
                        });
                    }
                }
            });
Ashish: here are comments on the code you attached. (Prashant: I started this review before you wrote comment #14.  But it doesn't look like you attached the new code, so I wouldn't have been able to review that anyway.)

- first, it is best if you can attach code in the form of a github pull request because then we can use github to easily comment on individual lines of code.  In this case I would have expected a pull request that added your js and css files to gaia/shared/js and gaia/shared/style or something like that.  The demo video could have been added as a separate attachment.  In any case, I'll go ahead and add some comments here.

- formatting, etc: please follow the style of existing gaia code. We use 2-space indents, and 80-character maximum line lengths. Line terminators should be Unix \n newlines, not Windows \r\n. When I look at your code, every line ends with "^M" because of your terminators. Your code should pass our linters. We use jshint and eslint. This will catch things like missing semicolons and the use of " instead of '. If you do your development in the gaia repo and use github, then the linters will be automatically run every time you make a commit, and the commit will fail until you fix the code issues.

- documentation: your code doesn't have many comments and doesn't have any high-level documentation, so it is pretty hard to understand. I know it is intended for soft key handling, but the word "soft" does not appear anywhere in the js file. As someone reading it for the first time, I've got to figure out what a "ContextNavigationMenu" is, and what the difference between a context menu and an options menu is.  The example you put in comment #10 was good, but even with that, I'd like some higher-level docs explaining what the module does and what terms like "context menu" and "options menu" mean in this code.

- the css files: these are pretty big, and look like maybe they're copying big chunks of CSS from our existing building blocks.  I'm not sure what you're going for here, but this seems like too much CSS for a utility library.  If this is what you needed to do to make a demo that works, that's fine, but this will need to be pared way down before landing it.  In particular, the very generic "bootstrap" stuff in the CSS files won't be appropriate for a generic utility library because they could easily conflict with an app's existing styles.  Beyond that, though I have not looked closely at this css.

- the JS file. The rest of my comments will be based on line numbers:

10: the key map doesn't need to be an instance property. This would be better as ContextNavigationMenu.KEY_MAP, I think. Even better would be LSK, CSK and RSK constants rather than a KEY_MAP object.

15: your constructor has a return statement which is almost always wrong.

22: this is a very expensive operation. Can you omit it or just test for null?

31, 34: there is no matching removeEventListener(). Every time an app creates a ContextNavigationMenu object, another event listener will be added on the window object, and all of them will remain active. Performance will get worse and worse and worse.  This is potentially a serious bug.  If we expect that apps will only ever have a single ContextNavigationMenu object, then maybe this should not be a class.

32, 35: you should only call prevent default on keys you actually handle.

40: it is silly to define a dummy function if none is provided. Just use an if statement and don't call the method if it is not defined.

41: what in the world is going on here? It looks like the callback for each key is passed two arguments and the second argument is a function that can be used to change the text of the soft key? That's the kind of thing that we need documentation for. Maybe I don't understand the use case here, but it seems like there ought to be a better way.

41: also, you're using textContent as a way for the callback to identify what soft key was pressed. That seems like a good idea, but this won't work as an identifier because textContent will be different in different locales. Passing something like lskKey.name might be reasonable, but textContent is not.

41: note that by using a method variable here, you're invoking the function with no 'this' value. It might be better to invoke it as this.contextMenuOptions.lskKey.method so that it at least uses the lskKey object as its this value.

42, 43: setting the textContent of an element to an unlocalized string is almost never appropriate, and setting data-l10n-id directly is discouraged. Our localization API has a method you can call to do the right thing with the l10n id here.

46: our style requires curly braces around all if and else clauses.

46: note that there is a bug here. If there is no button assigned to lsk, then this code for closing the menu will never be triggered.

49-62: same comments as above. Plus there is a lot of repetition here. It seems to me that there ought to be a way to factor this so you don't need to handle the three buttons with three separate cases.

66: you started by creating a div element, then registered a couple of long event handlers, and now you go back to create more elements. I'd put the element creation code all together in one place.

72, etc: all of this element creation code is not really independent of the element creation code above. All of these things are going to be created the first time the function is called. You never remove them, so there is no need to test for their existance separately. You should have all of your element creation code together, possible in a separate render() method.

81: I don't like this generic kind of styling. Instead, I think you should use some very specific class name like sk-left-soft-key. (You should probably have some kind of prefix like sk- for every CSS class name this utility library uses to help prevent collisions with existing CSS)

84: you should basically never set textContent (except to the empty string), but particularly not to an unlocalized string like .name

85: there's a better way to do this. You can probably find examples in our code. or find our l10n docs on mdn somewhere

89-109: lots of repetitive code here, with the same problems noted above. There is probably a better way.

111: overall, this is a confusing function because it mixes one-time element creation code with the multi-time initialization code.  It seems to me it would be better to separate these things into two separate functions. And wouldn't it be easier to just pass the options object to the showContextMenu() function and do the initialization there?

121: I don't think you need a WeakMap here, especially since it is a local variable instead of a long-lived property of the object.

142: not allowed

155: not allowed.  And what is this section option anyway? It is undocumented.

164: rather than build this entire HTML menu based on a JS data structure, I want to be using HTML markup as much as possible. It would be better to have the app author directly define the <menu> in their .html file rather than describe the menu they want with a complicated JS object.

172: this is the proper way to do localization. You should use it elsewhere in the file.

174: don't do it this way, not even as a fallback

188: what is this line about? The comments above clearly apply to the code below, not to this line.

190: can you explain why you're using a weak map for this? This is only useful if the lifetime of the keys (your button elements) is different than the lifetime of the weak map. But since you create a new set of buttons and a new map on each invocation, I'm pretty sure you don't need a weakmap here.  Just use a map. Or just set data-item-number on each button and use that to find the item by index or something.

212: where's this click event coming from? Are we expecting that we'll have spatial navigation code so that the arrow keys navigate the menu and the ok button dispatches a click event? Or is this just in here for testing at this point?

221: I think you can just say `if (!action)` here. If you do the typeof thing, then a null value will go through and you'll get an exception on the next line.

222: don't define a no-op method and then invoke it. Just don't invoke an undefined function.

224: consider allowing the user to specify a target object for the method, and call method.apply(action.target || null...)

246: please don't use && in place of a proper if statement. 

248: it seems like this if statement needs an else. If there is no div, then the menu will never be displayed, right? Maybe you should throw an exception in that case?  It seems to me that this indicates a design flaw in the API. It would be better if the two things were independent.

256: I'm not sure what this is for, but I'm skeptical about setting a top-level style for a utility library like this.  Is this something we do elsewhere in our shared/style/ files?

261: bad l10n

262: isn't the lsk supposed to be the cancel option, and doesn't that button close the menu? If so, it is confusing that you call the property "menuOpenText"

274-288: doesn't this just duplicate setup code from elsewhere in the file? You should refactor that.

As noted earlier in this bug, I'd like a solution where all (or as much as possible) of the configuration happens in the .html file. An app author should be able to write HTML and drop a JS and CSS file into their app and have it all just work. There should be no need for the app author to write any JS or define any JS data structures. Some of the code I've reviewed here may be useful in implementing that, but I don't want to expose any of these APIs to app authors.

So this was kind of a harsh review, and I'll be setting the feedback- flag to indicate negative feedback and to indicate that you are not on the right track here. I probably won't have time to review all STC code to this level of detail, but want to use this first batch of code as an example to show you what kind of code quality I expect.

Updated

3 years ago
Attachment #8668402 - Flags: feedback?(dflanagan) → feedback-

Comment 16

3 years ago
(In reply to David Flanagan [:djf] from comment #15)
> Ashish: here are comments on the code you attached. (Prashant: I started
> this review before you wrote comment #14.  But it doesn't look like you
> attached the new code, so I wouldn't have been able to review that anyway.)
> 
> - first, it is best if you can attach code in the form of a github pull
> request because then we can use github to easily comment on individual lines
> of code.  In this case I would have expected a pull request that added your
> js and css files to gaia/shared/js and gaia/shared/style or something like
> that.  The demo video could have been added as a separate attachment.  In
> any case, I'll go ahead and add some comments here.
> 
> - formatting, etc: please follow the style of existing gaia code. We use
> 2-space indents, and 80-character maximum line lengths. Line terminators
> should be Unix \n newlines, not Windows \r\n. When I look at your code,
> every line ends with "^M" because of your terminators. Your code should pass
> our linters. We use jshint and eslint. This will catch things like missing
> semicolons and the use of " instead of '. If you do your development in the
> gaia repo and use github, then the linters will be automatically run every
> time you make a commit, and the commit will fail until you fix the code
> issues.
> 
> - documentation: your code doesn't have many comments and doesn't have any
> high-level documentation, so it is pretty hard to understand. I know it is
> intended for soft key handling, but the word "soft" does not appear anywhere
> in the js file. As someone reading it for the first time, I've got to figure
> out what a "ContextNavigationMenu" is, and what the difference between a
> context menu and an options menu is.  The example you put in comment #10 was
> good, but even with that, I'd like some higher-level docs explaining what
> the module does and what terms like "context menu" and "options menu" mean
> in this code.
> 
> - the css files: these are pretty big, and look like maybe they're copying
> big chunks of CSS from our existing building blocks.  I'm not sure what
> you're going for here, but this seems like too much CSS for a utility
> library.  If this is what you needed to do to make a demo that works, that's
> fine, but this will need to be pared way down before landing it.  In
> particular, the very generic "bootstrap" stuff in the CSS files won't be
> appropriate for a generic utility library because they could easily conflict
> with an app's existing styles.  Beyond that, though I have not looked
> closely at this css.
> 
> - the JS file. The rest of my comments will be based on line numbers:
> 
> 10: the key map doesn't need to be an instance property. This would be
> better as ContextNavigationMenu.KEY_MAP, I think. Even better would be LSK,
> CSK and RSK constants rather than a KEY_MAP object.
> 
> 15: your constructor has a return statement which is almost always wrong.
> 
> 22: this is a very expensive operation. Can you omit it or just test for
> null?
> 
> 31, 34: there is no matching removeEventListener(). Every time an app
> creates a ContextNavigationMenu object, another event listener will be added
> on the window object, and all of them will remain active. Performance will
> get worse and worse and worse.  This is potentially a serious bug.  If we
> expect that apps will only ever have a single ContextNavigationMenu object,
> then maybe this should not be a class.
> 
> 32, 35: you should only call prevent default on keys you actually handle.
> 
> 40: it is silly to define a dummy function if none is provided. Just use an
> if statement and don't call the method if it is not defined.
> 
> 41: what in the world is going on here? It looks like the callback for each
> key is passed two arguments and the second argument is a function that can
> be used to change the text of the soft key? That's the kind of thing that we
> need documentation for. Maybe I don't understand the use case here, but it
> seems like there ought to be a better way.
> 
> 41: also, you're using textContent as a way for the callback to identify
> what soft key was pressed. That seems like a good idea, but this won't work
> as an identifier because textContent will be different in different locales.
> Passing something like lskKey.name might be reasonable, but textContent is
> not.
> 
> 41: note that by using a method variable here, you're invoking the function
> with no 'this' value. It might be better to invoke it as
> this.contextMenuOptions.lskKey.method so that it at least uses the lskKey
> object as its this value.
> 
> 42, 43: setting the textContent of an element to an unlocalized string is
> almost never appropriate, and setting data-l10n-id directly is discouraged.
> Our localization API has a method you can call to do the right thing with
> the l10n id here.
> 
> 46: our style requires curly braces around all if and else clauses.
> 
> 46: note that there is a bug here. If there is no button assigned to lsk,
> then this code for closing the menu will never be triggered.
> 
> 49-62: same comments as above. Plus there is a lot of repetition here. It
> seems to me that there ought to be a way to factor this so you don't need to
> handle the three buttons with three separate cases.
> 
> 66: you started by creating a div element, then registered a couple of long
> event handlers, and now you go back to create more elements. I'd put the
> element creation code all together in one place.
> 
> 72, etc: all of this element creation code is not really independent of the
> element creation code above. All of these things are going to be created the
> first time the function is called. You never remove them, so there is no
> need to test for their existance separately. You should have all of your
> element creation code together, possible in a separate render() method.
> 
> 81: I don't like this generic kind of styling. Instead, I think you should
> use some very specific class name like sk-left-soft-key. (You should
> probably have some kind of prefix like sk- for every CSS class name this
> utility library uses to help prevent collisions with existing CSS)
> 
> 84: you should basically never set textContent (except to the empty string),
> but particularly not to an unlocalized string like .name
> 
> 85: there's a better way to do this. You can probably find examples in our
> code. or find our l10n docs on mdn somewhere
> 
> 89-109: lots of repetitive code here, with the same problems noted above.
> There is probably a better way.
> 
> 111: overall, this is a confusing function because it mixes one-time element
> creation code with the multi-time initialization code.  It seems to me it
> would be better to separate these things into two separate functions. And
> wouldn't it be easier to just pass the options object to the
> showContextMenu() function and do the initialization there?
> 
> 121: I don't think you need a WeakMap here, especially since it is a local
> variable instead of a long-lived property of the object.
> 
> 142: not allowed
> 
> 155: not allowed.  And what is this section option anyway? It is
> undocumented.
> 
> 164: rather than build this entire HTML menu based on a JS data structure, I
> want to be using HTML markup as much as possible. It would be better to have
> the app author directly define the <menu> in their .html file rather than
> describe the menu they want with a complicated JS object.
> 
> 172: this is the proper way to do localization. You should use it elsewhere
> in the file.
> 
> 174: don't do it this way, not even as a fallback
> 
> 188: what is this line about? The comments above clearly apply to the code
> below, not to this line.
> 
> 190: can you explain why you're using a weak map for this? This is only
> useful if the lifetime of the keys (your button elements) is different than
> the lifetime of the weak map. But since you create a new set of buttons and
> a new map on each invocation, I'm pretty sure you don't need a weakmap here.
> Just use a map. Or just set data-item-number on each button and use that to
> find the item by index or something.
> 
> 212: where's this click event coming from? Are we expecting that we'll have
> spatial navigation code so that the arrow keys navigate the menu and the ok
> button dispatches a click event? Or is this just in here for testing at this
> point?
> 
> 221: I think you can just say `if (!action)` here. If you do the typeof
> thing, then a null value will go through and you'll get an exception on the
> next line.
> 
> 222: don't define a no-op method and then invoke it. Just don't invoke an
> undefined function.
> 
> 224: consider allowing the user to specify a target object for the method,
> and call method.apply(action.target || null...)
> 
> 246: please don't use && in place of a proper if statement. 
> 
> 248: it seems like this if statement needs an else. If there is no div, then
> the menu will never be displayed, right? Maybe you should throw an exception
> in that case?  It seems to me that this indicates a design flaw in the API.
> It would be better if the two things were independent.
> 
> 256: I'm not sure what this is for, but I'm skeptical about setting a
> top-level style for a utility library like this.  Is this something we do
> elsewhere in our shared/style/ files?
> 
> 261: bad l10n
> 
> 262: isn't the lsk supposed to be the cancel option, and doesn't that button
> close the menu? If so, it is confusing that you call the property
> "menuOpenText"
> 
> 274-288: doesn't this just duplicate setup code from elsewhere in the file?
> You should refactor that.
> 
> As noted earlier in this bug, I'd like a solution where all (or as much as
> possible) of the configuration happens in the .html file. An app author
> should be able to write HTML and drop a JS and CSS file into their app and
> have it all just work. There should be no need for the app author to write
> any JS or define any JS data structures. Some of the code I've reviewed here
> may be useful in implementing that, but I don't want to expose any of these
> APIs to app authors.
> 
> So this was kind of a harsh review, and I'll be setting the feedback- flag
> to indicate negative feedback and to indicate that you are not on the right
> track here. I probably won't have time to review all STC code to this level
> of detail, but want to use this first batch of code as an example to show
> you what kind of code quality I expect.

Hi David,

Thanks for the review. I just need to highlight few key points:

- As Ashish mentioned, this is just a prototype (only for demo purpose). No naming convention, jshinting, refracting or unit testing was done. We will be following Mozilla (https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/Coding_Style) and Google (http://google-styleguide.googlecode.com/svn/trunk/javascriptguide.xml) style guide. I also prefer Idiomatic (https://github.com/rwaldron/idiomatic.js) style guide.
- We just created a sample demo, that's why we didn't include any documentation there.
- Those css files are just copied from existing building blocks and will be pared down eventually. We just copied the existing styles for demo purpose.
- Regarding js files, no code review, no jshinting, no refracting, no naming convention was followed.
Line 111-246, was copied from existing gaia building blocks (/shared/js/option_menu.js), and not a single word was changed. 
- '15: your constructor has a return statement which is almost always wrong.' I never used to return anything from constructor, but it was mentioned in Idiomatic style guide so that's why we added that part.

There is a scenario where LSK, RSK and CSK functionality/display text will get changed when user selects (moves up/down) any option on a particular screen. So, if all the configuration is done in html file, then how to handle this dynamic change? Mentioned proposal (https://wiki.mozilla.org/WebAPI/Softkey_through_context_menu) is assuming that LSK, RSK and CSK won't change for a single screen but that is not there in our case. So developer will create/attach listeners 'n' number of menu tags if there are 'n' options?

David, We know you need that kind of code quality and we will be following that. Attached code was never intend to be a part of gaia repo. It was just for demo purpose.

Thanks again for taking your time to review this code. :)
Prashant,

Thanks for letting me know about shared/js/option_menu.js. Looks like there are some problems with that code.

Tell me more about when you think that user navigation within a context menu would cause changes to the softkey labels. What's an example of that? Let's see if we can figure out a way to map our UX needs to what HTML offers.  That's the original point of this particular bug, as the proposal in comment #1 shows.

I understand that this code was an unfinished prototype. Thanks for sharing it at this stage.

Comment 18

3 years ago
Created attachment 8669541 [details]
sample.png

David,

Check the attached screenshot. When user is navigating (same screen) from Password field to Show Password field, CSK and RSK text/functionality is changing. This is just one use case, but there can be many. 

So my question was, if all the configuration is done in html file, then how to handle this dynamic change? Mentioned proposal (https://wiki.mozilla.org/WebAPI/Softkey_through_context_menu) is assuming that LSK, RSK and CSK won't change for a single screen.

Updated

3 years ago
Flags: needinfo?(ashish.garg) → needinfo?(dflanagan)

Comment 19

3 years ago
(In reply to David Flanagan [:djf] from comment #8)
> 
> - I'm not sure if we have a way to trigger a gecko context menu from
> JavaScript. So we'd either have to add that ability with a gecko change, or
> we'd have to copy code from the system app into our utility library so that
> apps could display their own context menus.
>

No, I don't think we have API to trigger contextmenu events from Gaia. Copy the handing code from System app might be a feasible approach, and the library also needs to parse menu options from DOM, here is the Gecko code snippet we could reuse.

http://mxr.mozilla.org/mozilla-b2g37_v2_2/source/dom/browser-element/BrowserElementChildPreload.js#1013 
 
 
> Luke and Evelyn: what do you think of my accesskey modification of the
> contextmenu proposal from comment 1? Does this sound okay to you?  Does the
> Smart TV project have any code for anything like this?

Nothing related to TV. We came out this proposal for simple phone project when we discussed with Egor in June.
Flags: needinfo?(ehung)

Comment 20

3 years ago
(In reply to Evelyn Hung [:evelyn] from comment #19) 
> Nothing related to TV. We came out this proposal for simple phone project
> when we discussed with Egor in June.

err.. earlier than June, should be February.

BTW, I feel hacking on Gecko to add this functionality will be easier. IIRC, we could refer to mozcontextmenu event in browser API and do something similar. The implementation will be all JavaScript in Gecko, I believe it's not too hard to a front-end engineer.

Updated

3 years ago
Flags: needinfo?(lchang)
Prashant,

Thanks for the attachment. I misunderstood your question previous, but I understand it now.

When the soft keys need to change dynamically, we can just change the HTML dynamically. (Which is why I said that an implementation of the utility library would probably have to use a MutationObserver).  In the wireframes you attached, the LSK is always Cancel, but the RSK is sometimes Connect and is sometimes unused.  If we're creating soft keys based on HTML button elements with an accessKey attribute, the obvious way to do this is to disable the button (i.e. set the disabled attribute) when connecting is not an option. Note that this is exactly what would be required in a touch-based UI. If a button is disabled, then the user can't invoke the action, and shouldn't see the option on the soft key.

The wireframes also show various features mapped to CSK. But all of those have to do with form field input, and I think they will have to be handled at a different level. I certainly don't expect app authors to even have to think about soft key labels for basic form fields.  (Also, having the Abc/123 switch on the CSK won't work anyway. Won't we need the Ok key for selecting among various word options when in T9 mode?)

(In reply to Prashant Goel from comment #18)
> Created attachment 8669541 [details]
> sample.png
> 
> David,
> 
> Check the attached screenshot. When user is navigating (same screen) from
> Password field to Show Password field, CSK and RSK text/functionality is
> changing. This is just one use case, but there can be many. 
> 
> So my question was, if all the configuration is done in html file, then how
> to handle this dynamic change? Mentioned proposal
> (https://wiki.mozilla.org/WebAPI/Softkey_through_context_menu) is assuming
> that LSK, RSK and CSK won't change for a single screen.
Flags: needinfo?(dflanagan) → needinfo?(Prashant.Goel)
Thanks, Evelyn. Where in Gecko do you think is an appropriate place to put this functionality? Does it end up in b2g/chrome/content/forms.js or somewhere like that? Unless we have a gecko engineer working on the project who can help to architect this, I'm afraid we may end up with a gaia-based solution simply because that's the only area I can work confidently.

(In reply to Evelyn Hung [:evelyn] from comment #20)
> 
> BTW, I feel hacking on Gecko to add this functionality will be easier. IIRC,
> we could refer to mozcontextmenu event in browser API and do something
> similar. The implementation will be all JavaScript in Gecko, I believe it's
> not too hard to a front-end engineer.
Flags: needinfo?(ehung)

Comment 23

3 years ago
(In reply to David Flanagan [:djf] from comment #22)
> Thanks, Evelyn. Where in Gecko do you think is an appropriate place to put
> this functionality? 

IMO, should be in dom/browser-element/BrowserElementChildPreload.js and BrowserElementParent.js since it's part of mozbrowser API.
Flags: needinfo?(ehung)

Comment 24

3 years ago
(In reply to David Flanagan [:djf] from comment #21)
> Prashant,
> 
> Thanks for the attachment. I misunderstood your question previous, but I
> understand it now.
> 
> When the soft keys need to change dynamically, we can just change the HTML
> dynamically. (Which is why I said that an implementation of the utility
> library would probably have to use a MutationObserver).  In the wireframes
> you attached, the LSK is always Cancel, but the RSK is sometimes Connect and
> is sometimes unused.  If we're creating soft keys based on HTML button
> elements with an accessKey attribute, the obvious way to do this is to
> disable the button (i.e. set the disabled attribute) when connecting is not
> an option. Note that this is exactly what would be required in a touch-based
> UI. If a button is disabled, then the user can't invoke the action, and
> shouldn't see the option on the soft key.
> 
> The wireframes also show various features mapped to CSK. But all of those
> have to do with form field input, and I think they will have to be handled
> at a different level. I certainly don't expect app authors to even have to
> think about soft key labels for basic form fields.  (Also, having the
> Abc/123 switch on the CSK won't work anyway. Won't we need the Ok key for
> selecting among various word options when in T9 mode?)
> 
> (In reply to Prashant Goel from comment #18)
> > Created attachment 8669541 [details]
> > sample.png
> > 
> > David,
> > 
> > Check the attached screenshot. When user is navigating (same screen) from
> > Password field to Show Password field, CSK and RSK text/functionality is
> > changing. This is just one use case, but there can be many. 
> > 
> > So my question was, if all the configuration is done in html file, then how
> > to handle this dynamic change? Mentioned proposal
> > (https://wiki.mozilla.org/WebAPI/Softkey_through_context_menu) is assuming
> > that LSK, RSK and CSK won't change for a single screen.

David,

You proposed this right,
"So, I'd propose using the HTML accesskey attribute. Let's say that soft keys are function keys (LSK = F1, RSK = F2). Then any button with accesskey=f1 will automatically be mapped to the left soft key.  Our soft key library would copy the label of that button and display it over the LSK. It would hide the button, but generate a click event on on that hidden button it if the user presses the LSK key.

If a button has an accesskey attribute that maps to a soft key and it has a contextmenu attribute, then pressing that soft key would display the context menu instead of generating a click event."

See, acc. to the requirement and attached screenshot, if user is navigating up/down (eg. from Password field to Show Password field), button labels will change and its click functionality too. So if library is using MutationObeserver, then it will change softkey labels and will generate a click event if a button is pressed.

So my question is "Will an App developer has to change the button label and handle different click scenarios. And if Yes, then they have to create 'n' different labels and handle 'n' different clicks if there are 'n' different options (options here means, 'Password field', 'Show Password field' etc) on a particular view (same screen)"?
Flags: needinfo?(Prashant.Goel)

Updated

3 years ago
Flags: needinfo?(dflanagan)
Prashant,

Are you referring to the fact that in the wireframes, there is "Abc" over the CSK in one case and then when the user presses down arrow there is "Mark" over that key?

I don't know how that is going to be implemented, but given that it is specific to form-field interactions, that will have to be handled at the gecko level, not at the app level. So the app developer would not have to do anything to get the "Abc" to appear or the "Mark" label to appear. If we accept that UX design, we'll have to make it happen automatically somehow.

(I would guess, however, that we won't want to accept that UX. I thought the standard for flip-phone input was to use the * key for shifting between input modes, not the CSK.)

So anyway, I think that the answer to your question is no. But to the extent that dynamic changes to the soft keys are required, the requirements will be the same regardless of whether the soft keys are configured with JS data structures or HTML markup. To the maximum extent possible, we should use HTML markup.
Flags: needinfo?(dflanagan)
I don't know if the attached wireframe example (sample.png) is from the work that Litehouse has done for us or if it is an example from some other flip-phone design. In either case, though, it brings up important UX questions that we need to address.

Tony: does our contract with Litehouse have them doing UX design for platform-level features? We need UX for things like what happens when a checkbox has the focus. I assume that we want the Ok button to toggle it, but do we want the work "Mark" or "Unmark" to appear above the Ok button as shown in the wireframes? Similarly, what should happen if the user navigates to a button element in web content? Do we display the word "Click" above the Ok key? And perhaps most importantly, we need a spec for how text input is going to work. The attached wireframe seems to imply that the Ok button will be used as a shift key (shifting between numeric input and alphabetic input) but this seems like a radical departure from flip phone input which normally uses * as the shift key, I think.  And I suspect that we need to have the Ok button free for selecting among word choices for T9 input.  Do we have UX design work going on for how T9 input is going to work? For example, when T9 needs to display a list of possible words, will that list appear in the same part of the screen that displays soft key labels (which might hide things like the Cancel option) or will it display the words in some other way?

Morpheus: Do you know if the attached wireframe is from Litehouse's recent work? Does the original ClickUI design include things like how the user can interact with form fields? And does it make sense to use to use the Ok key for shifting between 123 and abc input mode as shown in that wireframe?
Flags: needinfo?(mochen)
Flags: needinfo?(aappleton)

Comment 27

3 years ago
(In reply to David Flanagan [:djf] from comment #25)
> Prashant,
> 
> Are you referring to the fact that in the wireframes, there is "Abc" over
> the CSK in one case and then when the user presses down arrow there is
> "Mark" over that key?
> 
> I don't know how that is going to be implemented, but given that it is
> specific to form-field interactions, that will have to be handled at the
> gecko level, not at the app level. So the app developer would not have to do
> anything to get the "Abc" to appear or the "Mark" label to appear. If we
> accept that UX design, we'll have to make it happen automatically somehow.
> 
> (I would guess, however, that we won't want to accept that UX. I thought the
> standard for flip-phone input was to use the * key for shifting between
> input modes, not the CSK.)
> 
> So anyway, I think that the answer to your question is no. But to the extent
> that dynamic changes to the soft keys are required, the requirements will be
> the same regardless of whether the soft keys are configured with JS data
> structures or HTML markup. To the maximum extent possible, we should use
> HTML markup.

David,

I am not referring to the CSK. See as you said,

"So, I'd propose using the HTML accesskey attribute. Let's say that soft keys are function keys (LSK = F1, RSK = F2). Then any button with accesskey=f1 will automatically be mapped to the left soft key.  Our soft key library would copy the label of that button and display it over the LSK. It would hide the button, but generate a click event on on that hidden button it if the user presses the LSK key."

Suppose an App developer created a button for 'Password field' field which gets mapped to the RSK by our utility library. He will handle the click event for that button which will be raised by our utility library.
So for 'Show Password field', again he has to create a new button with new label and handle its click event separately?
[Here I am assuming that the user is not changing the view and navigating up/down (from 'Password field' to 'Show Password field')]

If this is the case, then he has to create 'n' number of buttons with different labels and handle 'n' number of click events.
Flags: needinfo?(dflanagan)
(In reply to Prashant Goel from comment #27)

> Suppose an App developer created a button for 'Password field' field which
> gets mapped to the RSK by our utility library. He will handle the click
> event for that button which will be raised by our utility library.
> So for 'Show Password field', again he has to create a new button with new
> label and handle its click event separately?
> [Here I am assuming that the user is not changing the view and navigating
> up/down (from 'Password field' to 'Show Password field')]
> 
> If this is the case, then he has to create 'n' number of buttons with
> different labels and handle 'n' number of click events.

Okay, but it is not really a realistic scenario. Neither the password input field nor the checkbox actually have any features that are mapped to the RSK. Maybe there is some other case where you would be in that situation and would need to create n buttons.  You could of course have just a single event handler on a shared ancestor of those buttons, though. Another alternative would be to have just a single button whose textContent is changed as needed to change the RSK label. (And I'm not sure how this would be any different with a system that described soft keys through a JS data structure.)

Its not like we're adding new features or new event handlers here. On a touchscreen FirefoxOS device, this wifi password entry screen that we're using as our example has two buttons. It has a back button in the upper left and a "Connect" button in the upper right. The back button is always available. The Connect button is sometimes disabled and sometimes enabled. In the wireframe you attached, the Back button maps to the Cancel option on the LSK and the Connect button maps to the Connect option that is sometimes displayed on the RSK.

I'm claiming that for any given screen we're going to need the same number of event handlers for both the touch-based and non-touch versions of the UX.
Flags: needinfo?(dflanagan)

Comment 29

3 years ago
Ok David, that is fine.

As you suggest that the configuration should be done in HTML instead of JS (as we demonstrated with that demo), we will build another prototype and share with you soon.

We might disturb you if we need any help. :)
Hi David, 

Please find my response inline.

(In reply to David Flanagan [:djf] from comment #26)
> I don't know if the attached wireframe example (sample.png) is from the work
> that Litehouse has done for us or if it is an example from some other
> flip-phone design. In either case, though, it brings up important UX
> questions that we need to address.

Yes, the UX spec is from Litehouse. Now Litehouse creates these specs based on the building blocks guideline Mozilla UX team created. (You can refer to the link: https://mozilla.box.com/s/nwb39uotutu59yn9mnii08e0cdssfmy9)

> 
> Tony: does our contract with Litehouse have them doing UX design for
> platform-level features? We need UX for things like what happens when a
> checkbox has the focus. I assume that we want the Ok button to toggle it,
> but do we want the work "Mark" or "Unmark" to appear above the Ok button as
> shown in the wireframes? Similarly, what should happen if the user navigates
> to a button element in web content? Do we display the word "Click" above the
> Ok key?

Basically, CSK(center softkey = OK) usually functions as confirmation, and it only shows when the context needs to emphasize or it refers to other function than confirmation. You can find Left softkey & Right softkey section for more details I defined.

> And perhaps most importantly, we need a spec for how text input is
> going to work. The attached wireframe seems to imply that the Ok button will
> be used as a shift key (shifting between numeric input and alphabetic input)
> but this seems like a radical departure from flip phone input which normally
> uses * as the shift key, I think.  And I suspect that we need to have the Ok
> button free for selecting among word choices for T9 input.  Do we have UX
> design work going on for how T9 input is going to work? For example, when T9
> needs to display a list of possible words, will that list appear in the same
> part of the screen that displays soft key labels (which might hide things
> like the Cancel option) or will it display the words in some other way?

So far, we've defined a basic rule for input field, you can refer to Input Area at p. 13 in Guideline. However, since Litehouse hasn't received any confirmation on T9 support, carrier/OEM requirements, now they just create the basic mechanism and ask for confirmation. (please refer to http://r1l8an.axshare.com/#p=5_4_entry_mode) Have we confirmed anything related to T9 support and entry mode? Once Litehouse get confirmation, they will update the UX specs, as well as dedicated Back and Clear key.
Flags: needinfo?(mochen)
Flags: needinfo?(aappleton)

Comment 31

3 years ago
David,

So we are assuming that LSK and RSK won't change for a particular view. May be UX designs needs to be changed accordingly.
(In reply to Prashant Goel from comment #31)
> David,
> 
> So we are assuming that LSK and RSK won't change for a particular view. May
> be UX designs needs to be changed accordingly.

No, you can't assume that. I believe that most views will not change LSK and RSK. But some certainly will. We've been discussing an example where the Connect command on the RSK is sometimes enabled and sometimes disabled. That is one kind of change.

There are probably other times where the soft key label and soft key action change as well. App authors should be able to do that by changing the label in a button, or by removing the accesskey attribute on one button and setting it on another button. The solution you implement will have to be able to handle this kind of dynamic change to the DOM. And that is why I've said that I expect you'll have to use a MutationObserver as part of your implementation.
Flags: needinfo?(Prashant.Goel)
(In reply to Morpheus Chen [:morpheus]  UX from comment #30)
> 
> So far, we've defined a basic rule for input field, you can refer to Input
> Area at p. 13 in Guideline. However, since Litehouse hasn't received any
> confirmation on T9 support, carrier/OEM requirements, now they just create
> the basic mechanism and ask for confirmation. (please refer to
> http://r1l8an.axshare.com/#p=5_4_entry_mode) Have we confirmed anything
> related to T9 support and entry mode? Once Litehouse get confirmation, they
> will update the UX specs, as well as dedicated Back and Clear key.

Morpheus,

I think what is in 5.4 is terrible. First of all, the shift key should just shift, not display a popup of options. Look how many button presses are required to switch between letters and numbers! Putting the shift key on LSK means that there is no way to cancel an operation.  In 5.2, for example, once you start composing a new message, the LSK is used for shifting and there is no way to select Back or Cancel

And isn't it basically a de-facto standard for flip phone input that the * key shifts between input modes? I'm surprised we're considering anything else here. 

About T9: I'm pretty sure that our PRD requires T9-like text input. Whether or not OEMs decide to license the real T9 engine or just use our reference implementation is up to them. But there is presumably going to be some kind of real T9 or fake T9 engine on every Red Square device. So we need UX for how to present the output of that T9 engine to the user. Mostly this is about how to handle it when there is more than one word or prefix that matches the user's input.

Tony: Morpheus cleared the needinfo I'd set for you, so I'm resetting it. See comment #26 for my Litehouse contract question.
Flags: needinfo?(mochen)
Flags: needinfo?(aappleton)

Comment 34

3 years ago
(In reply to David Flanagan [:djf] from comment #32)
> (In reply to Prashant Goel from comment #31)
> > David,
> > 
> > So we are assuming that LSK and RSK won't change for a particular view. May
> > be UX designs needs to be changed accordingly.
> 
> No, you can't assume that. I believe that most views will not change LSK and
> RSK. But some certainly will. We've been discussing an example where the
> Connect command on the RSK is sometimes enabled and sometimes disabled. That
> is one kind of change.
> 
> There are probably other times where the soft key label and soft key action
> change as well. App authors should be able to do that by changing the label
> in a button, or by removing the accesskey attribute on one button and
> setting it on another button. The solution you implement will have to be
> able to handle this kind of dynamic change to the DOM. And that is why I've
> said that I expect you'll have to use a MutationObserver as part of your
> implementation.

Yes, that is correct. LSK and RSK might change, but not everytime.

MutationObserver will probably work in our case.
Flags: needinfo?(Prashant.Goel)
Hi David,

Thanks for your concerns, I want to take this opportunity to clarify things out.
Please find my comments inline.


(In reply to David Flanagan [:djf] from comment #33)
> (In reply to Morpheus Chen [:morpheus]  UX from comment #30)
> > 
> > So far, we've defined a basic rule for input field, you can refer to Input
> > Area at p. 13 in Guideline. However, since Litehouse hasn't received any
> > confirmation on T9 support, carrier/OEM requirements, now they just create
> > the basic mechanism and ask for confirmation. (please refer to
> > http://r1l8an.axshare.com/#p=5_4_entry_mode) Have we confirmed anything
> > related to T9 support and entry mode? Once Litehouse get confirmation, they
> > will update the UX specs, as well as dedicated Back and Clear key.
> 
> Morpheus,
> 
> I think what is in 5.4 is terrible. First of all, the shift key should just
> shift, not display a popup of options. Look how many button presses are
> required to switch between letters and numbers!

Definitely we all want to skip steps as much as possible to provide a quicker way by hot keys, however, when it comes to the target users, everything is different. It's necessary to give prominent terms for them to find, even though we already have  "*" or "#" key to switch between different input mode. (I will ask Litehouse to add the description in the UX specs.)

> Putting the shift key on LSK
> means that there is no way to cancel an operation.  In 5.2, for example,
> once you start composing a new message, the LSK is used for shifting and
> there is no way to select Back or Cancel
> 

Generally speaking, the most frequently used features we need to provide for input field includes Input mode switch, Back/Cancel and Delete, then other actions will be in Options. However, the most complicated one is Input mode switch. Basically, there will be T9 support, letters, numbers, symbols and other languages(If supports), and user can also switch between capitals and lower case for T9 support and letters. So, only using "*" key to switch between different input mode will be too much steps, and it will be too complicated to the target users. That's why we make the Input mode switch as LSK, and leave Back/Cancel and Delete to dedicated Back and Delete key. In order to make it consistent across all apps, Options occupies RSK.
        
 
> And isn't it basically a de-facto standard for flip phone input that the *
> key shifts between input modes? I'm surprised we're considering anything
> else here. 
> 

Just like what I mentioned above, we will provide this shortcut, but we need a visual prominent hint for the target users. And for your information, almost all feature phones provide the shortcut, but the mechanism sometimes is different, for example, some will trigger a dialog for user to choose, and some will need # key to switch between different languages. 

> About T9: I'm pretty sure that our PRD requires T9-like text input. Whether
> or not OEMs decide to license the real T9 engine or just use our reference
> implementation is up to them. But there is presumably going to be some kind
> of real T9 or fake T9 engine on every Red Square device. So we need UX for
> how to present the output of that T9 engine to the user. Mostly this is
> about how to handle it when there is more than one word or prefix that
> matches the user's input.

We did consider this, but it still depends on the final confirmation of T9 support. However, in Phase 2 delivery schedule which Jonathan provided, Predictive text is an item on the schedule, and it will be defined by the end of Oct 19. Litehouse will try to give a clearer definition on input mode.
> 
> Tony: Morpheus cleared the needinfo I'd set for you, so I'm resetting it.
> See comment #26 for my Litehouse contract question.
Flags: needinfo?(mochen)

Updated

3 years ago
Flags: needinfo?(aappleton)
(In reply to David Flanagan [:djf] from comment #26)

> I don't know if the attached wireframe example (sample.png) is from the work
> that Litehouse has done for us or if it is an example from some other
> flip-phone design. In either case, though, it brings up important UX
> questions that we need to address.

The sample.png is taken from Lighthouse's UX Design for Red Square. See link following
http://r1l8an.axshare.com/#p=6_1_3_2_available_networks
Password = ClickUI4

> Tony: does our contract with Litehouse have them doing UX design for
> platform-level features? We need UX for things like what happens when a
> checkbox has the focus. 

Looking at the Lighthouse work for Setting, it would appear they have already been doing this type of design e.g. check boxes. Refer to 6.1.3.2.8 & 6.1.3.2.9 as examples.


> I assume that we want the Ok button to toggle it,
> but do we want the work "Mark" or "Unmark" to appear above the Ok button as
> shown in the wireframes? 

If we do not want to use the terms "Mark" and "Unmark", this to be raised as a feedback comment to Morpheus.

> Similarly, what should happen if the user navigates
> to a button element in web content? Do we display the word "Click" above the
> Ok key? And perhaps most importantly, we need a spec for how text input is
> going to work. The attached wireframe seems to imply that the Ok button will
> be used as a shift key (shifting between numeric input and alphabetic input)
> but this seems like a radical departure from flip phone input which normally
> uses * as the shift key, I think.  And I suspect that we need to have the Ok
> button free for selecting among word choices for T9 input.  Do we have UX
> design work going on for how T9 input is going to work? For example, when T9
> needs to display a list of possible words, will that list appear in the same
> part of the screen that displays soft key labels (which might hide things
> like the Cancel option) or will it display the words in some other way?
(In reply to Tony Appleton from comment #36)
> (In reply to David Flanagan [:djf] from comment #26)
> 
> > I don't know if the attached wireframe example (sample.png) is from the work
> > that Litehouse has done for us or if it is an example from some other
> > flip-phone design. In either case, though, it brings up important UX
> > questions that we need to address.
> 
> The sample.png is taken from Lighthouse's UX Design for Red Square. See link
> following
> http://r1l8an.axshare.com/#p=6_1_3_2_available_networks
> Password = ClickUI4
> 
> > Tony: does our contract with Litehouse have them doing UX design for
> > platform-level features? We need UX for things like what happens when a
> > checkbox has the focus. 
> 
> Looking at the Lighthouse work for Setting, it would appear they have
> already been doing this type of design e.g. check boxes. Refer to 6.1.3.2.8
> & 6.1.3.2.9 as examples.
> 
> 

You can refer to the guideline which Mozilla provided for any detail related to Building blocks.
https://mozilla.box.com/shared/static/nwb39uotutu59yn9mnii08e0cdssfmy9.pdf

> > I assume that we want the Ok button to toggle it,
> > but do we want the work "Mark" or "Unmark" to appear above the Ok button as
> > shown in the wireframes? 
> 
> If we do not want to use the terms "Mark" and "Unmark", this to be raised as
> a feedback comment to Morpheus.
> 

Please refer to p. 20 for the rule for terms on soft key bar and take target users into consideration whether we need to show "Mark" for them or not. 

> > Similarly, what should happen if the user navigates
> > to a button element in web content? Do we display the word "Click" above the
> > Ok key? And perhaps most importantly, we need a spec for how text input is
> > going to work. The attached wireframe seems to imply that the Ok button will
> > be used as a shift key (shifting between numeric input and alphabetic input)
> > but this seems like a radical departure from flip phone input which normally
> > uses * as the shift key, I think.  And I suspect that we need to have the Ok
> > button free for selecting among word choices for T9 input.  Do we have UX
> > design work going on for how T9 input is going to work? For example, when T9
> > needs to display a list of possible words, will that list appear in the same
> > part of the screen that displays soft key labels (which might hide things
> > like the Cancel option) or will it display the words in some other way?

Comment 38

3 years ago
Created attachment 8674254 [details]
sk_key_utils.zip

David,

There was some problem in pushing the code to RED-SQUARE github repo. So I am attaching the files here.

This is just a prototype. Please let us know if we are on the right track. There are some TODO's. Please clarify those.
Flags: needinfo?(dflanagan)

Comment 39

3 years ago
David,

Sorry, forgot to add the usage part. An app developer has to write something like this in his HTML file:-

    <div role="softkey">
        <button type="button" accesskey="a">Back</button>
        <button type="button" accesskey="s">OK</button>
        <button type="button" accesskey="d" contextmenu="menu1">Options</button>
        <menu id="menu1" header="Header Text">
            <menuitem data-menuid="option1" label="Option1" icon="img/icons/icon48x48.png"></menuitem>
            <menuitem data-menuid="option2" label="Option2" icon="img/icons/icon48x48.png"></menuitem>
            <menuitem data-menuid="option3" label="Option3" icon="img/icons/icon48x48.png"></menuitem>
            <menuitem data-menuid="option4" label="Option4" icon="img/icons/icon48x48.png"></menuitem>
        </menu>
    </div>

Comment 40

3 years ago
David,

Added the pull request

https://github.com/harman-red-square/gaia/pulls

Comment 41

3 years ago
(In reply to Prashant Goel from comment #40)
> David,
> 
> Added the pull request
> 
> https://github.com/harman-red-square/gaia/pulls

Sorry, updated pull request

https://github.com/harman-red-square/gaia/pull/5
Created attachment 8676477 [details] [review]
link to patch on github
Flags: needinfo?(dflanagan)
Attachment #8676477 - Flags: feedback?(dflanagan)
Prashant: I've pasted your pull request URL into an attachment, and have set the feedback? flag on it for myself.  In the future, this is the way to take github pull requests and ask for feedback or reviews on bugzilla

I've gotten behind on my emails and reviews, but will try to look at this code soon.
Prashant,

I've left lots of comments on github. Overall, it looks like you're on a good track here. But there are some significant architectural matters that I've tried to clear up in my comments. Thanks.
Flags: needinfo?(Prashant.Goel)

Updated

3 years ago
Attachment #8676477 - Flags: feedback?(dflanagan)

Comment 45

3 years ago
Created attachment 8679522 [details] [review]
Github pull request

Please review
Flags: needinfo?(Prashant.Goel)
Attachment #8679522 - Flags: review?(dflanagan)

Updated

3 years ago
Flags: needinfo?(Prince.Lamba)

Updated

3 years ago
Flags: needinfo?(Prince.Lamba)

Comment 46

3 years ago
(In reply to Comment #35)

Are we deciding that input method switch can also be achieved via LSK ? If yes, then can we achieve same with current softkey architecture which is under review?
Flags: needinfo?(dflanagan)
Flags: needinfo?(aappleton)

Updated

3 years ago
Flags: needinfo?(aappleton) → needinfo?(mochen)
(In reply to Ashish Garg from comment #46)
> (In reply to Comment #35)
> 
> Are we deciding that input method switch can also be achieved via LSK ? 

Yes, in order to provide a prominent clue for target users, we decided to have input method switch on LSK, but user can also use Star or Pound key as hot key to switch between capitals/lower case letter, and input mode. Please refer to the link below for details.
http://r1l8an.axshare.com/#p=9_0_predictive_text_entry&fn=0

> If yes, then can we achieve same with current softkey architecture which is
> under review?

If this question is related to UX, could you please elaborate it?
Flags: needinfo?(mochen)

Updated

3 years ago
Whiteboard: [red square][ETA = 11/6]

Comment 48

3 years ago
Hello David,

Acc. to Morpheus Chen, whenever an input field gets the focus, LSK will switch between capitals/lower case letter. Our softkey library will detect this (a button added with a particular accesskey) and change the LSK accordingly.

We have a building block for input fields. This building block adds a button with a particular access key whenever it gets the focus and if focus is removed, it will remove the button.

Suppose there is a LSK key in a particular view and user navigates up/down, thus bringing focus to an input field, so this LSK key will be replaced by the LSK key of that input field. Now if input field's LSK is removed (when focus is removed), should an individual APP developer needs to put back that old LSK again? I think that will be an overhead for the APP developer to detect that change. What you say?
Hi Prashant,

Just to clarify for your first sentence, LSK will switch between not only capitals and lower case letter, but also Symbols and T9 Abc. Hope there is no misunderstanding.

Comment 50

3 years ago
(In reply to Morpheus Chen [:morpheus]  UX from comment #49)
> Hi Prashant,
> 
> Just to clarify for your first sentence, LSK will switch between not only
> capitals and lower case letter, but also Symbols and T9 Abc. Hope there is
> no misunderstanding.

Right. That is correct.

Comment 51

3 years ago
Hello David,

Let me summarize everything here:-


First of all the SoftKey usage part:-

<button type="button" accesskey="F1">Back</button>
<button type="button" accesskey="Enter">OK</button>

<button type="button" accesskey="F2" contextmenu="menu1">Options</button>

<menu id="menu1" header="Header Text">

  <menuitem label="Option1" icon="img/icons/icon48x48.png"></menuitem>
  <menuitem label="Option2" icon="img/icons/icon48x48.png"></menuitem>
  <menuitem label="Option3" icon="img/icons/icon48x48.png"></menuitem>
  <menuitem label="Option4" icon="img/icons/icon48x48.png"></menuitem>

</menu>

- First three are softkey buttons with an accesskey which will trigger the click event on that button. Don't change accesskey attribute's value now. You need to attach your own click eventlistener for handling click events.

- If you want to display a Menu, add a contextmenu attribute pointing to that menu element. No need to attach click eventlistener for this (ie <button type="button" accesskey="F2" contextmenu="menu1">Options</button>).



Next, the issue with input fields when they receive the focus and show the menus accordingly:-

Acc. to Morpheus Chen, whenever an input field gets the focus, LSK will switch between capitals/lower case letter/symbols/T9. Our softkey library will detect this (a button added with a particular accesskey) and change the LSK accordingly.

We have a building block for input fields. This building block adds a button with a particular access key whenever it gets the focus and if focus is removed, it will remove the button.

Suppose there is a LSK key in a particular view and user navigates up/down, thus bringing focus to an input field, so this LSK key will be replaced by the LSK key of that input field. Now if input field's LSK is removed (when focus is removed), should an individual APP developer needs to put back that old LSK again? I think that will be an overhead for the APP developer to detect that change. Or softkey library needs to store the value before focus moved to input field (which is wrong acc. to me) so that it can be display after wards (when focus is removed).


Lastly, our assumption for cancel key:- 

Right now we are assuming cancel button (in case options menu is open) to be LSK with text "Cancel" (in English), there might be some scenarios where cancel won't be LSK (can be RSK also).

So, Please share your thoughts on this. No need to check our earlier comments.
Prashant,

Sorry I haven't been able to be more responsive in this bug. I'm tied up this week with blocking bugs for our 2.5 smartphone release.

I think you should ignore the keyboard input issue for now. First off, I think that using LSK as a shift key is a bad idea and likely won't work well. It seems too modal to me.  Second, text input is going to have to be handled at the gecko and system app level. So if we do use LSK as a shift key, it is going to mean that the system app is intercepting the events and is displaying a new text input bar over top of the app's softkey bar. It seems to me that the best approach is to implement soft keys without considering text input, and then implement text input in the system app to override soft keys as needed.

For the option menu case, we already have building blocks for displaying option menus with cancel buttons. And I think we even have an automated way in gecko to display contextmenus.  So if the RSK says Options and the user presses the RSK key, then we should either have gecko display the context menu (if we can figure out how to make it do that) or we should, at the gaia level, use the existing option menu building block to display the options. If we use gecko to display the context menu, then we'll have to port the gecko code so that it properly displays a cancel option on one of the softkeys. (If that is in the system app, it could do this by layering a new softkey bar over the app's bar.) And if we use the existing Gaia building block, then that building block will have a cancel button, and we'll have to modify the building block to add an accesskey attribute to that cancel button so that the right thing happens.

The part of all this that seems the trickiest to me is that when there is more than one button with the same accesskey, we've got to figure out which one is the currently visible one and switch the soft button labels when the visibility changes. That's the part that I'm worried we will have trouble with.
Flags: needinfo?(dflanagan)

Comment 53

3 years ago
(in reply to comment#52)

We will attempt to modify gaia option menu building block to add accessKey attribute for cancel button and update you. In between we have started using existing SoftKey implementation.

Not able to understand your comments 
"The part of all this that seems the trickiest to me is that when there is more than one button with the same accesskey, we've got to figure out which one is the currently visible one and switch the soft button labels when the visibility changes. That's the part that I'm worried we will have trouble with."

Can you please elaborate in which case its possible to have two button with same accessKey attribute?
Flags: needinfo?(dflanagan)

Comment 54

3 years ago
Created attachment 8691888 [details] [diff] [review]
0001-Bug-1142855-Soft-Key-refactored-and-integrated-with-.patch

The final patchset
Attachment #8676477 - Attachment is obsolete: true
Attachment #8679522 - Attachment is obsolete: true
Attachment #8679522 - Flags: review?(dflanagan)

Updated

2 years ago
Flags: needinfo?(dflanagan)

Comment 55

3 months ago
Firefox OS is not being worked on
Status: NEW → RESOLVED
Last Resolved: 3 months ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.