Closed Bug 1235842 Opened 8 years ago Closed 8 years ago

Intermittent TEST-UNEXPECTED-FAIL | apps/sms/test/marionette/new_activity_test.js | Messages as "new" activity target Send new message Activity with malicious content

Categories

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

Product:
Firefox OS Graveyard
Component:
Gaia::SMS
Platform:
x86
macOS
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED FIXED

People

(Reporter: mikehenrty, Assigned: steveck)

References

Details

(Keywords: intermittent-failure, Whiteboard: [MJS])

Attachments

(1 file)

[gaia] steveck-chung:Bug-1235842-header-a11y-intermittent > mozilla-b2g:master
46 bytes, text/x-github-pull-request
azasypkin
: review+
azasypkin
: feedback+
Details | Review 
TEST-UNEXPECTED-FAIL | apps/sms/test/marionette/new_activity_test.js | Messages as "new" activity target Send new message Activity with malicious content
Error: timeout exceeded!
    at Object.Client.waitForSync (node_modules/marionette-client/lib/marionette/client.js:760:16)
    at Object.Client.waitFor (node_modules/marionette-client/lib/marionette/client.js:726:60)
    at Object.MarionetteHelper.waitForElement (node_modules/marionette-helper/index.js:142:12)
    at Object.messageInput (apps/sms/test/marionette/lib/views/shared/composer_accessors.js:21:31)
    at apps/sms/test/marionette/new_activity_test.js:97:36
    at modifiedTest (node_modules/marionette-client/lib/marionette/client.js:722:22)
    at Object.Client.waitForSync (node_modules/marionette-client/lib/marionette/client.js:754:9)
    at Object.Client.waitFor (node_modules/marionette-client/lib/marionette/client.js:726:60)
    at Context.<anonymous> (apps/sms/test/marionette/new_activity_test.js:96:44)
    at Test.MarionetteTest.run (node_modules/marionette-js-runner/lib/ui.js:25:31)

https://treeherder.mozilla.org/logviewer.html#?job_id=3247541&repo=gaia
Component: Gaia::Search → Gaia::SMS
Yeah, I've seen this as well.
Flags: needinfo?(azasypkin)
We also sometimes see this failure message as well:
TEST-UNEXPECTED-FAIL | apps/sms/test/marionette/new_activity_test.js | Messages as "new" activity target Send new message Activity with "body" and contact "number"

https://treeherder.mozilla.org/logviewer.html#?job_id=19187853&repo=mozilla-inbound
See Also: → 1236248
See Also: → 1236417
See Also: → 1236446
All these "See Also" bugs are likely will have a single fix.
See Also: → 1236542
Found another error message:
  error: timeout exceeded!
      at Object.Client.waitForSync (node_modules/marionette-client/lib/marionette/client.js:761:16)
      at Object.Client.waitFor (node_modules/marionette-client/lib/marionette/client.js:727:60)
      at Object.MarionetteHelper.waitForElement (node_modules/marionette-helper/index.js:143:12)
      at Object.headerTitle (apps/sms/test/marionette/lib/views/conversation/accessors.js:48:31)
      at Context.<anonymous> (apps/sms/test/marionette/new_activity_test.js:123:44)
      at Test.MarionetteTest.run (node_modules/marionette-js-runner/lib/ui.js:25:31)

Actually I can only reproduce this error in local or vagrant tool. Not sure way the headertext element could not be accessible at this moment.
Hi Oleg, based on comment 6 I found a weird behavior that messagesApp.Conversation.headerTitle is not accessible, but I can use element.scriptWith to fetch textContent like:

