1170725 - Click-to-play images

Status: RESOLVED FIXED

(Firefox for Android Graveyard :: General, defect)

Description

[(inactive) Jim Chen [:jchen] [:darchons]]

I think the "click-to-play images" functionality is already implementable as an add-on or inside browser.js, without additional platform changes.

We can use nsIContentPolicy to block images on load, and on long-tap, we can have a context menu item that uses nsIImageLoadingContent to reload the image. For CSS background images, I think the best way to show those images is to reload the page.

I found an existing add-on [1] that Martyn wrote that does image blocking, so he may have some thoughts on this.

[1] https://addons.mozilla.org/en-us/android/addon/mobile-image-blocker/

Comment 1

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

I started an add-on [1] to implement a few of the ideas in comment 0:
1. Use nsIContentPolicy to block all images
2. Use a long-tap context menu item to load images as needed

The add-on displays an "image-blocked" placeholder [2] to let the user know where images were supposed to display.

[1] https://github.com/mfinkle/clicktoviewimages
[2] http://people.mozilla.org/~mfinkle/fennec/screenshots/fennec-image-blocked-01.png

Comment 2

[Jonathan Almeida [:jonalmeida]]

Attachment: MozReview Request: Bug 1170725 - Click-to-play images r?mfinkle

Bug 1170725 - Click-to-play images r?margaret

This is a WIP patch that doesn't initalize the ImageBlockingPolicy.js class
as expected.

The init and uninit methods are commented out since the initializing is now
done in the MobileComponents.manifest. I think there might be a problem
there which is the cause.

The assumptions I've made is that I don't need to add a window watcher,
listening for the "domwindowopened" event.

Comment 3

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

https://reviewboard.mozilla.org/r/15903/#review14225

::: mobile/android/components/ImageBlockingPolicy.js:18
(Diff revision 1)
> +console.log("WEEEEEEEEEEEEEEE");

See my comment about console not being available

::: mobile/android/components/ImageBlockingPolicy.js:21
(Diff revision 1)
> +  classID: Components.ID("{f55f77f9-d33d-4759-82fc-60db3ee0bb91}"),

Did you make a new UUID? Or is thi still the one from my add-on?

::: mobile/android/components/ImageBlockingPolicy.js:50
(Diff revision 1)
> +  setEnabled: function(enabled) {

Eventually, we'll need to add some other mechanism to control the enable/disable feature. We can't ever call this method (without hacks) and prefs or notifications are cleaner.

::: mobile/android/components/ImageBlockingPolicy.js:56
(Diff revision 1)
> +    console.log("HELLLLOOOOOOOOOOOOOOOOO")

console is not available in a component. It's a feature of a DOM window.

Use Services.console.logMessageString or better yet, AndroidLog.jsm (look for examples in our code)

::: mobile/android/components/ImageBlockingPolicy.js:68
(Diff revision 1)
> +        dump(node.outerHTML + "\n");

dump might get sent to the android log in local builds, but even this should be replaced with AndroidLog.jsm

::: mobile/android/components/ImageBlockingPolicy.js:97
(Diff revision 1)
> +  // nsIFactory interface implementation

You can remove this function

::: mobile/android/components/ImageBlockingPolicy.js:106
(Diff revision 1)
> +  QueryInterface: XPCOMUtils.generateQI([Ci.nsIContentPolicy, Ci.nsIFactory])

You can remove Ci.nsIFactory from the list too

Comment 4

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

Also, look for other errors during Gecko startup in Android Log. That's when components are registered and initialized.

Comment 5

[:Margaret Leibovic]

Comment on attachment 8647142 [details]
MozReview Request: Bug 1170725 - Click-to-play images r?mfinkle

https://reviewboard.mozilla.org/r/15903/#review14307

Were you able to get this working with mfinkle's comment? The syntax for registering these components always confuses me, so I would look to existing examples to see what you might have done wrong.

For example, it looks like you might need this NSGetFactory logic:

http://mxr.mozilla.org/mozilla-central/source/mobile/android/components/DirectoryProvider.js#190

::: mobile/android/chrome/content/browser.js:2597
(Diff revision 1)
> +        }

Nit: identation is off here.

