959694 - Combine Rocketbar and statusbar

Status: RESOLVED FIXED

(Firefox OS Graveyard :: Gaia::System::Browser Chrome, defect)

Description

[Ben Francis [:benfrancis]]

Currently the Rocketbar input area is separate from the status bar, tapping or swiping the status bar makes the Rocketbar appear as a separate UI element.

The UX spec calls for the Rocketbar to be an expanded state of the status bar, where the statusbar title grows into the Rocketbar title and the statusbar icons disappear.

This bug is to track combining the two into one bar.

Comment 1

[Ben Francis [:benfrancis]]

Attachment: https://github.com/mozilla-b2g/gaia/pull/15538

Work in progress patch in case someone wants to carry on while I sleep. Will continue tomorrow.

Comment 2

[Kevin Grandon :kgrandon]

Currently working on a few things here.

Patch against your repo: https://github.com/benfrancis/gaia/pull/4
Combined patch against rocketbar branch (doesn't need to land if we land and squash in your branch first): https://github.com/mozilla-b2g/gaia/pull/15544

Comment 3

[Kevin Grandon :kgrandon]

Comment on attachment 8362734 [details] [review]
https://github.com/mozilla-b2g/gaia/pull/15538

Hi Ben,

Thanks for the awesome patch. I've tried to re-implement some of the existing functionality. Feel free to apply my patch and squash, or we can land both in the rocketbar branch.

1 - Fix unit and js marionette test issues. There's still a few marionette failures I'm trying to work out.
2 - Fixes header animation glitch, by sliding the rocketbar over the results pane. (Currently you get a glimpse of the homescreen).
3 - Fixes handling of cards when rocketbar is focused. Right now just uses opacity.
4 - Fixed inactive app title handling.

That's all I can do for now, but I'll try to do more in the morning. Thanks again!

Comment 4

[Ben Francis [:benfrancis]]

Nice job Kevin, have merged into my branch and will continue to try to fix the remaining glitches. Let's get this merged into the Rocketbar branch as soon as possible so it's  not blocking anyone else.

Comment 5

[Ben Francis [:benfrancis]]

OK, I think Kevin and I have managed to fix all the regressions and polish this branch a little. I've squashed the commits into one and it's pretty much ready to land in the Rocketbar branch. https://github.com/mozilla-b2g/gaia/pull/15538

There may still be one or more broken integration tests but I'm seeing inconsistent results between Travis and my local machine so I'm not sure. Hoping someone can confirm.

The performance of the transition to an expanded Rocketbar is not great because it animates font-size and height which we shouldn't really do. I think we know how to improve it, so we can either do that before we merge or just remove the animation for now (there isn't one in the current Rocketbar branch) and add it later, if it's a problem.


Meanwhile, Kevin has a much smaller patch which he has proposed as a temporary alternative if we don't want to use this patch for user testing https://github.com/mozilla-b2g/gaia/pull/15566 This patch keeps the statusbar and Rocketbar separate and just has the Rocketbar overlay the statusbar to achieve a similar visual effect.

To be honest this has actually made me wonder whether we should combine the two bars at all in the code, or just use smoke and mirrors to make it *appear* like they're the same thing...

In Josh's original video mockup of Haida, the statusbar and Rocketbar are very much one and the same thing. The Rocketbar contains the status icons even in its expanded state, and the text of the statusbar expands to become the text of the Rocketbar. However, as the UX spec has evolved, the two have become less intertwined. The status icons are no longer displayed on the Rocketbar and the text in the expanded state is often different to the text in the status state anyway. Francis is also now saying that titles shouldn't be displayed in the expanded state because they should be displayed as part of the task manager UI instead, and that we may no longer want the shortcut where you can tap the statusbar to go straight into the Rocketbar. All of these changes go towards de-coupling the two UI elements, though they still may visually look like one and the same thing.

Whilst it doesn't do everything that the bigger patch does, the alternative patch shows that we can achieve a similar visual effect without actually combining the two UI elements in code. There may be other areas like the loading state of the Rocketbar shrinking down into the status bar where it is useful for the bars to be combined, but it's possible we could work around those areas too.

While it would be very frustrating to have spent so much time combining the two bars and refactoring a lot of code, looking at it objectively I think there are arguments for both approaches.

