1397174 - Fix scale issue for icons like AdaptiveIconDrawable in Android O

Status: VERIFIED FIXED

(Firefox for Android Graveyard :: General, defect, P3)

Description

[Nevin Chen(Not active on Bugzilla)]

Follow up for bug 1372040

Comment 4

[Michael Comella (:mcomella) [NI reported issues only: ex-Mozilla]]

[triage] Awkward and embarrassing, but non-critical.

Comment 6

[Jan Henning [:JanH]]

Maybe this is something that should be looked at again after the API 26 migration has been finished?

Comment 8

[:sdaswani]

Vlad would https://bugzilla.mozilla.org/show_bug.cgi?id=1349522 solve this issue too?

Comment 9

[Jan Henning [:JanH]]

No, bug 1349522 is about providing an adaptive icon ourselves.

This bug is about the fact that if other apps use adaptive icons, we don't resize those icons correctly when displaying them within Firefox.

Comment 10

[Asif Youssuff]

Also being reported in the Firefox subreddit: https://www.reddit.com/r/firefox/comments/96eoao/firefox_you_feeling_okay/

Comment 11

[Jan Henning [:JanH]]

It's getting close to November and once Google's target API requirements are in force, even more other apps will presumably use adaptive icons.

Comment 12

[:sdaswani]

Bram, can you provide new Adaptive icons?

Comment 13

[Jan Henning [:JanH]]

This isn't about our own icons, this is about fixing our share menu so it doesn't look like https://i.imgur.com/lKlSOCL.jpg or https://i.redd.it/rsh3xrjia6011.jpg because *other* apps are using adaptive icons and we don't handle it properly. I'd do this myself, but I don't have a phone with Oreo and doing things blindly without being able to properly test was already painful enough for the updater notifications, so I won't attempt anything here.

Comment 14

[:sdaswani]

Sorry Jan, my bad. I will have the Softvision devs take a look.

Comment 15

[Bram Pitoyo [:bram]]

Attachment: [e-mail]

Comment 16

[Bram Pitoyo [:bram]]

Attachment: [e-mail]

Comment 17

[Bram Pitoyo [:bram]]

Hi Jan, one of the easiest way to ensure adaptive icon compatibility (for now, and in the future) is by invoking the system native share dialogue: attachment 9010460 [details].

The other way to solve this problem is by resizing our icons from 72dp down to 36dp: attachment 9010461 [details].

What do you think?

Comment 18

[Jan Henning [:JanH]]

I think one reason for using our own share menu is that we want to order share targets by usage frequency - compare also how the top two share targets are also accessible directly from the top-level menu. There also might be other customisations included that wouldn't be possible with the system share dialogue, so I think we should just fix our own menu to explicitly resize icons to our target size.

This size isn't always 36dp by the way - when invoked from the main menu, we use both smaller font and icon size than when sharing from a context menu. I didn't look into the reason for this difference, though.

Comment 19

[Bram Pitoyo [:bram]]

(In reply to Jan Henning [:JanH] from comment #18)
> […] I think we should just fix our own menu to
> explicitly resize icons to our target size.

Sounds like a plan. I figure that there were very good reasons why we use our own implementation.


> This size isn't always 36dp by the way - when invoked from the main menu, we
> use both smaller font and icon size than when sharing from a context menu. I
> didn't look into the reason for this difference, though.

Yeah. I saw that. When sharing from the main menu, the icon size is 24dp. In fact, all the icons there are consistent.

So, we shouldn’t change the main menu at all. The only icon size that we need make consistent are the ones inside the long-press context menu.

Does this give you enough information to fix this bug?

Comment 20

[Jan Henning [:JanH]]

Possibly, but I'd still prefer someone who can more easily test this to do it.

Comment 22

[Vlad Baicu]

Attachment: Bug 1397174 - Fix scale issue for AdaptiveIconDrawables. r=sdaswani

Added a check for AdaptiveIconDrawables and a way to resize them to our specific size.

Comment 23

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/57994656668d
Fix scale issue for AdaptiveIconDrawables. r=JanH

Comment 24

[Andrei Ciure[:aciure]]

https://hg.mozilla.org/mozilla-central/rev/57994656668d

Comment 26

[Mira Lobontiu (Android QA)]

Verified as fixed with Google Pixel (Android 9.0), on Beta 64.0b11.