::: mobile/android/components/ImageBlockingPolicy.js:16
(Diff revision 1)
> +let DATA_IMG = "data:image/svg+xml;charset=utf-8;base64,PD94bWwgdmVyc2lvbj0iMS4wIiBlbmNvZGluZz0idXRmLTgiPz4NCjwhRE9DVFlQRSBzdmcgUFVCTElDICItLy9XM0MvL0RURCBTVkcgMS4xLy9FTiIgImh0dHA6Ly93d3cudzMub3JnL0dyYXBoaWNzL1NWRy8xLjEvRFREL3N2ZzExLmR0ZCI%2BDQoNCjxzdmcgdmVyc2lvbj0iMS4xIg0KICAgICB4bWxucz0iaHR0cDovL3d3dy53My5vcmcvMjAwMC9zdmciDQogICAgIHhtbG5zOnhsaW5rPSJodHRwOi8vd3d3LnczLm9yZy8xOTk5L3hsaW5rIg0KICAgICB4PSIwIg0KICAgICB5PSIwIg0KICAgICB3aWR0aD0iMzIiDQogICAgIGhlaWdodD0iMzIiDQogICAgIHZpZXdCb3g9IjAgMCAzMiAzMiI%2BDQoNCiAgPGRlZnM%2BDQogICAgDQogICAgPG1hc2sgaWQ9Im1hc2stY3V0b3V0LWJsb2NrZWQtc2lnbiI%2BDQogICAgICA8cmVjdCB3aWR0aD0iMzIiIGhlaWdodD0iMzIiIGZpbGw9IiNmZmYiIC8%2BDQogICAgICA8Y2lyY2xlIGN4PSIyNSIgY3k9IjI1IiByPSI4IiBmaWxsPSIjMDAwIiAvPg0KICAgIDwvbWFzaz4NCg0KICAgIDxtYXNrIGlkPSJtYXNrLWN1dG91dC1mcmFtZSI%2BDQogICAgICA8cmVjdCB3aWR0aD0iMzIiIGhlaWdodD0iMzIiIGZpbGw9IiMwMDAiIC8%2BDQogICAgICA8cmVjdCB4PSIyIiB5PSI0IiB3aWR0aD0iMjgiIGhlaWdodD0iMjQiIHJ4PSIzIiByeT0iMyIgZmlsbD0iI2ZmZiIgLz4NCiAgICAgIDxyZWN0IHg9IjQiIHk9IjYiIHdpZHRoPSIyNCIgaGVpZ2h0PSIyMCIgcng9IjIiIHJ5PSIyIiBmaWxsPSIjMDAwIiAvPg0KICAgIDwvbWFzaz4NCiAgICANCiAgICA8bWFzayBpZD0ibWFzay1jdXRvdXQtYmxvY2tlZC1zaWduLWlubmVyIj4NCiAgICAgIDxyZWN0IHdpZHRoPSIzMiIgaGVpZ2h0PSIzMiIgZmlsbD0iI2ZmZiIgLz4NCiAgICAgIDxjaXJjbGUgY3g9IjI1IiBjeT0iMjUiIHI9IjQiIGZpbGw9IiMwMDAiIC8%2BDQogICAgPC9tYXNrPg0KDQogIDwvZGVmcz4NCg0KICA8ZyBpZD0iaWNvbi1mcmFtZSIgbWFzaz0idXJsKCNtYXNrLWN1dG91dC1ibG9ja2VkLXNpZ24pIj4NCiAgICA8cmVjdCBpZD0ic2hhcGUtYmFja2dyb3VuZCIgeD0iMiIgeT0iNCIgd2lkdGg9IjI4IiBoZWlnaHQ9IjI0IiByeD0iMyIgcnk9IjMiIGZpbGw9IiNmMGYxZjIiIC8%2BDQogICAgPHBvbHlnb24gcG9pbnRzPSIzLDI2IDExLDE2IDE4LDI2IiBmaWxsPSIjN2U3ZjgwIiAvPg0KICAgIDxwb2x5Z29uIHBvaW50cz0iMTEsMjYgMjMsMTAgMzEsMjAgMzEsMjYiIGZpbGw9IiM0YzRjNGQiIC8%2BDQogICAgPGNpcmNsZSBjeD0iOSIgY3k9IjExIiByPSIzIiBmaWxsPSIjOTc5ODk5IiAvPg0KICAgIDxyZWN0IHdpZHRoPSIzMiIgaGVpZ2h0PSIzMiIgZmlsbD0iIzY1NjU2NiIgbWFzaz0idXJsKCNtYXNrLWN1dG91dC1mcmFtZSkiIC8%2BDQogIDwvZz4NCiAgPGcgaWQ9Imljb24tYmxvY2tlZC1zaWduIj4NCiAgICA8Y2lyY2xlIGN4PSIyNSIgY3k9IjI1IiByPSI2IiBmaWxsPSIjNjU2NTY2IiBtYXNrPSJ1cmwoI21hc2stY3V0b3V0LWJsb2NrZWQtc2lnbi1pbm5lcikiIC8%2BDQogICAgPGxpbmUgeDE9IjIxIiB5MT0iMjkiIHgyPSIyOSIgeTI9IjIxIiBzdHJva2U9IiM2NTY1NjYiIHN0cm9rZS13aWR0aD0iMiIgLz4NCiAgPC9nPg0KDQo8L3N2Zz4NCg%3D%3D";