There's nothing to stop us landing this now and iterating on it, but I'm open to opinions.

The final visual design may well be a deciding factor as certain transitions will be easier to achieve with one option or the other.

Comment 6

[Kevin Grandon :kgrandon]

Thank you Ben for the very well-written analysis of where we are today. I have no doubt that we can fix the integration tests and merge your branch to the rocketbar branch - but I do think we need to do some analysis to see what is the best long term approach here.

I think the driving factor here is the UX - so we should be certain we are 100% clear on the requirements. For example, we know we want to animate height of the rocketbar - for which we will need some sort of hardware acceleration here, likely through translateY() or scale(). If we need to animate font, that could also impact the approach. It may actually be worth trying to implement animations in both approaches, and see what feels right. One idea I had today was that we could use scale() on the text in the statusbar, and then swap the bar out with one behind it. I can not say if this is the best approach for the job yet.

I'm going to suggest that we sit down over a meeting tomorrow and iterate the pros/cons of either approach. I am open to any solution still.

For reference, here is the commit which masks the two bars as one: https://github.com/mozilla-b2g/gaia/commit/8d0364d2882afaf14513e0edbef8446697cf9dd6

Comment 7

[Dale Harvey (:daleharvey)]

Comment on attachment 8362734 [details] [review]
https://github.com/mozilla-b2g/gaia/pull/15538

Clearing feedback as in flux, will give feedback on whatever you guys decide is best for landing

Comment 8

[Kevin Grandon :kgrandon]

I'm keeping bug 959353 to only get patches into master, so unblocking for now. Let's chat about what to do here in the Paris WW.

Comment 9

[Kevin Grandon :kgrandon]

Attachment: Animation POC /w transforms

Adding the animation POC here for reference. I have not yet tried this on a device, but it should work well. I'll see if I can do that this week.

Comment 10

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

I was attracted by this bug title.

Not sure what you want to achieve here,
but if it involves StatusBar rewrite, please consider to make it instantiable then we could have:

var System = new App();
System.StatusBar = new StatusBar();

For searchApp we need to reconsider the layering issue.
Because it introduces the third layer:

(call screen?)
----------
Search app + Task Manager
----------
Current app
--------------
Homescreen app
--------------

Thus results in bad visibility state management IMO.

Comment 11

[Ben Francis [:benfrancis]]

Attachment: Proof of concept Rocketbar transition

This is the proof of concept Rocketbar transition I worked on at the work week. I'm now working to integrate this into the system, hopefully with a final visual design when we get it.

Comment 12

[Ben Francis [:benfrancis]]

Attachment: https://github.com/mozilla-b2g/gaia/pull/16262

Work in progress patch for you to look at. Lots of things left to fix and tests to write.

Comment 13

[Ben Francis [:benfrancis]]

Patch is complete and have written lots of new tests but still fixing existing tests https://github.com/mozilla-b2g/gaia/pull/16262

Comment 14

[Ben Francis [:benfrancis]]

