962434 - [Window Management] Implement BrowserValueSelector

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::System::Window Mgmt, defect)

Description

[(inactive after 6/18) Alive Kuo [:alive]]

Move current fixed value selector into sub components of appWindow.

Comment 1

[Yura Zenevich [:yzen] (please use "needinfo")]

Hi Alive, I was looking into this issue and was wondering how to go about the mozChromeEvent that is fired against the main window. Should it be delegated to the right app iframe window?

Comment 2

[(inactive after 6/18) Alive Kuo [:alive]]

(In reply to Yura Zenevich [:yzen] from comment #1)
> Hi Alive, I was looking into this issue and was wondering how to go about
> the mozChromeEvent that is fired against the main window. Should it be
> delegated to the right app iframe window?

Do you mean you want to take this one?
I have no better idea than: let window manager find to top most window and render the overlay inside it.

Comment 3

[(inactive after 6/18) Alive Kuo [:alive]]

Yura if you want to make value selector ally friendly maybe we could go without this fix. It's too big and dangerous. We could talk for this if you could describe your trouble.

Comment 4

[Yura Zenevich [:yzen] (please use "needinfo")]

(In reply to Alive Kuo [:alive][NEEDINFO!][OOO until 6/30] from comment #3)
> Yura if you want to make value selector ally friendly maybe we could go
> without this fix. It's too big and dangerous. We could talk for this if you
> could describe your trouble.

Yes, I think it could be done without the changes to how the value selector is loaded. 
My first intention was to help with getting it working properly. I can post a patch of what I have so far, and you can let me know if you still think I should take the easier route?

Comment 5

[(inactive after 6/18) Alive Kuo [:alive]]

(In reply to Yura Zenevich [:yzen] from comment #4)
> (In reply to Alive Kuo [:alive][NEEDINFO!][OOO until 6/30] from comment #3)
> > Yura if you want to make value selector ally friendly maybe we could go
> > without this fix. It's too big and dangerous. We could talk for this if you
> > could describe your trouble.
> 
> Yes, I think it could be done without the changes to how the value selector
> is loaded. 
> My first intention was to help with getting it working properly. I can post
> a patch of what I have so far, and you can let me know if you still think I
> should take the easier route?

Sure, let's make it better.

Comment 6

[Yura Zenevich [:yzen] (please use "needinfo")]

Attachment: Github PR

Comment 7

[(inactive after 6/18) Alive Kuo [:alive]]

Gread work, Yura!
Give me some time to read it carefully :)

c.c. Rudy also.

Comment 8

[(inactive after 6/18) Alive Kuo [:alive]]

Comment on attachment 8450425 [details] [review]
Github PR

Basically ok, well done! Fix all the nits and we are ok to go I think.

Comment 9

[(inactive after 6/18) Alive Kuo [:alive]]

Comment on attachment 8450425 [details] [review]
Github PR

I'd like to have Rudy double check.

Comment 10

[Yura Zenevich [:yzen] (please use "needinfo")]

Comment on attachment 8450425 [details] [review]
Github PR

Fixed all nits.

Comment 11

[Rudy Lu [:rudyl] (inactive after 2015/7/3)]

Comment on attachment 8450425 [details] [review]
Github PR

I found an issue with the following STR,

1. Launch Contacts app. > Add a new contact.
2. Click birthday > Date to see date picker.
3. press [Home] button.
4. Re-enter Contacts app.
 Expected: the date picker should disappear. (and the focus should have been removed.)
 Actual: it would stay on the screen.

--
I would need more time to review the patch and test it, but f- so that you're notified.

Comment 12

[Yura Zenevich [:yzen] (please use "needinfo")]

(In reply to Rudy Lu [:rudyl] from comment #11)
> Comment on attachment 8450425 [details] [review]
> Github PR
> 
> I found an issue with the following STR,
> 
> 1. Launch Contacts app. > Add a new contact.
> 2. Click birthday > Date to see date picker.
> 3. press [Home] button.
> 4. Re-enter Contacts app.
>  Expected: the date picker should disappear. (and the focus should have been
> removed.)
>  Actual: it would stay on the screen.
> 
> --
> I would need more time to review the patch and test it, but f- so that
> you're notified.

Fixed this regression (I was under impression that I would not need to listen to 'appclosing', 'appopening', etc any more as a sub-component).

Comment 13

[(inactive after 6/18) Alive Kuo [:alive]]

(In reply to Yura Zenevich [:yzen] from comment #12)
> (In reply to Rudy Lu [:rudyl] from comment #11)
> > Comment on attachment 8450425 [details] [review]
> > Github PR
> > 
> > I found an issue with the following STR,
> > 
> > 1. Launch Contacts app. > Add a new contact.
> > 2. Click birthday > Date to see date picker.
> > 3. press [Home] button.
> > 4. Re-enter Contacts app.
> >  Expected: the date picker should disappear. (and the focus should have been
> > removed.)
> >  Actual: it would stay on the screen.
> > 
> > --
> > I would need more time to review the patch and test it, but f- so that
> > you're notified.
> 
> Fixed this regression (I was under impression that I would not need to
> listen to 'appclosing', 'appopening', etc any more as a sub-component).

You don't need AppWindowManager to redirect it. Read AppTransitionController, the events come from there.

Comment 14

[Yura Zenevich [:yzen] (please use "needinfo")]

Comment on attachment 8450425 [details] [review]
Github PR

Asking for another re-review, comments addressed.

Comment 15

[(inactive after 6/18) Alive Kuo [:alive]]

Comment on attachment 8450425 [details] [review]
Github PR

I am going to r+ with nits.
Hopefully no regressions but please watch the follow up if there's any.

Thank you for your effort!
Rudy, it's nice if you could double confirm.

Comment 16

[Rudy Lu [:rudyl] (inactive after 2015/7/3)]

Comment on attachment 8450425 [details] [review]
Github PR

There are still some code structure related issues to be addressed, IMHO.
So, I could not f+ this patch.

Please help address the issues commented on the github, and ask for feedback again.
Thank you.

--
BTW, in the end, I think we could separate time picker and date picker out of value selector since not all of them would be utilized by each app, but that could be done in a follow-up.

Comment 17

[Yura Zenevich [:yzen] (please use "needinfo")]

Comment on attachment 8450425 [details] [review]
Github PR

Updated the PR with comments addressed.

Comment 18

[Rudy Lu [:rudyl] (inactive after 2015/7/3)]

Comment on attachment 8450425 [details] [review]
Github PR

Yura,

Thanks for the updates, f+ with some nits to be addressed, especially about when to remove the event handlers part.

Comment 19

[Yura Zenevich [:yzen] (please use "needinfo")]

(In reply to Rudy Lu [:rudyl] from comment #18)
> Comment on attachment 8450425 [details] [review]
> Github PR
> 
> Yura,
> 
> Thanks for the updates, f+ with some nits to be addressed, especially about
> when to remove the event handlers part.

Hi Rudy, I fixed the last nit and added the comment re event handlers. Hopefully it's satisfying.

Comment 20

[Yura Zenevich [:yzen] (please use "needinfo")]

https://github.com/mozilla-b2g/gaia/commit/2281a46cf77422068c3a8342e6a0950f2486d3a6