1030366 - Hidden toolbar is not really hidden when editing contact

Status: RESOLVED FIXED

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

Description

[Eitan Isaacson [:eeejay]]

STR:
1. Open dialer app.
2. Select contacts tab.
3. Choose a contact.
4. Press edit button.

Result: The toolbar get transformed off the bottom of the screen, but is not actually hidden.
Expected: The toolbar should get visibility: hidden.

Comment 1

[Eitan Isaacson [:eeejay]]

Re-aligning priorities with 2.1 accessibility goals.

Comment 2

[Yura Zenevich [:yzen] (please use "needinfo")]

Here you go, Lalit. Let either of us know if you have any questions. Eitan has a nice list of steps to reproduce the bug so you should be able to do that in the gaia emulator on desktop.

Comment 3

[Julio Leal]

I would like to help!!! but I dont understand what has to hide. by tool bar do you mean the tree bottoms "dial , contacts and keyboard"? because the contact form gets visibility: hidden when you edit a contact!

Comment 4

[Lalit M [:darklord]]

Julio, this is what Yura said:
" Yes what we mean by visible is actually visible to the screen reader and not necessarily visible on the screen.

Screen reader is a module that let's a blind user interact with Gaia. The screen reader reads out what it is currently focused on the screen. But it also will read out things that are hidden off screen using CSS transitions. This is the case with your bug. The toolbar is moved off screen but it's CSS visibility property is still visible when it should be changed to hidden.

Screen reader simulator is actually available as part of the Gaia Dev tools when you run it with Firefox for desktop (one of the tabs on the right). You can enable and disable it there. Interaction is a little different when you have it on. Insisted of clicking to activate things you need to double click. To control screen reader's focus you can either swipe left and right or click and hold the mouse down and move it around the screen. You should be able to see the screen reader output in the text box on the right. 

Too see the issue described in the bug you need to turn it on and swipe right until the screen reader focus moves off screen and the screen reader announces the problematic toolbar. The correct behaviour is such that the screen reader focus never steps off screen and the toolbar is never announced when it's off screen."

Comment 5

[Yura Zenevich [:yzen] (please use "needinfo")]

