Context menu title with placeholder always shows ellipsis

RESOLVED FIXED in Firefox 59

Status

P5
normal
RESOLVED FIXED
2 years ago
9 months ago

People

(Reporter: blask, Assigned: lukas.jung, Mentored)

Tracking

({good-first-bug})

53 Branch
mozilla59
good-first-bug
Bug Flags:
qe-verify -

Firefox Tracking Flags

(firefox59 fixed)

Details

(Whiteboard: triaged)

Attachments

(6 attachments)

(Reporter)

Description

2 years ago
User Agent: Mozilla/5.0 (X11; Ubuntu; Linux x86_64; rv:53.0) Gecko/20100101 Firefox/53.0
Build ID: 20170524180630

Steps to reproduce:

Create a webextension with context menu with the following:

    browser.contextMenus.create({
        id: "id",
        parentId: "parent",
        title: "Add '%s'",
        contexts: ["selection"],
    });

Run the extension, select one letter on a page and open the context menu.


Actual results:

The context menu contains "Add 'L...'"


Expected results:

The context menu should contain "Add 'L'". An ellipsis should only be shown for text that would otherwise overflow the context menu.
Component: Untriaged → WebExtensions: Frontend
Product: Firefox → Toolkit

Updated

2 years ago
Priority: -- → P5
Whiteboard: triaged

Updated

2 years ago
Keywords: good-first-bug
Mentor: mixedpuppy

Comment 1

2 years ago
Hello guys Im new to open source is this guy free to take? Shall I start working on it?
Hey Vinayaka! Welcome! 

Please feel free to go ahead and start working on this. If you need any help, please do leave a comment or reach out to the mentor. :)

Comment 3

2 years ago
This bug doesnt have a language tag?? Where can I find the source code? Please can someone help me?
Shane, would you be able to help Vinayaka out with this?
Flags: needinfo?(mixedpuppy)
Hi Vinayaka,
Are you still interested in working on this?
Regards,
Shane
Flags: needinfo?(vinayakkamath2010)

Comment 6

2 years ago
Ya sure I tried working on it but failed miserably. I think I might need an enormous support I guess.
Flags: needinfo?(vinayakkamath2010)
Hi Vinayaka,
I've sent you an email directly, I think it will be easier to help you that way rather than through bug comments.
Shane
Duplicate of this bug: 1383527
I think that bug 1367165 can also be fixed in conjunction with this, and may be a contributor to this.

See http://searchfox.org/mozilla-central/source/browser/components/extensions/ext-menus.js#173-189
Flags: needinfo?(mixedpuppy)
See Also: → bug 1367165

Comment 10

2 years ago
Please review and tell corrections. I haven't committed anything onto the tree.
Flags: needinfo?(mixedpuppy)
Attachment #8897920 - Flags: review?(mixedpuppy)
Comment on attachment 8897920 [details] [diff] [review]
Removed the "..." in the ellipses

The intention here is to truncate the label if it is too long, in which case we do want the ellipses.  However, the ellipses is wrong, it should be the ellipsis character like we do here:

http://searchfox.org/mozilla-central/source/browser/base/content/nsContextMenu.js#126-130

"maxSelectionLength - 3" was done to accommodate the incorrect ellipsis and would be wrong as well after fixing the ellipsis character.

The calculation in general is wrong.  If label.length + selection.length is greater than the max length, we need to truncate the selection.  The selection should be truncated to selection.length - label.length - ellipsis.length.  I also think that the max selection length used in any case should be 15 characters as we do in nsContextMenu for the search menu.

So you probably want something like:

if selection.length > 15 or label.length + selection.length > maxLength then truncate

You dont need to do both r? and ni?, I'll see the r?
Flags: needinfo?(mixedpuppy)
Attachment #8897920 - Flags: review?(mixedpuppy) → review-

Comment 12

2 years ago
Ok Now I completely understand the problem. I'll try to fix this as soon as possible.

Comment 13

2 years ago
Please review the attachment. Added ellipsis only when required.
Attachment #8897953 - Flags: review?(mixedpuppy)
Comment on attachment 8897953 [details] [diff] [review]
Added the ellipsis only when required. Max selection length is 15 now.