(In reply to Alive Kuo [:alive][NEEDINFO!][:艾莉芙] from comment #10)
> I was attracted by this bug title.
> 
> Not sure what you want to achieve here,
> but if it involves StatusBar rewrite, please consider to make it
> instantiable then we could have:
> 
> var System = new App();
> System.StatusBar = new StatusBar();

Hi Alive,

This patch doesn't involve a StatusBar rewrite, just a few minor changes to expand/collapse the statusbar rather than show/hide the Rocketbar. A couple of new methods and CSS changes. I will flag you for review once I have finished writing tests. Please see my email to dev-gaia regarding Statusbar refactor.

Comment 15

[Ben Francis [:benfrancis]]

Comment on attachment 8375756 [details] [review]
https://github.com/mozilla-b2g/gaia/pull/16262

I'd like to kick off the review process on this patch, though I'm not sure whether it will have to wait until after the 1.4 branch to land. Either way I would prefer to have the Rocketbar turned off by default on master until it is more fully formed.

There are some intermittent integration test failures that need to be fixed, but I've been unable to reproduce them locally so any help with this would be appreciated.

There's quite a lot of code here and there's more I'd like to do but I think the patch has got plenty big enough and I plan to make further improvements in follow ups.

There are no new features added in this patch but I have re-factored the Rocketbar to be part of the status bar and expand/collapse rather than show/hide as per the UX spec, in a way that can be turned off with the Rocketbar setting. Swiping further down the screen triggers the task manager, though the transition to show the task manager is outside the scope of this bug.

This patch maintains the behaviour of the utility tray which can be dragged down from the right half of the status bar. Tapping the right side of the status bar to launch a settings tray is also outside the scope of this bug. I have moved the Rocketbar opening logic out of utility_tray.js and into rocketbar.js

The biggest risk of regressions probably comes from the changes to the statusbar, particularly increasing its height from 2rem to 2.4rem. Alive, I'd be grateful for your eyes on this.

You'll notice that there's an HTML element called #statusbar-background which I would rather not have but through prototyping we found it is necessary to make the CSS transitions performant.

I have created a new Marionette test harness for the Rocketbar and moved the Rocketbar integration tests out of the search app. I have also increased the unit test coverage. I can think of more integration tests we can write for the Rocketbar, but I think Kevin has filed a bug for that.

Tapping the everything.me search box on the homescreen no longer opens the Rocketbar because the Rocketbar will eventually be expanded on the homescreen as per bug 970935. I think we should turn the old behaviour of the e.me search bar back on so the e.me tests can be re-enabled.

I have rolled title.js into rocketbar.js and Rocketbar is now initialised in bootstrap.js.

Rocketbar is not currently instantiable, I wrote most of the code before that effort was announced.

Sorry the patch is so big, but I wanted to be thorough.

Comment 16

[Kevin Grandon :kgrandon]

Comment on attachment 8375756 [details] [review]
https://github.com/mozilla-b2g/gaia/pull/16262

So I'm seeing lots of regressions here in state management, and I suppose part of the problem is that we don't have existing integration tests for this stuff. The old utility tray tests covered some of this, but likely were not enough and should be updated instead of being deleted. (Or re-created once everything is working). Here are some of the things I noticed:

1 - Rocketbar stays expanded if I focus, then tap on content. I guess we should hide the rocketbar on this blur event? Please add a unit or integration test for it.

2 - Rocketbar into task manager transitions appear to be broken for me. When launching an app the homescreen flashes, and it should just expand the cart. I did apply your patch over latest master, so I'm wondering if that could be the problem?

3 - Like 2, transitions going *into* rocketbar seem to not be working well. It jumps instead of smoothly shrinks.

4 - If I open task manager, then start typing - the rocketbar will hide.

5 - If I close all apps in the task manager, the rocketbar should enter focused state.

I am generally happy with the code style, dom layout, and architecture. I do think we need to fix these before landing in master - but we could potentially land in a branch and file follow-up issues to resolve these if you would like help. I could also pitch in and send some patches your way, but I'm at MWC this week and have a shoddy internet connection.

Another option might be to start taking these changes in smaller chunks, ensuring each one is well tested. (E.g., do it in phases where 1 - we could expand the size of the statusbar, 2 - we could add input styling, 3 - we could add state management, and 4 - we could implement the transitions).

Clearing the review for now until we can figure out our landing strategy here, or get this fixed up for master.

Comment 17

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

Comment on attachment 8375756 [details] [review]
https://github.com/mozilla-b2g/gaia/pull/16262

I don't think we should change the height of the app on rocketbar shown/hide.
The app would rapidly get resize event if we do so. It's unnecessary, is it?

I'd like to talk to UX for this behavior.

Comment 18

[Ben Francis [:benfrancis]]

(In reply to Kevin Grandon :kgrandon from comment #16)
> So I'm seeing lots of regressions here in state management, and I suppose
> part of the problem is that we don't have existing integration tests for
> this stuff. The old utility tray tests covered some of this, but likely were
> not enough and should be updated instead of being deleted. (Or re-created
> once everything is working).

I removed the unit tests which corresponded to the code I removed from the utility tray, and added unit tests for all the methods I added to the Rocketbar. There are definitely more integration tests we should write for Rocketbar and I'm happy to do that separately, but I haven't added any new features here and I think you filed another bug to write the missing integration tests for existing features?

> 1 - Rocketbar stays expanded if I focus, then tap on content. I guess we
> should hide the rocketbar on this blur event? Please add a unit or
> integration test for it.

The UX spec says the user should swipe the Rocketbar up or tap the cancel button to collapse it. It also says that if Rocketbar results are scrolled or a suggestion is pressed the Rocketbar should stay in the expanded state, so we shouldn't collapse it on the blur event.

I haven't implemented the cancel button in the new Rocketbar yet so this could be considered a regression. Like I said there's a lot more to do but I felt this patch had got big enough.

> 2 - Rocketbar into task manager transitions appear to be broken for me. When
> launching an app the homescreen flashes, and it should just expand the cart.
> I did apply your patch over latest master, so I'm wondering if that could be
> the problem?

I'm not 100% sure what you mean here but as I mentioned in my review request, the task manager transition is out of the scope of this bug, the Rocketbar just sends the taskmanagershow event. I didn't consider this a regression as the transition didn't seem to be to spec anyway.

> 3 - Like 2, transitions going *into* rocketbar seem to not be working well.
> It jumps instead of smoothly shrinks.

Can you provide some STR or attach a video? This actually looks pretty smooth to me.

> 4 - If I open task manager, then start typing - the rocketbar will hide.

Good catch, I'll look into this.

> 5 - If I close all apps in the task manager, the rocketbar should enter
> focused state.

Oh sorry I missed this in the spec, is this the current behaviour on master? We're actually not handling closing/crashing apps well at all in the Rocketbar but I was planning on addressing that separately.

> Another option might be to start taking these changes in smaller chunks,
> ensuring each one is well tested. (E.g., do it in phases where 1 - we could
> expand the size of the statusbar, 2 - we could add input styling, 3 - we
> could add state management, and 4 - we could implement the transitions).

This is part 1, there's much more to do. I would rather not spend time breaking this patch down into chunks unless you can think of a really good reason for doing so?

Comment 19

[Ben Francis [:benfrancis]]

(In reply to Alive Kuo [:alive][NEEDINFO!][:艾莉芙] from comment #17)
> I don't think we should change the height of the app on rocketbar shown/hide.
> The app would rapidly get resize event if we do so.

This patch doesn't resize the app when you expand/collapse the Rocketbar, the Rocketbar just overlaps content. It does resize the statusbar from 2rem to 2.4rem but that is in the collapsed state everywhere.

> I'd like to talk to UX for this behavior.

The UX spec does specify that content should be "displaced" in some cases and "overlayed" in other cases [1]. If you have comments on the spec for the show/hide Rocketbar feature then please discuss in bug 941182

1. https://mozilla.app.box.com/files/0/f/1399872384

Comment 20

[Ben Francis [:benfrancis]]

Aha, I can't reproduce the integration test failures on OS X, but I can reproduce them on Linux.

Comment 21

[Kevin Grandon :kgrandon]

Comment on attachment 8375756 [details] [review]
https://github.com/mozilla-b2g/gaia/pull/16262

Ok, so I am fine giving an F+ here, and I think we can potentially land it in a branch, but I don't think we should land in master until we fix some of the issues so nothing regresses. If we do land it in a mozilla-b2g branch perhaps I would be more than happy to help follow-up on some of these items.. Or alternatively I can try to send pull requests to your branch.

Comment 22

[Ben Francis [:benfrancis]]

We shouldn't land it anywhere without r+, I don't want us to end up in the state we did with the Rocketbar branch again by landing loads of un-reviewed patches.

I am very happy to fix actual regressions and even add missing integration tests, but I think we can only consider something a regression if the feature was implemented to spec in the first place. Let me try to fix the integration tests and fix the obvious regressions tomorrow and we can re-evaluate then. IMO we're closer to the spec with this patch than without it but there's still a long way to go before we should turn Rocketbar on by default. I just want to get it landed somewhere where it will be tracked against master before I move on to stabilisation work :)

Comment 23

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

Comment on attachment 8375756 [details] [review]
https://github.com/mozilla-b2g/gaia/pull/16262

r+ for statusbar fixed height change.

Comment 24

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

BTW I found the app name in search input is not localized. I'm going to file bug for it.

Comment 25

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

Comment on attachment 8375756 [details] [review]
https://github.com/mozilla-b2g/gaia/pull/16262

After looking again I decide to cancel the review.
Hi Ben,
because not everything is moved into AppWindow, we need to set the top and height in separate css files instead.
Please grep 2rem in apps/system/style to change to 2.4rem
Something I found:
1) app install manager top
2) attention screen top + height
3) cards view margin-top + height
4) lockscreen top + height
5) permission manager top + height
6) system.css dialog overlay top/height
7) utility tray height
8) Value selector top
9) Wrapper progress top

