Closed Bug 962434 Opened 10 years ago Closed 10 years ago

[Window Management] Implement BrowserValueSelector

Categories

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

ARM
Gonk (Firefox OS)
defect
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: alive, Assigned: yzen)

References

Details

Attachments

(1 file)

Move current fixed value selector into sub components of appWindow.
Depends on: 995033
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?
Flags: needinfo?(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.
Flags: needinfo?(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.
(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?
Flags: needinfo?(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.
Flags: needinfo?(alive)
Attached file Github PR
Assignee: nobody → yzenevich
Attachment #8450425 - Flags: review?(alive)
Gread work, Yura!
Give me some time to read it carefully :)

c.c. Rudy also.
Comment on attachment 8450425 [details] [review]
Github PR

Basically ok, well done! Fix all the nits and we are ok to go I think.
Attachment #8450425 - Flags: review?(alive) → feedback+
Comment on attachment 8450425 [details] [review]
Github PR

I'd like to have Rudy double check.
Attachment #8450425 - Flags: feedback?(rlu)
Comment on attachment 8450425 [details] [review]
Github PR

Fixed all nits.
Attachment #8450425 - Flags: review?(alive)
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.
Attachment #8450425 - Flags: feedback?(rlu) → feedback-
(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).
Attachment #8450425 - Flags: feedback- → feedback?(rlu)
(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.
Attachment #8450425 - Flags: review?(alive)
Comment on attachment 8450425 [details] [review]
Github PR

Asking for another re-review, comments addressed.
Attachment #8450425 - Flags: review?(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.
Attachment #8450425 - Flags: review?(alive) → review+
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.
Attachment #8450425 - Flags: feedback?(rlu)
Comment on attachment 8450425 [details] [review]
Github PR

Updated the PR with comments addressed.
Attachment #8450425 - Flags: feedback?(rlu)
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.
Attachment #8450425 - Flags: feedback?(rlu) → feedback+
(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.
https://github.com/mozilla-b2g/gaia/commit/2281a46cf77422068c3a8342e6a0950f2486d3a6
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Depends on: 1040290
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: