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

Status: RESOLVED FIXED

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

Description

[Omega Feng [:Omega] [:馮於懋]]

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

Comment 1

[Joe Cheng [:jcheng] (please needinfo)]

Omega, there is no menu access in the main screen of message app, wonder where you propose to add this? Thanks

Comment 2

[Julien Wajsberg [:julienw]]

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?

Comment 3

[Omega Feng [:Omega] [:馮於懋]]

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"

Comment 4

[Joe Cheng [:jcheng] (please needinfo)]

triage: add to backlog

Comment 5

[Oleg Zasypkin [:azasypkin]]

Attachment: GitHub pull request URL

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 :)

Comment 6

[Julien Wajsberg [:julienw]]

(omega already answered in comment 3)

Comment 7

[Julien Wajsberg [:julienw]]

Comment on attachment 8404688 [details] [review]
GitHub pull request URL

looks good but I'd like some unit test ;)

Comment 8

[Oleg Zasypkin [:azasypkin]]

(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 :)

Comment 9

[Julien Wajsberg [:julienw]]

Comment on attachment 8404688 [details] [review]
GitHub pull request URL

So, you need to update the python tests as well ;)

Comment 10

[Julien Wajsberg [:julienw]]

Ask review from zac and me when you're ready !

Comment 11

[Oleg Zasypkin [:azasypkin]]

Comment on attachment 8404688 [details] [review]
GitHub pull request URL

Hey Zac!

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

Thanks!

Comment 12

[Zac C (:zac)]

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!

Comment 13

[Oleg Zasypkin [:azasypkin]]

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.

Comment 14

[Julien Wajsberg [:julienw]]

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.

Comment 15

[Oleg Zasypkin [:azasypkin]]

(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.

Comment 16

[Zac C (:zac)]

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.

Comment 17

[Oleg Zasypkin [:azasypkin]]

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!

Comment 18

[Julien Wajsberg [:julienw]]

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.

Comment 19

[Zac C (:zac)]

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.

Comment 20

[Oleg Zasypkin [:azasypkin]]

(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 :)

Comment 21

[Julien Wajsberg [:julienw]]

Ah yeah I forgot about wait_for_element_displayed :)

Comment 22

[Oleg Zasypkin [:azasypkin]]

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.

Comment 23

[Zac C (:zac)]

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.

Comment 24

[Julien Wajsberg [:julienw]]

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)

Comment 25

[Oleg Zasypkin [:azasypkin]]

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.

Comment 26

[Julien Wajsberg [:julienw]]

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 !

Comment 27

[Oleg Zasypkin [:azasypkin]]

Applied suggested test change, squashed and filed follow-up bug 998287 as discussed at GitHub :)

Comment 28

[Ryan VanderMeulen [:RyanVM]]

Master: https://github.com/mozilla-b2g/gaia/commit/e343cab5603cf2ff0ffa663c1a53eb2f012c2a80