Closed
Bug 961613
Opened 11 years ago
Closed 11 years ago
Button's icons are displayed incorrectly on HiDPI display
Categories
(Add-on SDK Graveyard :: General, defect, P1)
Tracking
(firefox29 fixed, firefox30 fixed)
RESOLVED
FIXED
mozilla30
People
(Reporter: zer0, Assigned: Gijs)
References
(Blocks 1 open bug)
Details
(Keywords: regression, testcase, Whiteboard: [Australis:P3][fixed-in-fx-team])
Attachments
(2 files)
On HiDPI the buttons display incorrect size of the icon, see:
https://bugzilla.mozilla.org/show_bug.cgi?id=951317#c4
This is related to bug 948582 too.
Reporter | ||
Comment 1•11 years ago
|
||
Unfortunately the workaround I mentioned in bug 951317 and bug 948582 didn't work as expected, we need to fix this issue on platform side using bug 882910.
To be more explicit: the way to fix it is adding `max-height` to the `xul:image` node inside the `toolbarbutton`. Unfortunately is an anonymous node, so I can't add it simply by JavaScript during the node creation of the button in `CustomizableUI`, because is not attached yet to a document.
I tried then to modify the `max-height` every after `onCustimizeStart`, `onCostumizeEnd`, and `onWidgetAfterDOMChange`: but is not enough, because during customization the `toolbarbutton` is "wrapped" in another element, and therefore seems "detached" from the document and "attached" again; that makes the anonymous content to be recreated – it basically lost the style set previously – and `onCostumizeStart` seems begin before that. In short, HiDPI display, everything works perfectly until we enter in customization mode: then, the button double it size if we enter in customization mode when the button is in the toolbar.
Unless we want to add a Mutation Observer for that, it should be much easier if we could add on platform side a CSS that specify in toolbars the max-height of `xul:image` of 18px. If for any reason we do not want do that for any `toolbarbutton` by default, we could add a specific sdk class to it – however, the "big icons" will breaks UI in regular XUL development, if we just add a specific class, but at least it will fix the HiDPI problem, that I think is a major one.
Depends on: 882910
Reporter | ||
Updated•11 years ago
|
QA Contact: zer0
Reporter | ||
Updated•11 years ago
|
Assignee: nobody → zer0
QA Contact: zer0
Priority: -- → P1
Reporter | ||
Comment 2•11 years ago
|
||
The fix of bug 882910 fixes this bug too.
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Comment 3•11 years ago
|
||
Not sure about Non-HiDPI, but on HiDPI it has been regressed…
Comment 4•11 years ago
|
||
(In reply to Sören Hentzschel from comment #3)
> Created attachment 8373002 [details]
> regression
>
> Not sure about Non-HiDPI, but on HiDPI it has been regressed…
Shall I file a new bug? The icon sizes on HiDPI displays were fixed but are broken in the Australis menu since a few days.
Flags: needinfo?(zer0)
Reporter | ||
Comment 5•11 years ago
|
||
It could be because the fix of bug 967115. Gijs?
Status: RESOLVED → REOPENED
Flags: needinfo?(zer0) → needinfo?(gijskruitbosch+bugs)
Resolution: FIXED → ---
Assignee | ||
Comment 6•11 years ago
|
||
(In reply to Matteo Ferretti [:matteo] [:zer0] from comment #5)
> It could be because the fix of bug 967115. Gijs?
How would I test this?
Flags: needinfo?(gijskruitbosch+bugs)
Assignee | ||
Comment 7•11 years ago
|
||
(In reply to Sören Hentzschel from comment #4)
> (In reply to Sören Hentzschel from comment #3)
> > Created attachment 8373002 [details]
> > regression
> >
> > Not sure about Non-HiDPI, but on HiDPI it has been regressed…
>
> Shall I file a new bug? The icon sizes on HiDPI displays were fixed but are
> broken in the Australis menu since a few days.
This should have been fixed by bug 970380. Did it not, and if not, can you provide a testcase?
Flags: needinfo?(cadeyrn)
Comment 8•11 years ago
|
||
I attached a add-on. No problems in Aurora, only in Nightly (tested with a fresh profile on a MacBook with retina display).
Flags: needinfo?(cadeyrn)
Assignee | ||
Comment 9•11 years ago
|
||
(In reply to Sören Hentzschel from comment #8)
> Created attachment 8377873 [details]
> aboutconfigbutton.xpi
>
> I attached a add-on. No problems in Aurora, only in Nightly (tested with a
> fresh profile on a MacBook with retina display).
I can reproduce on Nightly. Checking Aurora now, but if both of them were up-to-date, then it isn't either of those bugs' fault. We need a regression window. And next time, please file a new bug.
Assignee | ||
Comment 10•11 years ago
|
||
Works on Aurora for me, too.
Assignee | ||
Updated•11 years ago
|
Flags: needinfo?(gijskruitbosch+bugs)
Assignee | ||
Comment 11•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=d275eebfae04&tochange=b80f7eece913
This was (unintentionally, I'm sure...) regressed by https://hg.mozilla.org/mozilla-central/rev/5d90aed97e13 .
Assignee: nobody → gijskruitbosch+bugs
Flags: needinfo?(gijskruitbosch+bugs)
Keywords: regressionwindow-wanted
Assignee | ||
Comment 12•11 years ago
|
||
... and because bug 968595 landed on Aurora yesterday, today's Aurora will also be broken. Joy.
Seeing as the patches for bug 970380 already got review and approval and the fix here is bit-for-bit identical, I've just gone and relanded them:
remote: https://hg.mozilla.org/integration/fx-team/rev/9878506e1bac
remote: https://hg.mozilla.org/releases/mozilla-aurora/rev/3e2349bd0b1e
Stephen and/or Mike, can you please check the patches from bug 968595 didn't accidentally regress anything else?
status-firefox29:
--- → fixed
status-firefox30:
--- → affected
Flags: needinfo?(shorlander)
Flags: needinfo?(mdeboer)
Whiteboard: [Australis:P3] → [Australis:P3][fixed-in-fx-team]
Assignee | ||
Updated•11 years ago
|
Status: REOPENED → RESOLVED
Closed: 11 years ago → 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla30
Assignee | ||
Comment 13•11 years ago
|
||
Forgot the revlink:
https://hg.mozilla.org/mozilla-central/rev/9878506e1bac
Comment 14•11 years ago
|
||
S(In reply to :Gijs Kruitbosch from comment #12)
> Stephen and/or Mike, can you please check the patches from bug 968595 didn't
> accidentally regress anything else?
Shouldn't have affected this, patches from bug 970380 didn't touch those rules.
Flags: needinfo?(shorlander)
Assignee | ||
Comment 15•11 years ago
|
||
(In reply to Stephen Horlander [:shorlander] from comment #14)
> S(In reply to :Gijs Kruitbosch from comment #12)
> > Stephen and/or Mike, can you please check the patches from bug 968595 didn't
> > accidentally regress anything else?
>
> Shouldn't have affected this, patches from bug 970380 didn't touch those
> rules.
No, the point is, can you go over the patches in bug 968595 as landed (see below) and double-check that all of the changes are actually intentional, apart from the one that was fixed in this bug? I suspect that someone hg up -f'd or something like this and so any changes that landed to the same files as that bug touches inbetween the initial creation of the patches and the landing will have been reverted.
https://hg.mozilla.org/mozilla-central/rev/a040e19c30c1
https://hg.mozilla.org/mozilla-central/rev/1cf01a7bd18b
https://hg.mozilla.org/mozilla-central/rev/4ebb361218a1
https://hg.mozilla.org/mozilla-central/rev/fa8c62aeff33
https://hg.mozilla.org/mozilla-central/rev/87364536ec02
https://hg.mozilla.org/mozilla-central/rev/5d90aed97e13
Flags: needinfo?(shorlander)
Comment 16•11 years ago
|
||
(In reply to :Gijs Kruitbosch from comment #15)
> (In reply to Stephen Horlander [:shorlander] from comment #14)
> > S(In reply to :Gijs Kruitbosch from comment #12)
> > > Stephen and/or Mike, can you please check the patches from bug 968595 didn't
> > > accidentally regress anything else?
> >
> > Shouldn't have affected this, patches from bug 970380 didn't touch those
> > rules.
>
> No, the point is, can you go over the patches in bug 968595 as landed (see
> below) and double-check that all of the changes are actually intentional,
> apart from the one that was fixed in this bug? I suspect that someone hg up
> -f'd or something like this and so any changes that landed to the same files
> as that bug touches inbetween the initial creation of the patches and the
> landing will have been reverted.
>
>
>
> https://hg.mozilla.org/mozilla-central/rev/a040e19c30c1
> https://hg.mozilla.org/mozilla-central/rev/1cf01a7bd18b
> https://hg.mozilla.org/mozilla-central/rev/4ebb361218a1
> https://hg.mozilla.org/mozilla-central/rev/fa8c62aeff33
> https://hg.mozilla.org/mozilla-central/rev/87364536ec02
> https://hg.mozilla.org/mozilla-central/rev/5d90aed97e13
Oh, sorry, got it. I will double check.
Flags: needinfo?(shorlander)
Updated•11 years ago
|
Flags: needinfo?(mdeboer)
Updated•11 years ago
|
You need to log in
before you can comment on or make changes to this bug.
Description
•