Closed Bug 961613 Opened 6 years ago Closed 6 years ago

Button's icons are displayed incorrectly on HiDPI display

Categories

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

x86
macOS
defect

Tracking

(firefox29 fixed, firefox30 fixed)

RESOLVED FIXED
mozilla30
Tracking Status
firefox29 --- fixed
firefox30 --- fixed

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.
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
QA Contact: zer0
Assignee: nobody → zer0
QA Contact: zer0
Priority: -- → P1
The fix of bug 882910 fixes this bug too.
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Depends on: 967115
Attached image regression
Not sure about Non-HiDPI, but on HiDPI it has been regressed…
(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)
It could be because the fix of bug 967115. Gijs?
Status: RESOLVED → REOPENED
Flags: needinfo?(zer0) → needinfo?(gijskruitbosch+bugs)
Resolution: FIXED → ---
(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)
(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)
Attached file 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).
Flags: needinfo?(cadeyrn)
(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.
Works on Aurora for me, too.
Assignee: zer0 → nobody
Whiteboard: [Australis:P3]
Flags: needinfo?(gijskruitbosch+bugs)
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
Depends on: 968595, 970380
Flags: needinfo?(gijskruitbosch+bugs)
... 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?
Flags: needinfo?(shorlander)
Flags: needinfo?(mdeboer)
Whiteboard: [Australis:P3] → [Australis:P3][fixed-in-fx-team]
Status: REOPENED → RESOLVED
Closed: 6 years ago6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla30
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)
(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)
(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)
Flags: needinfo?(mdeboer)
You need to log in before you can comment on or make changes to this bug.