messagesApp.Conversation.header.scriptWith(function(el) {
  return el.querySelector('#messages-header-text').textContent;
}

Do you have any idea about this issue?
(In reply to Steve Chung [:steveck] from comment #7)
> Hi Oleg, based on comment 6 I found a weird behavior that
> messagesApp.Conversation.headerTitle is not accessible, but I can use
> element.scriptWith to fetch textContent like:
> 
> messagesApp.Conversation.header.scriptWith(function(el) {
>   return el.querySelector('#messages-header-text').textContent;
> }
> 
> Do you have any idea about this issue?

Well, I investigated it a bit and I see that "client.helper.waitForElement" doesn't return anything because header title is considered as "not accessible". And it's not surprising because we have aria-hidden=true on the header title. Surprising is that Screen Reader doesn't see header title only when user is forwarded from New Message view to Conversation view :) And I'm not sure why do we have this aria-hidden in the first place.

Yura, maybe you have some clues on this? Intuitively it seems that h1[aria-hidden=true] should be never visible by Screen Reader, but it's visible "sometimes" (if Conversation view is directly opened from Inbox - header title can be selected by Screen Reader, but if user is forwarded to Conversation view from New Message view after sending a message, h1 is no longer accessible).

Also integration test worked perfectly fine with this h1[aria-hidden=true] about month ago, but now it doesn't (and likely shouldn't). It is weird as Marionette has accessibility support for several months already (bug 1174314).

It's hard to find out why we have h1[aria-hidden=true] inside gaia-header as per blame it's already several years with us :), but we'll probably just need to get rid of it.  

[1] https://github.com/mozilla-b2g/gaia/blob/2f0602ec0c54bbe6361286e9213c24646dd74a2a/apps/sms/index.html#L189
Flags: needinfo?(azasypkin) → needinfo?(yzenevich)
Ah forgot to mention in comment 8 that it's easy to reproduce in Desktop Firefox + Screen Reader add-on:

* DEBUG=1 DESKTOP=0 make;
* .../firefox -profile .../profile-debug --no-remote
Still looking at it but wanted to give a quick reply about the Marionette accessibility support. You are right noticing that it used to work before. What changed around December, is that we flipped the default behavior for a11y checks. Now they run and will fail the test by default. If that's not desirable, test author should set the a11y checks flag to false. I did my best then to disable the tests that were reliably failing, I guess I missed this and possible a couple of more, especially if they are intermittent.

(In reply to Oleg Zasypkin [:azasypkin][⏰UTC+1] from comment #8)
> (In reply to Steve Chung [:steveck] from comment #7)
> > Hi Oleg, based on comment 6 I found a weird behavior that
> > messagesApp.Conversation.headerTitle is not accessible, but I can use
> > element.scriptWith to fetch textContent like:
> > 
> > messagesApp.Conversation.header.scriptWith(function(el) {
> >   return el.querySelector('#messages-header-text').textContent;
> > }
> > 
> > Do you have any idea about this issue?
> 
> Well, I investigated it a bit and I see that "client.helper.waitForElement"
> doesn't return anything because header title is considered as "not
> accessible". And it's not surprising because we have aria-hidden=true on the
> header title. Surprising is that Screen Reader doesn't see header title only
> when user is forwarded from New Message view to Conversation view :) And I'm
> not sure why do we have this aria-hidden in the first place.
> 
> Yura, maybe you have some clues on this? Intuitively it seems that
> h1[aria-hidden=true] should be never visible by Screen Reader, but it's
> visible "sometimes" (if Conversation view is directly opened from Inbox -
> header title can be selected by Screen Reader, but if user is forwarded to
> Conversation view from New Message view after sending a message, h1 is no
> longer accessible).
> 
> Also integration test worked perfectly fine with this h1[aria-hidden=true]
> about month ago, but now it doesn't (and likely shouldn't). It is weird as
> Marionette has accessibility support for several months already (bug
> 1174314).
> 
> It's hard to find out why we have h1[aria-hidden=true] inside gaia-header as
> per blame it's already several years with us :), but we'll probably just
> need to get rid of it.  
> 
> [1]
> https://github.com/mozilla-b2g/gaia/blob/
> 2f0602ec0c54bbe6361286e9213c24646dd74a2a/apps/sms/index.html#L189
Flags: needinfo?(yzenevich)
Yeah so after playing with it for a little bit, I think you are right and we should not have aria-hidden there at all. This should help with the test as well, I believe?
(In reply to Yura Zenevich [:yzen] from comment #10)
> What changed around December, is that we flipped the default behavior for
> a11y checks. Now they run and will fail the test by default. If that's not
> desirable, test author should set the a11y checks flag to false. I did my
> best then to disable the tests that were reliably failing, I guess I missed
> this and possible a couple of more, especially if they are intermittent.

Ah, that makes sense now!

> Yeah so after playing with it for a little bit, I think you are right and we
> should not have aria-hidden there at all. This should help with the test as
> well, I believe?

Nice, yeah it definitely fixes the test, we still have other intermittents here, but it's another story. But still curious why Screen Reader "sees" h1[aria-hidden=true] in some cases?

Thanks a lot!
(In reply to Oleg Zasypkin [:azasypkin][⏰UTC+1] from comment #12)
> (In reply to Yura Zenevich [:yzen] from comment #10)
> > What changed around December, is that we flipped the default behavior for
> > a11y checks. Now they run and will fail the test by default. If that's not
> > desirable, test author should set the a11y checks flag to false. I did my
> > best then to disable the tests that were reliably failing, I guess I missed
> > this and possible a couple of more, especially if they are intermittent.
> 
> Ah, that makes sense now!
> 
> > Yeah so after playing with it for a little bit, I think you are right and we
> > should not have aria-hidden there at all. This should help with the test as
> > well, I believe?
> 
> Nice, yeah it definitely fixes the test, we still have other intermittents
> here, but it's another story. But still curious why Screen Reader "sees"
> h1[aria-hidden=true] in some cases?

There might be an issue (if i remember correctly) in platform accessibility , where until hide accessible event is fired, the element is not yet hidden so if the focus is placed there, it will be spoken. And as far as I remember, aria-hidden="true" triggers that event a little later than if, say, we had CSS visibility/display set to hidden/none. Anyways, the intermittent visibility with aria-hidden should be fixed at some point when we re-implement its behavior in gecko (which is ongoing).

> Thanks a lot!
Comment on attachment 8706267 [details] [review]
[gaia] steveck-chung:Bug-1235842-header-a11y-intermittent > mozilla-b2g:master

Hi all,
Thanks for finding out the root cause! Although I suspect the activity issue in Bug 1233552 still affect the entire new_activity test, it's still nice to reduce the known issue and focus on other possible issues. This patch simply remove the aria-hidden since it's not necessary to make it hidden all the time, but please let me know it you have other idea or prefer to wait for other fixings.
Attachment #8706267 - Flags: feedback?(yzenevich)
Attachment #8706267 - Flags: feedback?(azasypkin)
Comment on attachment 8706267 [details] [review]
[gaia] steveck-chung:Bug-1235842-header-a11y-intermittent > mozilla-b2g:master

This element is also present in sms/views/conversation/index.html view. It needs to be fixed there as well.
Attachment #8706267 - Flags: feedback?(yzenevich)
(In reply to Yura Zenevich [:yzen] from comment #16)
> Comment on attachment 8706267 [details] [review]
> [gaia] steveck-chung:Bug-1235842-header-a11y-intermittent >
> mozilla-b2g:master
> 
> This element is also present in sms/views/conversation/index.html view. It
> needs to be fixed there as well.

Good catch! Although we didn't have test for split conversation view in gij yet, it's still important if we need to enable the split view mode in the future. Update the patch right away.
Comment on attachment 8706267 [details] [review]
[gaia] steveck-chung:Bug-1235842-header-a11y-intermittent > mozilla-b2g:master

Looks good to me, I think we should land it - maybe in a separate bug though - it's more like we're fixing accessibility issue in the first place :)

So r=me with a green Treeherder and local test run (to include disabled tests) for this PR.

Thanks!
Attachment #8706267 - Flags: feedback?(azasypkin) → feedback+
Assignee: nobody → schung
Comment on attachment 8706267 [details] [review]
[gaia] steveck-chung:Bug-1235842-header-a11y-intermittent > mozilla-b2g:master

Hi Oleg, I updated the patch the patch that changes the bug number in the list instead of removing it, since other activity related tests might still fail because of other reason. I changed the number to bug 1236446 because it's the only case I could reproduce in docker/server.
Attachment #8706267 - Flags: review?(azasypkin)
Comment on attachment 8706267 [details] [review]
[gaia] steveck-chung:Bug-1235842-header-a11y-intermittent > mozilla-b2g:master

Okay, let's decrease a number of new_activity intermittent bugs, we have few more anyway where we can investigate further, but they are likely the same as bug 1233552 comment 10 :)

Thanks!
Attachment #8706267 - Flags: review?(azasypkin) → review+
In master: https://github.com/mozilla-b2g/gaia/commit/01dccac16736083ae5940ff45ead28751c5a7740
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: