Closed Bug 1457278 Opened 6 years ago Closed 6 years ago

Include Accessibility tool in default set for Browser Toolbox, and hide Performance and Scratchpad by default

Categories

(DevTools :: Framework, enhancement)

enhancement
Not set
normal

Tracking

(firefox61 fixed)

RESOLVED FIXED
Firefox 61
Tracking Status
firefox61 --- fixed

People

(Reporter: bgrins, Assigned: bgrins)

References

Details

Attachments

(3 files)

I've just been doing some testing on accessibility in the browser chrome, and the tool is helpful. In order to make it easier for Firefox devs to find, I'd suggest we enable it by default in the Browser Toolbox.

To make room for it in the BT (since Scratchpad is also on by default) I'd suggest we disable the Performance tool since people use the Gecko Profiler instead. We could also consider disabling Memory tool, although I'm not sure if we have usage data for it specifically in the BT.
:yzen - are you comfortable enough with the state of the Accessibility tool when targeting browser chrome to turn it on by default in the Browser Toolbox? It seems to work fine, but I'd like to confirm with you.
Flags: needinfo?(yzenevich)
Hi Brian, I think at this point it works fairly well (there is an existing backlog but nothing blocking afaik). 

Caveat that you might've noticed, it does not, at the moment, let you peek into content frame in browser toolbox (which might change since our platform a11y has access to a11y proxy objects from content in parent process).

Also I did submit a PHD doc for a shield study to test it as default in Dev Edition to evaluate just that. While I like what you're suggesting, I would defer to you, Patrick, Harald et al to make this call.
Flags: needinfo?(yzenevich)
(In reply to Yura Zenevich [:yzen] from comment #2)
> Hi Brian, I think at this point it works fairly well (there is an existing
> backlog but nothing blocking afaik). 
> 
> Caveat that you might've noticed, it does not, at the moment, let you peek
> into content frame in browser toolbox (which might change since our platform
> a11y has access to a11y proxy objects from content in parent process).

By the way, the Fission work should provide a better long-term solution for digging into the content process in the BT. If the new platform support was only needed for supporting this use case (inspecting content from BT), I think I'd hold off on it. None of our tools support that well.

> Also I did submit a PHD doc for a shield study to test it as default in Dev
> Edition to evaluate just that. While I like what you're suggesting, I would
> defer to you, Patrick, Harald et al to make this call.

OK, I think since this is Browser Toolbox only we should have some more leeway to experiment, but I'll forward the question to get more opinions.

The specific proposal is:

1) Drop scratchpad from the default set in BT (since we are currently overflowing the toolbar in a default profile in BT, and you can use a normal Scratchpad window targeting chrome context to get the same effect).
2) Swap out performance tool with the accessibility tool in the BT (since we prefer people use Gecko Profiler / perf-html anyway for profiling). Greg, would that fit in with plans for exposing perf-html from the toolbox? AIUI that work is targeting the normal toolbox, and for chrome profiling people will still use the addon.
Flags: needinfo?(pbrosset)
Flags: needinfo?(hkirschner)
Flags: needinfo?(gtatum)
> While I like what you're suggesting, I would defer to you, Patrick, Harald et al to make this call.

Browser Toolbox is mostly internal and extensions focused, both audiences would benefit from A11Y testing – so I really like exposing this idea (and wonder if other panels would also work in the BT that we currently don't expose).

> 1) Drop scratchpad from the default set in BT (since we are currently overflowing the toolbar in a default profile in BT, and you can use a normal Scratchpad window targeting chrome context to get the same effect).
> 2) Swap out performance tool with the accessibility tool in the BT (since we prefer people use Gecko Profiler / perf-html anyway for profiling). Greg, would that fit in with plans for exposing perf-html from the toolbox? AIUI that work is targeting the normal toolbox, and for chrome profiling people will still use the addon.

On removing perf, I am not sure who would use the old Perf tool in the browser content. I could only imagine extension authors that are used to it from web development.

But with the toolbar redesign and improved overflow/sorting, would we it be enough to re-arrange the tabs by importance and let the long tail overflow?
Flags: needinfo?(hkirschner)
(In reply to :Harald Kirschner :digitarald from comment #6)
> > 1) Drop scratchpad from the default set in BT (since we are currently overflowing the toolbar in a default profile in BT, and you can use a normal Scratchpad window targeting chrome context to get the same effect).
> > 2) Swap out performance tool with the accessibility tool in the BT (since we prefer people use Gecko Profiler / perf-html anyway for profiling). Greg, would that fit in with plans for exposing perf-html from the toolbox? AIUI that work is targeting the normal toolbox, and for chrome profiling people will still use the addon.
> 
> On removing perf, I am not sure who would use the old Perf tool in the
> browser content. I could only imagine extension authors that are used to it
> from web development.
> 
> But with the toolbar redesign and improved overflow/sorting, would we it be
> enough to re-arrange the tabs by importance and let the long tail overflow?

