Closed Bug 992857 Opened 10 years ago Closed 10 years ago

"Attaching panels to buttons" documentation is misleading

Categories

(Add-on SDK Graveyard :: Documentation, defect)

Product:
Add-on SDK Graveyard
Component:
Documentation
Platform:
x86
macOS
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED FIXED

People

(Reporter: zer0, Unassigned)

Details

The current documentation about attaching panels to buttons is misleading in several ways:

1. It doesn't explain why is suggested the use of `ToggleButton` instead of `ActionButton` (and viceversa, it doesn't highlight that `ActionButton` can be used too but it's not suggested).

2. The snapshot is not correct – is the one related to `ActionButton`, doesn't shows the depressed state

3. The code is not correct – is the one related to `ActionButton`, only the class is changed, where for `ToggleButton` needs a bit more code to wire the button to the panel and viceversa, see: https://github.com/mozilla/addon-sdk/pull/1398

We should probably tell in documenation that panels can be linked to `ActionButton`, but emphasize that `ToggleButton` is the suggested way due to have the same look & feel of every other button in Firefox UI with a panel attached – a snapshot of the hamburger menu or download button should be enough I guess - and then shows the code that does that link.

Panel documentation link:
https://developer.mozilla.org/en-US/Add-ons/SDK/High-Level_APIs/panel#Attaching_panels_to_buttons
Thanks for reporting that. I've fixed the code and updated the screenshot. About this:

> We should probably tell in documenation that panels can be linked
> to `ActionButton`, but emphasize that `ToggleButton` is the suggested way

I originally did include documentation for this. Then I got told "Where is definitely possible attach a panel to `ActionButton`, we strongly suggest to use always `ToggleButton` instead ... I would avoid to suggest to the devs to use the wrong component for anchoring panels."

