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

RESOLVED FIXED

Status

Firefox OS
Gaia::System
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: borjasalguero, Assigned: borjasalguero)

Tracking

(Blocks: 1 bug)

unspecified
ARM
Gonk (Firefox OS)

Firefox Tracking Flags

(blocking-b2g:leo+, b2g-v1.2 fixed, b2g-v1.3 fixed)

Details

Attachments

(7 attachments)

(Assignee)

Description

4 years ago
Created attachment 800032 [details]
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.
(Assignee)

Updated

4 years ago
Assignee: nobody → fbsc
Blocks: 869434
blocking-b2g: --- → leo?
QA,

Is picker scrollable?

Also please check if this is a regression from 1.0.1?
Keywords: qawanted

Updated

4 years ago
QA Contact: laliaga

Comment 2

4 years ago
Created attachment 800890 [details]
LucasA_Activity Picker.png

Comment 3

4 years ago
Created attachment 800891 [details]
LucasA_Installed sharing apps.png

Comment 4

4 years ago
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
Keywords: qawanted
(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.
Keywords: qawanted
(Assignee)

Comment 6

4 years ago
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.
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.
(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

4 years ago
removing keyword per comment 8
Keywords: qawanted
(Assignee)

Updated

4 years ago
Summary: [System] Activity picker screen is not scrolling as expected. → [System][Dialogs][ActionMenu] Dialogs & Action menus (BB) are not scrolling as expected when there is a large list of items.
(Assignee)

Comment 10

4 years ago
Created attachment 802167 [details]
Issue in other screens as send report one
(Assignee)

Updated

4 years ago
Duplicate of this bug: 859708
(Assignee)

Comment 12

4 years ago
Created attachment 802616 [details]
Pull request

This is a small patch for fixing the scrolling issue in the BB.
Attachment #802616 - Flags: review?(kaze)
(Assignee)

Comment 13

4 years ago
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!
Attachment #802616 - Flags: review?(alive)
(Assignee)

Comment 14

4 years ago
Thanks Arnau for the tip!! :)
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.
Attachment #802616 - Flags: review?(alive)
(Assignee)

Comment 16

4 years ago
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! ;)
(Assignee)

Updated

4 years ago
Attachment #802616 - Flags: review?(arnau)
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.
(Assignee)

Comment 18

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

Comment 19

4 years ago
Comment on attachment 802616 [details]
Pull request

Unit tests added! :) R?
Attachment #802616 - Flags: review?(alive)
Comment on attachment 802616 [details]
Pull request

r+ for system app part.
Attachment #802616 - Flags: review?(alive) → review+
Alive,

Please provide risk analysis, I will + once I hear from you.
Flags: needinfo?(alive)
Comment on attachment 802616 [details]
Pull request

r=me with nits addressed. Nice!
Attachment #802616 - Flags: review?(kaze) → review+
(Assignee)

Comment 23

4 years ago
The risk of this patch is slow. We have included unit tests to the changes, and BB now support scrolling :).
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
(Assignee)

Comment 24

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

R+. Merged!
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
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(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.
Flags: needinfo?(alive)
But indeed this should be koi+. Please nom.
(Assignee)

Comment 28

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

Comment 29

4 years ago
Landed again.

https://github.com/mozilla-b2g/gaia/commit/c0eda140451672f78ecb7ebc3e4c043a8c5870ad
https://github.com/borjasalguero/gaia/commit/e0fbfca5535bf8b4d5e7c35e8ca2d6521ecd5c76
Status: REOPENED → RESOLVED
Last Resolved: 4 years ago4 years ago
Resolution: --- → FIXED
(Assignee)

Comment 30

4 years ago
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?
Flags: needinfo?(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.
Flags: needinfo?(ryanvm)
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)
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
(Assignee)

Comment 33

4 years ago
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']")
Looks like we learned a class that changes DOM structure would introduce problems to gaia-ui-tests. :/
(Assignee)

Comment 35

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

Comment 36

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

Comment 38

4 years ago
Created attachment 803714 [details]
UI-TEST Fix

This is a small fix for the new layout in the action menu of System.
Attachment #803714 - Flags: review?(ryanvm)
(Assignee)

Comment 39

4 years ago
Created attachment 803716 [details]
Origina PR with tests fixed.
(Assignee)

Comment 40

4 years ago
(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!
Flags: needinfo?(ryanvm)

Updated

4 years ago
Attachment #803714 - Flags: review?(ryanvm) → review?(zcampbell)
(Assignee)

Comment 41

4 years ago
Comment on attachment 803714 [details]
UI-TEST Fix

R+ by Bob Silverberg in GitHub
Attachment #803714 - Flags: review?(zcampbell) → review+
(Assignee)

Updated

4 years ago
Attachment #802616 - Flags: review?(arnau)
(Assignee)

Comment 42

4 years ago
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! :)
Status: REOPENED → RESOLVED
Last Resolved: 4 years ago4 years ago
Resolution: --- → FIXED
(Assignee)

Updated

4 years ago
Flags: needinfo?(ryanvm)
triage leo+'d.  please uplift patches to v1-train.
blocking-b2g: leo? → leo+
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
Flags: needinfo?(fbsc)
This bug was partially uplifted.

Uplifted c0eda140451672f78ecb7ebc3e4c043a8c5870ad to:
v1.2 already had this commit

Commit c0eda140451672f78ecb7ebc3e4c043a8c5870ad didn't uplift to branch v1-train
status-b2g-v1.2: --- → fixed
Duplicate of this bug: 859713
Duplicate of this bug: 912070
(Assignee)

Updated

4 years ago
Flags: needinfo?(borja.bugzilla)
This bug was partially uplifted.

Uplifted c0eda140451672f78ecb7ebc3e4c043a8c5870ad to:
v1.3 already had this commit

Commit c0eda140451672f78ecb7ebc3e4c043a8c5870ad didn't uplift to branch v1-train
status-b2g-v1.3: --- → fixed
You need to log in before you can comment on or make changes to this bug.