Closed
Bug 912904
Opened 11 years ago
Closed 11 years ago
[System][Dialogs][ActionMenu] Dialogs & Action menus (BB) are not scrolling as expected when there is a large list of items.
Categories
(Firefox OS Graveyard :: Gaia::System, defect)
Tracking
(blocking-b2g:leo+, b2g-v1.2 fixed, b2g-v1.3 fixed)
RESOLVED
FIXED
blocking-b2g | leo+ |
People
(Reporter: borjasalguero, Assigned: borjasalguero)
References
Details
Attachments
(7 files)
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•11 years ago
|
Comment 1•11 years ago
|
||
QA,
Is picker scrollable?
Also please check if this is a regression from 1.0.1?
Keywords: qawanted
Updated•11 years ago
|
QA Contact: laliaga
Comment 2•11 years ago
|
||
Comment 3•11 years ago
|
||
Comment 4•11 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
Comment 5•11 years ago
|
||
(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•11 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.
Comment 7•11 years ago
|
||
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•11 years ago
|
||
(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.
Assignee | ||
Updated•11 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•11 years ago
|
||
Assignee | ||
Comment 12•11 years ago
|
||
This is a small patch for fixing the scrolling issue in the BB.
Attachment #802616 -
Flags: review?(kaze)
Assignee | ||
Comment 13•11 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•11 years ago
|
||
Thanks Arnau for the tip!! :)
Comment 15•11 years ago
|
||
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•11 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•11 years ago
|
Attachment #802616 -
Flags: review?(arnau)
Comment 17•11 years ago
|
||
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•11 years ago
|
||
Your patch is included in my PR. I've just delivered the new patch with unit tests! :)
Assignee | ||
Comment 19•11 years ago
|
||
Comment on attachment 802616 [details]
Pull request
Unit tests added! :) R?
Attachment #802616 -
Flags: review?(alive)
Comment 20•11 years ago
|
||
Comment on attachment 802616 [details]
Pull request
r+ for system app part.
Attachment #802616 -
Flags: review?(alive) → review+
Comment 21•11 years ago
|
||
Alive,
Please provide risk analysis, I will + once I hear from you.
Flags: needinfo?(alive)
Comment 22•11 years ago
|
||
Comment on attachment 802616 [details]
Pull request
r=me with nits addressed. Nice!
Attachment #802616 -
Flags: review?(kaze) → review+
Assignee | ||
Comment 23•11 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
Closed: 11 years ago
Resolution: --- → FIXED
Assignee | ||
Comment 24•11 years ago
|
||
Comment 25•11 years ago
|
||
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 → ---
Comment 26•11 years ago
|
||
(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)
Comment 27•11 years ago
|
||
But indeed this should be koi+. Please nom.
Assignee | ||
Comment 28•11 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•11 years ago
|
||
Landed again.
https://github.com/mozilla-b2g/gaia/commit/c0eda140451672f78ecb7ebc3e4c043a8c5870ad
https://github.com/borjasalguero/gaia/commit/e0fbfca5535bf8b4d5e7c35e8ca2d6521ecd5c76
Status: REOPENED → RESOLVED
Closed: 11 years ago → 11 years ago
Resolution: --- → FIXED
Assignee | ||
Comment 30•11 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)
Comment 31•11 years ago
|
||
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)
Comment 32•11 years ago
|
||
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•11 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']")
Comment 34•11 years ago
|
||
Looks like we learned a class that changes DOM structure would introduce problems to gaia-ui-tests. :/
Assignee | ||
Comment 35•11 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•11 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.
Comment 37•11 years ago
|
||
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•11 years ago
|
||
This is a small fix for the new layout in the action menu of System.
Attachment #803714 -
Flags: review?(ryanvm)
Assignee | ||
Comment 39•11 years ago
|
||
Assignee | ||
Comment 40•11 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•11 years ago
|
Attachment #803714 -
Flags: review?(ryanvm) → review?(zcampbell)
Assignee | ||
Comment 41•11 years ago
|
||
Comment on attachment 803714 [details]
UI-TEST Fix
R+ by Bob Silverberg in GitHub
Attachment #803714 -
Flags: review?(zcampbell) → review+
Assignee | ||
Updated•11 years ago
|
Attachment #802616 -
Flags: review?(arnau)
Assignee | ||
Comment 42•11 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
Closed: 11 years ago → 11 years ago
Resolution: --- → FIXED
Assignee | ||
Updated•11 years ago
|
Flags: needinfo?(ryanvm)
Comment 43•11 years ago
|
||
triage leo+'d. please uplift patches to v1-train.
blocking-b2g: leo? → leo+
Comment 44•11 years ago
|
||
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)
Comment 45•11 years ago
|
||
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
Assignee | ||
Updated•11 years ago
|
Flags: needinfo?(borja.bugzilla)
Comment 48•11 years ago
|
||
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.
Description
•