Essentially the following files should be moved and componentized: shared/js/sim_picker.js -> elements/sim_picker/script.js shared/js/elements/sim_picker.html -> also to script.js, just until we start using HTML imports or have a better templating solution. shared/locales/sim_picker/sim_picker.en-US.properties -> shared/elements/sim_picker/locales/sim_picker.en-US.properties

Doug - I noticed that you did the original sim-picker and since this would be a really good candidate for a web component. I wanted to check with you to see if it was something you'd be interested in working with us on for 2.1. If you have time to port this that would be awesome. If you don't have time, but have time for some reviews, that would also be great. I suppose it is most similar to the gaia-menu component[1]. We could probably leverage gaia-menu directly, or extend it in some way. Let me know what you think. [1] https://github.com/mozilla-b2g/gaia/blob/master/shared/elements/gaia_menu/script.js

Sure, I can help with and review this. I don't know much about making shadow DOM elements so I'd probably need help there. Have you thought at all about the design? Right now, it's pretty tightly integrated into the JS of whatever is using it. I think we'd have to move a lot of these things to attrs and DOM events. I'm not sure how we'd replace the getOrPick function, though. Do you have any example code which is similar (gaia-menu doesn't seem to be)?

Hmm, the gaia-menu is probably the closest thing we currently have I imagine. I suppose a lot of this would be the first time we're trying to do this from a component, and I'm only vaguely familiar with the functionality of the sim picker. Looking briefly at the getOrPick, it would be no problem to surface this through a custom API on the component if we wanted, or we could do something more complex and have a type of data layer though I'm not sure if the extra complexity is necessary. For now I think that moving it into an API on the component would be ok? Also maybe the component could simply fire a custom event when an option is picked, or if there is only one option.

Yeah, that seems reasonable. I'm not sure how to expose a custom API from a component like you're talking about though, so an example of that would be helpful. If this is indeed the first time we're doing something like this, then even a rough outline or thought process would be helpful too. I'll assign this to myself for now and get to it when I can (probably next sprint).

(In reply to Doug Sherk (:drs) from comment #4) > Yeah, that seems reasonable. I'm not sure how to expose a custom API from a > component like you're talking about though, so an example of that would be > helpful. We're still figuring out patterns, so nothing set in stone, but here's a few examples of custom APIs on components: https://github.com/mozilla-b2g/gaia/blob/master/shared/elements/gaia_grid/script.js#L52 https://github.com/mozilla-b2g/gaia/blob/master/shared/elements/gaia_menu/script.js#L37 The confirm component also fires a custom event on the element, which is what I assume we would want to do for the sim-picker. Maybe some`sim-picker-select` event would be fired when the user selects a sim? https://github.com/mozilla-b2g/gaia/blob/master/shared/elements/gaia_confirm/script.js#L63 Thanks for taking this up, the more people that experiment with web components the better :)

Okay, thanks. I think I have a reasonable idea of where to go with this now.

We've been keeping notes on this and my feedback on WC so far here: https://etherpad.mozilla.org/web-components

WIP PR: https://github.com/mozilla-b2g/gaia/pull/22129 I'm stuck at this point with the shadow DOM not appearing when I call ComponentUtils.style.call(). For some reason, without this call, it works. I can also move the shadow DOM to the same level as the shadow host and it appears. Kevin, you said you'd take a look into this, so I'm needinfoing you. Thanks for your help so far.

Hey Doug - I'll take a look soon. I'm currently traveling with only 1 sim - is there a way to view this on a device or desktop with only 1 sim? I think if we can implement a small html test page to load in the browser it might make it easy to debug - but it looks like we would need to add some shims/mocks for the mobile APIs. I also left a note about localization files, looking good so far. Thanks!

Ok, I'll make a reduced test case and give you a patch to get it working with 1 SIM.

The PR now has an example which also has this problem. I included a few lines you can use to verify the problem: https://github.com/mozilla-b2g/gaia/pull/22129/files#diff-b11c46e499ed8813cee5a027fa257ae7R30

(In reply to Doug Sherk (:drs) from comment #11) > The PR now has an example which also has this problem. I included a few > lines you can use to verify the problem: > https://github.com/mozilla-b2g/gaia/pull/22129/files#diff- > b11c46e499ed8813cee5a027fa257ae7R30 I took a look at the example and noticed that the <content> seemed a bit weird as we aren't using it as an insertion point. Removing this seems to get content to show up for me. Left a comment on github as well.

This seems to work pretty well. I left one comment for a styling problem that I need to come back to. I also need to write/port tests.

I also updated the PR: https://github.com/mozilla-b2g/gaia/pull/22129

Comment on attachment 8464263 [details] [diff] [review] Create gaia-sim-picker component Review of attachment 8464263 [details] [diff] [review]: ----------------------------------------------------------------- ::: shared/elements/gaia_sim_picker/examples/example.js @@ +9,5 @@ > + for (var i = 0; i < showSimPickerButtons.length; i++) { > + var showSimPickerButton = showSimPickerButtons[i]; > + showSimPickerButton.addEventListener('click', function(e) { > + simPicker.getOrPick(e.target.dataset.defaultCardIndex, '12345', > + function(cardIndex) { I think you should do whatever will make the transition easier, but what do you think about surfacing this as an event on the sim picker element? E.g, var simPicker = document.getElementById('sim-picker'); simPicker.addEventListener('pick', onPick); ::: shared/elements/gaia_sim_picker/script.js @@ +49,5 @@ > + } > + > + var dialViaElt = this.shadowRoot.querySelector('#sim-picker-dial-via'); > + if (phoneNumber) { > + navigator.mozL10n.setAttributes(dialViaElt, I think the only difference is that this also sets the l10n-id and l10n-args properties? Personally I'd like to remove the hard mozL10n dependency if possible, but we can always do this later if that's easier. If we did want to remove it, I guess we would just call setAttribute() for both data-l10n-id and data-l10n-args.

Oops - forgot an "Overall" comment there :) Very happy with this, thanks for experimenting. I left a few comments, but I couldn't find anything serious that would prevent this from landing as-is. Thanks for taking care of this!

PR: https://github.com/mozilla-b2g/gaia/pull/22129 bug 1045820 is getting rapidly out of control and has turned into a port of every app, so it'll come a bit later.

(In reply to Kevin Grandon :kgrandon from comment #15) > I think the only difference is that this also sets the l10n-id and l10n-args > properties? Personally I'd like to remove the hard mozL10n dependency if > possible, but we can always do this later if that's easier. If we did want > to remove it, I guess we would just call setAttribute() for both > data-l10n-id and data-l10n-args. I forgot to mention that I disagree with this. We still have other dependencies on l10n, such as the ready() and once() functions. I also could easily imagine other WC needing these stubs.

Awesome, no harm in landing this now and iterating when needed while you start porting apps. Thanks for putting this together, I'll start reviewing soon. (In reply to Doug Sherk (:drs) from comment #18) > I forgot to mention that I disagree with this. We still have other > dependencies on l10n, such as the ready() and once() functions. I also could > easily imagine other WC needing these stubs. Gotcha. The l10n + components land is changing frequently, so I'm not sure what will be best down the road. Totally fine with this for now.

Comment on attachment 8464935 [details] [diff] [review] Create gaia-sim-picker component. Review of attachment 8464935 [details] [diff] [review]: ----------------------------------------------------------------- Overall I'm pretty happy with this :) I think I would like to let Anothony take a look, and maybe some of my comments addressed before leaving an R+. Thanks for knocking out, I think we're close here. ::: apps/communications/dialer/test/unit/gaia_sim_picker_test.js @@ +1,1 @@ > +/* globals MocksHelper, MockNavigatorMozIccManager, MockL10n, I'm not too familiar with the sim picker, is the only logic to test difference when we have and don't have TelephonyHelper? ::: shared/elements/gaia_sim_picker/locales/gaia_sim_picker.en-US.properties @@ +1,2 @@ > +# SIM list > +sim-picker-dial-via-with-number=Dial {{ phoneNumber }} via Would you be ok with going with new keys here? We currently prefix all of our component strings with the component name itself, potentially to avoid collisions with apps, and I think it would be nice to keep it consistent. The only change here would be to prefix these with 'gaia-'. E.g., gaia-sim-picker-dial-via-with-number https://github.com/mozilla-b2g/gaia/blob/master/shared/elements/gaia_grid/locales/gaia_grid.en-US.properties If you are ok with this, I'll add it the web component README.md which I'm putting together now. ::: shared/elements/gaia_sim_picker/script.js @@ +30,5 @@ > + }; > + > + proto._domBuilt = false; > + > + proto.localize = function() { Seems wrong to have an empty function here. Do we need to do any localization here? If not, let's remove this. ::: shared/elements/gaia_sim_picker/style.css @@ +1,3 @@ > +.sim-default { > + display: none; > + margin-left: 2px; Should we use rem here?

Comment on attachment 8464935 [details] [diff] [review] Create gaia-sim-picker component. Removing review to clear my queue. Ask again when Kevin's comments are addressed.

Thanks for the review. (In reply to Kevin Grandon :kgrandon from comment #20) > I'm not too familiar with the sim picker, is the only logic to test > difference when we have and don't have TelephonyHelper? Yes. TelephonyHelper is specific to the dialer, though. Maybe we could move mock_telephony_helper.js to shared/test/unit/mocks/dialer/ I had to make some additional changes here to support bug 1045820. Unfortunately I squashed them, but I think the only major thing is the addition of ElementMocksHelper. Updated PR.

Switching to using an event instead of a callback passed into getOrPick has caused problems in the apps using this. For example, this is really ugly: https://github.com/DouglasSherk/gaia/commit/3c5da16a5d2cb12e6a1b9f35eb5608603a04f019#diff-98e96f21adf938206005ba91e4984e69R628 It has also made our testing much more difficult. Maybe we could do both instead.

(In reply to Doug Sherk (:drs) from comment #23) > Switching to using an event instead of a callback passed into getOrPick has > caused problems in the apps using this. For example, this is really ugly: > https://github.com/DouglasSherk/gaia/commit/ > 3c5da16a5d2cb12e6a1b9f35eb5608603a04f019#diff- > 98e96f21adf938206005ba91e4984e69R628 > > It has also made our testing much more difficult. Maybe we could do both > instead. Ok, thanks for experimenting. If you feel a callback is the way to go, that works for me. I guess I was just thinking that you'd *always* have the event listener active and listening, but maybe that isn't too nice either.

Comment on attachment 8465963 [details] [diff] [review] Create gaia-sim-picker component. Review of attachment 8465963 [details] [diff] [review]: ----------------------------------------------------------------- I think this is a great start. We will undoubtedly need some API changes and react to changes in our component strategy - but we will tackle those when we get to them. Nice work! ::: shared/test/unit/mocks/elements/gaia_sim_picker/mock_gaia_sim_picker.js @@ +1,1 @@ > +/* exported MockGaiaSimPicker */ Is this file currently loaded anywhere? I couldn't find a reference to it, but maybe I missed it. My preference would be to still load the real custom element for tests, but I guess not everyone is a fan of that approach. In any case, if it's not referenced, should we skip landing it?

(In reply to Kevin Grandon :kgrandon from comment #24) > Ok, thanks for experimenting. If you feel a callback is the way to go, that > works for me. I guess I was just thinking that you'd *always* have the event > listener active and listening, but maybe that isn't too nice either. I want to fix this first before we land it. It'll make bug 1045280 a bit easier. We can land this after I've done that. (In reply to Kevin Grandon :kgrandon from comment #25) > ::: shared/test/unit/mocks/elements/gaia_sim_picker/mock_gaia_sim_picker.js > @@ +1,1 @@ > > +/* exported MockGaiaSimPicker */ > > Is this file currently loaded anywhere? I couldn't find a reference to it, > but maybe I missed it. My preference would be to still load the real custom > element for tests, but I guess not everyone is a fan of that approach. > > In any case, if it's not referenced, should we skip landing it? Yeah, it's used in bug 1045280. It's loaded by ElementMocksHelper in a couple of tests. It's not used directly.

Kevin, I could use your help in a couple of spots. We can chat on IRC too if that's better for you. These are the last things I have to deal with before this will be completely done, including bug 1045280 (other than review). PR: https://github.com/mozilla-b2g/gaia/pull/22386 1) The shadow DOM looks roughly like this: <gaia-sim-picker> <gaia-menu> <buttons></buttons> </gaia-menu> </gaia-sim-picker> It seems that the reason the menu is not being formatted correctly without my fix [1] is because the nested gaia-menu element is not actually being initialized. The dependencies are definitely being loaded, because I can add a gaia-menu element here [2] and that works fine. Inspecting the shadow DOM in Chrome confirms this. The <gaia-menu> element has no underlying shadow DOM. Do nested elements not work yet or something? 2) When I show or hide the SIM picker, the first click event after either of these seems to get ignored in content. I immediately thought that this was a focus issue, but after trying to set focus on the element being shown or blurring it when it's hidden, it doesn't seem to be. I removed the preventDefault from the 'contextmenu' event handler on the buttons that show the SIM picker, and that fixed it. But this fix is unacceptable because it triggers clicks inside the SIM picker itself when you lift your finger from the button that shows it. This wasn't a problem in the past. Do you know anything about this? Thanks. [1] https://github.com/mozilla-b2g/gaia/pull/22386/files#diff-7a208b07a721f29598b7d72c5d461cd0R11 [2] https://github.com/mozilla-b2g/gaia/pull/22386/files#diff-98bf553eb11786402e0627509cbd9974R28

(In reply to Doug Sherk (:drs) from comment #27) > 1) The shadow DOM looks roughly like this:... > It seems that the reason the menu is not being formatted correctly without > my fix [1] is because the nested gaia-menu element is not actually being > initialized. The dependencies are definitely being loaded, because I can add > a gaia-menu element here [2] and that works fine. > > Inspecting the shadow DOM in Chrome confirms this. The <gaia-menu> element > has no underlying shadow DOM. Do nested elements not work yet or something? This is strange. I recall the menu working properly before, but opening the example in nightly today seems to have the messed up formatting. Did anything change in your code? If nothing changed it could be a platform bug. If that's the case we should see if we can get a reduced test case and have the platform guys look at it. In either case we should explore some workarounds. One option might be to stop using <gaia-menu> or try surfacing it through a <content select="gaia-menu"> tag instead. I'll take a look and see if there's any workarounds I can easily find. > 2) When I show or hide the SIM picker, the first click event after either of > these seems to get ignored in content. I immediately thought that this was a > focus issue, but after trying to set focus on the element being shown or > blurring it when it's hidden, it doesn't seem to be. I removed the > preventDefault from the 'contextmenu' event handler on the buttons that show > the SIM picker, and that fixed it. But this fix is unacceptable because it > triggers clicks inside the SIM picker itself when you lift your finger from > the button that shows it. This wasn't a problem in the past. Do you know > anything about this? Have we tried using a stopPropagation() instead of preventDefault()?

(In reply to Kevin Grandon :kgrandon from comment #28) > This is strange. I recall the menu working properly before, but opening the > example in nightly today seems to have the messed up formatting. Did > anything change in your code? If nothing changed it could be a platform bug. Oh, I disabled my fix [1] so that you could see the brokenness. But my point here is that the underlying problem is more serious than I originally thought, so my CSS hack isn't a valid fix. [1] https://github.com/mozilla-b2g/gaia/pull/22386/files#diff-7a208b07a721f29598b7d72c5d461cd0R11 > If that's the case we should see if we can get a reduced test case and have > the platform guys look at it. In either case we should explore some > workarounds. One option might be to stop using <gaia-menu> or try surfacing > it through a <content select="gaia-menu"> tag instead. I'll take a look and > see if there's any workarounds I can easily find. I suspect that this isn't a platform bug since it happens in Chrome as well. > Have we tried using a stopPropagation() instead of preventDefault()? I just tried that and stopImmediatePropagation() and neither fixed it, unfortunately. This would be a bad fix anyways since it would depend on call ordering.

Ok, thanks Doug. I think the next steps here are to narrow down a test case to check if this is a platform bug, and what if any workarounds we have available. I'll try to help out this week and investigate.

I wrote a reduced test case for this. It seems that the problem is our template/cloneNode() logic. Here it is broken, using template.cloneNode(): http://people.mozilla.org/~dsherk/shadow-test-broken.html Here it is working, just using shadow.innerHTML = /* ... */: http://people.mozilla.org/~dsherk/shadow-test-working.html

Here the example is in jsfiddle in case we want to play with some live-editing of the example: http://jsfiddle.net/kevining/1b3zmnts/

William - if you have a moment, could you look at this link: http://jsfiddle.net/kevining/1b3zmnts/ We currently have a nested web component that is not getting instantiated for some reason. It does not work in Chrome as well, so I'm wondering if we're doing something crazy in violation of the spec. Could you take a look and see if we're doing something dumb? Thanks!

So it looks like what's happening in the test case is this: - Elements in a template contents are owned by a special document called the "template contents owner" (we do this to try and ensure that template contents stay inert and don't affect the main document). - Per spec, the custom elements registry is empty for the template contents owner. [1] - When you clone the template contents, the cloned nodes are created in the template contents owner document. - During the creation of the cloned elements, we don't have a custom element registration in the template contents owner document, so the created callback does not get called and the custom prototype is not set on the element. [2] - The cloned node then gets adoped into the main document. One of the issues we have here is that we are adopting a custom element from a document that does not have a custom element definition into a document where it does. This scenerio currently isn't worked out yet at the spec level because there are other complications (e.g. what prototype and callbacks do we use when the document we are adopting from and the document we are adopting into both have different custom element defintions?) [1] http://w3c.github.io/webcomponents/spec/custom/#creating-and-passing-registries [2] http://w3c.github.io/webcomponents/spec/custom/index.html#dfn-created-callback

I see. So it seems like there's no way to do this right now using template elements. The only reasonable alternative I can think of is to just set the innerHTML directly, similar to my test case. We can fix this properly when we can load HTML fragments in WC. Does this seem reasonable?

Thanks for digging into this William, that was really insightful! So it seems like we may be able to get around this by using a direct injection into the shadow root, instead of using a template? This might be ok for now, but seems like it might be limiting should we ever decide to use HTML imports. Anyway - I'll follow-up with you about how we want to track this spec work, and look at some workarounds with Doug here. I'll remove you from the CC here as it will go back to typical bug-churn I think, but feel free to add yourself :) Thanks!

(In reply to Doug Sherk (:drs) from comment #35) > I see. So it seems like there's no way to do this right now using template > elements. The only reasonable alternative I can think of is to just set the > innerHTML directly, similar to my test case. We can fix this properly when > we can load HTML fragments in WC. Does this seem reasonable? Yes, it seems reasonable to set innerHTML of the shadowRoot directly. This workaround seems to do the trick for me: http://jsfiddle.net/kevining/atwwjpng/

Updated PR. Ok, here it is with the innerHTML/template workaround. This also fixed the focus issue that I described in comment 27, so this is now completely ready to go. I also have a patch ready for bug 1045280 which I'll post and ask to be reviewed once we've landed this.

Comment on attachment 8472013 [details] [diff] [review] Create gaia-sim-picker component. Review of attachment 8472013 [details] [diff] [review]: ----------------------------------------------------------------- Thanks for doing this. I'm pretty happy with the state of this. Let's get it landed so we can iterate. Thanks! ::: shared/elements/gaia_sim_picker/script.js @@ +114,5 @@ > + // XXX: Remove once bug 1040922 is fixed > + navigator.mozL10n.ready(function() { > + var children = this.shadowRoot.children; > + for (var i = 0; i < children.length; i++) { > + navigator.mozL10n.translateFragment(children[i]); Interesting, does this not work on nested children? @@ +140,5 @@ > + > + var stylesheet = baseurl + '../gaia_menu/style.css'; > + > + // TODO: We should make this a template node and use template.cloneNode, but > + // the nested gaia-menu element doesn't get initialized for some reason when You might want to update this comment to say something along the lines that nested web components in templates do not yet work.

Updated PR. Rebased, with a few small changes to support differences in APIs and bug fixes since this was posted.

I did a first high-level overview of this, will come back to it tomorrow. My first comments: - I don't think we need to separate gaia_sim_picker_test.js in two. This will create confusion as to where put the tests later I believe. (and that solves the duplication of a lot of code) - long-tap-indicator.png is not needed afaict - element_mocks_helper.js could be confusing. CustomElementsHelper? WCElementsHelper? - stubs look like a duplication of mocks which would bitrot quickly I believe. I don't yet have suggestions on how to fix this.

Comment on attachment 8485609 [details] [diff] [review] Create gaia-sim-picker component. Review of attachment 8485609 [details] [diff] [review]: ----------------------------------------------------------------- This is not an r+ because of my previous comments. Otherwise it looks good. ::: shared/elements/gaia_sim_picker/script.js @@ +1,2 @@ > +'use strict'; > +/* global ComponentUtils */ Need to add LazyLoader @@ +31,5 @@ > + > + proto._simSelectedCallbacks = []; > + > + proto._dispatchSimSelected = function(cardIndex) { > + if (this.dispatchEvent(new CustomEvent( We should store an intermediate variable for better reading. ::: shared/elements/gaia_sim_picker/style.css @@ +9,5 @@ > +} > + > +/* FIXME/drs: For some reason, buttons aren't lining up correctly without these > + * styles. I have no idea why. Even with them, they're aligned to the top of the > + * menu instead of the bottom. */ Can we open a bug to fix gaia-menu?

Oh and as I told you on IRC, the example is not working because LazyLoader is missing.

(In reply to Anthony Ricaud (:rik) from comment #42) > ::: shared/elements/gaia_sim_picker/style.css > @@ +9,5 @@ > > +} > > + > > +/* FIXME/drs: For some reason, buttons aren't lining up correctly without these > > + * styles. I have no idea why. Even with them, they're aligned to the top of the > > + * menu instead of the bottom. */ > > Can we open a bug to fix gaia-menu? This was fixed after comment 34. I accidentally squashed the removal of it into bug 1045820. (In reply to Anthony Ricaud (:rik) from comment #43) > Oh and as I told you on IRC, the example is not working because LazyLoader > is missing. Should be fixed now. (In reply to Anthony Ricaud (:rik) from comment #41) > - I don't think we need to separate gaia_sim_picker_test.js in two. This > will create confusion as to where put the tests later I believe. (and that > solves the duplication of a lot of code) Done. > - long-tap-indicator.png is not needed afaict It is. Thanks for mentioning this, though. I missed a spot that this should have been used. > - element_mocks_helper.js could be confusing. CustomElementsHelper? > WCElementsHelper? I went with CustomElementsHelper. > - stubs look like a duplication of mocks which would bitrot quickly I > believe. I don't yet have suggestions on how to fix this. Yeah, agreed. I'm using the shared unit test mocks now. I'm posting an updated patch for bug 1045820 after this.

Comment on attachment 8493471 [details] [diff] [review] Create gaia-sim-picker component. Review of attachment 8493471 [details] [diff] [review]: ----------------------------------------------------------------- Maybe we could add some docs at the top custom_elements_helper.js to help people know how to use it. Doesn't have to be done in this bug though. r- thought because we also need to add gaia_sim_picker.{locale}.properties in all applications that were using it before. Right now, we can't see the SIM 1/SIM 2 label on multi sim action buttons.

Good catch, thanks. I fixed this in bug 1045820 but we will not be landing them together, so that's important. Updated PR.

> gaia-sim-picker-button=SIM{{ n }} Aren't we using a space between "SIM" and "n" across Gaia? Change of plan or mistake?