Sure, but if there's no use-case for the Perf tool in the BT I'd rather just drop it entirely to de-clutter and prevent confusion. It may also be more work to reorder the tools in this manner - I haven't looked at how that's implemented yet, but the current pref setting happens at every BT startup so we'd have to be careful to not wipe out the user's custom order if they re-order and then restart.
(In reply to Brian Grinstead [:bgrins] from comment #3)
> (In reply to Yura Zenevich [:yzen] from comment #2)
> > Hi Brian, I think at this point it works fairly well (there is an existing
> > backlog but nothing blocking afaik). 
> > 
> > Caveat that you might've noticed, it does not, at the moment, let you peek
> > into content frame in browser toolbox (which might change since our platform
> > a11y has access to a11y proxy objects from content in parent process).
> 
> By the way, the Fission work should provide a better long-term solution for
> digging into the content process in the BT. If the new platform support was
> only needed for supporting this use case (inspecting content from BT), I
> think I'd hold off on it. None of our tools support that well.

This is something that is not specific to the tool itself but rather how gecko accessibility works with e10s. Screen readers can only register with parent process (on Windows with COM it is not exactly true) so we have a complete tree in parent process, though for accessible objects for things that are in content process we only have proxies not the actual objects.
(In reply to Yura Zenevich [:yzen] from comment #8)
> (In reply to Brian Grinstead [:bgrins] from comment #3)
> > (In reply to Yura Zenevich [:yzen] from comment #2)
> > > Hi Brian, I think at this point it works fairly well (there is an existing
> > > backlog but nothing blocking afaik). 
> > > 
> > > Caveat that you might've noticed, it does not, at the moment, let you peek
> > > into content frame in browser toolbox (which might change since our platform
> > > a11y has access to a11y proxy objects from content in parent process).
> > 
> > By the way, the Fission work should provide a better long-term solution for
> > digging into the content process in the BT. If the new platform support was
> > only needed for supporting this use case (inspecting content from BT), I
> > think I'd hold off on it. None of our tools support that well.
> 
> This is something that is not specific to the tool itself but rather how
> gecko accessibility works with e10s. Screen readers can only register with
> parent process (on Windows with COM it is not exactly true) so we have a
> complete tree in parent process, though for accessible objects for things
> that are in content process we only have proxies not the actual objects.

Ah OK, good to know. I agree it would make sense to display what we already know about from the parent process side of things. And it'd be a useful representation to see what a screen reader is actually getting - in fact I could see explicitly *not* wanting to update the tool to connect directly to the content process from the BT even once we have that capability from the devtools side, for that reason.
I don't have a good mental model of the use cases for BT and what's important to people.
I will say that the new toolbox tabbar with overflow area and re-ordering with drag/drop opens up many more possibilities.

But I can't imagine anyone using the old perf tool in the BT since we have perf.html. I would hide it by default.
As for the accessibility tool, I don't have an issue adding it to the mix by default, but again, I don't really know who would use it, and how often.

Maybe we need a poll with internal BT users so we can know what are the top 5-10 panels that people want the most.
Flags: needinfo?(pbrosset)
I just talked to Luca (:rpl) about extension developers, and he's of the opinion that none of them use the BT, and if they are using the performance panel in there, they're doing it wrong :)

So, hiding the perf panel in the BT sounds fine to me. We want users of BT to use perf.html with the addon anyway.
For the record, I'm fine disabling perf and scratchpad and enabling a11y by default. BT is a place we can experiment with improvements a little easier. Let's do it.
Clearing needinfo based on Comment 11
Flags: needinfo?(gtatum)
Assignee: nobody → bgrinstead
Status: NEW → ASSIGNED
> For the record, I'm fine disabling perf and scratchpad and enabling a11y by default. BT is a place we can experiment with improvements a little easier. Let's do it.

Works for me.
Comment on attachment 8971373 [details]
Bug 1457278 - Stop putting scratchpad in the default tool set in the Browser Toolbox;

https://reviewboard.mozilla.org/r/240122/#review246878
Attachment #8971373 - Flags: review?(pbrosset) → review+
Comment on attachment 8971374 [details]
Bug 1457278 - Remove the performance tool from the default tool set in the Browser Toolbox;

https://reviewboard.mozilla.org/r/240124/#review246880
Attachment #8971374 - Flags: review?(pbrosset) → review+
Comment on attachment 8972039 [details]
Bug 1457278 - Show the accessibility tool in the default tool set in the Browser Toolbox;

https://reviewboard.mozilla.org/r/240764/#review246882
Attachment #8972039 - Flags: review?(pbrosset) → review+
Pushed by bgrinstead@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/2ae98aca0b7e
Stop putting scratchpad in the default tool set in the Browser Toolbox;r=pbro
https://hg.mozilla.org/integration/autoland/rev/e48966927c68
Remove the performance tool from the default tool set in the Browser Toolbox;r=pbro
https://hg.mozilla.org/integration/autoland/rev/acddc570c047
Show the accessibility tool in the default tool set in the Browser Toolbox;r=pbro
Summary: Include Accessibility tool in default set for Browser Toolbox, and consider hiding Performance → Include Accessibility tool in default set for Browser Toolbox, and hide Performance and Scratchpad by default
Depends on: 1460584
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.