948582 - Big icons in the Button API breaks UI

Status: RESOLVED FIXED

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

Description

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

I used icon from https://assets.mozillalabs.com/Brands-Logos/Firefox/logo-only/firefox_logo-only_RGB.png which completely destroyed firefox UI.

Comment 1

[Irakli Gozalishvili [:irakli] [:gozala] [@gozala]]

I think it's really critical issue, image size should not be able to take over the firefox UI.

Comment 2

[Dave Townsend [:mossop]]

Matteo, looks like this just needs you to set size limits on the toolbarbutton we create here?

Comment 3

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

So, this is basically a side effect of bug 882910, that I reported.
The fixes, is just set a size limit to the `xul:image` – as the bug above mention, is not doable doing that on the button itself. The problem is, we're using `CustomizableUI` APIs for that, and when we create the button node, the `xul:image` node doesn't exist yet, because is an anonymous content.

Notice that if you create a XUL button and you attach such big icon in regular extension development (no SDK) you will face the same problem. It's not a Button / SDK strictly related issue.
What the devs doing in such cases, is set the size limit using CSS, something that currently we cannot do in SDK, because is JS-only.
I suggested in bug 882910 to add such stylesheet's rule for button as default, but my suggestion was rejected.
In order to fix it, we have two alternatives: one, is doing some runtime-js-thing-after-node-creation, that I find a bit hacky, but it's doable. The other, is create a patch for Firefox that set a max height for navbar's button, menu panel's button, and palette at least.

The first thing can be done, but I personally believe this is something should be fixed on platform (CSS) side, even if it was rejected. As I said, regular extension will suffer the same issue, and I believe that we should provide the "right" default behavior.

Comment 4

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

My apologies, I already mentioned all those things above to Irakli in IRC but I forgot to update the bug.

Comment 5

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

As noted here: https://bugzilla.mozilla.org/show_bug.cgi?id=951317#c5 there is a regression on HiDPI display, that I think can be fixed with the same workaround I was thinking here. I will use this bug to track the progress; I will implement the workaround there and trying to push the fix on the platform side.

Comment 6

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

I opened a more explicit bug about HiDPI, bug 961613, because this bug is about just setting one big icon, and we have this issue currently also in XUL.

Unfortunately the workaround I mentioned doesn't work, see: https://bugzilla.mozilla.org/show_bug.cgi?id=961613#c1

Comment 7

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

The fix of bug 882910 fixes this bug too.

Comment 8

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

The new fix made for bug 882910 doesn't fix anymore this bug, so we have a regression here.

It means also that the capability to just set an icon that would be automatically resized to fit the available space, won't be supported as intended. Previously the devs was able to set just one larger icon - e.g. 64px or 32px:



 now that will break the UI.

Comment 9

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

Not sure why the code wasn't display in the previous comment, however it was:

  ActionButton({
    id:'my-button',
    label: 'My Button',
    icon: 'icon.png'
  });

So this is not possible anymore with larger icon. If we still want to have this feature, then we need to find a different way to solve bug 882910 – that why initially `max-height` was used instead of `height`, however that broke the toolbar on Windows, so another approach should be used.

Even if it would be nice to have this feature, please considering that maybe the amount of time we're spending on it is not worthy, at this point. And having just HiDPI fixed could be enough.
I hoped that the fix on bug 882910 took in consideration, as I did, this bug too, but probably was to complex keep this feature and fix the Windows issue at the same time, I don't know.

Comment 10

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

I'm closing it again 'cause it seems working as expected: probably my master repo wasn't clean after the changes I was made with the widget's default icon.