Closed Bug 1078295 Opened 5 years ago Closed 5 years ago

SMS app dialogs are missing all transitions

Categories

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

All
Gonk (Firefox OS)
defect
Not set

Tracking

(b2g-v2.1 affected, b2g-v2.2 verified, b2g-master verified)

RESOLVED FIXED
2.1 S7 (24Oct)
Tracking Status
b2g-v2.1 --- affected
b2g-v2.2 --- verified
b2g-master --- verified

People

(Reporter: cwiiis, Assigned: cwiiis)

References

Details

(Whiteboard: [systemsfe])

Attachments

(2 files)

If you open up any overflow menus/dialogs in SMS, and indeed when you close them, there are no transitions. This is contrary to the same elements in other applications (such as the system browser) and just doesn't feel very good given that we have transitions for almost every other interaction.

It's a reasonably easy fix, so assigning to myself.
Add transitions that match those in the system browser for the same element.
Attachment #8500483 - Flags: review?(felash)
(In reply to Chris Lord [:cwiiis] from comment #1)
> Created attachment 8500483 [details] [review]
> Add transitions to SMS app dialogs
> 
> Add transitions that match those in the system browser for the same element.

Thanks for sync up with system behavior, but some side effects here is you change dialog form styling directly:
- The message selection form is also dialog form but it's not behaves like a dialog. So the styling changes will make the selection UI invisible. Please use other role or other tag for representing the selection UI, or override the dialog styling in selection UI.
- Attachment action menu is not visible because we have a slightly different structure than general option menu, maybe it's time to remove it from html and apply shared option menu instead.
Attachment #8500483 - Flags: review?(felash)
Comment on attachment 8500483 [details] [review]
Add transitions to SMS app dialogs

Comments addressed, re-flagging for review.
Attachment #8500483 - Flags: review?(schung)
Comment on attachment 8500483 [details] [review]
Add transitions to SMS app dialogs

It looks good overall, but it got one gij error in gaia-try. Please see sms marionette lib selectMenuOption method that we already added some workaround to delay the tap action on menu, and we might need a longer delay because of animation applied here. Sorry for the late review.
Attachment #8500483 - Flags: review?(schung)
Comment on attachment 8500483 [details] [review]
Add transitions to SMS app dialogs

Hopefully all comments addressed again and all tests fixed.

I added two unit tests to make sure that multiple calls of show() have no effect and I fixed the failing marionette test by increasing the wait slightly and also having it wait for the menu to disappear after selecting an option (I had to change the attachment menu element for this to work correctly, as it's the form that disappears rather than its div parent).
Attachment #8500483 - Flags: review?(schung)
hmm, I thought the test_dialer_add_contact.py test failure was unrelated, but perhaps it isn't... Looking into it.
Comment on attachment 8500483 [details] [review]
Add transitions to SMS app dialogs

Confirmed, a real error - unflagging till I've fixed.
Attachment #8500483 - Flags: review?(schung)
hmm, one more failure that succeeds locally, but takes 22 seconds to do so - will look into it...
Comment on attachment 8500483 [details] [review]
Add transitions to SMS app dialogs

Added unit tests and integration test failures addressed.

Ends up I didn't realise the OptionMenu shared JS was used in the dialer as well, so I moved the CSS to a shared location and included that in dialer. So this now fixes a couple of missing transitions in the dialer app too.
Attachment #8500483 - Flags: review?(schung)
Comment on attachment 8500483 [details] [review]
Add transitions to SMS app dialogs

Since it also impacts Dialer, requesting a review from Doug as well.
Attachment #8500483 - Flags: review?(drs+bugzilla)
Only some unit test required, so I still keeps the review flag here. Please ping me once you complete.
Tests added, hopefully :)
Flags: needinfo?(schung)
Attachment #8500483 - Flags: review?(drs.bugzilla) → review+
Comment on attachment 8500483 [details] [review]
Add transitions to SMS app dialogs

Looks good now, thanks for all the efforts and tests.
Attachment #8500483 - Flags: review?(schung) → review+
Flags: needinfo?(schung)
Trigger the gij test again because of timeout.
Keywords: checkin-needed
Had to trigger again, but seeing as there are a lot of similar reports, I don't think the failures are caused by this patch (which will make some tests take slightly longer to run, but not by a large amount).

Merged: https://github.com/mozilla-b2g/gaia/commit/4ab6a8be1bf3282a944c95471e6649b17393bcf0
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Comment on attachment 8500483 [details] [review]
Add transitions to SMS app dialogs

[Approval Request Comment]
[Bug caused by] (feature/regressing bug #): Missing transitions in SMS and dialer apps
[User impact] if declined: Poor user experience (transitions in some places and not in others)
[Testing completed]: Tested locally (running on my phone for some days), covered quite comprehensively by tests
[Risk to taking this patch] (and alternatives if risky): Low risk of some dialogs not disappearing or showing when they should (though local and automated testing shows no such problems)
[String changes made]: None
Attachment #8500483 - Flags: approval-gaia-v2.1?
Keywords: checkin-needed
Target Milestone: --- → 2.1 S7 (24Oct)
Whiteboard: [systemsfe]
Chris, can you confirm if this is a regression in 2.1 or are you see the same behavior in 2.0 ?
Flags: needinfo?(chrislord.net)
(In reply to bhavana bajaj [:bajaj] from comment #17)
> Chris, can you confirm if this is a regression in 2.1 or are you see the
> same behavior in 2.0 ?

I don't think this code has changed, so this same behaviour almost certainly exists in 2.0.
Flags: needinfo?(chrislord.net)
Depends on: 1084252
(In reply to Chris Lord [:cwiiis] from comment #16)
> [Risk to taking this patch] (and alternatives if risky): Low risk of some
> dialogs not disappearing or showing when they should (though local and
> automated testing shows no such problems)

Looks like automated testing wasn't comprehensive enough given bug 1084252, so revising this to medium risk. Would require uplift of bug 1084252 (where I'll also add a marionette test for said failure).
(In reply to Chris Lord [:cwiiis] from comment #19)
> (In reply to Chris Lord [:cwiiis] from comment #16)
> > [Risk to taking this patch] (and alternatives if risky): Low risk of some
> > dialogs not disappearing or showing when they should (though local and
> > automated testing shows no such problems)
> 
> Looks like automated testing wasn't comprehensive enough given bug 1084252,
> so revising this to medium risk. Would require uplift of bug 1084252 (where
> I'll also add a marionette test for said failure).

I'm confident the fix I've crafted for this will stop this issue from occuring, so revising this again to very low risk, once bug 1084252 is fixed.
(In reply to Chris Lord [:cwiiis] from comment #18)
> (In reply to bhavana bajaj [:bajaj] from comment #17)
> > Chris, can you confirm if this is a regression in 2.1 or are you see the
> > same behavior in 2.0 ?
> 
> I don't think this code has changed, so this same behaviour almost certainly
> exists in 2.0.

Given this, and we want to avoid non-blocking in 2.1 at this point this is better-off getting fixed in 2.2 and along with 1084252. If the experience is really annoying and this looks like a ship-blocker please request blocking nomination once this is baked on trunk for a bit to consider uplift.
Attachment #8500483 - Flags: approval-gaia-v2.1? → approval-gaia-v2.1-
Depends on: 1088754
Depends on: 1094285
Depends on: 1112868
Depends on: 1091299
This bug has been verified as "pass" on latest nightly build of Flame v2.2&Master by the STR in Comment 0.

Actual results: When the optionMenu appears/disappears in Messages app and Dialer app, user can see the transitions.
See attachment: verified_Flame_v2.2.3gp
Reproduce rate: 0/10


Device: Flame v2.2 (Verified) 
Build ID               20150713002503
Gaia Revision          84d0c76370dcd3d25813b00de55194730884355b
Gaia Date              2015-07-09 13:09:14
Gecko Revision         https://hg.mozilla.org/releases/mozilla-b2g37_v2_2/rev/8d59402ba85a
Gecko Version          37.0
Device Name            flame
Firmware(Release)      4.4.2
Firmware(Incremental)  eng.cltbld.20150713.041147
Firmware Date          Mon Jul 13 04:11:59 EDT 2015
Bootloader             L1TC000118D0

Device: Flame master (Verified)
Build ID               20150713010204
Gaia Revision          e4b63559eba364892867eb381c3002d6518e5d6a
Gaia Date              2015-07-10 14:29:23
Gecko Revision         https://hg.mozilla.org/mozilla-central/rev/eab21ec484bb
Gecko Version          42.0a1
Device Name            flame
Firmware(Release)      4.4.2
Firmware(Incremental)  eng.cltbld.20150713.043935
Firmware Date          Mon Jul 13 04:39:47 EDT 2015
Bootloader             L1TC000118D0
QA Whiteboard: [MGSEI-Triage+]
You need to log in before you can comment on or make changes to this bug.