Descriptions should follow labels

RESOLVED FIXED

Status

RESOLVED FIXED
5 years ago
4 years ago

People

(Reporter: eeejay, Assigned: eeejay)

Tracking

({access})

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [b2ga11y p=1] )

Attachments

(1 attachment)

(Assignee)

Description

5 years ago
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.

Updated

5 years ago
Depends on: 995424
(Assignee)

Updated

5 years ago
Whiteboard: [b2ga11y 2.0]
(Assignee)

Updated

5 years ago
Whiteboard: [b2ga11y 2.0] → [b2ga11y p=2]
(Assignee)

Comment 1

4 years ago
Re-aligning priorities with 2.1 accessibility goals.
Whiteboard: [b2ga11y p=2] → [b2ga11y p=1]
(Assignee)

Comment 2

4 years ago
Created attachment 8465913 [details] [review]
Link to Github pull-request: https://github.com/mozilla-b2g/gaia/pull/22381

wip. almost ready for review. first waiting for try results.
(Assignee)

Comment 3

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

OK, ready for review..
Attachment #8465913 - Flags: review?(ejchen)
@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 ?
Flags: needinfo?(eitan)
I'll hold the review and wait for your comment, thanks :P
(Assignee)

Comment 6

4 years ago
(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>
Flags: needinfo?(eitan)
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 !
(Assignee)

Comment 8

4 years ago
(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 !
(Assignee)

Comment 9

4 years ago
(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?
Flags: needinfo?(ejchen)
(Assignee)

Updated

4 years ago
Attachment #8465913 - Flags: review?(ejchen)
(Assignee)

Comment 10

4 years ago
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.
Attachment #8465913 - Flags: review?(ejchen)
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.
Flags: needinfo?(ejchen)
(Assignee)

Comment 12

4 years ago
(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.
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.
Please ping me after you fixed them and rebased, thanks :)
(Assignee)

Comment 15

4 years ago
Thanks for all the input. I fixed the issues you mentioned and rebased.
Flags: needinfo?(ejchen)
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 ?
Flags: needinfo?(ejchen)
(Assignee)

Comment 17

4 years ago
(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!
Flags: needinfo?(ejchen)
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 !
Attachment #8465913 - Flags: review?(ejchen) → review+
Flags: needinfo?(ejchen)
(Assignee)

Updated

4 years ago
Depends on: 1055116
(Assignee)

Comment 19

4 years ago
Merged! (yikes)

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