Closed Bug 948582 Opened 11 years ago Closed 11 years ago

Big icons in the Button API breaks UI

Categories

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

x86
macOS
defect

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: irakli, Assigned: zer0)

References

Details

I think it's really critical issue, image size should not be able to take over the firefox UI.
Flags: needinfo?(zer0)
Flags: needinfo?(dtownsend+bugmail)
Priority: -- → P1
Matteo, looks like this just needs you to set size limits on the toolbarbutton we create here?
Assignee: nobody → zer0
Flags: needinfo?(dtownsend+bugmail)
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.
Flags: needinfo?(zer0)
My apologies, I already mentioned all those things above to Irakli in IRC but I forgot to update the bug.
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.
Depends on: 882910
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
The fix of bug 882910 fixes this bug too.
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
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.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
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.
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.
Status: REOPENED → RESOLVED
Closed: 11 years ago11 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.