Fernando Jiménez Moreno [:ferjm] Assignee
	Description • 12 years ago

Latest changes to the |PopupManager| regarding the |window.open| functionality has caused some regression issues ([1], [2], [3]) in the trusted UI. The newest UX for a popup opened via |window.open| meaningfully differs from the one proposed for the trusted UI. After talking to Alive we decided to take the trusted UI logic out of the PopupManager.

[1] https://github.com/mozilla-b2g/gaia/issues/5371
[2] https://github.com/mozilla-b2g/gaia/issues/5374
[3] https://github.com/mozilla-b2g/gaia/issues/5375

	Tim Guan-tin Chien [:timdream] (please needinfo)
	Comment 1 • 12 years ago

This make sense. Is there any visuals available to public as the basis of this decision?

	Fernando Jiménez Moreno [:ferjm] Assignee
	Comment 2 • 12 years ago

(In reply to Tim Guan-tin Chien [:timdream] (MoCo-TPE) from comment #1)
> This make sense. Is there any visuals available to public as the basis of
> this decision?

Nothing final yet. We've got a meeting tomorrow to discuss about the UX and visuals.

This is what we have so far -> https://www.dropbox.com/sh/a4n5vm792dwfmt8/qEOEE8qZbJ

	Fernando Jiménez Moreno [:ferjm] Assignee
	Comment 3 • 12 years ago

This patch takes the trusted UI logic out of the PopupManager and restores the way that the trusted UI worked before the changes regarding the |window.open| implementation (https://github.com/mozilla-b2g/gaia/pull/5020) (this means, a dialog on top of the homescreen, without a 'close' button or address bar and closed by the HOME button press event).

Please, note that this doesn't intend to be a final implementation of the trusted UI! It still shares the same HTML and CSS with the PopupManager, and this probably needs to be changed. I just wanted to unblock the QA team that is testing the nav.mozPay implementation [2] as soon as possible.

Alberto Pastor, from the Gaia team, will take the lead of the front-end work regarding the trusted UI from now on :)

[1] https://github.com/mozilla-b2g/gaia/pull/5020
[2] https://bugzilla.mozilla.org/show_bug.cgi?id=776417#c13

	Fernando Jiménez Moreno [:ferjm] Assignee
	Comment 4 • 12 years ago

I've added a few navigator.mozPay tests to the UITests app that can be used to test the trusted UI.

	Fernando Jiménez Moreno [:ferjm] Assignee
	Comment 5 • 12 years ago

BTW, the tests requires this https://github.com/mozilla-b2g/gaia/pull/5083 to be landed. It would be great if someone (Tim, Alive, Alberto?) could take a look at it and merge it if possible :)

	Andrew Overholt [:overholt]
	Comment 6 • 12 years ago

Is this refactoring to allow use of the trusted UI in multiple places?  What happens if we don't fix this?

	Jason Smith [:jsmith]
	Comment 7 • 12 years ago

(In reply to Andrew Overholt [:overholt] from comment #6)
> Is this refactoring to allow use of the trusted UI in multiple places?  What
> happens if we don't fix this?

The motivation isn't refactoring - the motivation is to get what was working for trusted UI back up and running by separating the trusted UI code from the popup manager code for now. If we don't fix this, then I'm blocked on payments testing right now, and by caveat, this could block development work as well in that arena.

	Fernando Jiménez Moreno [:ferjm] Assignee
	Comment 8 • 12 years ago

(In reply to Jason Smith [:jsmith] from comment #7)
> (In reply to Andrew Overholt [:overholt] from comment #6)
> > Is this refactoring to allow use of the trusted UI in multiple places?  What
> > happens if we don't fix this?
> 
> The motivation isn't refactoring - the motivation is to get what was working
> for trusted UI back up and running by separating the trusted UI code from
> the popup manager code for now. If we don't fix this, then I'm blocked on
> payments testing right now, and by caveat, this could block development work
> as well in that arena.

Jason has explained it very well. Not much to add from my side. As I explained on comment #3, first of all we are trying to take the trusted UI to its working state before the regressions introduced by [1], so Jason can continue with the payments testing. 

After talking with Alive we considered appropriate to separate the trusted UI from the PopupManager, since the latests UX for both components differ meaningfully (even if we have no final UX for the trusted UI). That's the reason of the 'refactor' keyword. Sorry if that leads to confusion.

The patch that I proposed isn't a complete refactor, but should at least unblock Jason and also let Alberto Pastor to take it as a starting point for a final front-end implementation of the trusted UI independent of the PopupManager.

[1] https://github.com/mozilla-b2g/gaia/pull/5020

	Andrew Overholt [:overholt]
	Comment 9 • 12 years ago

Thanks for the clarification.  Payments testing (and development) is important so let's block on this.

	(inactive after 6/18) Alive Kuo [:alive]
	Comment 10 • 12 years ago

Comment on attachment 666586 [details] [diff] [review]
v1

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

Non blocking comment: 
1. You should deal with `appwillopen` in the final version.
2. You would face z-index issue if you create a new overlay other than popup-container. 
    Suggest: to use a value upper than homescreen app's z-index.

Finally, I am wondering if I have correct right here to review :/

	(inactive after 6/18) Alive Kuo [:alive]
	Comment 11 • 12 years ago

(In reply to Fernando Jiménez Moreno [:ferjm] from comment #5)
> BTW, the tests requires this https://github.com/mozilla-b2g/gaia/pull/5083
> to be landed. It would be great if someone (Tim, Alive, Alberto?) could take
> a look at it and merge it if possible :)

Bugzilla now is the king, so I propose to move the PR to bugzilla for r.

	Fernando Jiménez Moreno [:ferjm] Assignee
	Comment 12 • 12 years ago

Thanks Alive!
Not sure what's the procedure to push to Gaia now. I am wondering if I should make a PR with the content of this patches or if I should set the 'checkin-needed' flag.
Vivien, Dietrich?

	Fernando Jiménez Moreno [:ferjm] Assignee
	Comment 13 • 12 years ago

https://github.com/mozilla-b2g/gaia/pull/5623

	alberto.pastor
	Comment 14 • 12 years ago

Pointer to Github pull-request

	alberto.pastor
	Comment 15 • 12 years ago

Pointer to Github pull-request

	alberto.pastor
	Comment 16 • 12 years ago

Sorry guys, was just trying the Addon that points a GH pull request as an attachment. Thought was going to ask for confirmation.

	Fernando Jiménez Moreno [:ferjm] Assignee
	Comment 17 • 12 years ago

https://github.com/mozilla-b2g/gaia/commit/0af9fb6dd1c1b23d04c878f58fb03f89ec1752d8

	Jason Smith [:jsmith]
	Comment 18 • 12 years ago

Verified on 10/11 build by ensuring that we can still make a basic payment and cancel payment against the mock payment provider.