(In reply to Julio Leal from comment #3)
> I would like to help!!! but I dont understand what has to hide. by tool bar
> do you mean the tree bottoms "dial , contacts and keyboard"? because the
> contact form gets visibility: hidden when you edit a contact!

There's only translateY styling done with transform and visibility is missing. Lalit, let me know if you have any questions or need some suggestions.

Comment 6

[Yura Zenevich [:yzen] (please use "needinfo")]

Unassigned since there's no response.

Comment 7

[Alexey Novak]

I would love to take over this bug. Could you please assign it to me.

Comment 8

[Eitan Isaacson [:eeejay]]

Here you go!

Comment 9

[Alexey Novak]

Attachment: pull request

So correct me if I'm wrong. I managed to reproduce the error using Screen Reader in Gaia Dev tools. We are talking here about bottom toolbar with buttons: call-log-view, contacts-view and keyboard-view.
User can swipe through this panel in the Edit Contact screen.

I added |visibility: hidden| flag for those elements in toolbar.css

Are there any test files I need to modify? I assume there should be one. Not very familiar with the whole python gaia unit testing so any hints here would be welcome.

Please let me know if I missed anything or looking into wrong direction.

p.s.
Also there are 2 mentors assigned to this bug so I'm not sure which name should I put in comments r=mentor

Comment 10

[Yura Zenevich [:yzen] (please use "needinfo")]

(In reply to Alexey Novak from comment #9)
> Created attachment 8465148 [details] [review]
> pull request
> 
> So correct me if I'm wrong. I managed to reproduce the error using Screen
> Reader in Gaia Dev tools. We are talking here about bottom toolbar with
> buttons: call-log-view, contacts-view and keyboard-view.
> User can swipe through this panel in the Edit Contact screen.
> 
> I added |visibility: hidden| flag for those elements in toolbar.css
> 
> Are there any test files I need to modify? I assume there should be one. Not
> very familiar with the whole python gaia unit testing so any hints here
> would be welcome.

It would be super awesome if you could write a UI test for that. Pretty much with steps described in steps to reproduce the bug: 
* open dialer
* go to contacts tab
* choose contact
* verify that the icons are visible to the screen reader
* select edit
* verify that the icons are invisible to the screen reader

> 
> Please let me know if I missed anything or looking into wrong direction.
> 
> p.s.
> Also there are 2 mentors assigned to this bug so I'm not sure which name
> should I put in comments r=mentor

Comment 11

[Eitan Isaacson [:eeejay]]

Comment on attachment 8465148 [details] [review]
pull request

I'm going to let Yura mentor this. When you are ready, flag him for feedback. The actual review will be done by a relevant gaia peer.

Comment 12

[Alexey Novak]

Thanks Yura and Eitan.

I will ping again once write the test.

Comment 13

[Alexey Novak]

Attachment: pull request

I wrote a test here. Please take a look and let me know if it looks OK or I need to change anything.

Also, this test DOES NOT work. I have been battling with |self.phone._keypad_toolbar_locator| and other selectors. Do not understand why but it does not want to understand those selectors. This check is very similar to the one in test_a11y_phone_select_toolbars.py It works there but it refuses to work in my tests. Any hints would be great.

Comment 14

[Alexey Novak]

Ah... just like in another bug the missing piece was |self.apps.switch_to_displayed_app()| but now I see that the check is failing.. will try to figure it out

Comment 15

[Yura Zenevich [:yzen] (please use "needinfo")]

Comment on attachment 8467460 [details] [review]
pull request

Added comments in Github. The fix is good and tests will pass once you address the comments. Thanks! Flag me again once you have them.

Comment 16

[Alexey Novak]

I will do it shortly. Thanks for the feedback.

Comment 17

[Alexey Novak]

sorry for the delay, I got caught with few other things but I think that should be able to work more on this bug.

I applied changes you proposed but still test failing even with those selectors you suggested.

Also functions |contacts.tap_new_contact()| , |new_contact_form.tap_done()| and |contact_item_detail.tap_edit()| are pretty complex functions. Do you want me to rewrite them?
I was trying to understand other similar code and I assume that the main point of those refactored functions would be using |self.accessibility.click(self.marionette.find(<selector>))| instead of |self.marionette.find(<selector>).tap()| am I correct ?

Comment 18

[Alexey Novak]

I added a11y_click functions which mimic existing tap functions but use |accessibility.click| instead.
Also gave test file a more proper name.
I think that all comments addressed... Although still trying to figure out why that selectors are still refusing to work for me and an implementation of a11y_click in Contact class fails with some sort of async error. Will try to figure it out.

Comment 19

[Yura Zenevich [:yzen] (please use "needinfo")]

(In reply to Alexey Novak from comment #18)
> I added a11y_click functions which mimic existing tap functions but use
> |accessibility.click| instead.
> Also gave test file a more proper name.
> I think that all comments addressed... Although still trying to figure out
> why that selectors are still refusing to work for me and an implementation
> of a11y_click in Contact class fails with some sort of async error. Will try
> to figure it out.

I added some comments in the PR.

Comment 20

[Yura Zenevich [:yzen] (please use "needinfo")]

So I tried running the test and it indeed fails. I replaced the failing a11y_click function with tap and test passes. This means that the a11y click is applied on an element that does not have a click listener that handles taps. You need to find which element the screen reader is actually clicks on and use that one.

Comment 21

[Alexey Novak]

I updated pull request according to your comments. Please take a look.

Comment 22

[Yura Zenevich [:yzen] (please use "needinfo")]

Comment on attachment 8467460 [details] [review]
pull request

Looks good with the screen reader. Ran tests on device too. Marking Anthony to take a look. Thanks.

Comment 23

[Anthony Ricaud (:rik)]

Comment on attachment 8467460 [details] [review]
pull request

Re-directing to Doug. This probably needs to also be reviewed by a UI test peer.

Comment 24

[Doug Sherk (:drs) (inactive)]

Comment on attachment 8467460 [details] [review]
pull request

(In reply to Anthony Ricaud (:rik) from comment #23)
> This probably needs to also be reviewed by a UI test peer.

Yes. I looked at the UI test changes but I'm not qualified to review them. I don't know who to direct this to, though, and there's no info on the module owners wiki page. I'll ask rwood tomorrow.

(Feel free to redirect the feedback I set on myself to review from a UI test peer.)

Comment 25

[Zac C (:zac)]

Comment on attachment 8467460 [details] [review]
pull request

I just nit-picked on some code naming because it's not clear and not consistent with the rest of the codebase, but that aside the test case looks well written :)
Thanks for helping to fix a bug :)

Comment 26

[Alexey Novak]

Thanks for your feedback Zac, it is great. I will address it shortly

Comment 27

[Alexey Novak]

I made the changes according to your feedback Zac. Let me know if it is good now.

Comment 28

[Anthony Ricaud (:rik)]

Comment on attachment 8467460 [details] [review]
pull request

Alexey: To make sure Zac takes a look, you should ask for another review. I just did it for you.

Comment 29

[Alexey Novak]

Thanks Anthony. Will know it for the future.

Comment 30

[Zac C (:zac)]

Comment on attachment 8467460 [details] [review]
pull request

r+, very nice :)

Comment 31

[Yura Zenevich [:yzen] (please use "needinfo")]

https://github.com/mozilla-b2g/gaia/commit/8fade507bb70dae2bf36b947eee780b514b02a21

Thanks, Alexey!