Can we put this in a file instead of a data URI?

::: mobile/android/components/ImageBlockingPolicy.js:68
(Diff revision 1)
> +        dump(node.outerHTML + "\n");

If you look at browser.js, you'll see that dump is actually defined as a function there:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/browser.js#316

But we don't have it in this context, so you would have to implement it yourself here.

Comment 6

[:Margaret Leibovic]

Also, I would put this behind a pref when we land this, since I don't think this is something that users would want by default.

Comment 7

[Jonathan Almeida [:jonalmeida]]

https://reviewboard.mozilla.org/r/15903/#review14225

> Did you make a new UUID? Or is thi still the one from my add-on?

This is the original UUID. I didn't know of a good reason to generate a new one.

> Eventually, we'll need to add some other mechanism to control the enable/disable feature. We can't ever call this method (without hacks) and prefs or notifications are cleaner.

True, this isn't used any where anymore. When I add a pref for it, image blocking will be controlled then.

> console is not available in a component. It's a feature of a DOM window.
> 
> Use Services.console.logMessageString or better yet, AndroidLog.jsm (look for examples in our code)

I probably should have, but I found out that console.log statements get magically turned into android verbose logs with the 'GeckoBrowser' tag.

Comment 8

[Jonathan Almeida [:jonalmeida]]

https://reviewboard.mozilla.org/r/15903/#review14307

> Can we put this in a file instead of a data URI?

This will eventually move to it's own file before a final commit, but I'm leaving it here for now for testing since I know it works and its just easier at the same time.

Comment 9

[Jonathan Almeida [:jonalmeida]]

So I figured out the issue with my patch (didn't define the prototype property), so I'll clean up, add a pref and separate the placeholder image from the js file.

(ﾉ◕ヮ◕)ﾉ*:･ﾟ✧

Comment 10

[Jonathan Almeida [:jonalmeida]]

Also need to figure out a way to stop blocking images of a very small size and missing context menu options for images with links.

Comment 11

[Jonathan Almeida [:jonalmeida]]

Do we want the option for image blocking to show as an about:config pref or a UI settings pref?

Comment 12

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

(In reply to Jonathan Almeida (:jonalmeida) from comment #11)
> Do we want the option for image blocking to show as an about:config pref or
> a UI settings pref?

If we want Settings UI (very likely), I'd treat that as a separate bug. Just use Gecko Prefs and about:config for now. Any Settings UI would build on the Gecko prefs.

Comment 13

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

(In reply to Jonathan Almeida (:jonalmeida) from comment #10)
> Also need to figure out a way to stop blocking images of a very small size
> and missing context menu options for images with links.

You could use a Gecko pref to control a threshold size (in kB?). If the pref is 0 all images are blocked.

Comment 14

[Jonathan Almeida [:jonalmeida]]

Attachment: possible-option-in-ui.png

(In reply to Mark Finkle (:mfinkle) from comment #13)
> (In reply to Jonathan Almeida (:jonalmeida) from comment #10)
> > Also need to figure out a way to stop blocking images of a very small size
> > and missing context menu options for images with links.
> 
> You could use a Gecko pref to control a threshold size (in kB?). If the pref
> is 0 all images are blocked.

I like this idea. I ended up adding the UI option with placeholder strings to help get a visual idea of the option. 

With Gecko prefs, we can get set the int value, but in UI we would need a text field or slider to set the value for the image size. We currently don't have any settings that do this though so we need to a new one.

Comment 15

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

Can I see a build check out what this UX currently feels like? I worry a long-press to load might be a bit much and would like to see if we can do a single-press to load.

Related, could we perhaps also limit the max dimension size of this placeholder image? so it looks less intimidating? :D 

Our empty states panels designs (bug 1091826) use 80dp square as the size for similar placeholders. Seems like a good place to start.

Comment 16

[:Margaret Leibovic]

Redirecting that NI to jonalmeida :)

Comment 17

[Jonathan Almeida [:jonalmeida]]

Sent a build to antlam via email because it was too large to attach here.

> Related, could we perhaps also limit the max dimension size of this placeholder image? so it looks less intimidating? :D 
We're using an svg so the image scales depending on the original image dimensions. It should avoid layout issues on websites if we were to use a fixed sized image, but I might be mistaken on that, there might be a better way to do it.

Comment 18

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

(In reply to Jonathan Almeida (:jonalmeida) from comment #17)
> We're using an svg so the image scales depending on the original image
> dimensions. It should avoid layout issues on websites if we were to use a
> fixed sized image, but I might be mistaken on that, there might be a better
> way to do it.

Right- what I mean is, can we set a maximum on the image such that there is always some padding (inwards/away) from the edges of the image.

I'm not sure a fixed sized image is a good solution here.. but I could be wrong. I think we just need to add padding to the SVG.

Comment 19

[Jonathan Almeida [:jonalmeida]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=02de32f24c87

Comment 20

[Jonathan Almeida [:jonalmeida]]

Comment on attachment 8647142 [details]
MozReview Request: Bug 1170725 - Click-to-play images r?mfinkle

Bug 1170725 - Click-to-play images r?margaret

Comment 21

[Jonathan Almeida [:jonalmeida]]

Comment on attachment 8647142 [details]
MozReview Request: Bug 1170725 - Click-to-play images r?mfinkle

Bug 1170725 - Click-to-play images r?mfinkle

Comment 22

[Jonathan Almeida [:jonalmeida]]

https://reviewboard.mozilla.org/r/15903/#review14747

::: mobile/android/installer/package-manifest.in:5
(Diff revision 3)
> -; Package file for the Fennec build. 
> +; Package file for the Fennec build.

Editor auto-removes EOL whitespaces. Sorry!

::: mobile/android/themes/core/images/placeholder_image.svg:3
(Diff revision 3)
> +<svg version="1.1" xmlns="http://www.w3.org/2000/svg" xmlns:xlink="http://www.w3.org/1999/xlink" x="0" y="0" width="32" height="32" viewBox="-2 -2 38 38">

I made changes to the viewBox as per antlam's comments to add some padding inside. Everything else on the image is as original.

Comment 23

[Jonathan Almeida [:jonalmeida]]

Anthony, I've added the changes and I'll send you a new apk as well if you want see how it looks.

Comment 24

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

Comment on attachment 8647142 [details]
MozReview Request: Bug 1170725 - Click-to-play images r?mfinkle

https://reviewboard.mozilla.org/r/15903/#review14755

::: mobile/android/app/mobile.js:357
(Diff revision 3)
> +pref("browser.privacy.image_blocking_enabled", false);

I don't think this is a "privacy" feature. What about "browser.image_blocking.enabled"? We could then add "browser.image_blocking.minsize" or something later, if we add a size allowance.

::: mobile/android/chrome/content/browser.js:1007
(Diff revision 3)
> +        target.setAttribute("data-ctv-show", "true");

I added the "data-ctv-*" attributes in add-on, but we may want to move away from those now that this is in product. Doesn't need to block landing, but let's file a followup bug to figure out what our options are.

::: mobile/android/chrome/content/browser.js:2600
(Diff revision 3)
> +          return false;

indent issue

::: mobile/android/components/ImageBlockingPolicy.js:16
(Diff revision 3)
> +let DATA_IMG = "chrome://browser/skin/images/placeholder_image.svg";

I used "DATA" because it was a data: URI. Rename to PLACEHOLDER_IMG.

Comment 25

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

Thanks for the builds, they're super helpful.

This is essentially what I'm looking for. I took it to a few sites to test it out. Just so we're not adding an arbitrary value, what value for the padding are you using here? We could probably reuse some of a value from other parts of our layouts.

Comment 26

[Jonathan Almeida [:jonalmeida]]

I'm not sure how this would translate into pixel values, but in the svg I've changed the viewbox attribute from "0 0 32 32" to "-2 -2 38 38". It's about 0.85% if the image is 32x32.

Not sure if that was helpful..

Comment 27

[Barbara Bermes [:barbara] - NI please!]

(In reply to Jonathan Almeida (:jonalmeida) from comment #17)
> Sent a build to antlam via email because it was too large to attach here.
> 
> > Related, could we perhaps also limit the max dimension size of this placeholder image? so it looks less intimidating? :D 
> We're using an svg so the image scales depending on the original image
> dimensions. It should avoid layout issues on websites if we were to use a
> fixed sized image, but I might be mistaken on that, there might be a better
> way to do it.

Could I get one as well :) I want to see/feel how it looks - thanks

Comment 28

[Barbara Bermes [:barbara] - NI please!]

(In reply to Jonathan Almeida (:jonalmeida) from comment #11)
> Do we want the option for image blocking to show as an about:config pref or
> a UI settings pref?

I would suggest to put it as UI settings pref, maybe Customize (and not Privacy as the screenshot showed it, I think), it's more a bandwidth concern than a privacy concern.

Comment 29

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

(In reply to Barbara Bermes [:bbermes] from comment #28)
> (In reply to Jonathan Almeida (:jonalmeida) from comment #11)
> > Do we want the option for image blocking to show as an about:config pref or
> > a UI settings pref?
> 
> I would suggest to put it as UI settings pref, maybe Customize (and not
> Privacy as the screenshot showed it, I think), it's more a bandwidth concern
> than a privacy concern.

Agree.

Comment 30

[Jonathan Almeida [:jonalmeida]]

https://reviewboard.mozilla.org/r/15903/#review14755

> I added the "data-ctv-*" attributes in add-on, but we may want to move away from those now that this is in product. Doesn't need to block landing, but let's file a followup bug to figure out what our options are.

Created bug 1196416 for this.

Comment 31

[Jonathan Almeida [:jonalmeida]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=b2a7d2380b34

Comment 32

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

(In reply to Mark Finkle (:mfinkle) from comment #29)
> (In reply to Barbara Bermes [:bbermes] from comment #28)
> > (In reply to Jonathan Almeida (:jonalmeida) from comment #11)
> > > Do we want the option for image blocking to show as an about:config pref or
> > > a UI settings pref?
> > 
> > I would suggest to put it as UI settings pref, maybe Customize (and not
> > Privacy as the screenshot showed it, I think), it's more a bandwidth concern
> > than a privacy concern.
> 
> Agree.

WFM too.

(In reply to Jonathan Almeida (:jonalmeida) from comment #26)
> I'm not sure how this would translate into pixel values, but in the svg I've
> changed the viewbox attribute from "0 0 32 32" to "-2 -2 38 38". It's about
> 0.85% if the image is 32x32.
> 
> Not sure if that was helpful..

Not really, hah. I tested it out and it is behaving a bit awkwardly for me.

Let's chat about this over vidyo next week since we still have some time. I'll ping on IRC

Comment 33

[Jonathan Almeida [:jonalmeida]]

Talked to antlam on IRC: I'm going to try to center the placeholder image in the place where the original image would be and give it a max size so it doesn’t look massive on huge images.

Will touch base with antlam next week.

Comment 34

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

just so I don't forget myself :)

Comment 35

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

Action item right now is on me to get the SVG from Shorlander and add some built-in padding.

Comment 36

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

(In reply to Anthony Lam (:antlam) from comment #35)
> Action item right now is on me to get the SVG from Shorlander and add some
> built-in padding.

Here it is:
http://people.mozilla.org/~shorlander/drops/blocked-image-01.svg

Comment 37

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

Attachment: icon_ttl_42.svg

Added padding!

Scaled it up from 32pt to 42pt but kept the icon the same (more padding). 

Let's see how this works

Comment 38

[Jonathan Almeida [:jonalmeida]]

Attachment: Screenshot_2015-08-24-16-43-00.png

Definitely looks much better! I just sent you a build with the change. It doesn't have the UI option in it, so just turn it on in gecko prefs (browser.image_blocking.enabled)

Comment 39

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

Nice! and thank you for sending that build over. I think this looks much better.

Great work!

Comment 40

[Jonathan Almeida [:jonalmeida]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=1c4b7b70c77b

Comment 41

[Jonathan Almeida [:jonalmeida]]

Comment on attachment 8647142 [details]
MozReview Request: Bug 1170725 - Click-to-play images r?mfinkle

Bug 1170725 - Click-to-play images r?mfinkle

Comment 42

[Jonathan Almeida [:jonalmeida]]

Attachment: MozReview Request: Bug 1195063 - Create a UI Settings option for changing the image threshold image size for click-to-play images r?mfinkle

Bug 1195063 - Create a UI Settings option for changing the image threshold image size for click-to-play images r?mfinkle

Comment 43

[Jonathan Almeida [:jonalmeida]]

I've put my match up for review, but it can only land once bug 1198438 is fixed.

Comment 44

[Jonathan Almeida [:jonalmeida]]

Comment on attachment 8655112 [details]
MozReview Request: Bug 1195063 - Create a UI Settings option for changing the image threshold image size for click-to-play images r?mfinkle

Oops.

Comment 45

[Jonathan Almeida [:jonalmeida]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=c486a05bdb95

Comment 46

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

Comment on attachment 8647142 [details]
MozReview Request: Bug 1170725 - Click-to-play images r?mfinkle

https://reviewboard.mozilla.org/r/15903/#review18237

Looks good!

::: mobile/android/chrome/content/browser.js:998
(Diff revision 4)
> +        target.setAttribute("data-ctv-show", "true");

Add UI Telemetry here, like this:

UITelemetry.addEvent("action.1", "contextmenu", null, "web_show_image");

::: mobile/android/chrome/content/browser.js:2568
(Diff revision 4)
> +        if(!Services.prefs.getBoolPref("browser.image_blocking.enabled"))

if (

and add { } around this block

Comment 47

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

https://reviewboard.mozilla.org/r/17825/#review18239

We should put the Setting behind a NIGHTLY_BUILD flag so we could hide the Setting on Aurora or higher, if needed. You should find other examples of settings behind a nightly flag to use as an example.

Comment 48

[Jonathan Almeida [:jonalmeida]]

Comment on attachment 8647142 [details]
MozReview Request: Bug 1170725 - Click-to-play images r?mfinkle

Bug 1170725 - Click-to-play images r?mfinkle

Comment 49

[Jonathan Almeida [:jonalmeida]]

Comment on attachment 8647142 [details]
MozReview Request: Bug 1170725 - Click-to-play images r?mfinkle

Fixed comments.

Comment 50

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

https://reviewboard.mozilla.org/r/15903/#review18257

::: mobile/android/chrome/content/browser.js:975
(Diff revision 5)
> +        target.setAttribute("data-ctv-show", "true");

UITelemetry goes here. This is the click handler.

::: mobile/android/chrome/content/browser.js:2537
(Diff revision 5)
> +            UITelemetry.addEvent("action.1", "contextmenu", null, "web_show_image");

Whoops. This goes up above.
This is the code that matches the DOM element and shows/hides the menu item.

Comment 51

[Jonathan Almeida [:jonalmeida]]

https://reviewboard.mozilla.org/r/15903/#review18257

My bad, wasn't paying attention.

Comment 52

[Jonathan Almeida [:jonalmeida]]

Comment on attachment 8647142 [details]
MozReview Request: Bug 1170725 - Click-to-play images r?mfinkle

Bug 1170725 - Click-to-play images r?mfinkle

Comment 53

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

Comment on attachment 8647142 [details]
MozReview Request: Bug 1170725 - Click-to-play images r?mfinkle

https://reviewboard.mozilla.org/r/15903/#review18261

::: mobile/android/chrome/content/browser.js:2531
(Diff revision 6)
> +        if(!Services.prefs.getBoolPref("browser.image_blocking.enabled")) {

if (

add the space

Comment 54

[Jonathan Almeida [:jonalmeida]]

Comment on attachment 8647142 [details]
MozReview Request: Bug 1170725 - Click-to-play images r?mfinkle

Bug 1170725 - Click-to-play images r?mfinkle

Comment 55

[Jonathan Almeida [:jonalmeida]]

Comment on attachment 8647142 [details]
MozReview Request: Bug 1170725 - Click-to-play images r?mfinkle

Bug 1170725 - Click-to-play images r?mfinkle

Comment 56

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

Comment on attachment 8647142 [details]
MozReview Request: Bug 1170725 - Click-to-play images r?mfinkle

https://reviewboard.mozilla.org/r/15903/#review18265

\o/

Comment 57

[Jonathan Almeida [:jonalmeida]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=601c4ea5e6d2

Comment 58

[Pulsebot]

https://hg.mozilla.org/integration/fx-team/rev/f603438f4a15

Comment 59

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/f603438f4a15

Comment 60

[Teodora Vermesan (:TeoVermesan)]

Tested with:
Device: Nexus 4 (Android 5.1)
Build: Firefox for Android 44.0a1 (2015-09-30)

It is a bit confusing, because in Settings it says "Load images only when you tap on them", but to display the image, you have to long tap on the image and from the context menu to choose "Show Image"

Also searching with bing search engine for example "wallpaper" and go to the Images category, images are displayed, even though "tap-to-load images" is enabled.

Comment 61

[Jonathan Almeida [:jonalmeida]]

NI for antlam - comment 60 re the copy. "Load images only when you long tap on them"?

Comment 62

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

(In reply to Jonathan Almeida (:jonalmeida) from comment #61)
> NI for antlam - comment 60 re the copy. "Load images only when you long tap
> on them"?

There's more context in bug 1195063 when we worked on this with Matej.

Matej, what're your thoughts on changing "tap" to "long press" here? It will look like this:

Tap-to-load images
Load images only when you long press on them

Comment 63

[Matej Novak [:matej]]

(In reply to Anthony Lam (:antlam) from comment #62)
> (In reply to Jonathan Almeida (:jonalmeida) from comment #61)
> > NI for antlam - comment 60 re the copy. "Load images only when you long tap
> > on them"?
> 
> There's more context in bug 1195063 when we worked on this with Matej.
> 
> Matej, what're your thoughts on changing "tap" to "long press" here? It will
> look like this:
> 
> Tap-to-load images
> Load images only when you long press on them

Can it just say "long press them"? "On them" sounds odd here.

Also, is "long press" vs. "long tap" standard language for this action?

Comment 64

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

(In reply to Matej Novak [:matej] from comment #63)
> (In reply to Anthony Lam (:antlam) from comment #62)
> > (In reply to Jonathan Almeida (:jonalmeida) from comment #61)
> > > NI for antlam - comment 60 re the copy. "Load images only when you long tap
> > > on them"?
> > 
> > There's more context in bug 1195063 when we worked on this with Matej.
> > 
> > Matej, what're your thoughts on changing "tap" to "long press" here? It will
> > look like this:
> > 
> > Tap-to-load images
> > Load images only when you long press on them
> 
> Can it just say "long press them"? "On them" sounds odd here.

"long press them" makes sense.

> Also, is "long press" vs. "long tap" standard language for this action?

I'd say as long as we're consistent with what we've been calling this interaction within our app, then it should be fine :)

Comment 65

[Anthony Lam (:antlam) Find me, @antlam on Slack!]

Want to file a bug? ^ :)

Comment 66

[Jonathan Almeida [:jonalmeida]]

Filed! (bug 1210129)

Comment 67

[:Margaret Leibovic]

Updating dependencies to make it easier to track outstanding issues here (follow-up work should block the original bug, not depend on it)

Comment 68

[Sylvestre Ledru [:Sylvestre]]

Release Note Request (optional, but appreciated)
[Why is this notable]: New feature
[Suggested wording]: Click-to-view Images to reduce bandwidth
[Links (documentation, blog post, etc)]: