912904 - [System][Dialogs][ActionMenu] Dialogs & Action menus (BB) are not scrolling as expected when there is a large list of items.

Status: RESOLVED FIXED

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

Description

[Borja Salguero [:borjasalguero]]

Attachment: Lista_portrait.png

When showing a large list of options when calling an activity (for example when calling 'share' activity from Gallery) the list is not shown completely and the title is hidden.

Comment 1

[Preeti Raghunath(:Preeti)]

QA,

Is picker scrollable?

Also please check if this is a regression from 1.0.1?

Comment 2

[Lucas Aliaga]

Attachment: LucasA_Activity Picker.png

Comment 3

[Lucas Aliaga]

Attachment: LucasA_Installed sharing apps.png

Comment 4

[Lucas Aliaga]

With several sharing apps installed, activity picker in gallery does not populate enough options to scroll on Inari 1.0.1 latest.

Environmental Variables
Build ID: 20130906043205
Gecko: http://hg.mozilla.org/releases/mozilla-b2g18_v1_0_1/rev/9c62297d11b0
Gaia: 054cdc27404e2daca91d3065d9783681032b2151
Platform Version: 18.0

See Attached: LucasA_Activity Picker.png, LucasA_Installed sharing apps.png

Comment 5

[Jason Smith [:jsmith]]

(In reply to Lucas A. from comment #4)
> With several sharing apps installed, activity picker in gallery does not
> populate enough options to scroll on Inari 1.0.1 latest.
> 
> Environmental Variables
> Build ID: 20130906043205
> Gecko: http://hg.mozilla.org/releases/mozilla-b2g18_v1_0_1/rev/9c62297d11b0
> Gaia: 054cdc27404e2daca91d3065d9783681032b2151
> Platform Version: 18.0
> 
> See Attached: LucasA_Activity Picker.png, LucasA_Installed sharing apps.png

That's not enough to test this. You need to find more apps to install to trigger this bug.

Comment 6

[Borja Salguero [:borjasalguero]]

For testing this you need to have a bunch of Apps which offer the same 'activity' (for example 'Share'). Im gonna try to modify the layout after checking with UX Team, due to all screens should behave in the same manner.

Comment 7

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

Borja you want to fix this or lemme find someone to fix this? This is a known issue to me and is a pretty easy one.

Comment 8

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

(In reply to Preeti Raghunath(:Preeti) from comment #1)
> QA,
> 
> Is picker scrollable?

No.

> 
> Also please check if this is a regression from 1.0.1?

No. v1.0.1 has the same problem.

Comment 9

[Natalya Kot [:nkot]]

removing keyword per comment 8

Comment 10

[Borja Salguero [:borjasalguero]]

Attachment: Issue in other screens as send report one

Comment 12

[Borja Salguero [:borjasalguero]]

Attachment: Pull request

This is a small patch for fixing the scrolling issue in the BB.

Comment 13

[Borja Salguero [:borjasalguero]]

Comment on attachment 802616 [details]
Pull request

Alive, I've changed the ListMenu and now we are using Building Blocks! So the issue of the scrolling is fixed now for the activity list :). Could you take a look? Thanks!

Comment 14

[Borja Salguero [:borjasalguero]]

Thanks Arnau for the tip!! :)

Comment 15

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

Comment on attachment 802616 [details]
Pull request

Thanks for being aggressive on this!

Are you working all the night? r- for this! (Joking)

So, is there a reason to rename listmenu to actionmenu? Is it because of naming of building block?
What if building block is changing all the way? I don't want to rename my module each time building blocks change (it always changes from my point of view) :)

If you insist on having this kind of big change, please have some unit tests.

Comment 16

[Borja Salguero [:borjasalguero]]

The goal of this patch is keeping current structure for not adding a lot of changes in all apps. However layout of 'action_menu' should be updated as part of BB, and applied to all apps. This will be fixed here https://bugzilla.mozilla.org/show_bug.cgi?id=901490.

> I don't want to rename my module each time building blocks change 

IMHO this name should not change in the future. We have agreed some components in our OS, and changing the name would change our OS concepts... Kaze wdyt? If you think that this could change, I can revert change name to 'ListMenu'.

Regarding the unit tests, Im gonna create a bunch of Unit Tests for sure! Stay tuned! ;)

Comment 17

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

Notice: 
Borja, we have two patches affecting list menu right now:
https://bugzilla.mozilla.org/show_bug.cgi?id=893560
and mine https://bugzilla.mozilla.org/show_bug.cgi?id=914564

Mine is fine but Greg's patch is targeting 1.2 feature, so we may need to avoid conflict here.

Comment 18

[Borja Salguero [:borjasalguero]]

