1434860 - Remove support for <toolbarbutton type="menu-button"/>

Status: RESOLVED FIXED

(Toolkit :: UI Widgets, task, P1)

Description

[Dão Gottwald [:dao]]

It looks like only webconsole.xul uses this but that's dead code (bug 1381834). In bug 1387253, we dropped support for putting this button type in the main toolbars.

Comment 1

[Brian Grinstead [:bgrins]]

In addition to removing the menu-button binding here, we should be able to fold together menu-button-base and download-subview-toolbarbutton (which will now be the only thing extending that binding)

Comment 2

[:Paolo Amadini]

If bug 1381834 takes longer than expected, I'll simply change the button type in the old console, and land the patch after the merge to Beta has happened.

Comment 3

[:Paolo Amadini]

Attachment: Bug 1434860 - Part 1 - Don't use menu-button toolbar buttons in the console.

What used to be the main action can now be achieved using the Shift modifier key.

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

Comment 4

[:Paolo Amadini]

Brian, there will be more parts to this bug, but I'm already posting the prerequisite patch to get your opinion on the removal of the styles.

I just dropped the colored dots because the result looks good enough, and probably even better visually. As I understand it, this version of the console will also be removed soon, hopefully in this same release, so this change will only be visible for some time on Nightly.

Comment 5

[Brian Grinstead [:bgrins]]

(In reply to :Paolo Amadini from comment #4)
> Brian, there will be more parts to this bug, but I'm already posting the
> prerequisite patch to get your opinion on the removal of the styles.
> 
> I just dropped the colored dots because the result looks good enough, and
> probably even better visually. As I understand it, this version of the
> console will also be removed soon, hopefully in this same release, so this
> change will only be visible for some time on Nightly.

This console UI is only visible anymore in the Browser Console and the Browser Toolbox. We are tracking enabling it there in Bug 1362023, and I expect that should be enabled by default in nightly very soon (I'll find out more details about that today).

I played around with the patch applied and think it should be OK to remove this feature in the meantime.

Comment 6

[:Paolo Amadini]

Attachment: Bug 1434860 - Part 2 - Remove support for menu-button toolbar buttons.

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

Comment 7

[:Paolo Amadini]

Comment on attachment 8959145 [details]
Bug 1434860 - Part 1 - Don't use menu-button toolbar buttons in the console.

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

Comment 8

[Brian Grinstead [:bgrins]]

The following tests seem to be failing:

- devtools/client/webconsole/test/browser_console_keyboard_accessibility.js
- devtools/client/webconsole/test/browser_webconsole_bug_601667_filter_buttons.js
- devtools/client/webconsole/test/browser_webconsole_filter_buttons_contextmenu.js

I don't think we should spend too much time on them, but if it's easy to remove the parts that were testing the now removed feature that would be good. I'd want to get confirmation that we are expecting to ride the train this release before we did that.

Comment 9

[:Paolo Amadini]

Sounds good. I also posted a work in progress for part 2, which I believe edits all the other call sites, and I started a tryserver build to get a list of the tests that need updating:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=f3c5464bec47e10cd246fa77169387ddabefe964

Comment 10

[Brian Grinstead [:bgrins]]

Comment on attachment 8959145 [details]
Bug 1434860 - Part 1 - Don't use menu-button toolbar buttons in the console.

https://reviewboard.mozilla.org/r/228012/#review234014

I'm OK with this change, but will want to review a version with the test changes. Expecting to enable the new UI in the Browser Console next week, but this doesn't necessarily have to wait for that.

Comment 11

[:Paolo Amadini]

Comment on attachment 8959145 [details]
Bug 1434860 - Part 1 - Don't use menu-button toolbar buttons in the console.

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

Comment 12

[:Paolo Amadini]

Comment on attachment 8959166 [details]
Bug 1434860 - Part 2 - Remove support for menu-button toolbar buttons.

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

Comment 13

[Brian Grinstead [:bgrins]]

Comment on attachment 8959145 [details]
Bug 1434860 - Part 1 - Don't use menu-button toolbar buttons in the console.

https://reviewboard.mozilla.org/r/228012/#review234890

Thank you for fixing the tests. This looks good to me with a green try

Comment 14

[Neil Deakin]

Comment on attachment 8959166 [details]
Bug 1434860 - Part 2 - Remove support for menu-button toolbar buttons.

https://reviewboard.mozilla.org/r/228034/#review236206

::: toolkit/content/tests/chrome/test_bug557987.xul
(Diff revision 2)
>    <script type="application/javascript" src="chrome://mochikit/content/tests/SimpleTest/SimpleTest.js"></script>  
>    <script type="application/javascript" src="chrome://mochikit/content/tests/SimpleTest/EventUtils.js"></script>
>  
> -  <toolbarbutton id="button" type="menu-button" label="Test bug 557987"
> -                                               onclick="eventReceived('click');"
> +  <toolbarbutton id="button" type="menu" label="Test bug 557987"
> +                                         onclick="eventReceived('click');">
> -                                               oncommand="eventReceived('command');">

I think you should leave this in and just compare the number of command events to 0.

Comment 15

[:Paolo Amadini]

The new console UI is now enabled by default, but I still tested this manually by flipping the preferences and it works.

I'll land this if the tryserver build is also green:

https://treeherder.mozilla.org/#/jobs?repo=try&revision=459284cfe26a12cdea973dfb577551dde14f9835

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/121aada41a89
Part 1 - Don't use menu-button toolbar buttons in the console. r=bgrins
https://hg.mozilla.org/integration/mozilla-inbound/rev/c6d2b827e435
Part 2 - Remove support for menu-button toolbar buttons. r=enn

Comment 17

[Andrei Ciure[:aciure]]

https://hg.mozilla.org/mozilla-central/rev/121aada41a89
https://hg.mozilla.org/mozilla-central/rev/c6d2b827e435