Closed Bug 1217997 Opened 4 years ago Closed 4 years ago

Sharing and copying image location gives us the placeholder image when using tap-to-load

Categories

(Firefox for Android :: General, defect)

43 Branch
defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 44
Tracking Status
firefox44 --- verified

People

(Reporter: jonalmeida, Unassigned)

References

Details

Attachments

(1 file)

STR:
- Enable Tap-to-load
- Look up cats on a search engine
- Long tap an image, to have the context menu open
- Choose either 'Share image' or 'Copy image location'
- Observe that the url received through your Intent/clipboard

Actual results:
- We get the placeholder image given to us (chrome://browser/skin/images/placeholder_image.svg)

Expected results:
- We should get back the original image url and the placeholder one should never be seen.
(In reply to Jonathan Almeida (:jonalmeida) from comment #0)
> STR:
> - Enable Tap-to-load
> - Look up cats on a search engine
> - Long tap an image, to have the context menu open
> - Choose either 'Share image' or 'Copy image location'
> - Observe that the url received through your Intent/clipboard
> 
> Actual results:
> - We get the placeholder image given to us
> (chrome://browser/skin/images/placeholder_image.svg)
> 
> Expected results:
> - We should get back the original image url and the placeholder one should
> never be seen.

Or we could just disable those menus for images that are blocked.
I think your comment 5 from bug 1217232 might actually fix the problem. Testing now..
Never mind, removing the context menus is a better idea than doing some weird rubbish; also seems like deceiving.
Bug 1217997 - Remove sharing and copying image when using tap-to-load images r?mfinkle
Attachment #8678368 - Flags: review?(mark.finkle)
Attachment #8678368 - Flags: review?(mark.finkle)
Comment on attachment 8678368 [details]
MozReview Request: Bug 1217997 - Remove sharing and copying image when using tap-to-load images r?mfinkle

https://reviewboard.mozilla.org/r/23193/#review20665

::: mobile/android/chrome/content/browser.js:2574
(Diff revision 1)
> +        if (aElement instanceof Ci.nsIDOMHTMLImageElement) {

I'm a bit worried that regular images (tap to view is disabled) will be nsIDOMHTMLImageElement and will have no "data-ctv-show" (so it will not be "true") and so we return false when we should return true.

Can you verify this works as expected with and without tap to view enabled?

You might need to use:

if (aElement.hasAttribute("data-ctv-src") && !aElement.hasAttribute("data-ctv-show") {
  return false;
}

::: mobile/android/chrome/content/browser.js:2575
(Diff revision 1)
> +        // The image is blocked by Tap-to-load Images

Needs an indent
https://reviewboard.mozilla.org/r/23193/#review20665

> I'm a bit worried that regular images (tap to view is disabled) will be nsIDOMHTMLImageElement and will have no "data-ctv-show" (so it will not be "true") and so we return false when we should return true.
> 
> Can you verify this works as expected with and without tap to view enabled?
> 
> You might need to use:
> 
> if (aElement.hasAttribute("data-ctv-src") && !aElement.hasAttribute("data-ctv-show") {
>   return false;
> }

You're right! This was a silly mistake on my side to not test it with tap to load disabled..
Attachment #8678368 - Flags: review?(mark.finkle)
Comment on attachment 8678368 [details]
MozReview Request: Bug 1217997 - Remove sharing and copying image when using tap-to-load images r?mfinkle

Bug 1217997 - Remove sharing and copying image when using tap-to-load images r?mfinkle
Attachment #8678368 - Flags: review?(mark.finkle) → review+
Comment on attachment 8678368 [details]
MozReview Request: Bug 1217997 - Remove sharing and copying image when using tap-to-load images r?mfinkle

https://reviewboard.mozilla.org/r/23193/#review20691

LGTM
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/9f4ad5529bc0
Status: NEW → RESOLVED
Closed: 4 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 44
Tested with:
Build: Firefox 44.0a1 (2015-10-28)
Device: Samsung S5 (Android 4.4.2)

With "Tap-to-load images" enabled, the following options are missing from context menu: "Share Image", "Copy Image Location", "Save Image", "Set Image As". Only "Show Image" option is displayed. After choosing it, all options are displayed for that image in context menu.
You need to log in before you can comment on or make changes to this bug.