1370228 - Context menu title with placeholder always shows ellipsis

Status: RESOLVED FIXED

(WebExtensions :: Frontend, defect, P5)

Description

[sblask]

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.

Comment 1

[Vinayaka Kamath]

Hello guys Im new to open source is this guy free to take? Shall I start working on it?

Comment 2

[Caitlin Neiman [:caitmuenster]]

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

[Vinayaka Kamath]

This bug doesnt have a language tag?? Where can I find the source code? Please can someone help me?

Comment 4

[Caitlin Neiman [:caitmuenster]]

Shane, would you be able to help Vinayaka out with this?

Comment 5

[Shane Caraveo (:mixedpuppy)]

Hi Vinayaka,
Are you still interested in working on this?
Regards,
Shane

Comment 6

[Vinayaka Kamath]

Ya sure I tried working on it but failed miserably. I think I might need an enormous support I guess.

Comment 7

[Shane Caraveo (:mixedpuppy)]

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

Comment 9

[Shane Caraveo (:mixedpuppy)]

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

Comment 10

[Vinayaka Kamath]

Attachment: Removed the "..." in the ellipses

Please review and tell corrections. I haven't committed anything onto the tree.

Comment 11

[Shane Caraveo (: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?

Comment 12

[Vinayaka Kamath]

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

Comment 13

[Vinayaka Kamath]

Attachment: Added the ellipsis only when required. Max selection length is 15 now.

Please review the attachment. Added ellipsis only when required.

Comment 14

[Shane Caraveo (: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.

Comment 15

[Vinayaka Kamath]

Attachment: Got the localized ellipsis and max size of the selection is 15.

Work on the test cases is pending. I'll resume it now.

Comment 16

[Shane Caraveo (: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.

Comment 17

[Vinayaka Kamath]

Attachment: Used Services.prefs.getComplexValue for getting localized ellipsis

I found one test which contains tests menus.

Comment 18

[Vinayaka Kamath]

(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.

Comment 19

[Shane Caraveo (:mixedpuppy)]

(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 20

[Shane Caraveo (:mixedpuppy)]

Comment on attachment 8897976 [details] [diff] [review]
Used  Services.prefs.getComplexValue for getting localized ellipsis

waiting on test updates

Comment 21

[Caitlin Neiman [:caitmuenster]]

Hey Vinayaka, how's it going with this bug?

Comment 22

[Daniel]

Hey, I'd also like to know if someone is looking into this, if anyone can answer. Thanks!

Comment 23

[Caitlin Neiman [:caitmuenster]]

Hey Vinayaka, we're going to open this bug up to another contributor to work on.

Comment 24

[Lukas Jung]

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.

Comment 25

[Andrew Swan [:aswan]]

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.

Comment 26

[Lukas Jung]

Attachment: Lukas' approach

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.

Comment 27

[Shane Caraveo (: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.

Comment 28

[Lukas Jung]

Attachment: only complete label length truncation but with tests.

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.

Comment 29

[Shane Caraveo (: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!

Comment 30

[Lukas Jung]

(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.

Comment 31

[Shane Caraveo (: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.

Comment 32

[Lukas Jung]

I also cannot add keywords. 
Guess you have to do that. Thanks in advance!

Comment 33

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/8882102c7640
"Context menu title with placeholder always shows ellipsis" [r=mixedpuppy]

Comment 34

[Stefan Hindli [:stefan_hindli]]

https://hg.mozilla.org/mozilla-central/rev/8882102c7640

Comment 35

[marius.santa]

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.

Comment 36

[Mike Conca [:mconca]]

Post-resolution assignment, to make sure contributors get credit.