Closed
Bug 1030366
Opened 10 years ago
Closed 10 years ago
Hidden toolbar is not really hidden when editing contact
Categories
(Firefox OS Graveyard :: Gaia::Dialer, defect)
Tracking
(Not tracked)
RESOLVED
FIXED
People
(Reporter: eeejay, Assigned: alexey.novak.mail, Mentored)
References
Details
(Keywords: access, Whiteboard: [b2ga11y p=1][good first bug])
Attachments
(1 file, 1 obsolete file)
(deleted),
text/x-github-pull-request
|
drs
:
review+
zcampbell
:
review+
yzen
:
a11y-review+
|
Details |
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.
Reporter | ||
Comment 1•10 years ago
|
||
Re-aligning priorities with 2.1 accessibility goals.
Whiteboard: [b2ga11y p=1]
Reporter | ||
Updated•10 years ago
|
Whiteboard: [b2ga11y p=1] → --do_not_change-- [good first bug]
Reporter | ||
Updated•10 years ago
|
Whiteboard: --do_not_change-- [good first bug] → [b2ga11y p=1][good first bug]
Updated•10 years ago
|
Mentor: yzenevich, eitan
Comment 2•10 years ago
|
||
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.
Assignee: nobody → lalit.hilmarsh
Comment 3•10 years ago
|
||
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•10 years ago
|
||
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•10 years ago
|
||
(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.
Flags: needinfo?(lalit.hilmarsh)
Comment 6•10 years ago
|
||
Unassigned since there's no response.
Assignee: lalit.hilmarsh → nobody
Flags: needinfo?(lalit.hilmarsh)
Assignee | ||
Comment 7•10 years ago
|
||
I would love to take over this bug. Could you please assign it to me.
Assignee | ||
Comment 9•10 years ago
|
||
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
Attachment #8465148 -
Flags: review?(eitan)
Comment 10•10 years ago
|
||
(In reply to Alexey Novak from comment #9)
> Created attachment 8465148 [details]
> 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
Reporter | ||
Comment 11•10 years ago
|
||
Comment on attachment 8465148 [details]
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.
Attachment #8465148 -
Flags: review?(eitan)
Assignee | ||
Comment 12•10 years ago
|
||
Thanks Yura and Eitan.
I will ping again once write the test.
Assignee | ||
Comment 13•10 years ago
|
||
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.
Attachment #8465148 -
Attachment is obsolete: true
Attachment #8467460 -
Flags: review?(yzenevich)
Assignee | ||
Comment 14•10 years ago
|
||
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•10 years ago
|
||
Comment on attachment 8467460 [details]
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.
Attachment #8467460 -
Flags: review?(yzenevich)
Assignee | ||
Comment 16•10 years ago
|
||
I will do it shortly. Thanks for the feedback.
Assignee | ||
Comment 17•10 years ago
|
||
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 ?
Assignee | ||
Comment 18•10 years ago
|
||
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•10 years ago
|
||
(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•10 years ago
|
||
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.
Assignee | ||
Comment 21•10 years ago
|
||
I updated pull request according to your comments. Please take a look.
Comment 22•10 years ago
|
||
Comment on attachment 8467460 [details]
pull request
Looks good with the screen reader. Ran tests on device too. Marking Anthony to take a look. Thanks.
Attachment #8467460 -
Flags: review?(anthony)
Attachment #8467460 -
Flags: a11y-review+
Comment 23•10 years ago
|
||
Comment on attachment 8467460 [details]
pull request
Re-directing to Doug. This probably needs to also be reviewed by a UI test peer.
Attachment #8467460 -
Flags: review?(anthony) → review?(drs+bugzilla)
Comment 24•10 years ago
|
||
Comment on attachment 8467460 [details]
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.)
Attachment #8467460 -
Flags: review?(drs+bugzilla)
Attachment #8467460 -
Flags: review+
Attachment #8467460 -
Flags: feedback?(drs+bugzilla)
Updated•10 years ago
|
Attachment #8467460 -
Flags: feedback?(drs+bugzilla) → review?(zcampbell)
Comment 25•10 years ago
|
||
Comment on attachment 8467460 [details]
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 :)
Attachment #8467460 -
Flags: review?(zcampbell) → review-
Assignee | ||
Comment 26•10 years ago
|
||
Thanks for your feedback Zac, it is great. I will address it shortly
Assignee | ||
Comment 27•10 years ago
|
||
I made the changes according to your feedback Zac. Let me know if it is good now.
Comment 28•10 years ago
|
||
Comment on attachment 8467460 [details]
pull request
Alexey: To make sure Zac takes a look, you should ask for another review. I just did it for you.
Attachment #8467460 -
Flags: review- → review?(zcampbell)
Assignee | ||
Comment 29•10 years ago
|
||
Thanks Anthony. Will know it for the future.
Comment 30•10 years ago
|
||
Comment on attachment 8467460 [details]
pull request
r+, very nice :)
Attachment #8467460 -
Flags: review?(zcampbell) → review+
Assignee | ||
Updated•10 years ago
|
Keywords: checkin-needed
Comment 31•10 years ago
|
||
You need to log in
before you can comment on or make changes to this bug.
Description
•