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)
Tracking
(Not tracked)
RESOLVED
FIXED
People
(Reporter: irakli, Assigned: zer0)
References
Details
I used icon from https://assets.mozillalabs.com/Brands-Logos/Firefox/logo-only/firefox_logo-only_RGB.png which completely destroyed firefox UI.
Reporter | ||
Comment 1•11 years ago
|
||
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
Comment 2•11 years ago
|
||
Matteo, looks like this just needs you to set size limits on the toolbarbutton we create here?
Assignee: nobody → zer0
Flags: needinfo?(dtownsend+bugmail)
Assignee | ||
Comment 3•11 years ago
|
||
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)
Assignee | ||
Comment 4•11 years ago
|
||
My apologies, I already mentioned all those things above to Irakli in IRC but I forgot to update the bug.
Assignee | ||
Comment 5•11 years ago
|
||
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.
Assignee | ||
Comment 6•11 years ago
|
||
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
Assignee | ||
Comment 7•11 years ago
|
||
The fix of bug 882910 fixes this bug too.
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Assignee | ||
Comment 8•11 years ago
|
||
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 → ---
Assignee | ||
Comment 9•11 years ago
|
||
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.
Assignee | ||
Comment 10•11 years ago
|
||
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 ago → 11 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•