Your patch is included in my PR. I've just delivered the new patch with unit tests! :)

Comment 19

[Borja Salguero [:borjasalguero]]

Comment on attachment 802616 [details]
Pull request

Unit tests added! :) R?

Comment 20

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

Comment on attachment 802616 [details]
Pull request

r+ for system app part.

Comment 21

[Preeti Raghunath(:Preeti)]

Alive,

Please provide risk analysis, I will + once I hear from you.

Comment 22

[Fabien Cazenave [:kaze]]

Comment on attachment 802616 [details]
Pull request

r=me with nits addressed. Nice!

Comment 23

[Borja Salguero [:borjasalguero]]

The risk of this patch is slow. We have included unit tests to the changes, and BB now support scrolling :).

Comment 24

[Borja Salguero [:borjasalguero]]

https://github.com/mozilla-b2g/gaia/commit/723e8490aa8240ada5f56f6aa8ee210fde5d11d1
https://github.com/borjasalguero/gaia/commit/5b573ae5b099bb2e0a50e26c723a2f949bc2a26a

R+. Merged!

Comment 25

[Ryan VanderMeulen [:RyanVM]]

Backed out because either this or bug 883298 was causing Gaia UI Test failures.
https://github.com/mozilla-b2g/gaia/commit/27efce9861ccd468c30014b44b8354670d43c20f

https://tbpl.mozilla.org/php/getParsedLog.php?id=27715778&tree=B2g-Inbound

Comment 26

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

(In reply to Preeti Raghunath(:Preeti) from comment #21)
> Alive,
> 
> Please provide risk analysis, I will + once I hear from you.

I personally don't suggest this to be leo+. Leo is in a very very late timing now.
Indeed, this is a bug. But not really blocking.
I wonder we should apply some more building block refactor patch blocks this patch if we need this for v1-train.

I couldn't give any assurance here before I see the v1-train-toward patch. Sorry.

Comment 27

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

But indeed this should be koi+. Please nom.

Comment 28

[Borja Salguero [:borjasalguero]]

(In reply to Ryan VanderMeulen [:RyanVM UTC-4] from comment #25)
> Backed out because either this or bug 883298 was causing Gaia UI Test
> failures.
> https://github.com/mozilla-b2g/gaia/commit/
> 27efce9861ccd468c30014b44b8354670d43c20f
> 
> https://tbpl.mozilla.org/php/getParsedLog.php?id=27715778&tree=B2g-Inbound

Hi Ryan,
I've been checking this in my device and it's working properly. Furthermore the test case is:
https://moztrap.mozilla.org/manage/case/3449/
And code related:
https://github.com/mozilla/gaia-ui-tests/blob/master/gaiatest/tests/settings/test_settings_wallpaper.py

As you can see this has no relation with this patch. I've created a new PR https://github.com/mozilla-b2g/gaia/pull/12150, and I will try to merge this again and I hope that tests now will be working as expected!

Comment 29

[Borja Salguero [:borjasalguero]]

Landed again.

https://github.com/mozilla-b2g/gaia/commit/c0eda140451672f78ecb7ebc3e4c043a8c5870ad
https://github.com/borjasalguero/gaia/commit/e0fbfca5535bf8b4d5e7c35e8ca2d6521ecd5c76

Comment 30

[Borja Salguero [:borjasalguero]]

Ryan,
Im really concerned about the marionette test and why is failing. The error is:

raise TimeoutException('Element %s not present before timeout' % locator)
TEST-UNEXPECTED-FAIL | test_settings_wallpaper.py TestWallpaper.test_change_wallpaper | TimeoutException: Element a[data-value='0'] not present before timeout
Return code: 2560
Marionette exited with return code 2560: harness failures
# TBPL FAILURE #

I think that this error it's because the test, because it's not testing as it should be (this error is not reproducible in the Device). How could we fix this?

Comment 31

[Ryan VanderMeulen [:RyanVM]]

1.) WHY did you re-land this without verifying that the test was passing first?!
2.) I have nothing to do with these tests besides trying to keep the tree green.
3.) Back this out NOW.

Comment 32

[Anthony Ricaud (:rik)]

I had to back this out because it is causing a Gaia unit test failure.
https://github.com/mozilla-b2g/gaia/commit/270bf8c0255e2d43a1d8e78378b1c6dc75faff3d