I hope next time we want to change height of statusbar we don't need to change that much but currently let's tolerate.

Please set review once you think it's done thanks!

Comment 26

[Ben Francis [:benfrancis]]

The integration tests are passing! https://travis-ci.org/mozilla-b2g/gaia/builds/19695397

I am deeply sorry for all the horrible things I said about Marionette, I was wrong. It turns out the tests were failing intermittently because the Rocketbar input was showing, but then sometimes hiding again because something during Gaia startup was stealing its focus.

There was a unit test fail in this build but that is failing on master too and is unrelated.

Zac, do you have any idea how I broke the Python UI test? (console output below).



test_context_menu_activity_picker (test_context_menu_activity_picker.TestContextMenuActivityPicker) ... ERROR

======================================================================

ERROR: None

----------------------------------------------------------------------

Traceback (most recent call last):

File "/home/travis/build/mozilla-b2g/gaia/travis_venv/local/lib/python2.7/site-packages/marionette_client-0.7.3-py2.7.egg/marionette/marionette_test.py", line 143, in run

testMethod()

File "/home/travis/build/mozilla-b2g/gaia/tests/python/gaia-ui-tests/gaiatest/tests/functional/system/test_context_menu_activity_picker.py", line 19, in test_context_menu_activity_picker

activities_list = context_menu_page.long_press_context_menu_body()

