Yura Zenevich [:yzen] (please use "needinfo") Assignee
	Description • 7 years ago

Similarly to Inspect Element menu item, Inspect Accessibility would select an accessible object in the Accessibility Inspector iff accessibility services and accessibility inspector tool are enabled.

	Yura Zenevich [:yzen] (please use "needinfo") Assignee
	Comment 1 • 6 years ago

Marking Julian to review Dev Tools bits and Shane the browser parts.

	Shane Caraveo (:mixedpuppy)
	Comment 2 • 6 years ago

Comment on attachment 8966252 [details] [diff] [review]
1428427 patch

r+ given the access key issue is resolved.


>diff --git a/browser/base/content/nsContextMenu.js b/browser/base/content/nsContextMenu.js

>+    var showInspectA11Y = this.inTabBrowser &&
>+                          Services.prefs.getBoolPref("devtools.enabled", true) &&
>+                          Services.prefs.getBoolPref("devtools.accessibility.enabled", true) &&
>+                          !Services.prefs.getBoolPref("devtools.policy.disabled", false) &&
>+                          // Only when accessibility service started.
>+                          Services.appinfo.accessibilityEnabled;

nit check accessibilityEnabled first.


>+  inspectA11Y() {
>+    return DevToolsShim.inspectA11Y(gBrowser.selectedTab, this.targetSelectors);
>+  },

Does this depend on a tab?  Would be nice to have this support non-tab browsers.


>diff --git a/browser/locales/en-US/chrome/browser/browser.dtd b/browser/locales/en-US/chrome/browser/browser.dtd

>+<!ENTITY inspectA11YContextMenu.label     "Inspect Accessibility Properties">
>+<!ENTITY inspectA11YContextMenu.accesskey "y">

Looks like this conflicts with copyImageContentsCmd.  I'm not certain what to do about that, maybe Julian has some thoughts.


>diff --git a/devtools/client/accessibility/test/browser_accessibility_context_menu_browser.js b/devtools/client/accessibility/test/browser_accessibility_context_menu_browser.js