>diff --git a/browser/components/extensions/ext-menus.js b/browser/components/extensions/ext-menus.js
>--- a/browser/components/extensions/ext-menus.js
>+++ b/browser/components/extensions/ext-menus.js
>@@ -183,9 +183,13 @@ var gMenuBuilder = {
>         // But if it makes sense let's try truncate selection text only, to handle cases like
>         // 'look up "%s" in MyDictionary' more elegantly.
>         let maxSelectionLength = gMaxLabelLength - label.length + 2;
>-        if (maxSelectionLength > 4) {
>-          selection = selection.substring(0, maxSelectionLength - 3) + "...";
>+        let ellipsis = "\u2026";

You still need to get the localized ellipsis if it is set (see nsContextMenu again)

>+        if (label.length + selection.length - 2 > gMaxLabelLength) {
>+          selection = selection.substring(0, maxSelectionLength - selection.length - ellipsis.length) + ellipsis;

Now that I look at it more, I think we can actually just get rid of this part completely and rely only on truncating selection if it is longer than 15 as below.

>         }
>+     	else if(selection.length > 15){
>+     		selection = selection.substring(0, selection.length - ellipsis.length) + ellipsis;

This is not truncating to 15.  I think we can just copy the logic here:

http://searchfox.org/mozilla-central/source/browser/base/content/nsContextMenu.js#1908-1917

We also need to look for any tests that may have been done for this and fix those.  Look for any menu tests in browser/components/extensions/test/ and see if any test the truncation.  If not, we'll want to add one.
Attachment #8897953 - Flags: review?(mixedpuppy)

Comment 15

2 years ago
Work on the test cases is pending. I'll resume it now.
Attachment #8897970 - Flags: review?(mixedpuppy)
Comment on attachment 8897970 [details] [diff] [review]
Got the localized ellipsis and max size of the selection is 15.

>diff --git a/browser/components/extensions/ext-menus.js b/browser/components/extensions/ext-menus.js
>--- a/browser/components/extensions/ext-menus.js
>+++ b/browser/components/extensions/ext-menus.js
>@@ -183,9 +183,19 @@ var gMenuBuilder = {
>         // But if it makes sense let's try truncate selection text only, to handle cases like
>         // 'look up "%s" in MyDictionary' more elegantly.
>         let maxSelectionLength = gMaxLabelLength - label.length + 2;

We can get rid of that line.  Can probably also remove the declaration of gMaxLabelLength if it is not used anywhere else.

>-        if (maxSelectionLength > 4) {
>-          selection = selection.substring(0, maxSelectionLength - 3) + "...";
>+        let ellipsis = "\u2026";
>+        try {
>+          ellipsis = gPrefService.getComplexValue("intl.ellipsis",
>+                                                   Ci.nsIPrefLocalizedString).data;

Lets use the more modern version: Services.prefs.getComplexValue

Looking good.
Attachment #8897970 - Flags: review?(mixedpuppy)
Assignee: nobody → vinayakkamath2010

Comment 17

2 years ago
I found one test which contains tests menus.
Attachment #8897976 - Flags: review?(mixedpuppy)

Comment 18

2 years ago
(In reply to Vinayaka Kamath from comment #17)
> Created attachment 8897976 [details] [diff] [review]
> Used  Services.prefs.getComplexValue for getting localized ellipsis
> 
> I found one test which contains tests menus.

http://searchfox.org/mozilla-central/source/browser/components/extensions/test/browser/browser_ext_menus.js

I hope this is the right one and I didn't find that it tests for truncation anywhere.
(In reply to Vinayaka Kamath from comment #18)

> I hope this is the right one and I didn't find that it tests for truncation
> anywhere.

I sent you an email with more details on what file to fix for tests.
Comment on attachment 8897976 [details] [diff] [review]
Used  Services.prefs.getComplexValue for getting localized ellipsis

waiting on test updates
Attachment #8897976 - Flags: review?(mixedpuppy)
Hey Vinayaka, how's it going with this bug?

Comment 22

a year ago
Hey, I'd also like to know if someone is looking into this, if anyone can answer. Thanks!
Hey Vinayaka, we're going to open this bug up to another contributor to work on.
Assignee: vinayakkamath2010 → nobody
(Assignee)

Comment 24

a year ago
Hey,
since noone seems to be working on this anymore, I'd like to do this one as my first bug.

I already solved the issue but in a different way to preserve the original logic
instead of truncating after 15 chars. (That seemed to be the last approach).

This is important with the 15 chars solution the label still can exceed 64 chars.
Then something like this can happen: "Do a task that is difficult to discribe and add 'selected Te...' to List no. 3..."

It'd be awesome if you could help me contribute to an open source project for the first time.
Excellent, thanks for your interest.  It sounds like you already have created a patch that addresses the bug?  If that's true, the next step is to post it here as an attachment to this bug and ask for it to be reviewed.  It looks like :mixedpuppy is the mentor for this bug, so you can do that by going to the details page for the attachment (after you've uploaded the patch), then setting the review flag to ? and typing :mixedpuppy into the text box.

If you don't have a patch ready but have other questions, you can just ask them here, you can use the "Need more information from" option at the bottom of the page to make sure somebody notices your question.
(Assignee)

Comment 26

a year ago
As said before I added the 64 char limit for the complete label again.
To manage the weird utf stuff I used Array.from() to separate the codepoints and joined them later.
Attachment #8937692 - Flags: review?(mixedpuppy)
Hi Lukas,
I'm going to want to see a test before reviewing this.  I did look, it looks ok but I haven't thought it through yet.  Here's an email I sent before to help with that, though it's been some time so if that test has changed line numbers may be different:

First, to run tests you can do the following

./mach test path/to/tests

Specifically for menu tests, you could run

./mach test browser/components/extensions/test/browser/browser_ext_contextMenus.js

The tests may run twice, once for in-process and once for remote process.  You can run the test on only one to shorten the time while you're working:

./mach mochitest --tag remote-webextensions browser/components/extensions/test/browser/browser_ext_contextMenus.js

There are a bunch of browser_ext_contextMenus tests in that directory.

It looks like there is a test for selection here:

http://searchfox.org/mozilla-central/source/browser/components/extensions/test/browser/browser_ext_contextMenus.js#93-99

It is probably easiest to modify the test to use a long selection, then test the title.

The text is being selected here:

http://searchfox.org/mozilla-central/source/browser/components/extensions/test/browser/browser_ext_contextMenus.js#252-262

It looks like 100 characters are being selected, so that is good.

And it looks like the label is being tested in this area:

http://searchfox.org/mozilla-central/source/browser/components/extensions/test/browser/browser_ext_contextMenus.js#284-334

So what I would do is run that test and see what fails, then fix the failures.  You can also see that "..." is used rather than the ellipsis character in the tests, so you'll need to address that.  I think you can just replace "..." with the unicode character \u2026 for the test.


Also, be sure to run ./mach eslint path/to/file for any files you edit before uploading the patch.  That will warn you of any formatting errors.
Flags: needinfo?(lukas.jung)
Attachment #8937692 - Flags: review?(mixedpuppy)
(Assignee)

Comment 28

a year ago
If we added the 16 chars limit for the selection (15 + ellipsis), we'd have to write more tests.
So this patch actually does what the code before intended to do and replaced the three dots with the ellipsis. (code and test)
The test passed and the eslint didn't find any problems.

Don't know if I'm able to write a new test for the 16 chars selection limit.
At least it would take some time/help.

But this patch should at least fix the one letter selection with three dots problem.
Flags: needinfo?(lukas.jung)
Attachment #8938392 - Flags: review?(mixedpuppy)
Comment on attachment 8938392 [details] [diff] [review]
only complete label length truncation but with tests.

I think you might need 

codePointsToRemove = completeLabelLength - gMaxLabelLength + 1

to account for adding the ellipsis.  Please verify and update the patch.  Once done, you can add the checkin-needed keyword.

Thanks!
Attachment #8938392 - Flags: review?(mixedpuppy) → review+
(Assignee)

Comment 30

a year ago
(In reply to Shane Caraveo (:mixedpuppy) from comment #29)
> Comment on attachment 8938392 [details] [diff] [review]
> only complete label length truncation but with tests.
> 
> I think you might need 
> 
> codePointsToRemove = completeLabelLength - gMaxLabelLength + 1
> 
> to account for adding the ellipsis.  Please verify and update the patch. 
> Once done, you can add the checkin-needed keyword.
> 
> Thanks!

If I had done what you described, there would be the same issue again.
The variable is incremented before adding the ellipsis.

Please look at it again.
Flags: needinfo?(mixedpuppy)
(In reply to Lukas Jung from comment #30)

> The variable is incremented before adding the ellipsis.

Ahh, I missed that when I was looking at how much was being removed.
Flags: needinfo?(mixedpuppy)
(Assignee)

Comment 32

a year ago
I also cannot add keywords. 
Guess you have to do that. Thanks in advance!
Flags: needinfo?(mixedpuppy)
Flags: needinfo?(mixedpuppy)
Keywords: checkin-needed

Comment 33

a year ago
Pushed by aciure@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8882102c7640
"Context menu title with placeholder always shows ellipsis" [r=mixedpuppy]
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/8882102c7640
Status: UNCONFIRMED → RESOLVED
Last Resolved: a year ago
status-firefox59: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla59

Comment 35

a year ago
Is manual testing required on this bug? If Yes, please provide some STR and the proper webextension(if required), if No set the “qe-verify-“ flag.
Flags: needinfo?(mixedpuppy)
Flags: needinfo?(mixedpuppy) → qe-verify-
Post-resolution assignment, to make sure contributors get credit.
Assignee: nobody → lukas.jung
Duplicate of this bug: 1367165

Updated

9 months ago
Product: Toolkit → WebExtensions
You need to log in before you can comment on or make changes to this bug.