File "/home/travis/build/mozilla-b2g/gaia/tests/python/gaia-ui-tests/gaiatest/apps/ui_tests/regions/context_menu.py", line 29, in long_press_context_menu_body

return Activities(self.marionette)

File "/home/travis/build/mozilla-b2g/gaia/tests/python/gaia-ui-tests/gaiatest/apps/system/regions/activities.py", line 26, in __init__

self.wait_for_condition(lambda m: view.location['y'] == 20)

File "/home/travis/build/mozilla-b2g/gaia/tests/python/gaia-ui-tests/gaiatest/apps/base.py", line 50, in wait_for_condition

Wait(self.marionette, timeout).until(method, message=message)

File "/home/travis/build/mozilla-b2g/gaia/travis_venv/local/lib/python2.7/site-packages/marionette_client-0.7.3-py2.7.egg/marionette/wait.py", line 143, in until

cause=last_exc)

TimeoutException: TimeoutException: Timed out after 10.0041401386 secondsNone

TEST-UNEXPECTED-FAIL | test_context_menu_activity_picker.py test_context_menu_activity_picker.TestContextMenuActivityPicker.test_context_menu_activity_picker |

----------------------------------------------------------------------

Ran 1 test in 22.034s

FAILED (errors=1)

Comment 27

[Zac C (:zac)]

I'll debug it locally and email you benfrancis.

Comment 28

[Zac C (:zac)]

Email sent to Ben.

Comment 29

[Ben Francis [:benfrancis]]

Thanks Zac.

Travis is now green!

Now to fix those regressions and re-write all the unit tests.

Comment 30

[Ben Francis [:benfrancis]]

Fixed regressions:
* If I open task manager, then start typing - the rocketbar will hide.
* If I close all apps in the task manager, the rocketbar should enter focused state.

Now need to address Alive's comments and re-write the unit tests.

Comment 31

[Ben Francis [:benfrancis]]

Comment on attachment 8375756 [details] [review]
https://github.com/mozilla-b2g/gaia/pull/16262

Thanks for the helpful review comments guys.

I've gone through and increased the size of the status bar everywhere I can find and I've re-written the unit tests to use sinon. I've also re-based against master and added in Dale's new integration test.

The transitions into and out of the task manager definitely need a lot of work, but I feel that's outside the scope of this bug.

There are certainly lots more integration tests I would like to write for the Rocketbar as follow-ups.

Submitting for re-review with a view to finally getting this patch landed on the bubble-tea branch where it can park for a while :)

Comment 32

[Kevin Grandon :kgrandon]

Comment on attachment 8375756 [details] [review]
https://github.com/mozilla-b2g/gaia/pull/16262

