Closed Bug 988711 Opened 6 years ago Closed 6 years ago

[Messages] There is a Settings entry in each single message

Categories

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

ARM
Gonk (Firefox OS)
defect
Not set

Tracking

(tracking-b2g:backlog, b2g-v2.0 fixed)

RESOLVED FIXED
2.0 S1 (9may)
tracking-b2g backlog
Tracking Status
b2g-v2.0 --- fixed

People

(Reporter: Omega, Assigned: azasypkin)

References

Details

(Whiteboard: ux-most-wanted)

Attachments

(1 file)

46 bytes, text/x-github-pull-request
zcampbell
: review+
julienw
: review+
julienw
: feedback+
Details | Review
Repro Steps:
1) In Messages app, tap an existing message
2) Tap menu

Actual:
There is a Settings entry

Expected:
The Settings entry should move to the main screen of Messages app

OS version: 1.3.0.0-prerelease
Platform version: 28.0a2
Build identifier: 20140128164615
Update channel hamachi/1.3.0/nightly
Omega, there is no menu access in the main screen of message app, wonder where you propose to add this? Thanks
Flags: needinfo?(ofeng)
Joe, yes there is one :) (I don't remember if it was added in 1.3 or 1.4 though, Steve would know as he did it).

Omega, we have currently access to the settings from both places, in master. Can you try a master build and tell us what you'd like to change?
Yes, I've tried the master build. The entry in the "Main screen > Menu" is what I need. So we can just remove the entries from the "New message > Menu" and "Existing message > Menu"
triage: add to backlog
blocking-b2g: 1.5? → backlog
Assignee: nobody → azasypkin
Status: NEW → ASSIGNED
Settings entry removed from the following places:

"New message > Menu";
"Existing message > Menu".

Note: for "New message" case we now have only one option left 'Add\Remove Subject'. Haven't found anything against it in Firefox OS guidelines though, but read something similar in Windows Metro guidelines some time ago - so just curios if we have something on that :)
Attachment #8404688 - Flags: review?(felash)
(omega already answered in comment 3)
Flags: needinfo?(ofeng)
Comment on attachment 8404688 [details] [review]
GitHub pull request URL

looks good but I'd like some unit test ;)
Attachment #8404688 - Flags: review?(felash) → feedback+
(In reply to Julien Wajsberg [:julienw] from comment #7)
> Comment on attachment 8404688 [details] [review]
> GitHub pull request URL
> 
> looks good but I'd like some unit test ;)

Thanks, sorry, forgot to push updated unit-test file - pushed it now. Please, let me know if you think we can test more than that. r? once again :)
Attachment #8404688 - Flags: review?(felash)
Whiteboard: ux-most-wanted
Blocks: 994991
Comment on attachment 8404688 [details] [review]
GitHub pull request URL

So, you need to update the python tests as well ;)
Attachment #8404688 - Flags: review?(felash)
Ask review from zac and me when you're ready !
Comment on attachment 8404688 [details] [review]
GitHub pull request URL

Hey Zac!

Can you please review my changes in gaia-ui-tests?

Thanks!
Attachment #8404688 - Flags: review?(zcampbell)
Comment on attachment 8404688 [details] [review]
GitHub pull request URL

One test is failing on Hamachi device but apart from that I'm impressed with how you've adopted the test style well, thanks for your work!
Attachment #8404688 - Flags: review?(zcampbell) → review-
Comment on attachment 8404688 [details] [review]
GitHub pull request URL

