757496 - Support mozKeyboard.setSelectedOption[s]

Status: RESOLVED FIXED

(Firefox OS Graveyard :: General, defect)

Comment 1

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: Patch v0.1

The patch add support for the mozKeyboard.setSelectedOption[s] proposal.

Comment 2

[[:fabrice] Fabrice Desré]

Comment on attachment 626065 [details] [diff] [review]
Patch v0.1

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

Overall looks ok, but I'd like to have a second look.

::: b2g/chrome/content/forms.js
@@ +3,5 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this file,
> + * You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +

general comment: you are using single quotes everywhere. I'd prefer double quotes.

@@ +13,5 @@
> +let HTMLSelectElement = Ci.nsIDOMHTMLSelectElement;
> +let HTMLOptGroupElement = Ci.nsIDOMHTMLOptGroupElement;
> +let HTMLOptionElement = Ci.nsIDOMHTMLOptionElement;
> +
> +var FormAssistant = {

nit: s/var/let

@@ +32,5 @@
> +        }
> +        break;
> +      }
> +
> +      case 'blur': {

there's no addEventListener() for "blur". There's one for "click" but you don't manage it anywhere.

@@ +51,5 @@
> +
> +  receiveMessage: function fa_receiveMessage(msg) {
> +    let json = msg.json;
> +    switch (msg.name) {
> +      case 'Forms:Select:Choice':

do you plan to have more messages in the future? If yes I'm ok with the switch(), if not just remove it.

@@ +122,5 @@
> +        'inGroup': false,
> +        'text': child.text,
> +        'disabled': child.disabled,
> +        'selected': child.selected,
> +        'optionIndex': optionIndex++

I don't think you need the quotes for the properties at all. This is a JS object, not a json string

::: b2g/chrome/content/shell.xul
@@ +20,1 @@
>    <script type="application/javascript" src="chrome://browser/content/shell.js"/>

?? this is not in the current code

::: b2g/components/MozKeyboard.js
@@ +57,5 @@
> +  uninit: function mozKeyboardUninit() {
> +    Services.obs.removeObserver(this, "inner-window-destroyed");
> +    this._window = null;
> +    this._utils = null;
> +    this._focusHandler = null;

you also have to remove the message listener

@@ +69,5 @@
> +  },
> +
> +  setSelectedOption: function mozKeyboardSetSelectedOption(index) {
> +    messageManager.sendAsyncMessage('Forms:Select:Choice', {
> +      'index': index

nit: double quotes

@@ +75,5 @@
> +  },
> +
> +  setSelectedOptions: function mozKeyboardSetSelectedOptions(indexes) {
> +    messageManager.sendAsyncMessage('Forms:Select:Choice', {
> +      'indexes': indexes || []

nit: double quotes

@@ +99,5 @@
> +            'data': json
> +          };
> +
> +          let event = this._window.document.createEvent('CustomEvent');
> +          event.initCustomEvent('focuschanged', true, false, detail);

use the constructor for the event, like in https://mxr.mozilla.org/mozilla-central/source/dom/base/BrowserElementParent.js#139

::: b2g/components/b2g.idl
@@ +18,5 @@
> +  void sendKey(in long keyCode, in long charCode);
> +
> +  void setSelectedOption(in jsval index);
> +  void setSelectedOptions(in jsval indexes);
> +

can you document these? I have no idea what it's for.

@@ +26,5 @@
>  [scriptable, uuid(0f2abcca-b483-4539-a3e8-345707f75c44)]
>  interface nsITCPSocketEvent : nsIDOMEvent {
>   readonly attribute DOMString data;
>  };
>  

?? this is not in the current code

Comment 3

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

(In reply to Fabrice Desré [:fabrice] from comment #2)
> > +      }
> > +
> > +      case 'blur': {
> 
> there's no addEventListener() for "blur". There's one for "click" but you
> don't manage it anywhere.

shame! 

 
> @@ +51,5 @@
> > +
> > +  receiveMessage: function fa_receiveMessage(msg) {
> > +    let json = msg.json;
> > +    switch (msg.name) {
> > +      case 'Forms:Select:Choice':
> 
> do you plan to have more messages in the future? If yes I'm ok with the
> switch(), if not just remove it.


Yep. I plan to add mozKeyboard.setValue (for date, etc..)

> 
> @@ +122,5 @@
> > +        'inGroup': false,
> > +        'text': child.text,
> > +        'disabled': child.disabled,
> > +        'selected': child.selected,
> > +        'optionIndex': optionIndex++
> 
> I don't think you need the quotes for the properties at all. This is a JS
> object, not a json string

But...but... my text editor will complains! :)

> 
> ::: b2g/chrome/content/shell.xul
> @@ +20,1 @@
> >    <script type="application/javascript" src="chrome://browser/content/shell.js"/>
> 
> ?? this is not in the current code


I have a few patches applied to my tree to test things ;)


I will addressed the other comments in a new patch, thanks!

Comment 4

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: Patch v0.2

Comment 5

[[:fabrice] Fabrice Desré]

Comment on attachment 627224 [details] [diff] [review]
Patch v0.2

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

We're almost there, just need to clarify the messages exchanges between forms.js and MozKeyboard.js

::: b2g/chrome/content/forms.js
@@ +113,5 @@
> +    "choices": []
> +  };
> +
> +  // Build up a flat JSON array of the choices.
> +  // In HTML, it"s possible for select element choices to be under a

nit: it's

::: b2g/components/MozKeyboard.js
@@ +87,5 @@
> +  get onfocuschange() {
> +    return this._focusHandler;
> +  },
> +
> +  receiveMessage: function mozKeyboardReceiveMessage(msg) {

Here you're using the same code path for Forms:Select and Forms:Blur
That is consistent with forms.js but why having two messages? Either merge them or make clear that you don't need the "type" for the blur one

@@ +113,5 @@
> +        }
> +        break;
> +    }
> +
> +    handler.handleEvent(this._window.CustomEvent("focuschanged", detail));

|new this._window.CustomEvent()| ?

::: b2g/components/b2g.idl
@@ +18,5 @@
> +  void sendKey(in long keyCode, in long charCode);
> +
> +  // Select the <select> option specified by index.
> +  // If this method is called on a <select> that support multiple
> +  // selection, then a the option specified by index will be added to

Nit: "then the option ..."

Comment 6

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

Attachment: Patch v0.3

The patch use a single Forms:Input message from forms.js to the keyboard component and forward the json object sent by forms.js. Tweaking the fields does not required to change MozKeyboard.js anymore.

Comment 7

[[:fabrice] Fabrice Desré]

Comment on attachment 627737 [details] [diff] [review]
Patch v0.3

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

You changed many arguments to be |xxx| instead of |aXxx|. That's not in line with the usual coding guidelines, and that would be nice to fix that before landing.

::: b2g/components/b2g.idl
@@ +4,5 @@
>  
>  #include "domstubs.idl"
>  #include "nsIDOMEvent.idl"
>  
>  [scriptable, uuid(3615a616-571d-4194-bf54-ccf546067b14)]

you need to change the uuid

Comment 8

[Vivien Nicolas (:vingtetun) (:21) - (NOT reading bugmails, needinfo? please)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/2e509c91883a

Comment 9

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

https://hg.mozilla.org/mozilla-central/rev/2e509c91883a