1) [system] cards view > hide cardsview > "before each" hook:
TypeError: screen.mozLockOrientation is not a function
at showCardSwitcher (http://system.gaiamobile.org:8080/js/cards_view.js:176)
at (anonymous) (http://system.gaiamobile.org:8080/test/unit/cards_view_test.js:153)
at wrapper (http://test-agent.gaiamobile.org:8080/common/test/mocha_generators.js:60)
at run (http://test-agent.gaiamobile.org:8080/common/vendor/mocha/mocha.js:3709)
at next (http://test-agent.gaiamobile.org:8080/common/vendor/mocha/mocha.js:3973)
at (anonymous) (http://test-agent.gaiamobile.org:8080/common/vendor/mocha/mocha.js:3984)
at (anonymous) (http://test-agent.gaiamobile.org:8080/common/vendor/mocha/mocha.js:4932)

Comment 33

[Borja Salguero [:borjasalguero]]

Actually I've found the bug in the tests. The test should change this line to:
https://github.com/mozilla/gaia-ui-tests/blob/master/gaiatest/apps/settings/regions/display.py#L13

From_
_wallpaper_button_locator = (By.CSS_SELECTOR, "a[data-value='0']")
To:
_wallpaper_button_locator = (By.CSS_SELECTOR, "button[data-value='0']")

Comment 34

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

Looks like we learned a class that changes DOM structure would introduce problems to gaia-ui-tests. :/

Comment 35

[Borja Salguero [:borjasalguero]]

(In reply to Alive Kuo [:alive] from comment #34)
> Looks like we learned a class that changes DOM structure would introduce
> problems to gaia-ui-tests. :/

Actually the problem is getting UI-Tests tied to wrong selectors. In this case test should not use 'a' or 'button', because the selector '[data-value='0']' is enough. I will try to fix the Unit test of TBPL as well :(

Comment 36

[Borja Salguero [:borjasalguero]]

(In reply to Anthony Ricaud (:rik) from comment #32)

> TypeError: screen.mozLockOrientation is not a function
> at showCardSwitcher (http://system.gaiamobile.org:8080/js/cards_view.js:176)
> at (anonymous)
> (http://system.gaiamobile.org:8080/test/unit/cards_view_test.js:153)
> at wrapper

This bug is currently in Master. Is *not* related with this patch. I will try to fix this as well or check who added this.

Comment 37

[Jason Smith [:jsmith]]

FWIW - if you know what needs to be fixed in the UI Tests, then feel free to open a pull request against https://github.com/mozilla/gaia-ui-tests with the fix you need.

Comment 38

[Borja Salguero [:borjasalguero]]

Attachment: UI-TEST Fix

This is a small fix for the new layout in the action menu of System.

Comment 39

[Borja Salguero [:borjasalguero]]

Attachment: Origina PR with tests fixed.

Comment 40

[Borja Salguero [:borjasalguero]]

(In reply to Jason Smith [:jsmith] from comment #37)
> FWIW - if you know what needs to be fixed in the UI Tests, then feel free to
> open a pull request against https://github.com/mozilla/gaia-ui-tests with
> the fix you need.

Hi Jason. Sorry, I discovered the 'gaia-ui-test' error after landing in Gaia :(. I've created a patch for that https://github.com/mozilla/gaia-ui-tests/pull/1362 

Ryan, could you take a look? Thanks!

Comment 41

[Borja Salguero [:borjasalguero]]

Comment on attachment 803714 [details]
UI-TEST Fix

R+ by Bob Silverberg in GitHub

Comment 42

[Borja Salguero [:borjasalguero]]

Fix in gaia-ui-tests merged, and local unit tests in Green with double check, so I hope this will land properly without more issues with tests! :)

Comment 43

[Tony Chung [:tchung]]

triage leo+'d.  please uplift patches to v1-train.

Comment 44

[John Ford [:jhford] CET/CEST Berlin Time]

I was not able to uplift this bug to v1-train.  If this bug has dependencies which are not marked in this bug, please comment on this bug.  If this bug depends on patches that aren't approved for v1-train, we need to re-evaluate the approval.  Otherwise, if this is just a merge conflict, you might be able to resolve it with:

  git checkout v1-train
  git cherry-pick -x -m1 c0eda140451672f78ecb7ebc3e4c043a8c5870ad
  <RESOLVE MERGE CONFLICTS>
  git commit

Comment 45

[John Ford [:jhford] CET/CEST Berlin Time]

This bug was partially uplifted.

Uplifted c0eda140451672f78ecb7ebc3e4c043a8c5870ad to:
v1.2 already had this commit

Commit c0eda140451672f78ecb7ebc3e4c043a8c5870ad didn't uplift to branch v1-train

Comment 48

[John Ford [:jhford] CET/CEST Berlin Time]

This bug was partially uplifted.

Uplifted c0eda140451672f78ecb7ebc3e4c043a8c5870ad to:
v1.3 already had this commit

Commit c0eda140451672f78ecb7ebc3e4c043a8c5870ad didn't uplift to branch v1-train