Nice work! I am going to leave R- for now until we clean up a few more things. I think it's important to change the rocketbar tests to match other system tests in using sinon mocks - let me know if you want help here.

I am sad that we are breaking a bunch of task manager stuff that is currently working well on master imo - but if we land on bubble tea we should be able to quickly fix that.

Anyway - if we get the tests done (and travis is happy), then I have no problem landing on bubble-tea.

Comment 33

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

Comment on attachment 8375756 [details] [review]
https://github.com/mozilla-b2g/gaia/pull/16262

quick r+ for statusbar height part.
I suggest to open another pull request for bubble-tea.

Comment 34

[Ben Francis [:benfrancis]]

Comment on attachment 8375756 [details] [review]
https://github.com/mozilla-b2g/gaia/pull/16262

I have changed the unit tests to use sinon to mock the DOM objects as well as the methods, as you suggested. The downside of doing it this way and calling init() in setup is that it can have side effects and by using the real properties of the Rocketbar object we risk not isolating the unit tests from each other as well. So I have moved the initialisation of Rocketbar's properties into the init() function so that they get reset for each test.

Your point about multiple calls to bind() effecting performance is interesting, it would be good to try to quantify that through measurement because if it has a significant effect there could be lots of places in Gaia where this would be an improvement. I prefer not to have a single event handler for all events because I personally find putting everything in one giant event handler function makes the code less readable.

It's unfortunate that the transitions into and out of the task manager do not look as nice with this patch applied, but please be aware that I have not made any changes to the task manager. I think it just looks bad because the Rocketbar is now closer to the spec, but the task manager has not yet been updated to use the transitions from the spec. We can improve this in time.

Travis is green!

Comment 35

[Kevin Grandon :kgrandon]

Comment on attachment 8375756 [details] [review]
https://github.com/mozilla-b2g/gaia/pull/16262

Nice work! We are almost there but I noticed a few more problems. I've left comments on github for where I think is causing these issues. If we can fix this let's land it in bubble-tea.

1 - The keyboard change event isn't working (the homescreen is still being resized). I have a marionette test for this, but it's disabled due to infrastructure limitations. Will try to fix this in the future. Please add/remove the listener on the body element.

2 - The E.me search experience is broken when tapping on a suggestion.

Comment 36

[Ben Francis [:benfrancis]]

I've rebased this against bubble-tea and created a pull request for that https://github.com/mozilla-b2g/gaia/pull/16837

Travis doesn't look very happy on that branch right now so will follow up with Alive/Tim tomorrow regarding landing.

Comment 37

[Ben Francis [:benfrancis]]