(In reply to Zac C (:zac) from comment #12)
> Comment on attachment 8404688 [details] [review]
> GitHub pull request URL
> 
> One test is failing on Hamachi device but apart from that I'm impressed with
> how you've adopted the test style well, thanks for your work!

Thanks for review! We discussed with Julien how to fix Messages.launch method. There were two options:

#1 Rely on events generated by PerformanceTestingHelper. The problem with this approach is that it works only when "window.mozPerfHasListener" is true. It's enabled for performance tests only. So, not sure about consequences that may occur if we enable it for the rest of tests.

#2 Rely on 'window.onload' event + timeout to be sure that all onload handlers have finished processing.

Current PR includes approach #2.
Attachment #8404688 - Flags: review- → review?(zcampbell)
Zac, I remember we took approach #1 for other apps, maybe you can point to the existing code for them?

Approch #2 should work for us... now. But it may break when we start doing more lazy loading. So it's less future-proof.
(In reply to Julien Wajsberg [:julienw] from comment #14)
> Zac, I remember we took approach #1 for other apps, maybe you can point to
> the existing code for them?
> 
> Approch #2 should work for us... now. But it may break when we start doing
> more lazy loading. So it's less future-proof.

Julien, are you referring to bug 922609 where #1 approach was discussed?

I also see two more approaches that are used across apps:

#3 Apps are setting special 'ready' css class to some node inside the app and relying on it in tests - looks hacky as well as approach #2 :)

#4 Apps are setting special variable in global objects, like ThreadsUI.ready - true...

Ideally I'd rather fire not performance related event as this has another purpose, but smth like custom window event 'app-ready' with details about app. Every app decides when it's really ready for interaction.
Yes #3 and #4 is what we try to do in the UI tests mostly as we'd prefer to interact in a passive manner with the app. CSS class events like this often correspond with the hiding of a loading overlay so are quite convenient.
I had a very quick look through the Messages js code and I couldn't see any kind of global that would be useful to mean "loaded" but I'm not terribly familiar with the code.
Ok, there are a plenty of ways to detect app's ready state, but let's choose one and move forward :) I'm still new here, so probably can't see all pros and cons of every approach, so I'll just recap what I think about every approach:

#0 Use custom global event like 'app-ready'
  Pros: 
    * looks natural for me to have such event as potentially can be reused for other purposes. I'm thinking whether infrastructure code(b2g, gecko) that runs our app may need to know when app is ready to interact. That reminds me Win Metro apps that should notify OS that app is ready for the user, and if app can't do so in the max provided time period, OS shuts down app as unresponsive. Probably we may need something like this in the future.  
    * it has only one purpose that we actually need in tests - to indicate that app is ready for user interaction.
  Cons: 
    * we don't have it now, so we need to add it
    * we should not forget about it when we change app 'readiness' conditions (true for all cases though).
    * use execute_async_script to work with it in tests (the only way I know)

#1 Use PerformanceTestingHelper events
  Pros: 
    * we already have it
  Cons: 
    * it just has different performance related purpose, so using it in our particular case looks a bit hacky for me
    * can't use it as it's enabled for performance tests only. Is there any rationale behind that?
    * use execute_async_script to work with it in tests

#2 Use window.onload + timeout
  Pros: 
    * The only pros is that it just works right now, without any side modifications in the code or test infrastructure
  Cons:
    * Too unreliable, timeouts it's a disaster in a long term
    * use execute_async_script to work with it in tests (the only way I know)

#3 Use 'ready' CSS class
  Pros:
    * Can be reused for appropriate styling of the ready app, so looks reasonable to have one
    * No need in using execute_async_script, so tests are much simpler
  Cons:
    * We don't have it right now (at least I don't see something similar)
    * This class should be added by single peace of code. But as app potentially can be loaded directly into any panel\view (like opening SMS app directly from Compose view not from Inbox) that can have it's own criteria for 'readiness' it should somehow signal about it's readiness to this 'single peace of code' to set the appropriate css class that can be achieved with #0 for example :)

#4 Use global 'ready' field has almost the same pros and cons as #3, but it can be used for styling.

So let me guys know what you prefer and I'll do it!
Flags: needinfo?(felash)
Flags: needinfo?(zcampbell)
I like a "ready" class on the body.

