Closed Bug 1069616 Opened 10 years ago Closed 10 years ago

Reload button is not labeled

Categories

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

Product:
Firefox OS Graveyard
Component:
Gaia::Browser
Platform:
All
Gonk (Firefox OS)
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED FIXED

People

(Reporter: eeejay, Assigned: ShellHacker, Mentored)

References

Details

(Keywords: access, Whiteboard: [b2ga11y p=1][good first bug][lang=html])

Attachments

(1 file, 1 obsolete file)

https://github.com/mozilla-b2g/gaia/pull/24670
(deleted), text/x-github-pull-request
alive
: review+
yzen
: a11y-review+
Details 
The reload button is not labeled for accessibility
Whiteboard: [b2ga11y p=1] → [b2ga11y p=1][good first bug][lang=html]
Mentor: yzenevich
Attached file https://github.com/mozilla-b2g/gaia/pull/24328 (obsolete) (deleted) — 
Does this look fine ?
Attachment #8493581 - Flags: review?(yzenevich)
Comment on attachment 8493581 [details]
https://github.com/mozilla-b2g/gaia/pull/24328

Hi Sudheesh, thanks for the proposed patch.

I should've mentioned in the description that the buttons are actually part of the .urlbar block inside app_chrome.js (see: https://github.com/mozilla-b2g/gaia/blob/master/apps/system/js/app_chrome.js#L105-L106).

The right approach would just involve adding an appropriate data-l10n-id attribute together with the entry in the .properties file. The entry should be similar to this one for example: https://github.com/mozilla-b2g/gaia/blob/master/apps/system/locales/system.en-US.properties#L19, where we set the localized aria-label attribute only. It would also be great to have a quick check in app_chrome's unit tests to make sure that the attribute is set correctly.

Would you still like to take this bug? If so, feel free to take it or I can assign it to you.

Thanks!
Attachment #8493581 - Flags: review?(yzenevich)
Flags: needinfo?(sudheesh1995)
Taking this
Assignee: nobody → sudheesh1995
Flags: needinfo?(sudheesh1995)
Okay I've tried looking around to solve this and I am facing a few problems.

1. the apps/browser seems to be missing after the pull from upstream.
2. in the file apps/system/js/app_chrome.js#L105-L106 which looks like 

<button type="button" class="reload-button"></button>
<button type="button" class="stop-button"></button>

Does it have to be converted to ?

<button type="button" class="reload-button" data-l10n-id="reloadButton"></button>
<button type="button" class="stop-button" data-l10n-id="stopButton"></button>

and then in the properties file ideally which should ideally be under 
apps/browser/locales/accessibility.en-US.properties

in that file I should add

reloadButton.ariaLabel = reload
stopButton.ariaLabel = stop