(Oh, and I fixed Kevin's final review comments and the Travis is green against master)

Comment 38

[Ben Francis [:benfrancis]]

Alive, Tim,

How do you feel about merging this into bubble-tea? https://github.com/mozilla-b2g/gaia/pull/16837

There are currently two failing unit tests, but they seem to be unrelated. Once Travis is green are you happy to land?

Comment 39

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

Sure!

Comment 40

[Ben Francis [:benfrancis]]

gsvelto helped me identify that the unit test is caused by bug 978562 which should now be fixed on master.

Comment 41

[Ben Francis [:benfrancis]]

Merged into bubble-tea https://github.com/mozilla-b2g/gaia/commit/028f1e19132f35e387eed9cb4ec7a7da33b3d7d0

There is one failing UI test, but that test has been failing on bubble-tea for some time.

Comment 42

[Zac C (:zac)]

(In reply to Ben Francis [:benfrancis] from comment #41)
> Merged into bubble-tea
> https://github.com/mozilla-b2g/gaia/commit/
> 028f1e19132f35e387eed9cb4ec7a7da33b3d7d0
> 
> There is one failing UI test, but that test has been failing on bubble-tea
> for some time.

The stacktrace of the UI test failure contains the same line that you modified in your patch; how can you be sure that you haven't affected the test?
Why not just fix it now as you'll have to anyway when bubble-tea goes into master.

Comment 43

[[e-mail]]

Zac, thanks for reminding.

We are trying to identify the UI test issue now. The issue is related to context menu which not show correctly on screen. I think it's not related to this issue.

We definitely will fix it before goes into master.

Comment 44

[Zac C (:zac)]

(In reply to Fred Lin [:gasolin] from comment #43)
> Zac, thanks for reminding.
> 
> We are trying to identify the UI test issue now. The issue is related to
> context menu which not show correctly on screen. I think it's not related to
> this issue.
> 
> We definitely will fix it before goes into master.

I think you might be missing on bubble-tea the commit from this comment?

Comment 45

[Michael Henretty [:mikehenrty][:mhenretty]]

Did this land on master with the bubble-tea merge?

Comment 46

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Not yet.

Comment 47

[Tim Guan-tin Chien [:timdream] (please needinfo)]

Actually, according Fred's note, this commit has too much conflicts with the current master and we feel it's safer for the original author to rebase it. Michael, since Ben is on PTO, would you be able to resolve the conflict? I think Fred did some of them in many of his master->bubble-tea merge commits so he should be able to point out where they are too.

Comment 48

[[e-mail]]

Since bubble-tea has several significant system patch, we tend to land them in order to ensure the stability. Or there will be lots of conflict between those bubble-tea patches.

I'll try to send PR first if there's not a lot of conflicts, or will notify the author to land it manually. Hope we can totally merge bubble-tea within 1~2 weeks.

Comment 49

[Michael Henretty [:mikehenrty][:mhenretty]]

Thanks Fred! As Tim mentioned, the original author Ben is on holiday for another week, so if there are too many merge conflicts when you try to merge, let's wait for him. If one week is too long to wait, feel free to flag me and I can try to have a look. Thanks again!

Comment 50

[[e-mail]]

Ben, due to many conflict occurred while cherry-picking this patch, please help create a PR, check travis is green and land it to master. Thanks.

Comment 51

[Ben Francis [:benfrancis]]

Merged into master :)

https://github.com/mozilla-b2g/gaia/commit/ac28dec0257997231f403e056dd71a326fb90010

Comment 52

[Jason Smith [:jsmith]]

Didn't see a response in IRC, so pinging through the bug to be sure.

We need to back this out for causing bug 991698.

Comment 53

[Ryan VanderMeulen [:RyanVM]]

Conflicts.

Comment 54

[Jason Smith [:jsmith]]

Ben has left for the day - let me redirect this to Gregor to get someone to work on backing this out from hist team.

Comment 55

[Michael Henretty [:mikehenrty][:mhenretty]]

Sam Foster fixed the conflicts.

backed out: https://github.com/mozilla-b2g/gaia/commit/8b00ea2544b465212e8180134c493f6b4dcaeb14

Comment 56

[Kevin Grandon :kgrandon]

My hunch is that this is because you are adding the listener to prevent resizing inside the focus method, and that is getting called from handleCardViewClosed. handleCardViewClosed should not be called unless rocketbar is enabled.

Comment 57

[Ben Francis [:benfrancis]]

Attachment: https://github.com/mozilla-b2g/gaia/pull/18051

Kevin,

I've attached a pull request which contains two commits which need to land together. One is reverting the revert of the original patch for this bug without changes (re-landing). The other commit is a patch for bug 978937 which will prevent 991698 from regressing again.

It also adds back a handleEvent method as you suggested, which also makes it easier to remove event listeners.

I thought it would be easier to review this way, but I can squash the commits before landing if you think it's necessary.

Thanks

Comment 58

[Kevin Grandon :kgrandon]

Comment on attachment 8402851 [details] [review]
https://github.com/mozilla-b2g/gaia/pull/18051

Seems like a reasonable approach to me. Technically I think they're supposed to be squashed to make the single commit atomic, but landing a revert + follow-up is fine by me here.

Comment 59

[Kevin Grandon :kgrandon]

We do need to fix the test before landing if there is a problem. I restarted the job, but I noticed this failing: 

1) Rocketbar "before each" hook:

NoSuchElement: (7) Unable to locate element: #os-logo

Comment 60

[Ben Francis [:benfrancis]]

Thanks Kevin,

I saw that too but now Travis seems to be passing repeatedly and I can't get it to fail, maybe it was a glitch in the Matrix. Will keep an eye out to see whether it's an intermittent.

Squashed the two commits and merged into master in https://github.com/mozilla-b2g/gaia/commit/dc25c96c46eeb58d10cdd06329c61500508ba3de