I think we need 2 things from this:
* know if the "ready" event already happened (like a "document.readyState === 'complete'" for the application)
* if it has not happened already, have a way to know when it happens (like document's events "readystatechange", "load", "DOMContentReady")

A "ready" class on the body would do body: the first is obvious, the second using a mutation observer.
Flags: needinfo?(felash)
Marionette already has lots of handling for generic stuff like document.readyState but any lazy loading or stuff after the normal events will completely mess with the test.

I've got a bit of a backlog at the moment but I'll try and run Oleg's setup as is tomorrow and give feedback. I am not entirely convinced it will work as we already wait for the load event in the javascript atom that launches the Messages app. It's all the stuff that goes on after the load event that causes issues.
(In reply to Julien Wajsberg [:julienw] from comment #18)
> I like a "ready" class on the body.
> 
Ok! As per comment 16, it should work for Zac too :)

> A "ready" class on the body would do body: the first is obvious, the second
> using a mutation observer.
I guess you mean using MutationObserver only for in app use when\if it's needed, because for marionette tests we can just use wait_for_element_displayed based on 'app-ready' CSS selector.

(In reply to Zac C (:zac) from comment #19)
> Marionette already has lots of handling for generic stuff like
> document.readyState but any lazy loading or stuff after the normal events
> will completely mess with the test.
> 
> I've got a bit of a backlog at the moment but I'll try and run Oleg's setup
> as is tomorrow and give feedback. I am not entirely convinced it will work
> as we already wait for the load event in the javascript atom that launches
> the Messages app. It's all the stuff that goes on after the load event that
> causes issues.

Thanks! As now we all agreed on option #3 I'll update tests accordingly and ping you once it's ready so you don't need to run it twice :)
Ah yeah I forgot about wait_for_element_displayed :)
I've updated my PR with code that manages App's readiness(at least how I see it) + updated python test accordingly. Julien, could you please take a look?

But while doing so I noticed that we have b2g bug 958738 that prevents us from testing anything related to message threads inside b2g including our new code for app readiness(as we set ready flag only once threads are rendered). I've added small workaround for that in PR, so that we can run it locally and on travis (does travis use b2g to run gaia-ui-tests?). But probably we need to discuss it further.

Zac, it would be great if you can verify tests from the latest PR. Even if we change something in our app readiness code while review it shouldn't affect tests as now we're relying on the css ready class.
Attachment #8404688 - Flags: feedback+ → feedback?
Depends on: 958738
Attachment #8404688 - Flags: feedback? → feedback?(felash)
Comment on attachment 8404688 [details] [review]
GitHub pull request URL

The tests run excellently on device now!
On the test side I am r+

I'll leave the remaining r? and merge to Julien.
Attachment #8404688 - Flags: review?(zcampbell) → review+
Flags: needinfo?(zcampbell)
Comment on attachment 8404688 [details] [review]
GitHub pull request URL

looks good,

some comments in the tests especially, but good work !

(adding r- just to show to any visitor that it's not ready do land)
Attachment #8404688 - Flags: review-
Attachment #8404688 - Flags: feedback?(felash)
Attachment #8404688 - Flags: feedback+
Comment on attachment 8404688 [details] [review]
GitHub pull request URL

(In reply to Julien Wajsberg [:julienw] from comment #24)
> Comment on attachment 8404688 [details] [review]
> GitHub pull request URL
> 
> looks good,
> 
> some comments in the tests especially, but good work !
> 
> (adding r- just to show to any visitor that it's not ready do land)

Thanks for review! I've updated PR as we discussed. Travis is green, so asking r? again.
Attachment #8404688 - Flags: review- → review?(felash)
Comment on attachment 8404688 [details] [review]
GitHub pull request URL

r=me with the additional nits and maybe the suggested test change if you think it's useful.

Please squash, put r=julien,zac in the commit log, and put 'checkin-needed' as keyword once you're ready :)

Thanks !
Attachment #8404688 - Flags: review?(felash) → review+
Applied suggested test change, squashed and filed follow-up bug 998287 as discussed at GitHub :)
Keywords: checkin-needed
Master: https://github.com/mozilla-b2g/gaia/commit/e343cab5603cf2ff0ffa663c1a53eb2f012c2a80
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → 2.0 S1 (9may)
blocking-b2g: backlog → ---
You need to log in before you can comment on or make changes to this bug.