Fix screen reader visibility issues around the e.me.

RESOLVED FIXED

Status

RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: yzen, Assigned: yzen)

Tracking

({access})

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

46 bytes, text/x-github-pull-request
ranbena
: review+
crdlc
: feedback-
Details | Review | Splinter Review
(Assignee)

Description

4 years ago
There are several visibility issue around the e.me UI. Specifically footer is visible most of the time even when it's not on the screen.
(Assignee)

Updated

4 years ago
Component: Gaia → Gaia::Everything.me
(Assignee)

Comment 1

4 years ago
Created attachment 8438589 [details] [review]
Github PR
Attachment #8438589 - Flags: review?(amirn)

Comment 2

4 years ago
Comment on attachment 8438589 [details] [review]
Github PR

I am sorry, but I do not have time to review this until vertical homescreen work is done.
Attachment #8438589 - Flags: review?(amirn)
(In reply to Amir Nissim (Everything.me) from comment #2)
> Comment on attachment 8438589 [details] [review]
> Github PR
> 
> I am sorry, but I do not have time to review this until vertical homescreen
> work is done.

If you have other priorities, that is fine.

Is there a specific time you would like us to re-ping you with these issues? Or is there someone else with more capacity for reviewing? Unflagged attachments are where patches go to die..

Comment 4

4 years ago
(In reply to Eitan Isaacson [:eeejay] from comment #3)
> Is there a specific time you would like us to re-ping you with these issues?
> Or is there someone else with more capacity for reviewing? Unflagged
> attachments are where patches go to die..

You can try me again in 3-4 weeks, or ping ran@everything.me, though I think the homescreen app will not be relevant by then.
(Assignee)

Comment 5

4 years ago
Comment on attachment 8438589 [details] [review]
Github PR

It looks like both e.me and homescreen are still going to be present on tablets (until the ux has a way forward with vertical screen).

I hope you might have time to take a look at the PR here. If so, I will also ask for a second round from a homescreen owner/peer.
Attachment #8438589 - Flags: review?(ran)
Comment on attachment 8438589 [details] [review]
Github PR

1. One comment on GH.
2. The visibility hiding must take place after the transition has completed or else, the transition won't be seen at all. BUT, the transition isn't complete on master either. So, there's no real change.

Adding Cristian for feedback on this.
Attachment #8438589 - Flags: review?(ran)
Attachment #8438589 - Flags: review+
Attachment #8438589 - Flags: feedback?(crdlc)
Comment on attachment 8438589 [details] [review]
Github PR

I am pretty sure that this change kills the animation. Although Ran says that we don't have animation currently :) So.. if we had animation in the past and we don't have animation now, we have another bug, but this code does not look good to me. You should add some class .visible and set it when the animation finishes. But this is my feedback, I am not nor owner neither peer of evme, so go ahead with Ran's comment
Attachment #8438589 - Flags: feedback?(crdlc) → feedback-
(Assignee)

Comment 8

4 years ago
(In reply to Ran Ben Aharon (Everything.me) from comment #6)
> Comment on attachment 8438589 [details] [review]
> Github PR
> 
> 1. One comment on GH.
> 2. The visibility hiding must take place after the transition has completed
> or else, the transition won't be seen at all. BUT, the transition isn't
> complete on master either. So, there's no real change.
> 
> Adding Cristian for feedback on this.

I added transition: visibility with the same duration time as for transform, this should at least have the same time effect as the transform transition, just wanted to run it by you. This way I believe there's no need for the additional after-animation class.
Flags: needinfo?(ran)
I like the approach. But wouldn't that mean that it'll take it that much time to appear as well, hindering the "footer up" animation?
Flags: needinfo?(ran)
(Assignee)

Comment 10

4 years ago
(In reply to Ran Ben Aharon (Everything.me) from comment #9)
> I like the approach. But wouldn't that mean that it'll take it that much
> time to appear as well, hindering the "footer up" animation?

From what I understand, visibility property will not have a hidden value as soon as the transition starts.
(Assignee)

Comment 11

4 years ago
Marking with ni? so it doesn't slip off the radar.
Flags: needinfo?(ran)
I tested the visibility transition thing in a test page and you're right. It works as you said.

I can't seem to test this on a device. For some reason I'm stuck on verticalhome.
Go ahead and land it if it looks unchanged.
Flags: needinfo?(ran)
(Assignee)

Comment 13

4 years ago
(In reply to Ran Ben Aharon (Everything.me) from comment #12)
> I tested the visibility transition thing in a test page and you're right. It
> works as you said.
> 
> I can't seem to test this on a device. For some reason I'm stuck on
> verticalhome.

You can trigger the old home screen with the app-manager if you have certified apps debugging enabled. Alternatively, you can try with FF nightly by going to http://homescreen.gaiamobile.org:8080/ when the gaia starts.

> Go ahead and land it if it looks unchanged.
(Assignee)

Comment 14

4 years ago
https://github.com/mozilla-b2g/gaia/commit/3b8989a1037b180d79a49a8e135dc0e87676aa3a
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.