Closed Bug 1524539 Opened 9 months ago Closed 5 months ago

Finish porting browser_autocomplete_a11y_label.js to QuantumBar

Categories

(Firefox :: Address Bar, defect, P2)

defect
Points:
3

Tracking

()

RESOLVED FIXED
Firefox 68
Iteration:
68.4 - Apr 29 - May 12
Tracking Status
firefox68 --- fixed

People

(Reporter: standard8, Assigned: dao)

References

Details

(Whiteboard: [fxsearch])

Attachments

(1 file)

Once the Accessibility review (bug 1524320) for the QuantumBar is complete, we'll need to finish porting browser_autocomplete_a11y_label.js to test the expected result.

Bug 1524536 is already porting most of the test, but it can't be finished until we know what exactly we want there.

No longer depends on: quantumbar-a11y
Assignee: nobody → dao+bmo
Status: NEW → ASSIGNED

So the old urlbar builds special accessible labels: https://searchfox.org/mozilla-central/rev/aae527894a97ee3bbe0c2cfce9c67c59e8b8fcb9/browser/base/content/urlbarBindings.xml#2432

The new urlbar currently exposes whatever text would be presented visually as the accessible label. Are there specific reasons why we'd prefer the old behavior?

Flags: needinfo?(mzehe)

Yes, because the text that is present does not always cover all information, for example those presented by icons, like "is this a tag, bookmark, open tab etc.". Those things have to be properly translated. You can look at what currently gets exposed via the Accessibility Inspector, by looking at the Name property of each option. And then compare that to what you see visually and what information is missing from the name. The name is what gets spoken by NVDA whenever a new option gains focus.

Flags: needinfo?(mzehe)

(In reply to Marco Zehe (:MarcoZ) from comment #2)

Yes, because the text that is present does not always cover all information, for example those presented by icons, like "is this a tag, bookmark, open tab etc.".

Seems like that could be solved by setting accessible labels on these icons? This to me seems preferable over / less brittle than setting a separate accessible label on the whole result row.

Flags: needinfo?(mzehe)

It really depends. The advantage of the label is that we have full control over what gets spoken, for example a slight pause by inserting a comma in a place where we think breaking up the information makes sense. Or such. If we let the engine collect all child accessible names, we get one long string of text. Broken up only by spaces. Imagine someone speaking a whole string of information that can be 30 or more seconds long without taking a breath (letting the engine collect the names) versus giving the information structure by pausing a little here and there, taking small breaths, so to speak (controlling the flow of information by setting a custom label). This sometimes can mean the difference between "merely accessible" and "pleasant to use".

Flags: needinfo?(mzehe)

(In reply to Marco Zehe (:MarcoZ) from comment #4)

It really depends. The advantage of the label is that we have full control over what gets spoken, for example a slight pause by inserting a comma in a place where we think breaking up the information makes sense. Or such. If we let the engine collect all child accessible names, we get one long string of text. Broken up only by spaces.

This is basically what the old urlbar does anyway. It takes some kind of normalized label and then appends some extra words separated by spaces. It's rather crude.

This code is ten years old. :) And since the aim of the QuantumBar is to improve things, it means we can improve here, too, by making the new label less crude. And an aria-label which we have full control over is definitely the better way to do it than just collecting children.

Blocks: quantumbar-tests
No longer blocks: quantumbar-release
Points: --- → 3
Priority: P3 → P2
Type: enhancement → defect
Iteration: --- → 68.3 - Apr 15 - 28
Iteration: 68.3 - Apr 15 - 28 → 68.4 - Apr 29 - May 12

Given that one goal of the quantumbar is extensibility, and we can't expect add-ons to set special accessible labels, I'm gonna go with using the text presented to sighted users, and adding accessible labels to icons where they're currently missing.

Summary: Finish porting browser_autocomplete_a11y_label.js to QuantumBar → Implement accessible result labels and finish porting browser_autocomplete_a11y_label.js to QuantumBar

Comment on attachment 9062445 [details]
Bug 1524539 - Finish porting browser_autocomplete_a11y_label.js to QuantumBar.

This fails with:

FAIL Result label should be: <search term>— Search with <engine name> - "foo bar — Search with browser_searchSuggestionEngine searchSuggestionEngine.xml" == "foobar— Search with browser_searchSuggestionEngine searchSuggestionEngine.xml"

The fake engine returns "foobar" and I'm not sure how that gets turned into "foo bar" (which doesn't happen for the other suggestion, "foofoo"). Is this something the accessibility service would do?

Flags: needinfo?(surkov.alexander)
Attachment #9062445 - Attachment description: Bug 1524539 - Finish porting browser_autocomplete_a11y_label.js to QuantumBar. → Bug 1524539 - [WIP] Finish porting browser_autocomplete_a11y_label.js to QuantumBar.

Alex is on parental leave. I'm forwarding the request to Jamie

Flags: needinfo?(surkov.alexander) → needinfo?(jteh)

(In reply to Dão Gottwald [::dao] from comment #9)

The fake engine returns "foobar" and I'm not sure how that gets turned into "foo bar" (which doesn't happen for the other suggestion, "foofoo"). Is this something the accessibility service would do?

The suggestions wrap the parts of the result which match the user's input with <strong>. For example, if the user types "foo" and the result is "foobar", you get <strong>foo</strong>bar. I'm guessing that's what's happening here.

  1. A11y splits the string for text nodes which are direct children of a block element:
    data:text/html,<div role="option"><strong>a</strong>b</div>
  2. It doesn't do this if both the text nodes are not direct children of the block element:
    data:text/html,<div role="option"><strong>a</strong><span>b</span></div>
  3. It doesn't do this if the container is an inline element:
    data:text/html,<span role="option"><strong>a</strong>b</span>

Point 2) would suggest that 1) is a bug. I think (but haven't debugged this) that this is caused by the block stuff here:
https://searchfox.org/mozilla-central/source/accessible/base/nsTextEquivUtils.cpp#118
It does that for all text node children. It really shouldn't split the string when there's an inline element in the middle.

I'm not quite sure how to fix this yet. It's going to require some careful thought. Unfortunately, I don't think a11y engineers have the cycles to work on this right now.

One workaround would be making sure that non-matched text gets wrapped in spans. Using the above foobar example, <strong>foo</strong><span>bar</span>. That's a pretty ugly and a11y specific hack, though.

Flags: needinfo?(jteh)
Summary: Implement accessible result labels and finish porting browser_autocomplete_a11y_label.js to QuantumBar → Finish porting browser_autocomplete_a11y_label.js to QuantumBar
Attachment #9062445 - Attachment description: Bug 1524539 - [WIP] Finish porting browser_autocomplete_a11y_label.js to QuantumBar. → Bug 1524539 - Finish porting browser_autocomplete_a11y_label.js to QuantumBar.

(In reply to James Teh [:Jamie] from comment #11)

Point 2) would suggest that 1) is a bug. I think (but haven't debugged this) that this is caused by the block stuff here:
https://searchfox.org/mozilla-central/source/accessible/base/nsTextEquivUtils.cpp#118
It does that for all text node children. It really shouldn't split the string when there's an inline element in the middle.

I'm not quite sure how to fix this yet. It's going to require some careful thought. Unfortunately, I don't think a11y engineers have the cycles to work on this right now.

I just made the test expect this quirk for now.

See Also: → 1550644
Pushed by dgottwald@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/37b9048629cf
Finish porting browser_autocomplete_a11y_label.js to QuantumBar. r=Jamie
Status: ASSIGNED → RESOLVED
Closed: 5 months ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 68
You need to log in before you can comment on or make changes to this bug.