Hide placeholder svg path in context menu when unblocking image

RESOLVED FIXED in Firefox 44

Status

()

Firefox for Android
General
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: jonalmeida, Assigned: jonalmeida)

Tracking

43 Branch
Firefox 44
Points:
---

Firefox Tracking Flags

(firefox44 verified)

Details

MozReview Requests

()

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(4 attachments)

(Assignee)

Description

2 years ago
Created attachment 8677154 [details]
screenshot.png

Is it possible to hide the path where the placeholder image is stored? It looks a bit fugly.

(See screenshot)
(Assignee)

Comment 1

2 years ago
NI for feedback.
Flags: needinfo?(mark.finkle)
Let's look at how the contextmenu get's it's title:

_innerShow is the place where we use a Prompt to display the menu:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/browser.js#2977

_innerShow calls _findTitle, but only uses the title if the contextmennu is not a "tabbed" menu:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/browser.js#2981

_findTitle walks the DOM tree looking for a suitable title, calling _getTitle:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/browser.js#2891

_getTitle looks for a "title" attribute, or falls back to a form of URL, calling _getUrl:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/browser.js#2827

_getUrl looks at several different ways to get a URL based on the element type. We care about images:
http://mxr.mozilla.org/mozilla-central/source/mobile/android/chrome/content/browser.js#2836

You could add some logic in that code block to look for our special attribute and return the real URL:
node.getAttribute("data-ctv-src")
Flags: needinfo?(mark.finkle)
(Assignee)

Comment 3

2 years ago
Created attachment 8678322 [details] [diff] [review]
Hide placeholder svg path in context menu when unblocking image
(Assignee)

Updated

2 years ago
Assignee: nobody → jonalmeida942
Status: NEW → ASSIGNED
(Assignee)

Comment 4

2 years ago
Thanks for the walk through, that was really helpful! Less afraid of browserjs now, but I don't know if it's best to modify browserjs for this specific use case.

I've added the logic into ImageBlockingPolicy.js instead.
(Assignee)

Updated

2 years ago
Attachment #8678322 - Flags: feedback?(mark.finkle)
Comment on attachment 8678322 [details] [diff] [review]
Hide placeholder svg path in context menu when unblocking image

Let's not create more temporary attributes. This is a real feature now, so don't be afraid to add code to browser.js

It will only be a 2 or 3 lines of code, and you might be adding more to disable other menus.
Attachment #8678322 - Flags: feedback?(mark.finkle) → feedback-
(Assignee)

Comment 6

2 years ago
Created attachment 8678366 [details]
MozReview Request: Bug 1217232 - Hide placeholder svg path in context menu when unblocking image r?mfinkle

Bug 1217232 - Hide placeholder svg path in context menu when unblocking image r?mfinkle
Attachment #8678366 - Flags: review?(mark.finkle)
Comment on attachment 8678366 [details]
MozReview Request: Bug 1217232 - Hide placeholder svg path in context menu when unblocking image r?mfinkle

https://reviewboard.mozilla.org/r/23185/#review20667

::: mobile/android/chrome/content/browser.js:2836
(Diff revision 1)
> +        var blockedImageUrl = node.getAttribute("data-ctv-src");

Use 'let' instead of 'var'
Change 'blockedImageUrl' to  'originalURL'
Attachment #8678366 - Flags: review?(mark.finkle) → review+
(Assignee)

Comment 8

2 years ago
https://reviewboard.mozilla.org/r/23185/#review20667

Fixed comments.
(Assignee)

Comment 9

2 years ago
Comment on attachment 8678366 [details]
MozReview Request: Bug 1217232 - Hide placeholder svg path in context menu when unblocking image r?mfinkle

Bug 1217232 - Hide placeholder svg path in context menu when unblocking image r?mfinkle
Attachment #8678366 - Flags: review+ → review?(mark.finkle)
(Assignee)

Updated

2 years ago
Attachment #8678366 - Flags: review?(mark.finkle) → review+
(Assignee)

Updated

2 years ago
Keywords: checkin-needed

Comment 10

2 years ago
https://hg.mozilla.org/integration/fx-team/rev/60d33952d4ca
Keywords: checkin-needed

Comment 11

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/60d33952d4ca
Status: ASSIGNED → RESOLVED
Last Resolved: 2 years ago
status-firefox44: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 44
Created attachment 8679920 [details]
Screenshot_2015-10-28-12-14-06.png

Verified as fixed using:
Build: Firefox 44.0a1 (2015-10-27)
Device: Samsung S5 (Android 4.4.2)
status-firefox44: fixed → verified
You need to log in before you can comment on or make changes to this bug.