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

RESOLVED FIXED in Firefox 61

Status

()

P1
normal
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: dao, Assigned: Paolo)

Tracking

(Blocks: 3 bugs)

Trunk
mozilla61
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox61 fixed)

Details

Attachments

(2 attachments)

(Reporter)

Description

a year ago
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.
(Reporter)

Updated

a year ago
status-firefox60: affected → ---
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)
(Assignee)

Comment 2

a year ago
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.
Assignee: nobody → paolo.mozmail
Status: NEW → ASSIGNED
Priority: -- → P1
Comment hidden (mozreview-request)
(Assignee)

Comment 4

a year ago
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.
Flags: needinfo?(bgrinstead)
(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.
Flags: needinfo?(bgrinstead)
Blocks: 1381834
No longer depends on: 1381834
Comment hidden (mozreview-request)
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.
(Assignee)

Comment 9

a year ago
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
(Assignee)

Updated

a year ago
Blocks: 1445993

Comment 10

a year ago
mozreview-review
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.
Attachment #8959145 - Flags: review?(bgrinstead)
Comment hidden (mozreview-request)
Comment hidden (mozreview-request)
(Assignee)

Updated

a year ago
Attachment #8959145 - Flags: review?(bgrinstead)
Attachment #8959166 - Flags: review?(enndeakin)
(Assignee)

Updated

a year ago
Attachment #8959145 - Flags: review?(bgrinstead)
(Assignee)

Updated

a year ago
Blocks: 1430028

Comment 13

a year ago
mozreview-review
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
Attachment #8959145 - Flags: review?(bgrinstead) → review+

Comment 14

a year ago
mozreview-review
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.
Attachment #8959166 - Flags: review?(enndeakin) → review+
(Assignee)

Comment 15

a year ago
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

a year ago
Pushed by paolo.mozmail@amadzone.org:
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

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/121aada41a89
https://hg.mozilla.org/mozilla-central/rev/c6d2b827e435
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
status-firefox61: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
You need to log in before you can comment on or make changes to this bug.