Closed Bug 1209293 Opened 9 years ago Closed 9 years ago

Add option for click-to-play images depending on network bearer

Categories

(Firefox for Android Graveyard :: General, defect)

Product:
Firefox for Android Graveyard
Component:
General
Version:
43 Branch
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(firefox45 verified)

Status:
VERIFIED FIXED
Milestone:
Firefox 45
Tracking Flags:
Tracking Status
firefox45 --- verified

People

(Reporter: jonalmeida, Assigned: jonalmeida)

References

Details

Attachments

(2 files)

MozReview Request: Bug 1209293 - Add option for click-to-play images depending on network bearer r=mfinkle
40 bytes, text/x-review-board-request
mfinkle
: review+
Details
Screenshot.png
110.39 KB, image/png
Details 
So that there would be a UI option list with:
 - Always block
 - Block when on cellular
 - Don't block at all
Blocks: 1170725
(In reply to Jonathan Almeida (:jonalmeida) from comment #1)
> Maybe start searching around here:
> https://developer.mozilla.org/en-US/docs/Web/API/MozMobileNetworkInfo

Wrong link, this is where I'll start looking: https://developer.mozilla.org/en-US/docs/Web/API/NetworkInformation
(In reply to Jonathan Almeida (:jonalmeida) from comment #3)
> (In reply to Jonathan Almeida (:jonalmeida) from comment #1)
> > Maybe start searching around here:
> > https://developer.mozilla.org/en-US/docs/Web/API/MozMobileNetworkInfo
> 
> Wrong link, this is where I'll start looking:
> https://developer.mozilla.org/en-US/docs/Web/API/NetworkInformation

I'm not 100% sure that API is really available or not. Even if it is, you need a DOM window.

It might be easier to use the XPCOM interface [1] used for a similar feature in the SSDP code [2] to determine if the current network connection is Wifi or not.

[1] http://mxr.mozilla.org/mozilla-central/source/netwerk/base/nsINetworkLinkService.idl#47
[2] http://mxr.mozilla.org/mozilla-central/source/toolkit/modules/secondscreen/SimpleServiceDiscovery.jsm#120
(In reply to Mark Finkle (:mfinkle) from comment #4)
> (In reply to Jonathan Almeida (:jonalmeida) from comment #3)
> > (In reply to Jonathan Almeida (:jonalmeida) from comment #1)
> > > Maybe start searching around here:
> > > https://developer.mozilla.org/en-US/docs/Web/API/MozMobileNetworkInfo
> > 
> > Wrong link, this is where I'll start looking:
> > https://developer.mozilla.org/en-US/docs/Web/API/NetworkInformation
> 
> I'm not 100% sure that API is really available or not. Even if it is, you
> need a DOM window.
> 
> It might be easier to use the XPCOM interface [1] used for a similar feature
> in the SSDP code [2] to determine if the current network connection is Wifi
> or not.
> 
> [1]
> http://mxr.mozilla.org/mozilla-central/source/netwerk/base/
> nsINetworkLinkService.idl#47
> [2]
> http://mxr.mozilla.org/mozilla-central/source/toolkit/modules/secondscreen/
> SimpleServiceDiscovery.jsm#120

Thanks! Using the XPCOM interface definitely much easier.
Bug 1209293 - Add option for click-to-play images depending on network bearer r?mfinkle

- Replaced bool pref for list option: Disabled, Enabled, or Only over cellular
- Pref browser.image_blocking.enabled -> browser.image_blocking
- Converted the early return to check for cellular as well.
- Note that in the cellular check, I do not consider LINK_TYPE_USB as "cellular".
- Tested this on a physical device with all combinations
    of WiFi ON/OFF (falling back to cellular and each menu state.
- Tested if menu options work as well for each state mentioned above.
Attachment #8680545 - Flags: review?(mark.finkle)
https://reviewboard.mozilla.org/r/23639/#review21131

::: mobile/android/base/locales/en-US/android_strings.dtd:220
(Diff revision 1)
>  <!ENTITY pref_tap_to_load_images_summary2 "Load images only when you long press them">

I guess we won't be using the summary string anymore. Although it is super helpful since it may be hard to decipher what this feature from only the name.
Comment on attachment 8680545 [details]
MozReview Request: Bug 1209293 - Add option for click-to-play images depending on network bearer r=mfinkle

https://reviewboard.mozilla.org/r/23639/#review21311

::: mobile/android/app/mobile.js:354
(Diff revision 1)
> -pref("browser.image_blocking.enabled", false);
> +pref("browser.image_blocking", "disabled");

We prefer using integers for multi-state preferences. Look at line 350 for example.

::: mobile/android/base/locales/en-US/android_strings.dtd:219
(Diff revision 1)
>  <!ENTITY pref_tap_to_load_images_title "Tap-to-load images">

"Tap-to-load" is not correct. We should not use the string. This feature is about controlling bandwidth, blocking images. We need a better string.

::: mobile/android/base/locales/en-US/android_strings.dtd:223
(Diff revision 1)
> +<!ENTITY pref_tap_to_load_images_data "Only over cellular">

"Only over cellular" could probably be improved too.

We need UX and others to weigh in.

::: mobile/android/chrome/content/browser.js:2612
(Diff revision 1)
> -        if (!Services.prefs.getBoolPref("browser.image_blocking.enabled")) {
> +        let enabledPref = Services.prefs.getCharPref("browser.image_blocking");

Maybe we can drop the enabled check here, and remove the usingCellar function. We could just check for "data-ctv-src" and let ImageBlockingPolicy handle the enabling check.

I think.
Attachment #8680545 - Flags: review?(mark.finkle)
(In reply to Mark Finkle (:mfinkle) from comment #8)
> ::: mobile/android/base/locales/en-US/android_strings.dtd:219
> (Diff revision 1)
> >  <!ENTITY pref_tap_to_load_images_title "Tap-to-load images">
> 
> "Tap-to-load" is not correct. We should not use the string. This feature is
> about controlling bandwidth, blocking images. We need a better string.

I'm not sure of a good alternative.

> ::: mobile/android/base/locales/en-US/android_strings.dtd:223
> (Diff revision 1)
> > +<!ENTITY pref_tap_to_load_images_data "Only over cellular">
> 
> "Only over cellular" could probably be improved too.
> 
> We need UX and others to weigh in.

For the pref 'Download updates automatically', we use the string "Only over Wi-Fi" so I used that with "cellular" since we've used that to describe mobile networks in other strings.

Anthony, what do you think?
Flags: needinfo?(alam)
(In reply to Mark Finkle (:mfinkle) from comment #8)
> Comment on attachment 8680545 [details]
> MozReview Request: Bug 1209293 - Add option for click-to-play images
> depending on network bearer r?mfinkle
> 
> https://reviewboard.mozilla.org/r/23639/#review21311
> 
> ::: mobile/android/chrome/content/browser.js:2612
> (Diff revision 1)
> > -        if (!Services.prefs.getBoolPref("browser.image_blocking.enabled")) {
> > +        let enabledPref = Services.prefs.getCharPref("browser.image_blocking");
> 
> Maybe we can drop the enabled check here, and remove the usingCellar
> function. We could just check for "data-ctv-src" and let ImageBlockingPolicy
> handle the enabling check.
> 
> I think.

Yes, you're right, it works!

> 
> ::: mobile/android/app/mobile.js:354
> (Diff revision 1)
> > -pref("browser.image_blocking.enabled", false);
> > +pref("browser.image_blocking", "disabled");
> 
> We prefer using integers for multi-state preferences. Look at line 350 for
> example.
> 

Will make these changes once bug 1208167 lands - both of these patches tread over each other. I'll need to change the TAP_TO_LOAD_ENABLED probe in Histogram.json from boolean to enumerated.
I'm wondering if we need this level of control right away. 

Currently, we added this new feature and the subtitle is used almost as a tip to teach our users how to "show" an image that is blocked. CCing Barbara for her thoughts.

In the meantime, you could try:

"Tap-to-load images"
A: Always enabled
B: When using cellular data
C: Disabled
Flags: needinfo?(alam)
(In reply to Anthony Lam (:antlam) from comment #11)
> I'm wondering if we need this level of control right away. 
> 
> Currently, we added this new feature and the subtitle is used almost as a
> tip to teach our users how to "show" an image that is blocked.

We could still use the text as a tip

> "Tap-to-load images"

Remember, it's not actually tapping to load. This is a bad tip.

> A: Always enabled
> B: When using cellular data
> C: Disabled

A: Load images only when you long press
B: Load images only when on Wifi
C: Always load images
Not sure which copy to use over here as well.
Flags: needinfo?(alam)
(In reply to Richard Newman [:rnewman] from comment #14)
> I'd like to pause this party and point you to
> 
> http://developer.android.com/reference/android/net/ConnectivityManager.
> html#isActiveNetworkMetered%28%29

Interesting, but not required to pause this patch for now. This bug limit to Wifi or not, a broad split. The metered network check would allow us to split up the cell data (non-Wifi) split.

Anthony - Let's nail down the copy, all of the copy. I don't want to end up in a similar situation as we are in with Tab Queues. I also want to be consistent with the way we use "Wifi only" in other settings. having some use "wifi only" and some using "cellular only" seems confusing.
(In reply to Mark Finkle (:mfinkle) from comment #15)

> Interesting, but not required to pause this patch for now. This bug limit to
> Wifi or not, a broad split. The metered network check would allow us to
> split up the cell data (non-Wifi) split.

Nope -- the point of the metered check is to spot things like "I'm tethered to my friend's phone over wifi", which is a situation in which you _don't_ want to do expensive things, even though you're on wifi.

Specifically, this comes down to the DHCP lease including ANDROID_METERED as a vendor hint. In particular, all modern Android devices will set this when hotspotting.

So this choice should really be something like:

  Always block images
  Block when on cellular                ( because cell is slow )
  Block when on a metered connection    ( because it's expensive; also typically covers cell )
  Always load images

or

  Always block images
  Block when on cellular or metered connections  ( most or all cell connections are metered )
  Always load images

My assertion here is that users want to block images for two reasons:

1. They want a faster web.
2. They don't want to pay cash money for images.

The only accurate way to do the latter is to check for a metered connection. You can have metered wifi and unlimited LTE…
Given that metered support needs to be exposed to XPCOM, and we don't use metered in any other feature, I'm still not inclined to block this bug on it.

We can file new bugs to add metered support to our networking and network-related features:
* Updater
* SSDP
* Images
* Sync
That I agree with. Something is better than nothing.

But let's not stop at 'something' -- shoehorning this into GeckoNetworkManager (alongside stuff like @JNITarget getMNC and getConnectionType) isn't hard.

Even better if we figure out the strings for the right solution up-front.
(In reply to Mark Finkle (:mfinkle) from comment #12)
> (In reply to Anthony Lam (:antlam) from comment #11)
> > I'm wondering if we need this level of control right away. 
> > 
> > Currently, we added this new feature and the subtitle is used almost as a
> > tip to teach our users how to "show" an image that is blocked.
> 
> We could still use the text as a tip
> 
> > "Tap-to-load images"
> 
> Remember, it's not actually tapping to load. This is a bad tip.
> 
> > A: Always enabled
> > B: When using cellular data
> > C: Disabled
> 
> A: Load images only when you long press
> B: Load images only when on Wifi
> C: Always load images

Agree, Tap-to-load doesn't seem like it makes sense. It's time to change it. 

Why don't we just call it what it does? "Block images" seems straight forward to me.

As for "load", shouldn't we say "unblock" instead? That would keep our terminologies to 2 (Block, and unblock). It'd also be more consistent if we're going with that for the toast/snackbar in bug 1211295.

Block images
- Always
- Unblock over Wi-Fi
- Never

My logic for following this format of A: always, B: exception, and C: never, is that we do it for Download updates automatically, Plugins, and Cookies. From a brief look at Chrome, they also follow this pattern.

I know that your suggestion essentially means the same thing but the language is a bit indirect. For scan-ability, it might be better to use those words where we can. It also looks and reads more straight forward.

Could we also be over thinking this? I don't use these features so I haven't seen what the norm is. 

But on mobile, there's almost always only two interactions: press, and long press. I worry we're just creating confusion here because we feel like we need to work in a "tip" for "how to unblock" in this dialog. But perhaps these users already are familiar with it.

Or perhaps we can work it in through a better method like a second screen (full page) for this pref [1] (Chrome does this for Data Saver). These are simple and gives us more room to explain more complicated features. I think they could be helpful to us in more ways than one. I think Sebastian mentioned support issues but I can't remember right now.

Mfinkle, thoughts?

(In reply to Mark Finkle (:mfinkle) from comment #15)
> (In reply to Richard Newman [:rnewman] from comment #14)
> > I'd like to pause this party and point you to
> > 
> > http://developer.android.com/reference/android/net/ConnectivityManager.
> > html#isActiveNetworkMetered%28%29
> 
> Interesting, but not required to pause this patch for now. This bug limit to
> Wifi or not, a broad split. The metered network check would allow us to
> split up the cell data (non-Wifi) split.
> 
> Anthony - Let's nail down the copy, all of the copy. I don't want to end up
> in a similar situation as we are in with Tab Queues. I also want to be
> consistent with the way we use "Wifi only" in other settings. having some
> use "wifi only" and some using "cellular only" seems confusing.

Agree. I noticed that there's a lot of reference to "over Wi-Fi" so let's stick with that.

[1] https://www.google.com/design/spec/patterns/settings.html#settings-labels-secondary-text
Flags: needinfo?(alam) → needinfo?(mark.finkle)
(In reply to Anthony Lam (:antlam) from comment #19)

> Agree, Tap-to-load doesn't seem like it makes sense. It's time to change it. 
> 
> Why don't we just call it what it does? "Block images" seems straight
> forward to me.

This seems like a good plan.

> As for "load", shouldn't we say "unblock" instead? That would keep our
> terminologies to 2 (Block, and unblock). It'd also be more consistent if
> we're going with that for the toast/snackbar in bug 1211295.

This is great reasoning, but I think we should also consider the opposite direction, in case it makes more sense to be "positive".

> Block images
> - Always
> - Unblock over Wi-Fi
> - Never

Display images
 - Always
 - Only over Wi-Fi
 - Never

> My logic for following this format of A: always, B: exception, and C: never,
> is that we do it for Download updates automatically, Plugins, and Cookies.
> From a brief look at Chrome, they also follow this pattern.

Agreed. This is a good pattern. I would be fine with either approach, but I like the "Display images" because we are using "Only on Wi-Fi" in the same context as other places.

> But on mobile, there's almost always only two interactions: press, and long
> press. I worry we're just creating confusion here because we feel like we
> need to work in a "tip" for "how to unblock" in this dialog. But perhaps
> these users already are familiar with it.

I'm willing to agree here and say we should do some UR to see if we need any hint. If we do, maybe it's best to use in the page itself, and not in Settings.

> Or perhaps we can work it in through a better method like a second screen
> (full page) for this pref [1] (Chrome does this for Data Saver). These are
> simple and gives us more room to explain more complicated features. I think
> they could be helpful to us in more ways than one. I think Sebastian
> mentioned support issues but I can't remember right now.

We should consider using this pattern for settings. Maybe not this setting, but you are right, this is a pattern we should be using when needed.
Flags: needinfo?(mark.finkle)
Leaving NI for previous comment.
Flags: needinfo?(alam)
(In reply to Mark Finkle (:mfinkle) from comment #20)
> (In reply to Anthony Lam (:antlam) from comment #19)
> This is great reasoning, but I think we should also consider the opposite
> direction, in case it makes more sense to be "positive".
> 
> > Block images
> > - Always
> > - Unblock over Wi-Fi
> > - Never
> 
> Display images
>  - Always
>  - Only over Wi-Fi
>  - Never

I'm not too worried about "block" being too negative. I think it's rather neutral in the context of Settings. It's actually an example that Google guidelines refer to as well [1]

> > My logic for following this format of A: always, B: exception, and C: never,
> > is that we do it for Download updates automatically, Plugins, and Cookies.
> > From a brief look at Chrome, they also follow this pattern.
> 
> Agreed. This is a good pattern. I would be fine with either approach, but I
> like the "Display images" because we are using "Only on Wi-Fi" in the same
> context as other places.

Hm, that's a good point.

With that in mind, I'd suggest:

Show images
 - Always
 - Only over Wi-Fi
 - Never

I don't like "display" because I think it's actually more "jargony" than we might think. From a brief look on readability-score.com, It seems to actually be of a much higher reading level than "show".

I am aware that we might be "underselling" the feature here a bit now (with this new language). But that it seems so "simple" is a good thing I think. I don't think that's a problem. It's actually more straight forward and accessible/understandable this way.

> > Or perhaps we can work it in through a better method like a second screen
> > (full page) for this pref [1] (Chrome does this for Data Saver). These are
> > simple and gives us more room to explain more complicated features. I think
> > they could be helpful to us in more ways than one. I think Sebastian
> > mentioned support issues but I can't remember right now.
> 
> We should consider using this pattern for settings. Maybe not this setting,
> but you are right, this is a pattern we should be using when needed.

Agree, let's table this for another bug. 

I think we could start taking advantage of these second screen/full-page prefs more often. But not in scope for this bug.

[1] https://www.google.com/design/spec/patterns/settings.html#settings-grouping-settings
Flags: needinfo?(mark.finkle)
Flags: needinfo?(jonalmeida942)
Flags: needinfo?(alam)
(In reply to Anthony Lam (:antlam) from comment #22)

> With that in mind, I'd suggest:
> 
> Show images
>  - Always
>  - Only over Wi-Fi
>  - Never

Sold!
Flags: needinfo?(mark.finkle)
Attachment #8680545 - Flags: review?(mark.finkle)
Comment on attachment 8680545 [details]
MozReview Request: Bug 1209293 - Add option for click-to-play images depending on network bearer r=mfinkle

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/23639/diff/1-2/
Attached image Screenshot.png — 
Done!
Assignee: nobody → jonalmeida942
Flags: needinfo?(jonalmeida942)
Comment on attachment 8680545 [details]
MozReview Request: Bug 1209293 - Add option for click-to-play images depending on network bearer r=mfinkle

https://reviewboard.mozilla.org/r/23639/#review22257

::: mobile/android/base/locales/en-US/android_strings.dtd:216
(Diff revision 2)
> -<!ENTITY pref_tap_to_load_images_title "Tap-to-load images">
> +<!ENTITY pref_tap_to_load_images_title "Show images">

pref_tap_to_load_images_title -> pref_tap_to_load_images_title2

::: mobile/android/base/locales/en-US/android_strings.dtd:217
(Diff revision 2)
>  <!ENTITY pref_tap_to_load_images_summary2 "Load images only when you long press them">

Remove if not used anymore

::: mobile/android/base/strings.xml.in:212
(Diff revision 2)
>    <string name="pref_tap_to_load_images_summary2">&pref_tap_to_load_images_summary2;</string>

Remove if not used anymore

::: mobile/android/components/ImageBlockingPolicy.js:30
(Diff revision 2)
> -      return Ci.nsIContentPolicy.ACCEPT;
> +    if (this._enabled() == 1 || (this._usingCellular() && this._enabled() == 2)) {

Change to:
if (this.\_enabled() == 1 || (this.\_enabled() == 2 && this.\_usingCellular())) \{

so we can short-circuit and bail on the cheaper | \_enabled() == 2 | check
Attachment #8680545 - Flags: review?(mark.finkle)
(In reply to Mark Finkle (:mfinkle) from comment #26)

> Change to:
> if (this.\_enabled() == 1 || (this.\_enabled() == 2 &&
> this.\_usingCellular())) \{
> 
> so we can short-circuit and bail on the cheaper | \_enabled() == 2 | check

MozReview added the '\' in there for some reason. Ignore those.
Attachment #8680545 - Attachment description: MozReview Request: Bug 1209293 - Add option for click-to-play images depending on network bearer r?mfinkle → MozReview Request: Bug 1209293 - Add option for click-to-play images depending on network bearer r=mfinkle
Attachment #8680545 - Flags: review?(mark.finkle)
Comment on attachment 8680545 [details]
MozReview Request: Bug 1209293 - Add option for click-to-play images depending on network bearer r=mfinkle

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/23639/diff/2-3/
Comment on attachment 8680545 [details]
MozReview Request: Bug 1209293 - Add option for click-to-play images depending on network bearer r=mfinkle

https://reviewboard.mozilla.org/r/23639/#review22271

LGTM
Attachment #8680545 - Flags: review?(mark.finkle) → review+
https://reviewboard.mozilla.org/r/23639/#review22273

::: mobile/android/components/ImageBlockingPolicy.js:30
(Diff revision 3)
> -      return Ci.nsIContentPolicy.ACCEPT;
> +    if (this._enabled() == 1 || (this._enabled() == 2 && this._usingCellular())) {

I'm going to add consts to the number values so it's less ambiguous.
https://hg.mozilla.org/mozilla-central/rev/75bcd8adb694
https://hg.mozilla.org/mozilla-central/rev/d46b88805cf0
Status: NEW → RESOLVED
Closed: 9 years ago
status-firefox45: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 45
Tested using:
Device: Nexus 9 (Android 6.0) and Xiaomi Mi i4 (Android 5.0.2)
Build: Firefox for Android 45.0a1 (2015-11-11)

"Show images" is displayed in the "Advanced" sub-menu with the following options:
-Always
-Only over Wi-Fi
-Never

Are these behaviors expected?
1. "Never" option is the default one. 
2. "Never" option shows the image
3. "Always" option displays the placeholder on images on phone; on tablet some images are displayed and some have the placeholder icon.
Flags: needinfo?(jonalmeida942)
(In reply to Teodora Vermesan (:TeoVermesan) from comment #35)
> Tested using:
> Device: Nexus 9 (Android 6.0) and Xiaomi Mi i4 (Android 5.0.2)
> Build: Firefox for Android 45.0a1 (2015-11-11)
> 
> "Show images" is displayed in the "Advanced" sub-menu with the following
> options:
> -Always
> -Only over Wi-Fi
> -Never
> 
> Are these behaviors expected?
> 1. "Never" option is the default one. 
> 2. "Never" option shows the image
> 3. "Always" option displays the placeholder on images on phone; on tablet
> some images are displayed and some have the placeholder icon.

No this is not expected. The patch in comment 34 will fix this, but won't hit Nightly until tomorrow.

Please re-test tomorrow. The expected outcome:
1. "Always" option is the default.
2. "Always" will show the image.
3. "Never" will not show the image.
4. When blocking, all images should be blocked. Please file a new bug for pages where images are still displayed.
Flags: needinfo?(jonalmeida942)
(In reply to Mark Finkle (:mfinkle) from comment #36)

> 1. "Always" option is the default.
> 2. "Always" will show the image.
> 3. "Never" will not show the image.

Verified as fixed using:
Device: Nexus 6 (Android 5.1)
Build: Firefox for Android 45.0a1 (2015-11-13)

> 4. When blocking, all images should be blocked. Please file a new bug for pages where images are still displayed.
Filled Bug 1224543
Verified as fixed on Firefox 45 Beta 1
Status: RESOLVED → VERIFIED
status-firefox45: fixed → 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.

Attachment

General

Creator:
Created:
Updated:
Size: