725990 - Add capability to link from notification

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, enhancement)

Description

[Lawrence Mandel [:lmandel] (use needinfo)]

Notifications (door hangars) on mobile should provide the capability to include links in the text. An specific requirements is for the implementation of bug 725987 we would like to have a "Learn more" link in a notification.

Comment 1

[:Margaret Leibovic]

I think we'd want some UX input on what these links should look like.

Comment 2

[Madhava Enros [:madhava]]

We should decorate these like we do links in content and on the about: pages, so that they're recognizable as tappable thigns.

Have a look at the styling in the Sync setup screens, too -- they've got a blue link on black background.

Comment 3

[Brian Nicholson (:bnicholson)]

Attachment: patch

Adds link support to doorhangers. Links are specified as part of the doorhanger options, e.g.,

let options = {
  link: {
    label: "Learn more",
    url: "http://..."
  }
};
NativeWindow.doorhanger.show(msg, value, buttons, tab, options);

Comment 4

[:Margaret Leibovic]

Comment on attachment 603142 [details] [diff] [review]
patch

>diff --git a/mobile/android/base/resources/layout/doorhanger.xml b/mobile/android/base/resources/layout/doorhanger.xml

>     <TextView android:id="@+id/doorhanger_title"
>               android:layout_width="fill_parent"
>               android:layout_height="wrap_content"
>               android:textAppearance="?android:attr/textAppearanceMedium"
>               android:textColor="?android:attr/textColorPrimary"
>+              android:textColorLink="#ACC4D5"

Is hard-coding a color like this okay? We're using a system color for textColor, so shouldn't we also be using some system color for the link?

Comment 5

[Brian Nicholson (:bnicholson)]

(In reply to Margaret Leibovic [:margaret] from comment #4)
>
> Is hard-coding a color like this okay? We're using a system color for
> textColor, so shouldn't we also be using some system color for the link?

TBH, I don't know what our current conventions are, if we have any, for using colors. We use system colors in some layouts, but we use hard-coded custom values in  others. If we use system colors, which ones do we use? How do we know they'll look good on all devices?

For custom values, we should probably at least factor them out into styles like those resources/values/styles.xml and resources/values/sync_styles.xml to clean things up a bit (note that these files also contain hard-coded colors). But it might be better to reserve that for a separate patch.

Comment 6

[:Margaret Leibovic]

(In reply to Brian Nicholson (:bnicholson) from comment #5)
> TBH, I don't know what our current conventions are, if we have any, for
> using colors. We use system colors in some layouts, but we use hard-coded
> custom values in  others. If we use system colors, which ones do we use? How
> do we know they'll look good on all devices?

We should talk to UX folks and formalize these conventions (or lack thereof) for future reference. I'm not certain our existing color rules were all made intentionally.

> For custom values, we should probably at least factor them out into styles
> like those resources/values/styles.xml and resources/values/sync_styles.xml
> to clean things up a bit (note that these files also contain hard-coded
> colors). But it might be better to reserve that for a separate patch.

Yeah, let's file a follow-up for that.

Comment 7

[:Margaret Leibovic]

Comment on attachment 603142 [details] [diff] [review]
patch

>diff --git a/mobile/android/base/DoorHanger.java b/mobile/android/base/DoorHanger.java

>+            SpannableString titleWithLink = new SpannableString(title + " " + linkLabel);

This doesn't look RTL-friendly, but I'm not sure what the expectation is for a link coming before or after the title text in RTL mode (also, more generally, I'm not sure how careful we've been to properly support RTL locales in other parts of the Java UI). Maybe file a follow-up to look into whether or not this is a problem?

Comment 8

[Brian Nicholson (:bnicholson)]

http://hg.mozilla.org/integration/mozilla-inbound/rev/55051c4e95c7

Comment 9

[Brian Nicholson (:bnicholson)]

Filed bug 733588 for style cleanup.

I don't know which system color to use for the links - if we should even use system colors here - so I kept the hard-coded color for now (though I did move it to colors.xml). If necessary, we can come back and change this to an Android system color in bug 733588.

As discussed with gavin in IRC, we've been appending the link onto the right side of the doorhanger label for awhile now in desktop. There haven't been any problems, so we should be okay here.

Comment 10

[Marco Bonardo [:mak] (Away Apr 25 - May 5)]

https://hg.mozilla.org/mozilla-central/rev/55051c4e95c7

Comment 11

[Mark Finkle (:mfinkle) (use needinfo?)]

In About Home, we are using #22629e for the link color in 3 places:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/resources/layout/abouthome_content.xml#69

Comment 12

[:Margaret Leibovic]

(In reply to Mark Finkle (:mfinkle) from comment #11)
> In About Home, we are using #22629e for the link color in 3 places:
> http://mxr.mozilla.org/mozilla-central/source/mobile/android/base/resources/
> layout/abouthome_content.xml#69

We should address that (and all other hard-coded colors) in bug 733588.