Closed Bug 1428441 Opened 7 years ago Closed 6 years ago

Add "Show accessibility properties" context menu item for inspector tree node.

Categories

(DevTools :: Accessibility Tools, enhancement, P2)

Product:
DevTools
Component:
Accessibility Tools
Version:
57 Branch
Type:
enhancement

Tracking

(firefox61 fixed)

Status:
RESOLVED FIXED
Milestone:
Firefox 61
Tracking Flags:
Tracking Status
firefox61 --- fixed

People

(Reporter: yzen, Assigned: yzen)

References

Details

(Keywords: dev-doc-complete)

Attachments

(1 file, 2 obsolete files)

1428441 patch v3
(deleted), patch
pbro
: review+
Details | Diff | Splinter Review 
If accessibility inspector is enabled and accessibility services are running, add "Show accessibility properties" menu item to have an ability to inspect accessibility for a given DOM node.
Severity: normal → enhancement
Priority: -- → P2
Component: Developer Tools: Inspector → Developer Tools: Accessibility Tools
Attached patch 1428441 patch (obsolete) (deleted) — — Splinter Review 

        Attachment #8965021 -
        Flags: review?(pbrosset)

    
    

    
  
Comment on attachment 8965021 [details] [diff] [review]
1428441 patch

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

::: devtools/client/accessibility/test/browser_accessibility_context_menu_inspector.js
@@ +2,5 @@
> +/* Any copyright is dedicated to the Public Domain.
> +http://creativecommons.org/publicdomain/zero/1.0/ */
> +"use strict";
> +
> +const TEST_URI = `<html>

nit: when test html pages are so simple, I like writing them on 1 line with a data-url, like this:
data:text/html;charset=utf-8,<h1 id="h1">header</h1><p id="p">paragraph</p>

But, up to you.

@@ +18,5 @@
> +  let env = await addTestTab(buildURL(TEST_URI));
> +  let { panel } = env;
> +
> +  let toolbox = await gDevTools.showToolbox(panel.target, "inspector");
> +  let inspector = toolbox.getCurrentPanel();

Since you already import the inspector's shared-head, I think you could replace those 2 lines with
let {toolbox, inspector} = await openInspector();

@@ +39,5 @@
> +       "accessibility panel to open");
> +  showA11YPropertiesNode.click();
> +  await accessibilitySelected;
> +
> +  let selected = await panel.once("new-accessible-front-selected");

It would make more sense to me if you also started waiting for this event *before* clicking the item:

info("Triggering 'Show Accessibility Properties' and waiting for " +
     "accessibility panel to open and the object to be selected");
let panelSelected = toolbox.once("accessibility-selected");
let objectSelected = panel.once("new-accessible-front-selected");
showA11YPropertiesNode.click();
await panelSelected;
await objectSelected;

@@ +44,5 @@
> +  let expectedSelected = await panel.getAccessibleForNode(
> +    inspector.selection.nodeFront);
> +  is(selected, expectedSelected, "Accessible front selected correctly");
> +
> +  disableAccessibilityInspector(env);

This seems like an important thing for all tests to do when they end. And really has nothing to do with the tests themselves, it's more related to the test runner. So it might be good to move this to a registerCleanupFunction() call in your head.js file.
Just a suggestion.

::: devtools/client/accessibility/test/head.js
@@ +293,5 @@
>    executeSoon(() => target.activeTab.navigateTo(url));
>    return once(target, waitForTargetEvent);
>  }
> +
> +async function selectNodeAndWaitForAnimations(data, inspector, reason = "test") {

Looks like this was copied from devtools\client\inspector\animation\test\head.js
The accessibility panel doesn't really have anything to do with web animations, so the function name should be changed and the comment removed.
Also, selecting a node in the inspector can be done by using the selectNode that comes from the inspector's shared-head.js file. I don't think there is a need for you to wrap it here. Just use selectNode directly.

@@ +306,5 @@
> + * Open the inspector menu and return all of it's items in a flat array
> + * @param {InspectorPanel} inspector
> + * @return An array of MenuItems
> + */
> +async function openContextMenuAndGetAllItems(inspector) {

This function already exists in devtools\client\inspector\test\head.js, it's a bit unfortunate to have to duplicate it here. So maybe instead, move the function from devtools\client\inspector\test\head.js to devtools\client\inspector\test\shared-head.js and then you will be able to just use it in your tests without having to redefine it here.

::: devtools/client/framework/toolbox.js
@@ +2555,5 @@
>        jsterm.inspectObjectActor(objectActor);
>      }
>    },
>  
> +  getAccessibleActor: async function(nodeFront) {

I'm thinking that maybe we don't need to bridge between the 2 panels like this.
Here's a suggestion, let me know what you think:
- we add a new actor method to the WalkerActor (the normal DOM walker that is, not the accessible walker)
- this method could be named something like hasAccessibleProperties, and would take a nodeActor as argument
- we remove this method here, and also the one you added in accessibility-panel.js
- instead, we use the new one from the inspector, when the menu is created.

The added value is not having to add these 2 methods here that link the 2 panels together somehow. And it avoids having to load the a11y panel at all just to check something we can easily check more easily elsewhere.

::: devtools/client/inspector/inspector.js
@@ +1519,5 @@
>  
> +    if (this.selection.isElementNode() &&
> +        Services.prefs.getBoolPref("devtools.accessibility.enabled") &&
> +        Services.appinfo.accessibilityEnabled) {
> +      let acc = await this._toolbox.getAccessibleActor(this.selection.nodeFront);

This call is potentially costly because it requires to load an entire panel if it has not been loaded yet. So this will slow down the opening of the context menu. I'm just concerned that it may be perceived by users as the inspector being slow.

I would advise calling this now, but not awaiting the response. Instead, have a promise resolution handler that later go back to the item and enable/disable it as needed.

Note that with my previous comment, you would not be calling this method here, but instead this.walker.hasAccessibleProperties(this.selected.nodeFront), or something like this.

@@ +1527,5 @@
> +          label: INSPECTOR_L10N.getStr("inspectorShowAccessibilityProperties.label"),
> +          click: () => this.showAccessibilityProperties(acc),
> +        }));
> +      } else {
> +        menu.append(new MenuItem({

I'm not entirely sure we should have an else part here. What about only showing the item when we can show actually properties?

@@ +1937,5 @@
> +    // Select new accessible for a node front.
> +    a11yPanel.selectAccessible(accessibleFront);
> +    // new-accessible-front-selected tells us when the accessible has been
> +    // selected.
> +    await onNewAccessible;

There's a bit too much comments here. One is repeated too. What about simplifying to:

// Select the object in the panel, waiting for the event that tells us it has been done.
let onSelected = a11yPanel.once("new-accessible-front-selected");
a11yPanel.selectAccessibly(accessibleFront);
await onSelected;

        Attachment #8965021 -
        Flags: review?(pbrosset)

    
    

    
  
(In reply to Patrick Brosset <:pbro> from comment #2)
> Comment on attachment 8965021 [details] [diff] [review]
> 1428441 patch
> 
> Review of attachment 8965021 [details] [diff] [review]:
> -----------------------------------------------------------------
> @ +18,5 @@
> > +  let env = await addTestTab(buildURL(TEST_URI));
> > +  let { panel } = env;
> > +
> > +  let toolbox = await gDevTools.showToolbox(panel.target, "inspector");
> > +  let inspector = toolbox.getCurrentPanel();
> 
> Since you already import the inspector's shared-head, I think you could
> replace those 2 lines with
> let {toolbox, inspector} = await openInspector();
> 

This would require also loading test-actor-registry.js, I can however simplify this since I already have a toolbox.

    
    

    
  

      
      
      
      

        Attached patch
          
          
        1428441 patch v2 (obsolete) (deleted)
        — 
          — Splinter Review
      

    


  
Hopefully addressed your suggestions :)

        Attachment #8965021 -
        Attachment is obsolete: true

        Attachment #8965452 -
        Flags: review?(pbrosset)

    
    

    
  

      
      
      
      

        Attached patch
          
          
        1428441 patch v3 (deleted)
        — 
          — Splinter Review
      

    


  
Added a backward compatibility check with actorHasMethod

        Attachment #8965452 -
        Attachment is obsolete: true

        Attachment #8965452 -
        Flags: review?(pbrosset)

        Attachment #8965549 -
        Flags: review?(pbrosset)

    
    

    
  
Comment on attachment 8965549 [details] [diff] [review]
1428441 patch v3

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

::: devtools/client/inspector/inspector.js
@@ +1499,5 @@
>        label: INSPECTOR_L10N.getStr("inspectorShowDOMProperties.label"),
>        click: () => this.showDOMProperties(),
>      }));
>  
> +    if (this.selection.isElementNode() &&

Please move this whole block into a separate function like this.buildA11YMenuItem

@@ +1507,5 @@
> +        label: INSPECTOR_L10N.getStr("inspectorShowAccessibilityProperties.label"),
> +        click: () => this.showAccessibilityProperties(),
> +        disabled: true
> +      });
> +      this.target.actorHasMethod("domwalker", "hasAccessibilityProperties")

If this code is moved to another method, you can make this method async/await, so it's easier to read, but don't call the method itself with await, to avoid blocking the construction of the menu on this one item.

        Attachment #8965549 -
        Flags: review?(pbrosset) → review+

    
    

    
  
Pushed by yura.zenevich@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/86b6ee084e0c
adding 'Show Accessibility Properties' context menu item for inspector markup view nodes. r=pbro

    
    

    
  
https://hg.mozilla.org/mozilla-central/rev/86b6ee084e0c
Status: ASSIGNED → RESOLVED
Closed: 6 years ago

          status-firefox61:
          --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 61

    
    

    
  
The issue is still reproducible on WIndows 7 x64 on the latest Nightly version 61.0a1. (Build ID: 20180419224145).
Should I re-open the issue or submit a new bug?
Flags: needinfo?(yzenevich)

    
    

    
  
Sorry for the inconvenience, it was  actually a misunderstanding I realized after acknowledging https://bugzilla.mozilla.org/show_bug.cgi?id=1453237.
Flags: needinfo?(yzenevich)

    
  
Product: Firefox → DevTools

          You need to log in
          before you can comment on or make changes to this bug.
        






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: