Closed Bug 1390735 Opened 7 years ago Closed 7 years ago

(photon) Support tint color for PageAction icon

Categories

(Firefox for Android Graveyard :: General, enhancement)

Unspecified
Android
enhancement
Not set
normal

Tracking

(firefox57 verified)

VERIFIED FIXED
Firefox 57
Tracking Status
firefox57 --- verified

People

(Reporter: jwu, Assigned: jwu)

References

Details

(Whiteboard: [FNC][SPT57.3][MVP])

Attachments

(2 files)

Attached image page_action_icon.png
On Photon, page action icons might need two tint colors for both normal/private mode. The attachment provides an example that the droid icon on toolbar isn't clear when in private mode.

If "PageActions:Add" message sent from Gecko can tell us if the page icon needs tint color or not, the Java side can provide proper tint color when switching between normal/private mode.
Looking at the screenshot: Is this hard cut off of the URL expected? We used to have a fading edge there.
The hard cut off is a known issue because the fading edge supported in FadedMultiColorTextView.java cannot meet Photon's requirement. We use bug 1379552 to record this issue. 

But before we start investigating it, the new design for lightweight theme(bug 1389164) needs to be finalized first because it might change the background of text in URL.
Comment on attachment 8897801 [details]
Bug 1390735 - Support tint color for page action button.

https://reviewboard.mozilla.org/r/169098/#review174806

::: mobile/android/chrome/content/CastingApps.js:562
(Diff revision 2)
>        this.pageAction.id = PageActions.add({
>          title: Strings.browser.GetStringFromName("contextmenu.sendToDevice"),
>          icon: "drawable://casting_active",
>          clickCallback: this.pageAction.click,
> -        important: true
> +        important: true,
> +        useTint: false

Could we just choose a major color in Java end and do the tint for the image?
Attachment #8897801 - Flags: review?(cnevinchen)
Comment on attachment 8897801 [details]
Bug 1390735 - Support tint color for page action button.

https://reviewboard.mozilla.org/r/169098/#review174806

> Could we just choose a major color in Java end and do the tint for the image?

The idea here is that: 

1. The image is provided by JS, and only JS know which image should be tinted or not.
2. When Java receives a "PageActions:Add" message from Gecko, it displays the image by the file name defined in the message.
3. Since Java only has file name, it doesn't know which image should be tinted or not, therefore, JS needs provide another parameter `useTint` in the message. If `useTint` is enabled, the image would be tinted. 
4. The tint color is defined in Java side that would make us easy to change the colors in the future if necessary.
Attachment #8897801 - Flags: review?(cnevinchen)
Comment on attachment 8897801 [details]
Bug 1390735 - Support tint color for page action button.

https://reviewboard.mozilla.org/r/169098/#review176648

::: mobile/android/base/java/org/mozilla/gecko/toolbar/PageActionLayout.java:152
(Diff revision 2)
>          }
>          mPageActionList.add(insertAt, pageAction);
>  
>          ResourceDrawableUtils.getDrawable(mContext, imageData, new ResourceDrawableUtils.BitmapLoader() {
>              @Override
> -            public void onBitmapFound(final Drawable d) {
> +            public void onBitmapFound(Drawable d) {

I prefer "Never Reassign Arguments". I think it's better to creat  another local variable instead of re-assign the parameter
Attachment #8897801 - Flags: review?(cnevinchen) → review+
Comment on attachment 8897801 [details]
Bug 1390735 - Support tint color for page action button.

https://reviewboard.mozilla.org/r/169098/#review177176
Attachment #8897801 - Flags: review?(walkingice0204) → review+
Comment on attachment 8897801 [details]
Bug 1390735 - Support tint color for page action button.

https://reviewboard.mozilla.org/r/169098/#review178636

Isn't this used by webextensions too? How should a webextensions know if it needs to tint? Why can't we always tint? Anyhow - let's file a bug for the webextension case if needed.
Attachment #8897801 - Flags: review?(s.kaspari) → review+
Comment on attachment 8897801 [details]
Bug 1390735 - Support tint color for page action button.

https://reviewboard.mozilla.org/r/169098/#review178636

1. I'm not sure webextension behavior, but if it also communicates with frontend through "PageActions:Add", then it should work fine as well.
2. Because of the design, currently only Javascript knows if it needs to tint or not. We should consider refactoring the interface to let Java take the charge to decide how to tint icons.
3. There are two kinds of icon we don't tint, the first is that it already has primary color, like reader mode icon(http://imgur.com/a/iTub6); the second is the 3-party addon, directly tint 3-party icon doesn't seem like a good idea.

We have expected we'll receive some feedbacks and we might fire some followup bugs if needed.
Pushed by topwu.tw@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/dc2d8596c9c5
Support tint color for page action button. r=nechen,sebastian,walkingice
Whiteboard: [FNC][SPT57.3][MVP]
https://hg.mozilla.org/mozilla-central/rev/dc2d8596c9c5
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 57
Verified as fixed on Nightly 57 (2017-08-30).
Devices:
Samsung Galaxy Note 4 (Android 5.0.1)
Oneplus Two (Android 6.0.1)
HTC Nexus 9 (Android 7.1.1)
Google Pixel (Android 7.1.2)
Status: RESOLVED → VERIFIED
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.