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

Status: RESOLVED FIXED

(DevTools :: Framework, enhancement)

Description

[Brian Grinstead [:bgrins]]

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.

Comment 1

[Brian Grinstead [:bgrins]]

: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.

Comment 2

[Yura Zenevich [:yzen] (please use "needinfo")]

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.

Comment 3

[Brian Grinstead [:bgrins]]

(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.

Comment 4

[Brian Grinstead [:bgrins]]

Attachment: Bug 1457278 - Stop putting scratchpad in the default tool set in the Browser Toolbox;

We currently overflow the toolbar menu in a clean profile in the Browser Toolbox.
Scratchpad can be accessed from the parent process by opening the Scratchpad window
and targeting chrome, so this patch removes it from the default set.

Review commit: https://reviewboard.mozilla.org/r/240122/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/240122/

Comment 5

[Brian Grinstead [:bgrins]]

Attachment: Bug 1457278 - Remove the performance tool from the default tool set in the Browser Toolbox;

The performance tool for browser chrome has been replaced by Gecko Profiler and perf-html,
and the accessibility tool is a useful new tool that will be better found in the default
tool set.

Review commit: https://reviewboard.mozilla.org/r/240124/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/240124/

Comment 6

[:Harald Kirschner :digitarald]

> 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?

Comment 7

[Brian Grinstead [:bgrins]]

(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.

Comment 8

[Yura Zenevich [:yzen] (please use "needinfo")]

(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.

Comment 9

[Brian Grinstead [:bgrins]]

(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.

Comment 10

[Patrick Brosset <:pbro>]

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.

Comment 11

[Patrick Brosset <:pbro>]

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.

Comment 12

[Patrick Brosset <:pbro>]

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.

Comment 13

[Brian Grinstead [:bgrins]]

Attachment: Bug 1457278 - Show the accessibility tool in the default tool set in the Browser Toolbox;

This is a new tool that will be better found if it is in the default set.

Review commit: https://reviewboard.mozilla.org/r/240764/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/240764/

Comment 14

[Brian Grinstead [:bgrins]]

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

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/240122/diff/1-2/

Comment 15

[Brian Grinstead [:bgrins]]

Comment on attachment 8971374 [details]
Bug 1457278 - Remove the performance tool from the default tool set in the Browser Toolbox;

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/240124/diff/1-2/

Comment 16

[Brian Grinstead [:bgrins]]

Clearing needinfo based on Comment 11

Comment 17

[Greg Tatum [:gregtatum] (REO Fx132)]

> 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 18

[Patrick Brosset <:pbro>]

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

Comment 19

[Patrick Brosset <:pbro>]

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

Comment 20

[Patrick Brosset <:pbro>]

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

Comment 21

[Pulsebot]

Pushed by [e-mail]:
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

Comment 22

[Raul Gurzau (:RaulG)]

https://hg.mozilla.org/mozilla-central/rev/2ae98aca0b7e
https://hg.mozilla.org/mozilla-central/rev/e48966927c68
https://hg.mozilla.org/mozilla-central/rev/acddc570c047