Kris Maglione [:kmag] Reporter
	Description • 8 years ago

      No description provided.

	Caitlin Neiman [:caitmuenster]
	Comment 1 • 8 years ago

To be discussed during Nov. 15, 2016 web extensions triage meeting.

	Andy McKay
	Comment 2 • 8 years ago

Kris, could describe this feature in a bit more detail please?

	Kris Maglione [:kmag] Reporter
	Comment 3 • 8 years ago

Basically, we just need to create a menu-button type toolbar button, and then use something like contextMenus API to allow the extension to add menu items to it.

	Martin Giger [:freaktechnik]
	Comment 4 • 8 years ago

This would be a secondary menu button (a little arrow pointing down) and the main action of the page action would still be something different?

	Kris Maglione [:kmag] Reporter
	Comment 5 • 8 years ago

Yes.

	brunoais
	Comment 6 • 8 years ago

Are you open for someone to make a proposal for this through a legacy addon exposing an API for a web extensions addon to use?

	Andy McKay
	Comment 7 • 8 years ago

(In reply to brunoais from comment #6)
> Are you open for someone to make a proposal for this through a legacy addon
> exposing an API for a web extensions addon to use?

Could you expand on that please, I'm not sure I follow.

	brunoais
	Comment 8 • 8 years ago

I write a legacy extension with the ideas of these docs:
https://webextensions-experiments.readthedocs.io/en/latest/how.html
and the same ideology as:
https://github.com/web-ext-experiments/logins

Then I do an example Web Extension that exemplifies of the new API in action just like logins example already does.

	Andy McKay
	Comment 9 • 8 years ago

Ah right, that sounds great.

	brunoais
	Comment 10 • 8 years ago

I won't be able to start on this in this month but I can start working on this in December. Anyone who wants to do it instead of me, feel free to do it and warning here to avoid both doing the same thing.

	brunoais
	Comment 12 • 8 years ago

I prefer how they did with greasemonkey.
The vertical drawing in the middle making it clear there are two buttons seem better IMO.
Oh! By the way,
does anyone think we can allow any very small image or character instead of forcing the use of an inverted triangle?

	flying sheep
	Comment 14 • 8 years ago

> I prefer how they did with greasemonkey. The vertical drawing in the middle making it clear there are two buttons seem better IMO.

That’s the default appearance for <button type="menu-button">. And I also like it better. In Qt, a button with ABP’s appearance would behave like a BrowserAction with panel, i.e. it’s a (non-segmented) button that signifies that it triggers no immediate action but opens a panel.

> does anyone think we can allow any very small image or character instead of forcing the use of an inverted triangle?

sure! actually the segmented bookmark button is implemented like this. its right half is the arrow part of a menu-button, with overriden icon.

	flying sheep
	Comment 15 • 8 years ago

by the way: bug 1320876 isn’t actually a duplicate of this. there’s also real combined buttons, which that one is about.

those combined buttons could also be used to get a segmented button with multiple icons.

	Sergio
	Comment 16 • 7 years ago

(In reply to Kris Maglione [:kmag] from comment #3)
> Basically, we just need to create a menu-button type toolbar button, and
> then use something like contextMenus API to allow the extension to add menu
> items to it.

Or not just menu buttons but panels too, I've an extensions that uses the drop marker to open a panel and the main button to do the main addon function.

	Andreas Wagner [:TheOne] [use NI]
	Comment 17 • 7 years ago

Personas Plus would need this (in case we want to keep the same UI).

	Michael Buckley
	Comment 18 • 7 years ago

Here is a patch that I put together, not the most elegant solution, but it works well.

It defines a new entry for the manifest "dropmarker_popup" works kind of like "default_popup" except it only gets shown when the user clicks on the dropmarker.

I just released as I write this, I forgot to write the getter and setter methods for it. But that should just be a copy and paste job. I will do it another day, I have spent more time then I have already on this for now.  I am interested in feedback on what people think of this approach? If it is good, I will add a completed patch with the missing methods.

With adding icons.  Should it be just add something like "dropmarker_icons" that works the same as "default_icons" or should it simply be a single image?

	flying sheep
	Comment 19 • 7 years ago

If the button is a menu-button, then the click-event’s originalTarget is another DOM-object with the class “toolbarbutton-menubutton-button”. Checking this is more robust than your click position check.

Note that this element is only available if type==="menu-button"

	Michael Buckley
	Comment 20 • 7 years ago

flying sheep, I did try that at first as well (well not exactly, but a variation of it). The problem is I am looking for the dropmaker, not that button.  You can't just assume that every click that is not on the button is on the dropmaker.  There is for example a space of about 2px (a guess) wide on the left side of the button. As I said in the last post, it is not elegant, but it works.

The first thing I was looking for was to act directly on the dropmaker. But the originalTarget is never set to the dropmaker. Also you don't appear to be able to attach events directly to the dropmaker, they never fire.

	flying sheep
	Comment 21 • 7 years ago

I don't think that problem should be fixed here.

Hovering those 2 pixels will also give the visual feedback that the icon part of the button isn't hovered, which also needs to be fixed.

We should directly fix the CSS of menu buttons until the hover style and therefore behavior matches what we want.

We could use invisible borders or even pointer-events:none for that.

	Michael Buckley
	Comment 22 • 7 years ago

So what do you think add something like this to the browser.css (somewhere after the 'toolbar .toolbarbutton-1' which is around line 724)?

toolbar .toolbarbutton-1[type="menu-button"] {
    padding:0;
}

The 2px should be redundant anyway since the toolbarbutton inside it has the padding.  I could put it in another patch, and post it as another bug that is blocking this one (if that is the correct way to proceed).

The test that I would make is not for the class then but rather if event.originalTarget.getAttribute('anonid') == 'button'.  I think would make more sense.

Let me know, and when I have time I will try and get the patches together, along with finishing the implementation (the missing methods, plus I will try adding an icon).

What version of Firefox could I realistically hope to get this into? I kind of want it for a few of my add-ons.

	flying sheep
	Comment 23 • 7 years ago

I wouldn’t say it’s blocking. It’s more like a minor UX problem that surfaces here, but can be fixed independently.

If we still have a clickable rim after this CSS fix, we could also e.g. go with giving the icon a transparent (but clickable) border and corresponding negative margin.

I also want it for my addons. I really think they should provide a way to fast-track Addon API patches, or there’ll be a long period in which some addons are broken.

	Hector Zhao [:hectorz]
	Comment 24 • 7 years ago

FYI, just noticed the styles for `toolbarbutton[type="menu-button"]` was removed in bug 1387253, because "WebExtensions can't add such buttons".

	flying sheep
	Comment 25 • 7 years ago

this just means the reverse patch has to be added to this one. How are they coming along, btw?

	Dão Gottwald [:dao]
	Comment 26 • 7 years ago

(In reply to flying sheep from comment #25)
> this just means the reverse patch has to be added to this one. How are they
> coming along, btw?

It's a lot of styling that was removed and I don't think we're ready to commit to maintaining this again.

	flying sheep
	Comment 27 • 7 years ago

I can only interpret this the following way, which can’t be true for a contribution friendly project:

> The order of patches matters. Contributing to firefox is a
> race where the faster patcher wins and shifts the goal posts.
> Patches will be rejected if they’re slightly too late.

I hope you’re not saying this and there’s a way to get this patch (+necessary styling) in.

	Karoly Pados
	Comment 28 • 7 years ago

+1 in support for this ticket. As an add-on developer, I also want a way to show a menu added using the contextMenu API on left-click on toolbar (or address bar) button.

	Dão Gottwald [:dao]
	Comment 29 • 7 years ago

(In reply to flying sheep from comment #27)
> I hope you’re not saying this and there’s a way to get this patch
> (+necessary styling) in.

I don't think we'll add the styling back. However I expect this could be implemented as two <toolbarbutton>s within a <toolbaritem> rather than <toolbarbutton type="menu-button">.

	flying sheep
	Comment 30 • 7 years ago

Sure, whatever works! The two-part bookmarks button works that way.

And we could add menu-buttons together with multipart-buttons in that style, further enhancing addon authoring possibilities.

	Michael Buckley
	Comment 31 • 7 years ago

Sorry for no progress on this, been really busy.  Going on holidays next week, so hopefully have some time to play around with this again.

Implementing it as two toolbarbuttons within a toolbaritem would be reinventing the wheel, and also means you would have to add even more styling to make it work.  It would also greatly increase the complexity of the code.  In fact I would suspect a total rewrite of how ext-browserAction.js works might be called for.  

In short avoiding <toolbarbutton type="menu-button"> simply because you don't want to bring some style rules back would be very counter productive.

	Dão Gottwald [:dao]
	Comment 32 • 6 years ago

Bug 1434860 completely removed support for <toolbarbutton type="menu-button"/>.

	David Durst [:ddurst]
	Comment 33 • 6 years ago

Clearing priority and flags for re-triage.