914941 - Descriptions should follow labels

Status: RESOLVED FIXED

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

Description

[Eitan Isaacson [:eeejay]]

For example in the main panel there is an item with the label of "Language" and a description of English (US) in small letters, markup:
<li>
  <small id="language-desc" class="menu-item-desc">English (US)</small>
  <a id="menuItem-languageAndRegion" class="menu-item" href="#languages" data-l10n-id="language">Language</a>
</li>

The <small> tag should follow the <a>. This will allow the screen reader to browse in logical order.

Comment 1

[Eitan Isaacson [:eeejay]]

Re-aligning priorities with 2.1 accessibility goals.

Comment 2

[Eitan Isaacson [:eeejay]]

Attachment: Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/22381

wip. almost ready for review. first waiting for try results.

Comment 3

[Eitan Isaacson [:eeejay]]

Comment on attachment 8465913 [details] [review]
Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/22381

OK, ready for review..

Comment 4

[EJ Chen [:eragonj][:小龍哥][Contributor mode on](inactive)]

@Eitan, 

If we are going to fix this, why not just move <small> tag one line down and fix related CSS ? 

Because If we put tag inside another tag, if we are going to manipulate the outer elements in the future, there would be really a high chance that we would mess up with the inner one. 

For example (https://github.com/mozilla-b2g/gaia/pull/22381/files#diff-6f6f4b5331d2d51f8d0d2e9c9dc5293fR57), you have to create another inner DOM element and this is not straightforward enough to understand there is such an element in the DOM tree by reading the HTML file.

Any idea ?

Comment 5

[EJ Chen [:eragonj][:小龍哥][Contributor mode on](inactive)]

I'll hold the review and wait for your comment, thanks :P

Comment 6

[Eitan Isaacson [:eeejay]]

(In reply to EJ Chen [:eragonj][:小龍哥] from comment #4)
> @Eitan, 
> 
> If we are going to fix this, why not just move <small> tag one line down and
> fix related CSS ? 
> 
> Because If we put tag inside another tag, if we are going to manipulate the
> outer elements in the future, there would be really a high chance that we
> would mess up with the inner one. 
> 
> For example
> (https://github.com/mozilla-b2g/gaia/pull/22381/files#diff-
> 6f6f4b5331d2d51f8d0d2e9c9dc5293fR57), you have to create another inner DOM
> element and this is not straightforward enough to understand there is such
> an element in the DOM tree by reading the HTML file.
> 
> Any idea ?

We could do that. It will be more disruptive to markup. Would this be good?

<li>
  <small data-name="deviceinfo.hardware"></small>
  <span id="model-name" data-l10n-id="model-name"> Model </span>
</li>

would turn into:
<li class="multiline">
  <span id="model-name" data-l10n-id="model-name"> Model </span>
  <small data-name="deviceinfo.hardware"></small>
</li>

and..
<li>
  <small id="fdnSettings-desc"></small>
  <a data-href="#call-fdnSettings" data-l10n-id="fdnSettings">Fixed dialing numbers</a>
</li>

would turn into:
<li>
  <a data-href="#call-fdnSettings">
    <span data-l10n-id="fdnSettings">Fixed dialing numbers</span>
    <small id="fdnSettings-desc"></small>
  </a>
</li>

and...
<li hidden id="device-visible">
  <label class="pack-switch">
    <input type="checkbox" data-ignore>
    <small id="bluetooth-device-name"></small>
    <span data-l10n-id="bluetooth-visible-to-all">Visible to all</span>
  </label>
</li>

would turn into:
<li hidden id="device-visible">
  <label class="pack-switch">
    <input type="checkbox" data-ignore>
    <span data-l10n-id="bluetooth-visible-to-all">Visible to all</span>
    <small id="bluetooth-device-name"></small>
  </label>
</li>

Comment 7

[EJ Chen [:eragonj][:小龍哥][Contributor mode on](inactive)]

Eitan, in your implementation, did you think about truncation cases ? I quickly give it a try and the original truncation mechanism would be broken. 

And also, after discussing with Arthur, we all think it would be better not to use **tag in tag** way to achieve this. Instead, can we try to rearrange the order and update CSS to make the same thing ?

Any idea would be appreciated ! Thanks !

Comment 8

[Eitan Isaacson [:eeejay]]

(In reply to EJ Chen [:eragonj][:小龍哥] from comment #7)
> Eitan, in your implementation, did you think about truncation cases ? I
> quickly give it a try and the original truncation mechanism would be broken. 
> 
> And also, after discussing with Arthur, we all think it would be better not
> to use **tag in tag** way to achieve this. Instead, can we try to rearrange
> the order and update CSS to make the same thing ?
> 

I think it would be challenging because the 'a' tag and the 'label > span' tag are styled to take up the whole height of the 'li'. This is so :active colors it correct, and that you get a large touch target. So the best I came up with is the markup above.

Note, this is going to alter the 'label > span' styling a lot. But I think it would be good to anyway move the menu-item styling up to the label from the span element in those cases.

> Any idea would be appreciated ! Thanks !

Comment 9

[Eitan Isaacson [:eeejay]]

(In reply to EJ Chen [:eragonj][:小龍哥] from comment #7)
> Eitan, in your implementation, did you think about truncation cases ? I
> quickly give it a try and the original truncation mechanism would be broken. 
> 

Where do you see this issue?

Comment 10

[Eitan Isaacson [:eeejay]]

Comment on attachment 8465913 [details] [review]
Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/22381

Pushed a new version for review. This one is a lot more extensive. When using the 'flex' display method, anonymous text nodes won't inherit the text-overflow property. So I made it a point to wrap all text in span tags.

Comment 11

[EJ Chen [:eragonj][:小龍哥][Contributor mode on](inactive)]

Let me keep r? flag and remove ni? flag here. 

I am working on some urgent bugs / features and let me hold the review process first. And to me, this patch is kinda  risky and maybe we can keep this later until 2.2. For us, there are still some rooms to improve, after having a concrete ideas, I'll post it back here later.

Thanks Eitan.

Comment 12

[Eitan Isaacson [:eeejay]]

(In reply to EJ Chen [:eragonj][:小龍哥] from comment #11)
> Let me keep r? flag and remove ni? flag here. 
> 
> I am working on some urgent bugs / features and let me hold the review
> process first. And to me, this patch is kinda  risky and maybe we can keep
> this later until 2.2. For us, there are still some rooms to improve, after
> having a concrete ideas, I'll post it back here later.
> 
> Thanks Eitan.

I would love to get some more input about risk, and how to manage it. This patch would be a huge improvement for accessibility. There never is a good time to do this, and we are pretty early on the 2.1 cycle at this point. I could definitely help with any following bugs, if they come up, and fix them.

Comment 13

[EJ Chen [:eragonj][:小龍哥][Contributor mode on](inactive)]

Hey Eitan, just left some comments on github and there are some places need to be changed.

After doing some quick experiments, your way is right and better for our current dom structure. So let's just follow this ! Thanks :)

I'll keep trying to see is there anything broken or not, and please remember to rebase to latest master !

Thanks Eitan.

Comment 14

[EJ Chen [:eragonj][:小龍哥][Contributor mode on](inactive)]

Please ping me after you fixed them and rebased, thanks :)

Comment 15

[Eitan Isaacson [:eeejay]]

Thanks for all the input. I fixed the issues you mentioned and rebased.

Comment 16

[EJ Chen [:eragonj][:小龍哥][Contributor mode on](inactive)]

thanks Eitan ! Left some comments there and please give it a check ! 

And also, I think we can file another bug for RTL language ? I don't want you get overloaded in this patch. WDYT ?

Comment 17

[Eitan Isaacson [:eeejay]]

(In reply to EJ Chen [:eragonj][:小龍哥] from comment #16)
> thanks Eitan ! Left some comments there and please give it a check ! 
> 
> And also, I think we can file another bug for RTL language ? I don't want
> you get overloaded in this patch. WDYT ?

Thanks, EJ, I updated the branch with all your input. I also did some brief testing with rtl, and fixed the issues I saw. If you think it is a much more extensive issue, we could open another bug. But I think we are nearly compliant with rtl now. Let me know!

Comment 18

[EJ Chen [:eragonj][:小龍哥][Contributor mode on](inactive)]

Comment on attachment 8465913 [details] [review]
Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/22381

Thanks Eitan !

Just left final comments on Github ! Please update them first and made sure CI is green before merging codes.

Thanks again for your great works !

Comment 19

[Eitan Isaacson [:eeejay]]

Merged! (yikes)

https://github.com/mozilla-b2g/gaia/commit/f797e094882b8a0c6e9362e42c901ea57ead4475