Now that the apps/browser/* is missing, where should I make the required changes ?
Hi Sudheesh, comments inline:

(In reply to Sudheesh Singanamalla (:ShellHacker) from comment #4)
> Okay I've tried looking around to solve this and I am facing a few problems.
> 
> 1. the apps/browser seems to be missing after the pull from upstream.

Correct it is now integrated into system. To actually see the UI you would need to follow some internal link from the rocket bar or other resource.

> 2. in the file apps/system/js/app_chrome.js#L105-L106 which looks like 
> 
> <button type="button" class="reload-button"></button>
> <button type="button" class="stop-button"></button>
> 
> Does it have to be converted to ?
> 
> <button type="button" class="reload-button"
> data-l10n-id="reloadButton"></button>
> <button type="button" class="stop-button" data-l10n-id="stopButton"></button>

Correct

> 
> and then in the properties file ideally which should ideally be under 
> apps/browser/locales/accessibility.en-US.properties

Since the changes are for app_chrome.js which is in apps/system you should just add them the:
apps/system/locales/system.en-US.properties

> 
> in that file I should add
> 
> reloadButton.ariaLabel = reload
> stopButton.ariaLabel = stop

Correct but make sure that the labels are capitalized and also add a comment like this (https://github.com/mozilla-b2g/gaia/blob/master/apps/system/locales/system.en-US.properties#L127-L128): above the entries that you are adding.

> 
> Now that the apps/browser/* is missing, where should I make the required
> changes ?

See above.
Hope this patch goes well :)
Attachment #8493581 - Attachment is obsolete: true
Attachment #8498740 - Flags: review?(yzenevich)
Update: The tests look good, except for the Li test.

when i run jshint apps/system/js/app_chrome.js which is flagging these errors

TEST-UNEXPECTED-FAIL | apps/system/js/app_chrome.js | line 105, col 100, Line is too long.
TEST-UNEXPECTED-FAIL | apps/system/js/app_chrome.js | line 106, col 96, Line is too long.

Added to these I get a few more errors like

apps/system/js/app_chrome.js: line 105, col 100, Line is too long.
apps/system/js/app_chrome.js: line 106, col 96, Line is too long.
apps/system/js/app_chrome.js: line 98, col 11, Missing semicolon.
apps/system/js/app_chrome.js: line 117, col 11, Missing semicolon.
apps/system/js/app_chrome.js: line 96, col 9, 'className' is defined but never used.
apps/system/js/app_chrome.js: line 115, col 9, 'className' is defined but never used.

These semicolons are technically not missing in L98 and L117, the return statement is broken into multiple lines and then ended with a ';' How should we correct this ?

Also L96, L115, Should we silence them by exporting them as /* exported className */
(In reply to Sudheesh Singanamalla (:ShellHacker) from comment #7)
> Update: The tests look good, except for the Li test.
> 
> when i run jshint apps/system/js/app_chrome.js which is flagging these errors
> 
> TEST-UNEXPECTED-FAIL | apps/system/js/app_chrome.js | line 105, col 100,
> Line is too long.
> TEST-UNEXPECTED-FAIL | apps/system/js/app_chrome.js | line 106, col 96, Line
> is too long.

Ya, slit the lines over 80 into 2 lines (105-106).

> 
> Added to these I get a few more errors like
> 
> apps/system/js/app_chrome.js: line 105, col 100, Line is too long.
> apps/system/js/app_chrome.js: line 106, col 96, Line is too long.
> apps/system/js/app_chrome.js: line 98, col 11, Missing semicolon.
> apps/system/js/app_chrome.js: line 117, col 11, Missing semicolon.
> apps/system/js/app_chrome.js: line 96, col 9, 'className' is defined but
> never used.
> apps/system/js/app_chrome.js: line 115, col 9, 'className' is defined but
> never used.
> 
> These semicolons are technically not missing in L98 and L117, the return
> statement is broken into multiple lines and then ended with a ';' How should
> we correct this ?

The new multi-line format is not supported by the linter so it's ok.

> 
> Also L96, L115, Should we silence them by exporting them as /* exported
> className */
Comment on attachment 8498740 [details]
https://github.com/mozilla-b2g/gaia/pull/24670

Very close, thanks!

I added a couple of comments on the pull request. Also please add a unit test that verifies that the aria-label attribute is set correctly for those two buttons.
Attachment #8498740 - Flags: review?(yzenevich)
(In reply to Yura Zenevich [:yzen] from comment #9)

Yay 

> Comment on attachment 8498740 [details]
> https://github.com/mozilla-b2g/gaia/pull/24670
> 
> Very close, thanks!

Fixed them. Pushed back on the same PR

> I added a couple of comments on the pull request. 

I haven't written a unit test before, but I am trying to look through apps/system/test/unit/app_chrome_test.js , Am I looking in the right place ?

> Also please add a unit
> test that verifies that the aria-label attribute is set correctly for those
> two buttons.
(In reply to Sudheesh Singanamalla (:ShellHacker) from comment #10)
> (In reply to Yura Zenevich [:yzen] from comment #9)
> 
> Yay 
> 
> > Comment on attachment 8498740 [details]
> > https://github.com/mozilla-b2g/gaia/pull/24670
> > 
> > Very close, thanks!
> 
> Fixed them. Pushed back on the same PR
> 
> > I added a couple of comments on the pull request.

Thanks! I left one more comment in PR, sorry I missed it the first time.

> 
> I haven't written a unit test before, but I am trying to look through
> apps/system/test/unit/app_chrome_test.js , Am I looking in the right place ?

Yes this is correct. You can add them somewhere around this suite:
https://github.com/mozilla-b2g/gaia/blob/master/apps/system/test/unit/app_chrome_test.js#L256

What you'd assert is that the attribute is set to what you expect.

> 
> > Also please add a unit
> > test that verifies that the aria-label attribute is set correctly for those
> > two buttons.
Fixed the case problems. I was looking through and noticed there already seems to be two tests for reload and stop


    test('reload', function() {
      var app = new AppWindow(fakeWebSite);
      var chrome = new AppChrome(app);
      var stubReload = this.sinon.stub(app, 'reload');
      chrome.handleEvent({ type: 'click', target: chrome.reloadButton });
      assert.isTrue(stubReload.called);
    });

    test('stop', function() {
      var app = new AppWindow(fakeWebSite);
      var chrome = new AppChrome(app);
      var stubStop = this.sinon.stub(app, 'stop');
      chrome.handleEvent({ type: 'click', target: chrome.stopButton });
      assert.isTrue(stubStop.called);
    });

Should I put the same code in the URLbar suite
Actually you can just integrate your asserts in there:

(In reply to Sudheesh Singanamalla (:ShellHacker) from comment #12)
> Fixed the case problems. I was looking through and noticed there already
> seems to be two tests for reload and stop
> 
> 
>     test('reload', function() {
>       var app = new AppWindow(fakeWebSite);
>       var chrome = new AppChrome(app);
>       var stubReload = this.sinon.stub(app, 'reload');

        // assert.equal(chrome.reloadButton.getAttribute...

>       chrome.handleEvent({ type: 'click', target: chrome.reloadButton });
>       assert.isTrue(stubReload.called);
>     });
> 
>     test('stop', function() {
>       var app = new AppWindow(fakeWebSite);
>       var chrome = new AppChrome(app);
>       var stubStop = this.sinon.stub(app, 'stop');

        // same here

>       chrome.handleEvent({ type: 'click', target: chrome.stopButton });
>       assert.isTrue(stubStop.called);
>     });
> 
> Should I put the same code in the URLbar suite
Pushed back changes.
Comment on attachment 8498740 [details]
https://github.com/mozilla-b2g/gaia/pull/24670

a11y-r+ me , thanks!

Alive, would you review this since it's a system pull request?
Attachment #8498740 - Flags: review?(alive)
Attachment #8498740 - Flags: a11y-review+
Merged: https://github.com/mozilla-b2g/gaia/commit/25f3434b4371b743cd8430b3521288f396a2259e

Thanks for the fix, Sudheesh!
Status: NEW → RESOLVED
Closed: 10 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: