787395 - Add Sidebars components type

Status: RESOLVED FIXED

(Add-on SDK Graveyard :: General, defect, P1)

Description

[Matteo Ferretti [:zer0] [:matteo]]

As the UX mockups shows:

http://people.mozilla.com/~shorlander/files/addons-in-toolbar-i01/addons-in-toolbar.html

Some widgets needs to display vertical content that doesn't overlap with the content of the current tab (e.g. Twitter's stream, devtools, etc).

Comment 1

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Might also be nice to have an option to have the bar along the bottom of the window instead of the side, for content that's wider than it is tall.

Comment 2

[Gregg Lind (Fx Strategy and Insights - Shield - Heartbeat )]

"Arbitrary chrome priv'd html, stuck onto anything in a box" is still my holy grail on this stuff :)  At least allow style and placement overrides!

(Use case:  deep ui experiments, like icon based right click menus)

Comment 4

[Erik Vold [:erikvold] (please needinfo? me)]

(In reply to Gregg Lind (User Research - Test Pilot) from comment #2)
> "Arbitrary chrome priv'd html, stuck onto anything in a box" is still my
> holy grail on this stuff :)  At least allow style and placement overrides!

That sounds good to me :)

Comment 5

[Erik Vold [:erikvold] (please needinfo? me)]

I wrote a JEP for this over at https://github.com/mozilla/addon-sdk/wiki/JEP-Sidebars

Comment 6

[Erik Vold [:erikvold] (please needinfo? me)]

Attachment: Pointer to Github pull request: https://github.com/mozilla/addon-sdk/pull/997

Pointer to Github pull-request

Comment 7

[Erik Vold [:erikvold] (please needinfo? me)]

Comment on attachment 750243 [details]
Pointer to Github pull request: https://github.com/mozilla/addon-sdk/pull/997

I'm reusing the built-in sidebar here, we could implement our own though.

Comment 8

[Erik Vold [:erikvold] (please needinfo? me)]

