Closed Bug 1406051 Opened 3 years ago Closed 3 years ago

Double context menu separator on image link when selecting text

Categories

(Firefox :: Menus, defect, P4)

58 Branch
defect

Tracking

()

VERIFIED FIXED
Firefox 58
Tracking Status
firefox-esr52 --- unaffected
firefox56 --- wontfix
firefox57 --- wontfix
firefox58 --- verified

People

(Reporter: kanatay01, Assigned: eoger)

References

()

Details

(Keywords: regression)

Attachments

(2 files)

Attached image double_separator.jpg
User Agent: Mozilla/5.0 (Windows NT 10.0; WOW64; rv:58.0) Gecko/20100101 Firefox/58.0
Build ID: 20171005100211

Steps to reproduce:

1. Select text.
2. Open the context menu by clicking on image link (e.g. on <a href=""><img src=""></a>).
Severity: normal → minor
Component: Untriaged → Menus
Blocks: 1359894
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: needinfo?(eoger)
Keywords: regression
Priority: -- → P4
Fairly minor regression, doesn't affect functionality, marking wontfix for 56.
Assignee: nobody → eoger
Flags: needinfo?(eoger)
Comment on attachment 8917142 [details]
Bug 1406051 - Don't show two separators in context menu.

https://reviewboard.mozilla.org/r/188150/#review194176

We should have a test for this. You should be able to add to browser/base/content/test/general/browser_contextmenu.js

::: browser/base/content/browser-sync.js:469
(Diff revision 1)
>                                contextMenu.onImage || contextMenu.onCanvas ||
>                                contextMenu.onVideo || contextMenu.onAudio ||
>                                contextMenu.onLink || contextMenu.onTextInput);
>  
> -    ["context-sendpagetodevice", "context-sep-sendpagetodevice"]
> +    // Avoids double separator on images with links.
> +    const hideSeparator = contextMenu.isContentSelected && contextMenu.onLink && contextMenu.onImage;

Can you break this up into multiple lines?
Attachment #8917142 - Flags: review?(jaws) → review+
I don't have a strong opinion if we should uplift this to 57, it's a fairly minor visual regression. What do you think Liz?
Pushed by eoger@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/4730ce3175f8
Don't show two separators in context menu. r=jaws
https://hg.mozilla.org/mozilla-central/rev/4730ce3175f8
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 58
Given where we are in the cycle, I think this should probably ride the 58 train, but feel free to set the 57 status back to affected and request Beta approval if you feel strongly otherwise.
I have reproduced this bug with Nightly 58.0a1 (2017-10-08) on Ubuntu 16.04, 64 bit!

The fix is now verified on Latest Nightly 58.0a1.

Build ID 	20171022100058
User Agent 	Mozilla/5.0 (X11; Linux x86_64; rv:58.0) Gecko/20100101 Firefox/58.0
QA Whiteboard: [bugday-20171018]
Thanks for the verification. Updating the tracking flags and bug status.
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.