I think that if we don't want people to do it, we should probably not allow it in the API and not document it either. How strong is "suggested way"? If people write add-ons that anchor panels to Action Buttons, will they pass AMO review?
(In reply to Will Bamberg [:wbamberg] from comment #1)

> I originally did include documentation for this. Then I got told "Where is
> definitely possible attach a panel to `ActionButton`, we strongly suggest to
> use always `ToggleButton` instead ... I would avoid to suggest to the devs
> to use the wrong component for anchoring panels."

And this is what this bug is about: we should suggest to the developers the right way to attach panels to buttons. The previous documentation was using `ActionButton`, that wasn't correct, and the actual documentation is not correct either for the reasons exposed above – basically `ActionButton` was replaced by `ToggleButton` and that's all.

We should suggest to the devs to use the proper component, explain why `ToggleButton` is suggested, and provide the correct code – the code in the documentation is broken, there is no link between the button and the panel, so once the panel is hide by clicking somewhere else, the button is still in depressed state.

> I think that if we don't want people to do it, we should probably not allow
> it in the API and not document it either.

I'm okay if we want to omit the code for `ActionButton`, but still we should explain why `ToggleButton`, and write the proper code to wire that button to a panel.

> How strong is "suggested way"? If
> people write add-ons that anchor panels to Action Buttons, will they pass
> AMO review?

That's a good question. I've no idea how be consistent with native UI will impact on the AMO review.

But still, we should at least fix the code in the documentation and provide the correct snapshot.
(In reply to Will Bamberg [:wbamberg] from comment #1)
> Thanks for reporting that. I've fixed the code and updated the screenshot.

Missed the line – I always think it's the "in reply line" when there is a quote after.

Let's keep this bug open then to figured out what we should do about the `ActionButton`, especially depends by how much will impact to the AMO review.
(In reply to Will Bamberg [:wbamberg] from comment #1)
> Thanks for reporting that. I've fixed the code and updated the screenshot.
> About this:
> 
> > We should probably tell in documenation that panels can be linked
> > to `ActionButton`, but emphasize that `ToggleButton` is the suggested way
> 
> I originally did include documentation for this. Then I got told "Where is
> definitely possible attach a panel to `ActionButton`, we strongly suggest to
> use always `ToggleButton` instead ... I would avoid to suggest to the devs
> to use the wrong component for anchoring panels."
> 
> I think that if we don't want people to do it, we should probably not allow
> it in the API and not document it either. How strong is "suggested way"? If
> people write add-ons that anchor panels to Action Buttons, will they pass
> AMO review?

I think they will pass AMO review, certainly the low level review. I think we just shouldn't document being able to attach to ActionButton at all. If a developer figure it out then fine but I don't think it's worth the extra time to make it impossible in code.
Will, what's remaining to do here?
Flags: needinfo?(wbamberg)
I think nothing - it doesn't say you can attach panels to ActionButtons, but from Mossop's comment 4 it shouldn't.
Flags: needinfo?(wbamberg)
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Why should nobody attach panels to action buttons. It's intuitive and makes sense for most add-ons and extensions with drop-downs. Sure you don't want to do them in all cases but the Active.js and Toggle.js are virtually identical. Both should have attachable panel support.
(In reply to Hopecraft from comment #7)

> Why should nobody attach panels to action buttons. It's intuitive and makes
> sense for most add-ons and extensions with drop-downs. Sure you don't want
> to do them in all cases but the Active.js and Toggle.js are virtually
> identical. Both should have attachable panel support.

In fact, you can. But it's not suggested, and I'm not sure it will be pass the AMO review – we should ask to some reviewer about that – because it won't be consistent with the current Firefox UI: any button that have a panel attached remains in depressed state when the panel is visible. With ActionButton that is not possible, so the add-on that attach a panel to an ActionButton will looks different from the rest of buttons with a panel. Where, with ToggleButton, you will have the same UI consistency, and a better look & feels for the user.
(In reply to Matteo Ferretti [:matteo] [:zer0] from comment #8)
> (In reply to Hopecraft from comment #7)
> 
> > Why should nobody attach panels to action buttons. It's intuitive and makes
> > sense for most add-ons and extensions with drop-downs. Sure you don't want
> > to do them in all cases but the Active.js and Toggle.js are virtually
> > identical. Both should have attachable panel support.
> 
> In fact, you can. But it's not suggested, and I'm not sure it will be pass
> the AMO review – we should ask to some reviewer about that – because it
> won't be consistent with the current Firefox UI: any button that have a
> panel attached remains in depressed state when the panel is visible. With
> ActionButton that is not possible, so the add-on that attach a panel to an
> ActionButton will looks different from the rest of buttons with a panel.
> Where, with ToggleButton, you will have the same UI consistency, and a
> better look & feels for the user.

First off with the current public SDK and using current Aurora attaching panels to ActionButtons is not supported and causes cfx to crash or the js to error depending on how you try to do it. Believe me, I've tired all morning. I successfully attached a panel to a widget but it robs the button-like effect. And even with the documentation's own Attaching Panels to Buttons with ToggleButton code results in invalid testing.

Second, I don't believe it necessarily needs to stay in a depressed manner when clicked. Buttons are meant to be pushed, and come back up, activating whatever. A nice flash effect, like my group has used in extensions before, works very nicely to tell the user "Add-On Panel Activating"
(In reply to Hopecraft from comment #9)

> First off with the current public SDK and using current Aurora attaching
> panels to ActionButtons is not supported and causes cfx to crash or the js
> to error depending on how you try to do it.

In Firefox > 29 should be supported just passing the button as position. And definitely shouldn't crash cfx. Could you please file a bug with the step to reproduce the issue?

> And even with the documentation's own Attaching Panels
> to Buttons with ToggleButton code results in invalid testing.

What do you mean "in invalid testing"?

> Second, I don't believe it necessarily needs to stay in a depressed manner
> when clicked.

UI has to be consistent, and this is what UX decided for the buttons that have a panel attached, in Firefox.
You're free to attach the panel to the ActionButton, but it won't be consistent with Firefox UI experience, that's all. And that's why we suggest to use ToggleButton instead.
> What do you mean "in invalid testing"?

I mean you put in the code they provide in a test add-on you make and it doesn't actually result in the effect of attachment or even generation of a panel. The only thing that occurs is the button toggles back and forth manually.

>UI has to be consistent, and this is what UX decided for the buttons that have a panel attached, in >Firefox.
>You're free to attach the panel to the ActionButton, but it won't be consistent with Firefox UI >experience, that's all. And that's why we suggest to use ToggleButton instead.

I have no found any button except action button that actually triggers the generation of a panel and an attached panel is only generated using a widget. 

>In Firefox > 29 should be supported

Even testing or running overriding the browser versions and using the current SDK versions didn't render it to work. I may submit a but report but I have little experience in doing so. I am primarily a web designer and graphic ui designer, coding is one of my second-tier skills.
(In reply to Hopecraft from comment #11)
> > What do you mean "in invalid testing"?

> I mean you put in the code they provide in a test add-on you make and it
> doesn't actually result in the effect of attachment or even generation of a
> panel. The only thing that occurs is the button toggles back and forth
> manually.

Not sure I'm following you, but I think I need more context on that, and definitely this is not the bug we should continue the discussion.

> >UI has to be consistent, and this is what UX decided for the buttons that have a panel attached, in >Firefox.
> >You're free to attach the panel to the ActionButton, but it won't be consistent with Firefox UI >experience, that's all. And that's why we suggest to use ToggleButton instead.
> 
> I have no found any button except action button that actually triggers the
> generation of a panel and an attached panel is only generated using a
> widget. 

Every other buttons in Firefox UI, with a panel attached, behaves like that. For example, the download button, or buttons with menu, or the bookmark button, or the new "hambuger" button.
With Add-on SDK, we have to provide the same look & feel of the "native" UI.

> >In Firefox > 29 should be supported

> Even testing or running overriding the browser versions and using the
> current SDK versions didn't render it to work. I may submit a but report but
> I have little experience in doing so.

So, I used the last Firefox Aurora (on Mac), execute the code provided in the documentation about attaching panels to toggle buttons, and everything seems work as expected. I run the add-on using `cfx run -b /Applications/FirefoxAurora.app` on the terminal.

I also used `ActionButton` instead, and it works as well. If you have trouble, please file the bug and we'll try to understand why in your case it doesn't work. You can click here: https://bugzilla.mozilla.org/enter_bug.cgi?product=Add-on%20SDK&component=General

And fill the fields. It's important that you describe what's the issue and how to reproduce it, and what do you expect.
If you have any question, let me know. You can also join the #jetpack channel in IRC to get assistance.
You need to log in before you can comment on or make changes to this bug.