Eitan Isaacson [:eeejay] Reporter
	Description • 9 years ago

Currently, there is a visible progress bar that shows when it is loading and then disappears.

	Ross Assignee
	Comment 1 • 9 years ago

Autolander didn't grab this for some reason...

The progressbar was changed to an aria-liveregion and has its aria-valuetext updated on loading/loaded.  Seems to work well on my phone.  Let me know if there's anything I overlooked and if not, I'll write some tests.

	Eitan Isaacson [:eeejay] Reporter
	Comment 2 • 9 years ago

Comment on attachment 8621504 [details] [review]
Screen reader announces progress status for browser loading/loaded

Generally good. Have the aria attributes set in the bootstrap of gaia-progress and not in the actual app.

	Ross Assignee
	Comment 3 • 9 years ago

Okay(In reply to Eitan Isaacson [:eeejay] from comment #2)
> Comment on attachment 8621504 [details] [review]
> Screen reader announces progress status for browser loading/loaded
> 
> Generally good. Have the aria attributes set in the bootstrap of
> gaia-progress and not in the actual app.

Okay, so I went in and restricted the changes to the Shared/Gaia-Progress element location as opposed to adjusting the aria-attributes in the App Chrome app itself.  Seems to work rather well on my phone.

One consideration I had was whether or not we should be localizing the aria-valuetext attribute -- right now it's hardcoded to say "loading"/"loaded" and there's no recourse for other languages.  I was thinking the best way to do that would be to have a separate data-l10n-ID for each state, and use the stop/start functions to modify those id's as opposed to the aria-valuetext itself.  Does that sound right or should we just leave this alone for now?

As far as the tests were concerned, I tried to modify "App_Chrome_Test.js" but noticed that it was already failing two tests on master, and interestingly enough those failures have to do with <gaia-progress>.  For whatever reason, none of the <gaia-progress> prototype functions seem to be registering properly with the test suite... Couldn't figure out why.

	Eitan Isaacson [:eeejay] Reporter
	Comment 4 • 9 years ago

(In reply to Ross from comment #3)
> Okay(In reply to Eitan Isaacson [:eeejay] from comment #2)
> > Comment on attachment 8621504 [details] [review]
> > Screen reader announces progress status for browser loading/loaded
> > 
> > Generally good. Have the aria attributes set in the bootstrap of
> > gaia-progress and not in the actual app.
> 
> Okay, so I went in and restricted the changes to the Shared/Gaia-Progress
> element location as opposed to adjusting the aria-attributes in the App
> Chrome app itself.  Seems to work rather well on my phone.

Cool!

> 
> One consideration I had was whether or not we should be localizing the
> aria-valuetext attribute -- right now it's hardcoded to say
> "loading"/"loaded" and there's no recourse for other languages.  I was
> thinking the best way to do that would be to have a separate data-l10n-ID
> for each state, and use the stop/start functions to modify those id's as
> opposed to the aria-valuetext itself.  Does that sound right or should we
> just leave this alone for now?

Yes. This should not be flagged for review until it is localizeable. And yes, you would set the localized string programmatically. 

> 
> As far as the tests were concerned, I tried to modify "App_Chrome_Test.js"
> but noticed that it was already failing two tests on master, and
> interestingly enough those failures have to do with <gaia-progress>.  For
> whatever reason, none of the <gaia-progress> prototype functions seem to be
> registering properly with the test suite... Couldn't figure out why.

Those tests only fail locally, or on try too? If they pass on try, you are good.

	Ross Assignee
	Comment 5 • 9 years ago

> Those tests only fail locally, or on try too? If they pass on try, you are good.

Managed to solve this one.  It was indeed just a local failure and it had to do with the fact that I was running the tests in Firefox Developer with Web Components disabled.  https://developer.mozilla.org/en-US/docs/Web/Web_Components#Enabling_Web_Components_in_Firefox did the trick!

Beyond that, everything's localized and the tests are passing.  Let me know!

	Eitan Isaacson [:eeejay] Reporter
	Comment 6 • 9 years ago

Comment on attachment 8621504 [details] [review]
Screen reader announces progress status for browser loading/loaded

Yikes, sorry I missed this. I could give feedback but no formal review. We will need a module peer for that. Sending this to timdream.

	Tim Guan-tin Chien [:timdream] (please needinfo)
	Comment 7 • 9 years ago

Comment on attachment 8621504 [details] [review]
Screen reader announces progress status for browser loading/loaded

Sorry for the delay. I think this is alright, but changes to shared/elements/ might need to uplift to the component repo? Not sure...

	Wilson Page [:wilsonpage]
	Comment 8 • 9 years ago

Comment on attachment 8621504 [details] [review]
Screen reader announces progress status for browser loading/loaded

This is an older component that was born before gaia-components. We also have gaia-compoenents/gaia-progress [1] which IIRC is being used by some of the new Spark apps.

It would make sense to land a similar patch over there if other apps are likely to follow Spark's direction.

[1] https://github.com/gaia-components/gaia-progress

	Kevin Grandon :kgrandon
	Comment 9 • 9 years ago

Comment on attachment 8621504 [details] [review]
Screen reader announces progress status for browser loading/loaded

While we may use stuff in gaia-components in the future, we should not block people from migrating to what we have in shared/ today. As a shared/ peer, I'm happy to R+ this based on feedback from Eitan/Tim.

We should also certainly upstream to gaia-components. Thank you for your contribution!

	Ross Assignee
	Comment 10 • 9 years ago

Hey Kevin,

I added the checkin-needed keyword on this one but Autolander hasn't done anything with it (yet).  Not sure what the deal is, but figured I'd see if you could land this manually maybe?

	Ryan VanderMeulen [:RyanVM]
	Comment 11 • 9 years ago

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

	Kevin Grandon :kgrandon
	Comment 12 • 9 years ago

(In reply to Ross from comment #10)
> Hey Kevin,
> 
> I added the checkin-needed keyword on this one but Autolander hasn't done
> anything with it (yet).  Not sure what the deal is, but figured I'd see if
> you could land this manually maybe?

Working on getting this fixed, likely under a new keyword. For now checkin-needed will alert sheriffs like Ryan though. Thanks!

	Ross Assignee
	Comment 13 • 9 years ago

(In reply to Kevin Grandon :kgrandon from comment #12)
> (In reply to Ross from comment #10)
> > Hey Kevin,
> > 
> > I added the checkin-needed keyword on this one but Autolander hasn't done
> > anything with it (yet).  Not sure what the deal is, but figured I'd see if
> > you could land this manually maybe?
> 
> Working on getting this fixed, likely under a new keyword. For now
> checkin-needed will alert sheriffs like Ryan though. Thanks!

Got it, sounds good.  Thanks for following up! :)