Emil Ghitta, QA [:emilghitta] Reporter
	Description • 6 years ago

[Affected versions]:
Firefox 63.0a1 (BuildId:20180801223951).
Firefox 62.0b13 (BuildId:20180730180407).
Firefox 61.0.1 (BuildId:20180704003137).
Firefox 60.1.0esr (BuildId:20180621121604).

[Affected platforms]:
Windows 10 64bit.

[Preconditions]
Enable NVDA screen reader.

[Steps to reproduce]:
1. Launch Firefox.
2. Access the following webpage: https://en.wikipedia.org/wiki/Main_Page.
3. Hover over the "Toggle reader view" button.

[Expected result]:
The screen reader reads the Reader View button on hover.

[Actual result]:
Screen reader doesn't read the Reader View button on hover.

[Regression range]:
This seems to be a regression:

Last good revision: b8da99694cb6a61ea346e6369333c3b88a283bdc
First bad revision: 5ed5cde4cb2c746ece77517c778208a59b2f1de6

Pushlog:
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=b8da99694cb6a61ea346e6369333c3b88a283bdc&tochange=5ed5cde4cb2c746ece77517c778208a59b2f1de6

	Emil Ghitta, QA [:emilghitta] Reporter
	Comment 1 • 6 years ago

Hi Dao,

It seems that mozregression pointed out Bug 1441788 for causing this regression.

Can you please take a look into this?

Thanks!

	:Gijs (he/him)
	Comment 2 • 6 years ago