(In reply to Erik Vold [:erikvold] [:ztatic] from comment #7)
> Comment on attachment 750243 [details]
> Pointer to Github pull request: https://github.com/mozilla/addon-sdk/pull/997
> 
> I'm reusing the built-in sidebar here, we could implement our own though.

Here is an example:

require('sdk/ui/sidebar').SideBar({
  id: 'test-example-sidebar-thing',
  label: 'TEST',
  url: require('self').data.url('index.html')
}).show();

Comment 9

[Dave Townsend [:mossop]]

Comment on attachment 750243 [details]
Pointer to Github pull request: https://github.com/mozilla/addon-sdk/pull/997

Along the right lines but I think we've had discussions about whether or not to share open/closed state across windows. I think the result was that we don't share state but a newly opened window will take the state of the last window, ZER0 can confirm that. This code seems to match that except it doesn't look like it supports newly opened windows at all. We'll probably want to add to the API to be able to test whether the sidebar is open or closed for a given window.

Comment 10

[Erik Vold [:erikvold] (please needinfo? me)]

I've upgraded the dog food, it now tastes better.

There are still some weird bugs tho, like causing crashes if I use window.toggleSidebar() to close the sidebar, and the show event isn't emitted for a test when all of the tests are run together, but when I run them separately they work..

I'll have to look in to it all more tomorrow morning.

The branch is darn close I think, it took many hacks..

Comment 11

[Erik Vold [:erikvold] (please needinfo? me)]

Just to be clear I don't think the 3 current bugs that this bug depends on are blockers, I have hacked around them so far and the hacks seem to work.

Comment 12

[Erik Vold [:erikvold] (please needinfo? me)]

Attachment: Pointer to Github pull request: https://github.com/mozilla/addon-sdk/pull/1027

Pointer to Github pull-request

Comment 13

[Erik Vold [:erikvold] (please needinfo? me)]

Comment on attachment 761186 [details]
Pointer to Github pull request: https://github.com/mozilla/addon-sdk/pull/1027

Alright this is at least 80% of the way to done, I will add some tests, but the sidebar component is mostly done and ready for a review I think.

Comment 14

[Erik Vold [:erikvold] (please needinfo? me)]

Alright, I've written all of the tests that I wanted to do, so I just need to merge this branch with the button branch now.

Comment 15

[Erik Vold [:erikvold] (please needinfo? me)]

Note bug 885638 another toggleSidebar bug

Comment 16

[Erik Vold [:erikvold] (please needinfo? me)]

Ok, here is the crash causing code example, try this in Scratchpad:

let sidebar = window.document.getElementById('sidebar');
sidebar.addEventListener('load', function() {
  let panel = sidebar.contentDocument.getElementById('web-panels-browser');
  panel.addEventListener('DOMWindowCreated', function() {
    window.toggleSidebar();
  })
}, true)
window.openWebPanel('test', 'https://mozilla.org');

And BOOM! Fx crashes.

Comment 17

[Matteo Ferretti [:zer0] [:matteo]]

Not sure if it's related, but I had a similar issue in the past – maybe you remember the "Crash Me" add-on I made during a weekly meeting.

So the issue was that the document inside was still loading, and close the container immediately made Firefox unhappy. So I basically waited for the document to be "interactive". Following your code, it should be:

let sidebar = window.document.getElementById('sidebar');
sidebar.addEventListener('load', function() {
  let panel = sidebar.contentDocument.getElementById('web-panels-browser');
  panel.addEventListener('DOMWindowCreated', function(event) {
    let document = event.explicitOriginalTarget;
    
    document.addEventListener('readystatechange', function ready() {
      if (this.readyState === 'interactive') {
        document.removeEventListener('readystatechange', ready);
        window.toggleSidebar();
      }
    })
  })
}, true)
window.openWebPanel('test', 'https://mozilla.org');

I used this workaround until the bug was fixed, not sure if it makes sense in your context but at least it's an additional clue that can be used to fix the bug on platform side.

Comment 18

[Erik Vold [:erikvold] (please needinfo? me)]

Note: It appears that sidebars are not leaking, so the crash causing code in window.toggleSidebar (see bug 885949) isn't necessary to avoid leaks, at least in our use of web panel sidebars.

Comment 19

[Erik Vold [:erikvold] (please needinfo? me)]

(In reply to Matteo Ferretti [:matteo] [:zer0] from comment #17)
> Not sure if it's related, but I had a similar issue in the past – maybe you
> remember the "Crash Me" add-on I made during a weekly meeting.
> 
> So the issue was that the document inside was still loading, and close the
> container immediately made Firefox unhappy. So I basically waited for the
> document to be "interactive". Following your code, it should be:
> 
> let sidebar = window.document.getElementById('sidebar');
> sidebar.addEventListener('load', function() {
>   let panel = sidebar.contentDocument.getElementById('web-panels-browser');
>   panel.addEventListener('DOMWindowCreated', function(event) {
>     let document = event.explicitOriginalTarget;
>     
>     document.addEventListener('readystatechange', function ready() {
>       if (this.readyState === 'interactive') {
>         document.removeEventListener('readystatechange', ready);
>         window.toggleSidebar();
>       }
>     })
>   })
> }, true)
> window.openWebPanel('test', 'https://mozilla.org');
> 
> I used this workaround until the bug was fixed, not sure if it makes sense
> in your context but at least it's an additional clue that can be used to fix
> the bug on platform side.

I think this could introduce close lag and race conditions issues that are avoidable.

Comment 20

[Erik Vold [:erikvold] (please needinfo? me)]

Attachment: Pointer to Github pull request: https://github.com/mozilla/addon-sdk/pull/1048

Pointer to Github pull-request

Comment 21

[Erik Vold [:erikvold] (please needinfo? me)]

Attachment: Pointer to Github pull request: https://github.com/mozilla/addon-sdk/pull/1048

Pointer to Github pull-request

Comment 22

[[github robot]]

Commit pushed to australis at https://github.com/mozilla/addon-sdk

https://github.com/mozilla/addon-sdk/commit/86f3d5419ac8c31551de345a1f9d4072f4c4ce0d
Bug 787395 Adding a Sidebar module r=@ZER0

Conflicts:
	test/test-url.js

Comment 23

[[github robot]]

Commit pushed to australis at https://github.com/mozilla/addon-sdk

https://github.com/mozilla/addon-sdk/commit/613c6496a035b1c1846c8db36936f32673f879bd
Bug 787395 Adding a Sidebar module r=@ZER0

Conflicts:
	test/test-url.js

Comment 24

[Dave Townsend [:mossop]]

Is this fixed now?

Comment 25

[[github robot]]

Commit pushed to master at https://github.com/mozilla/addon-sdk

https://github.com/mozilla/addon-sdk/commit/613c6496a035b1c1846c8db36936f32673f879bd
Bug 787395 Adding a Sidebar module r=@ZER0

Comment 26

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

I'm going through the list of open bugs that github robot has commented in. Is this bug fixed, Matteo?

Comment 27

[Matteo Ferretti [:zer0] [:matteo]]

I would say yes.

Comment 28

[Kohei Yoshino [:kohei]]

Wow, I didn't know this API. My sample restartless extension https://github.com/kyoshino/simple-sidebar is no longer needed!

Updated the doc: https://developer.mozilla.org/en-US/docs/Creating_a_Firefox_sidebar