1450218 - Replace extensionsOverlay.xul with hooking into the about:addons page directly

Status: RESOLVED FIXED

(Thunderbird :: Toolbars and Tabs, enhancement)

Description

[Jorg K (CEST = GMT+2)]

We need this to add the forward/back arrows and add CSS.

Aceman, a job for you?

Comment 1

[:aceman]

Sadly this is a tab and not a window, so we can't easily use bug 1450288 to inject the code.
Maybe what is brewing in bug 1448808?

Comment 2

[:aceman]

I have some ideas how to do this.

Comment 3

[:aceman]

Attachment: 1450218.patch WIP

This is a proof of concept, rewriting the XUL overlay into JS.
Can you see if the design is the same and also the css file is loaded properly (including the extensionOverlay.css in themes/shared)?

Comment 4

[:aceman]

Try run at https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=0a0e03101c86475c1ef6f04c8e2643ecee61de50 . But of course this needs manual inspection if the arrows behave right. I could only confirm that the "Legacy" label is hidden so at least the mail/base/content/extensionOverlay.css is applied.

Comment 5

[Richard Marti (:Paenglab)]

Comment on attachment 8971436 [details] [diff] [review]
1450218.patch WIP

Thanks. Here it looks like the buttons are animated growing, not a bad effect. How would this look on a slow machine? No difference else visible to the overlay version.

Comment 6

[:aceman]

Yes, that is what I saw too (on a fast machine), there are just small squares at first and then turn into the full arrows. Even though I load the stylesheet first.

Comment 7

[:aceman]

Comment on attachment 8971436 [details] [diff] [review]
1450218.patch WIP

Review of attachment 8971436 [details] [diff] [review]:
-----------------------------------------------------------------

So unless we can find a way to hide that "animation" (elements appearing later), we can continue with the patch.
I think we will encounter this often now, because overlays could apply the elements instantly while the document was built. All our replacements first wait until the document is done and then inject elements dynamically. Of course, that may be visible to the user for a moment.

Comment 8

[Jorg K (CEST = GMT+2)]

Comment on attachment 8971436 [details] [diff] [review]
1450218.patch WIP

One needs to delete messenger.manifest and/or the old XUL file, otherwise there are two sets of arrows ;-(

The animation is a little annoying. Could we try setting the left side to hidden before we insert the arrows and then display it?

Comment 9

[:aceman]

Maybe, but that flashing (hiding/unhiding) could be visible too. I don't think we can keep the pane hidden before it is first exposed.
Let's fight a kludge with a kludge ;)

Comment 10

[Jorg K (CEST = GMT+2)]

Attachment: 1450218.patch

I have the impressions that this avoids the unwanted "animation". Can you confirm?

Comment 11

[:aceman]

Comment on attachment 8972179 [details] [diff] [review]
1450218.patch

Review of attachment 8972179 [details] [diff] [review]:
-----------------------------------------------------------------

Sorry, I do not think I see any difference.

Comment 12

[Jorg K (CEST = GMT+2)]

Hmm, I think the animation is a little less annoying on Windows. Richard, how do your opt builds on the various platforms look?

Comment 13

[Richard Marti (:Paenglab)]

Comment on attachment 8972179 [details] [diff] [review]
1450218.patch

I see no real difference to aceman's patch.

Comment 14

[Richard Marti (:Paenglab)]

Attachment: 1450218-with CSS.patch

This patch is based on aceman's patch with a CSS rule added. It defines a height for the #nav-header. When the #nav-header is loaded it gets directly the height and shouldn't grow slower with the appearance of the arrows.

It still isn't without movement but the animation isn't so visible as before. The height can only apply when the #nav-header is loaded through JS. I tried also to position the categories absolutely but this didn't work.

Comment 15

[:aceman]

Comment on attachment 8972427 [details] [diff] [review]
1450218-with CSS.patch

Review of attachment 8972427 [details] [diff] [review]:
-----------------------------------------------------------------

This is slightly better, mostly when opening the addon tab for the second time (when the arrow images are already preloaded).

Comment 16

[Jorg K (CEST = GMT+2)]

Comment on attachment 8972427 [details] [diff] [review]
1450218-with CSS.patch

I don't see much difference, but let's go with this. It's Aceman's patch plus:
#nav-header {
  height: 50px;
}

Comment 17

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/f767643c49b4
replace entensionsOverlay.xul for about:addons with injecting elements via JS. ui-r=Paenglab, r=jorgk