>+add_task(async function testNoShowAccessibilityPropertiesContextMenu() {
>+  let tab = await addTab(buildURL(TEST_URI));
>+  let { linkedBrowser: browser } = tab;
>+
>+  let contextMenu = document.getElementById("contentAreaContextMenu");
>+  let awaitPopupShown = BrowserTestUtils.waitForEvent(contextMenu, "popupshown");
>+  await BrowserTestUtils.synthesizeMouse("#h1", 0, 0, {
>+    type: "contextmenu",
>+    button: 2,
>+    centered: true,
>+  }, browser);
>+  await awaitPopupShown;
>+
>+  info("Popup shown");

you can drop the info for shown/hidden (in other tests as well).

>+addA11YPanelTask("Test show accessibility properties context menu in browser.",

>+    info("Triggering 'Inspect Accessibility Properties' and waiting for " +
>+         "accessibility panel to open");
>+    let awaitPopupHidden = BrowserTestUtils.waitForEvent(contextMenu, "popuphidden");
>+    inspectA11YPropsItem.click();
>+    contextMenu.hidePopup();
>+
>+    await awaitPopupHidden;

probably not necessary to wait for the context menu to be hidden, later things are waiting on other ui to happen.

	Julian Descottes [:jdescottes]
	Comment 3 • 6 years ago

Comment on attachment 8966252 [details] [diff] [review]
1428427 patch

Review of attachment 8966252 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good, thanks Yura! Just a few minor comments.

::: browser/base/content/browser-context.inc
@@ +445,5 @@
>                  hidden="true"
>                  label="&inspectContextMenu.label;"
>                  accesskey="&inspectContextMenu.accesskey;"
>                  oncommand="gContextMenu.inspectNode();"/>
> +      <menuitem id="context-inspect-a11y"

Top of the file mentions:

  # NB: IF YOU ADD ITEMS TO THIS FILE, PLEASE UPDATE THE WHITELIST IN
  # BrowserUITelemetry.jsm. SEE BUG 991757 FOR DETAILS.

But I looked at a few "recent" bugs (Bug 862399, Bug 1276880) that updated this file and none of them seem to have updated BrowserUITelemetry. 
I guess this is just about updating _contextMenuItemWhitelist with "inspect-a11y" here? Let's ni? Gijs.

::: browser/base/content/nsContextMenu.js
@@ +439,5 @@
>                        Services.prefs.getBoolPref("devtools.inspector.enabled", true) &&
>                        !Services.prefs.getBoolPref("devtools.policy.disabled", false);
>  
> +    var showInspectA11Y = this.inTabBrowser &&
> +                          Services.prefs.getBoolPref("devtools.enabled", true) &&

Let's keep it as is for now, but we should discuss in which cases it would be ok to show this entry for discoverability purposes. 

If devtools is disabled, it could still initialize devtools and perform the action. If the accessibility panel is not enabled, maybe it could enable it?

@@ +450,5 @@
>      this.showItem("context-viewinfo", shouldShow);
>      // The page info is broken for WebExtension popups, as the browser is
>      // destroyed when the popup is closed.
>      this.setItemAttr("context-viewinfo", "disabled", this.webExtBrowserType === "popup");
>      this.showItem("inspect-separator", showInspect);

In theory we should display this if `showInspect || showInspectA11y` but realistically we can never have the a11y panel enabled without the inspector. Maybe make showInspectAlly = showInspect && (other conditions), to make it extra clear?

::: browser/locales/en-US/chrome/browser/browser.dtd
@@ +295,5 @@
>  <!ENTITY inspectContextMenu.label     "Inspect Element">
>  <!ENTITY inspectContextMenu.accesskey "Q">
>  
> +<!ENTITY inspectA11YContextMenu.label     "Inspect Accessibility Properties">
> +<!ENTITY inspectA11YContextMenu.accesskey "y">

Indeed, it conflicts with an existing key.

Quickly parsing the current menu items and accesskeys, averything from a-z is already taken except for j, x and z. The issue with our inspect elements is that they can be displayed whatever the target is, so they can conflict with 100% of the already defined accesskeys. Let's skip the access key for now.

::: devtools/client/framework/devtools.js
@@ +690,5 @@
> +    let target = TargetFactory.forTab(tab);
> +
> +    let toolbox = await gDevTools.showToolbox(
> +      target, "accessibility", null, null, startTime);
> +    await toolbox.initInspector();

The accessibility panel normally calls initInspector() in its initialization. 
It should already be done when await showToolbox is done I think?

::: devtools/startup/DevToolsShim.jsm
@@ +168,5 @@
> +   *        selectors can be needed if the element is nested in frames and not directly
> +   *        in the root document.
> +   * @return {Promise} a promise that resolves when the accessible node is selected in the
> +   *         accessibility inspector or that resolves immediately if DevTools are not
> +   *         installed.

nit: enabled rather than installed (we updated the language recently since we no longer aim to install devtools for the moment)

@@ +172,5 @@
> +   *         installed.
> +   */
> +  inspectA11Y: function(tab, selectors) {
> +    if (!this.isEnabled()) {
> +      return Promise.resolve();

In this case, the menu item is not displayed if devtools are not enabled, so not super important. But could we have the same logic as in inspectNode()? (ie open the install page if devtools are not policy  disabled)

	Yura Zenevich [:yzen] (please use "needinfo") Assignee
	Comment 4 • 6 years ago

(In reply to Shane Caraveo (:mixedpuppy) from comment #2)
> Comment on attachment 8966252 [details] [diff] [review]
> 1428427 patch
> 
> r+ given the access key issue is resolved.
> 
> 
> >diff --git a/browser/base/content/nsContextMenu.js b/browser/base/content/nsContextMenu.js
> 
> >+  inspectA11Y() {
> >+    return DevToolsShim.inspectA11Y(gBrowser.selectedTab, this.targetSelectors);
> >+  },
> 
> Does this depend on a tab?  Would be nice to have this support non-tab
> browsers.
> 

Right now this functionality mimics what the inspect node does, it would be nice indeed in non-tab browser, will file a bug.

	Yura Zenevich [:yzen] (please use "needinfo") Assignee
	Comment 5 • 6 years ago

(In reply to Julian Descottes [:jdescottes][:julian] from comment #3)
> Comment on attachment 8966252 [details] [diff] [review]
> 1428427 patch
> 
> Review of attachment 8966252 [details] [diff] [review]:
> -----------------------------------------------------------------
> ::: browser/base/content/nsContextMenu.js
> @@ +439,5 @@
> >                        Services.prefs.getBoolPref("devtools.inspector.enabled", true) &&
> >                        !Services.prefs.getBoolPref("devtools.policy.disabled", false);
> >  
> > +    var showInspectA11Y = this.inTabBrowser &&
> > +                          Services.prefs.getBoolPref("devtools.enabled", true) &&
> 
> Let's keep it as is for now, but we should discuss in which cases it would
> be ok to show this entry for discoverability purposes. 
> 
> If devtools is disabled, it could still initialize devtools and perform the
> action. If the accessibility panel is not enabled, maybe it could enable it?
> 

The main reason, I wanted to have all this constraints is because a11y panel starts accessibility service. Which might be unwanted in some cases. I think this is a good post Shield Study task, or once a11y panel telemetry starts coming in.

	Pulsebot
	Comment 6 • 6 years ago

Pushed by yura.zenevich@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/efa95de482d7
add 'Inspect Accessibility Properties' context menu item when right clicking on content element. r=jdescottes, mixedpuppy

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

https://hg.mozilla.org/mozilla-central/rev/efa95de482d7

	Chris Mills [:cmills]
	Comment 8 • 6 years ago

My accessibility inspector doc covers this:
https://developer.mozilla.org/en-US/docs/Tools/Accessibility_inspector#Notable_related_features

I also mention that it's been written in the Fx61 rel notes:
https://developer.mozilla.org/en-US/Firefox/Releases/61#Developer_tools