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)
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)
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.
Comment hidden (mozreview-request) |
Comment 2•7 years ago
|
||
Looking at the screenshot: Is this hard cut off of the URL expected? We used to have a fading edge there.
Assignee | ||
Comment 3•7 years ago
|
||
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 hidden (mozreview-request) |
Comment 5•7 years ago
|
||
mozreview-review |
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)
Assignee | ||
Comment 6•7 years ago
|
||
mozreview-review-reply |
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.
Assignee | ||
Updated•7 years ago
|
Attachment #8897801 -
Flags: review?(cnevinchen)
Comment 7•7 years ago
|
||
mozreview-review |
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 8•7 years ago
|
||
mozreview-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 hidden (mozreview-request) |
Comment 10•7 years ago
|
||
mozreview-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+
Assignee | ||
Comment 11•7 years ago
|
||
mozreview-review-reply |
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.
Comment 12•7 years ago
|
||
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
Updated•7 years ago
|
Whiteboard: [FNC][SPT57.3][MVP]
Comment 13•7 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/dc2d8596c9c5
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 57
Comment 14•7 years ago
|
||
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
Updated•3 years ago
|
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•