(In reply to Emil Ghitta, QA [:emilghitta] from comment #1)
> It seems that mozregression pointed out Bug 1441788 for causing this
> regression.

Jamie, why would switching from a tooltiptext attribute to a tooltip change anything here? Even if it would, there's also a label attribute on the node, which is what I'd expect to be plugging in that information here and providing something to be read...

	James Teh [:Jamie]
	Comment 3 • 6 years ago

(In reply to :Gijs (Not available 3-19 Aug; he/him) from comment #2)
> Jamie, why would switching from a tooltiptext attribute to a tooltip change
> anything here?

The a11y engine uses tooltiptext as the name exposed for a11y, or if there's already a name (e.g. label attribute), tooltiptext gets exposed as the a11y description. In contrast, the tooltip attribute points to something that only appears on hover; a11y can't just get the text from it. This is problematic for three reasons:

1. It means the information is only accessible to users if they hover the mouse (and most screen reader users don't).
2. Even if they do hover the mouse, the information only appears in a separate tooltip node. That is, the information isn't exposed on the accessible for the Reader button. The association is purely visual: the tooltip visually appears near the button.
3. NVDA does have support for reading tooltips when they appear, but it only works for native Win32 tooltips for performance reasons.

The XUL tooltip attribute is an a11y problem in general. Another example of this is bug 1410757.

Can you explain why we prefer this tooltip technique rather than tooltiptext? Is it so the information doesn't have to be calculated ahead of time or is there some other reason?

> Even if it would, there's also a label attribute on the node,
> which is what I'd expect to be plugging in that information here and
> providing something to be read...

I don't see a label attribute. From browser.xul:

914                     <image id="reader-mode-button"
915                            class="urlbar-icon urlbar-page-action"
916                            tooltip="dynamic-shortcut-tooltip"
917                            role="button"
918                            hidden="true"
919                            onclick="ReaderParent.buttonClick(event);"/>

	Jared Wein [:jaws] (please needinfo? me)
	Comment 4 • 6 years ago

(for reply to comment 3 and since Dao fixed bug 1441788)

	Dão Gottwald [:dao] Assignee
	Comment 5 • 6 years ago

I could work around this using aria-label, but there seems to be a more fundamental question about the dynamic-shortcut-tooltip implementation from bug 940116.

	Jared Wein [:jaws] (please needinfo? me)
	Comment 6 • 6 years ago

I think we should keep dynamic-shortcut-tooltip but in UpdateDynamicShortcutTooltipText we can set aria-label on the triggerNode to the same value from the gDynamicTooltipCache. In my testing NVDA will read an updated value of the aria-label attribute.

	Dão Gottwald [:dao] Assignee
	Comment 7 • 6 years ago

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #6)
> I think we should keep dynamic-shortcut-tooltip but in
> UpdateDynamicShortcutTooltipText we can set aria-label on the triggerNode to
> the same value from the gDynamicTooltipCache. In my testing NVDA will read
> an updated value of the aria-label attribute.

UpdateDynamicShortcutTooltipText is called from onpopupshowing. Does the popup even show in the first place if you don't trigger it with the mouse?

	Jared Wein [:jaws] (please needinfo? me)
	Comment 8 • 6 years ago

No, it doesn't. I overlooked that.

I'm not even sure how to move keyboard focus to these buttons. I can only get keyboard focus to move to the site-identity button by using Ctrl+L to get focus on the location bar then Shift+Tab to move focus to the site identity buttons.

	Dão Gottwald [:dao] Assignee
	Comment 9 • 6 years ago

I'd expect you need to use NVDA to navigate the accessible elements.

	James Teh [:Jamie]
	Comment 10 • 6 years ago

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #8)
> I'm not even sure how to move keyboard focus to these buttons.

Currently, you can't. However, I'm working on fixing that in bug 1436086.

(In reply to Dão Gottwald [::dao] from comment #9)
> I'd expect you need to use NVDA to navigate the accessible elements.

You can get to the button with NVDA review commands:

1. Press alt+d to focus the address bar.
2. Repeatedly use the next object command to move through the page actions buttons. This command is NVDA+numpad6 with NVDA set to desktop layout, NVDA+shift+rightArrow when set to laptop layout. "NVDA" is insert or alternatively caps lock if you have the latter enabled in settings.

Note that it's probably simpler for most devs to confirm this using the Accessibility Inspector in the Browser Toolbox. If you find the DOM node for the button in the DOM Inspector, you should be able to right click it and select Show Accessibility Properties.

	Jared Wein [:jaws] (please needinfo? me)
	Comment 11 • 6 years ago

I can confirm using NVDA and the Accessibility pane that there is no label for this button, and I was able to access it using Jamie's steps from comment #10.

With Fluent we should be able to remove the UpdateDynamicShortcutTooltipText function since we can build out those tooltips inside of the .ftl files and can most likely remove usage of the dynamic-shortcut-tooltip altogether.

Can you use the aria-label workaround for now and then the Fluent conversion project can tackle removing the dynamic-shortcut-tooltip implementation?

Looking at UpdateDynamicShortcutTooltipText and the related strings, there are none that are context-specific and would need to be changed depending on external factors such as the target of the item or how many

	Dão Gottwald [:dao] Assignee
	Comment 12 • 6 years ago

Did some drive-by cleanup while I was touching this code.

	Pulsebot
	Comment 13 • 6 years ago

Pushed by dgottwald@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/758955ee971a
Set aria-label on Reader View button and set menuitem-specific attributes directly there instead of on the command element. r=jaws

	Tiberius Oros[:tiberius_oros]
	Comment 14 • 6 years ago
bugherder

https://hg.mozilla.org/mozilla-central/rev/758955ee971a

	Emil Ghitta, QA [:emilghitta] Reporter
	Comment 15 • 6 years ago

This issue is verified fixed using Firefox 64.0a1 (BuildId:20180905223809) on Windows 10 64bit.

The NVDA screen reader successfully reads the Reader View button on hover.

	Dão Gottwald [:dao] Assignee
	Comment 16 • 6 years ago

Approval Request Comment
[Feature/Bug causing the regression]: bug 1441788
[User impact if declined]: see comment 0
[Is this code covered by automated tests?]: no
[Has the fix been verified in Nightly?]: yes
[Needs manual test from QE? If yes, steps to reproduce]: 
[List of other uplifts needed for the feature/fix]: /
[Is the change risky?]: no
[Why is the change risky/not risky?]: just adding the aria-label attribute to the button
[String changes made/needed]: no

	Pascal Chevrel:pascalc (PTO until April 26)
	Comment 17 • 6 years ago

Comment on attachment 9006826 [details] [diff] [review]
patch for uplift

Approved for 63 beta 4

	Cristina Coroiu [:ccoroiu]
	Comment 18 • 6 years ago
bugherder uplift

https://hg.mozilla.org/releases/mozilla-beta/rev/a78183c5d8ba

	Emil Ghitta, QA [:emilghitta] Reporter
	Comment 19 • 6 years ago

This issue is verified fixed using Firefox 63.0b4 (BuildId:20180906162647) on Windows 10 64bit.

	Ryan VanderMeulen [:RyanVM]
	Comment 20 • 6 years ago

Jamie, is there value in uplifting this to ESR60?

	James Teh [:Jamie]
	Comment 21 • 6 years ago

While this is a trivial, very low risk patch for a clear regression, I don't think many users are going to be hurt by this; I haven't seen any complaints at all in the wild. I don't think it's worth uplifting to ESR.