If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

[Window Management] Implement BrowserValueSelector

RESOLVED FIXED

Status

Firefox OS
Gaia::System::Window Mgmt
RESOLVED FIXED
4 years ago
2 years ago

People

(Reporter: alive, Assigned: yzen)

Tracking

(Blocks: 1 bug)

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

46 bytes, text/x-github-pull-request
alive
: review+
alive
: feedback+
rudyl
: feedback+
Details | Review | Splinter Review
Move current fixed value selector into sub components of appWindow.
Depends on: 995033
(Assignee)

Comment 1

3 years ago
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.
(Assignee)

Comment 4

3 years ago
(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)
(Assignee)

Comment 6

3 years ago
Created attachment 8450425 [details] [review]
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)
(Assignee)

Comment 10

3 years ago
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-
(Assignee)

Comment 12

3 years ago
(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).
(Assignee)

Updated

3 years ago
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)
(Assignee)

Comment 14

3 years ago
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)
(Assignee)

Comment 17

3 years ago
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+
(Assignee)

Comment 19

3 years ago
(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.
(Assignee)

Comment 20

3 years ago
https://github.com/mozilla-b2g/gaia/commit/2281a46cf77422068c3a8342e6a0950f2486d3a6
Status: NEW → RESOLVED
Last Resolved: 3 years ago
Resolution: --- → FIXED
Depends on: 1040290
Depends on: 1094550
Depends on: 1167050
You need to log in before you can comment on or make changes to this bug.