1196395 - No plurals form for context menus when multiple pages are selected in Library

Status: RESOLVED FIXED

(Firefox :: Bookmarks & History, defect)

Description

[scootergrisen]

User Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:40.0) Gecko/20100101 Firefox/40.0
Build ID: 20150812163655

Steps to reproduce:

Go to Library.
Select one page in the list and right click.
Says "Delete This Page" in the context menu.
Select multiple pages in the list and right click.



Actual results:

Says "Delete This Page" in the context menu.


Expected results:

Maybe change to "Delete Page" or maybe "Delete Page(s)".

Or maybe "Delete Pages"/"Delete These Pages" when multiple pages is selected and "Delete Page"/"Delete This Page" when single page is selected.

Same with "Bookmark This Page...".

Comment 1

[Harshal Lele]

i would like to work on this, but it's my first bug.Can you help me with this?

Comment 2

[YF (Yang)]

(In reply to Harshal Lele from comment #1)
> i would like to work on this, but it's my first bug.Can you help me with
> this?

https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide
https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/How_to_Submit_a_Patch

https://developer.mozilla.org/en-US/docs/Mozilla/Localization/Localization_and_Plurals
https://dxr.mozilla.org/mozilla-central/search?q=%22Delete+This+Page%22 and othre strings.

Comment 3

[Harshal Lele]

Attachment: bug1196395_RenamedLabel.patch

The expected action mentioned in the bug was to pluralise the label and change it to something like "Delete these Pages". But in Firefox 44  and the latest nightly, this bug only happens when pages in the History section are selected.If pages from the Bookmarks section are selected,the option in the Context Menu is labeled "Delete". So, in order to be more consistent with those other places in the Library, i changed the label in the History section to "Delete".

Comment 4

[Marco Bonardo [:mak]]

Comment on attachment 8721620 [details] [diff] [review]
bug1196395_RenamedLabel.patch

Review of attachment 8721620 [details] [diff] [review]:
-----------------------------------------------------------------

The reason this contextual menu says "Delete this page" instead of "Delete" is because we completely remove the page from history, Delete could have made the user think we are only removing a single visit from "today" or whatever container the page is.
BUT, honestly, it *never* worked. I got plenty of bugs from users who thought we were just removing from a single container.

Users keep being confused about what removing a page from history does, so maybe we should just go the easy way and name it "Delete" as you suggest, or we should use something more precise. The risk going more precise though is that it will be even more confusing with "Forget about this site"...

I think we need someone from UX and evaluate if we can just simplify the string, or they have a better idea of how to express "Completely remove this page from history" that cannot be confused with "Forget about this site".

::: browser/locales/en-US/chrome/browser/places/places.dtd
@@ -42,5 @@
>  <!ENTITY cmd.restoreFromFile.accesskey  "C">
>  
>  <!ENTITY cmd.bookmarkLink.label         "Bookmark This Page…">
>  <!ENTITY cmd.bookmarkLink.accesskey     "B">
> -<!ENTITY cmd.delete.label               "Delete This Page">

From a technical point of view, when you change a string you should also change the entity name. Any kind of change, for example in this case you could just rename it to cmd.delete.label2 since.
The reason is that localizers tools cannot notice when an entity changes, but they can notice when an entity is removed and another one is added.

Comment 5

[(Currently slow to respond) Philipp Sackl [:phlsa] (Firefox UX) please use needinfo]

Comment on attachment 8721620 [details] [diff] [review]
bug1196395_RenamedLabel.patch

We could use »Remove page from history« instead, which would indicate that the action is more global. This would mean that we'd still need a pluralized version to fix this specific bug though.

Michelle, what's your opinion on this?

Comment 6

[Marco Bonardo [:mak]]

(In reply to Philipp Sackl [:phlsa] (Firefox UX) please use needinfo from comment #5)
> Comment on attachment 8721620 [details] [diff] [review]
> bug1196395_RenamedLabel.patch
> 
> We could use »Remove page from history« instead, which would indicate that
> the action is more global. This would mean that we'd still need a pluralized
> version to fix this specific bug though.

What about just "Remove from history"? There is a selection visible the user right clicked on so I don't think we need to repeat what we are removing?

Comment 7

[(Currently slow to respond) Philipp Sackl [:phlsa] (Firefox UX) please use needinfo]

Yeah, ere's what Michelle (our content strategist) sent me. It goes into a similar direction:

---
It appears that the browser displays different options depending on whether you have highlighted one link or more than one.  My recommendations are to use the same verb in both places to help a user differentiate between their action affecting one page or an entire site, as follows:

if single link is highlighted, options are:
Delete Page
Delete Site

if two or more links are highlighted from same domain , display
Delete Pages
Delete Site

if two more links are highlighted from more than one domain, display only
Delete Pages

Also note the issue with the word "Page" in the Bookmark This Page menu item. Can you also update as follows:

if single link is highlighted, display:
Bookmark Page

if two or more links are highlighted, display
Bookmark Pages

(so, removing "This" and the ellipsis)

LMK if you have any questions.
---

Comment 8

[Deepjyoti Mondal]

Hi, I would like to work on this bug. Can you please point out the files where I need to make changes.

Comment 9

[Marco Bonardo [:mak]]

Imo, this is not completely actionable yet.

Comment 7 is a good start, but it doesn't take into account various problems:
1. views are grouped into containers, "Delete Page" doesn't clarify that the deletion will involve all the containers and not just the selected one. That's why I suggested "Delete Page from History"
2. Delete Page and Delete Site are doing totally different actions, the former is removing history, the latter is removing Everything (cookies, form history, passwords, history, ...) for the site. I really think we should not use the same verb there.

On the other side I agree with the idea to remove "this" and move to plural forms where needed though, that's something that could be done even now.

Delete this page and Bookmark this page are at http://searchfox.org/mozilla-central/source/browser/locales/en-US/chrome/browser/places/places.dtd#44 and on

though, to use plural forms, you'll have to move those to http://searchfox.org/mozilla-central/source/browser/locales/en-US/chrome/browser/places/places.properties and add some code to manage plurals. See
https://developer.mozilla.org/en-US/docs/Mozilla/Localization/Localization_and_Plurals#Developing_with_PluralForm (Note, you can use PlacesUtils.getPluralString)
This should happen either when we build the context menu, or when we update commands.
Most of the relevant code for those is in http://searchfox.org/mozilla-central/source/browser/components/places/content/controller.js

Comment 10

[Deepjyoti Mondal]

Hi Marco, I went through those files,especially controller.js. I checked the buildContextMenu function but I cannot find where the content of the context menuitem labels are being set.I think the number of selected items needs to be checked before setting the label for 'delete this/these page/pages' context menu item.Can you please tell me where i will find the code for these.

Comment 11

[Marco Bonardo [:mak]]

(In reply to Deepjyoti Mondal from comment #10)
> Hi Marco, I went through those files,especially controller.js. I checked the
> buildContextMenu function but I cannot find where the content of the context
> menuitem labels are being set.

The labels are set through entities from the dtd file currently, you won't find code that does that. But to properly support plural forms we need code, so we must add such code.

> I think the number of selected items needs to
> be checked before setting the label for 'delete this/these page/pages'
> context menu item.

right. I think metadata we get from buildSelectionMetadata is an array and you could reuse it's .length for that. But I didn't test it.

Comment 12

[Mathan Kumar]

Hi, I'd like to fix this bug as my first bug.
I have setup the build environment for Firefox and made the changes for plural form. Shall I create the patch and send it for review ?

Comment 13

[Pavan Karthik [:matrixisreal]]

Hi, I would like to work on this bug,do assign it to me if you have noone else to.
Thanks.

Comment 14

[Pavan Karthik [:matrixisreal]]

I have gone through the above discussion and have few doubts.
1.In the step one if  steps to reproduce,what does library mean.
2.Is it possible to reproduce this bug on a Nightly build on Linux machine?
3.Is the requirement set for the patch same as the Comment7

Comment 15

[Pavan Karthik [:matrixisreal]]

I've reproduced the bug,so no need to clarify 1 and 2 in the above comment. :D

Comment 16

[Pavan Karthik [:matrixisreal]]

And I could use some help to understand the codeflow and also on how to use JS debuggers like firebug to debug the firefox code.
Thanks

Comment 17

[fiveNinePlusR]

(In reply to Pavan Karthik from comment #16)
> And I could use some help to understand the codeflow and also on how to use
> JS debuggers like firebug to debug the firefox code.
> Thanks

To get started with debugging firefox itself you can use the browser toolbox:

https://developer.mozilla.org/en-US/docs/Tools/Browser_Toolbox

to get familiar with the buildContextMenu function I would add a breakpoint to the first line in the function and trigger the breakpoint to stop by using firefox. Once it's stopped on the breakpoint I would step through each line and see what's going on. inspect the variables in scope as well and get an idea of what happens typically.

Comment 18

[Pavan Karthik [:matrixisreal]]

Hi,
I need some help to understand the codeflow

1)While debugging I found that the call stack to buildcontextmenu() is ---
    PC_buildContextMenu@chrome://browser/content/places/controller.js:596
    buildContextMenu@chrome://browser/content/places/tree.xml:682
    onpopupshowing@chrome://browser/content/places/places.xul:1
2)AFIU http://searchfox.org/mozilla-central/source/browser/components/places/content/placesOverlay.xul#197 
is the popups xul code and
http://searchfox.org/mozilla-central/source/browser/locales/en-US/chrome/browser/places/places.dtd#46             has its entites 

But I couldnt understand how the places.xul and placesOverlay.xul are related.
Please give some insights on where to add the code.

It would be very helpful if you could suggest some time to discuss this on IRC
Thanks :D

Comment 19

[Marco Bonardo [:mak]]

(In reply to Pavan Karthik from comment #18)
> But I couldnt understand how the places.xul and placesOverlay.xul are
> related.

placesOverlay.xul is an overlay that is injected in all the windows and provides things used all around in the ui like places modules, the context menu, the places tooltips...
places.xul instead is the Library window. Being a window it also includes placesOverlay.xul, so it can use the places context menu.

Comment 20

[Pavan Karthik [:matrixisreal]]

Attachment: v1.patch

Delete/bookmark page if one page is selected
Delete/Bookmark pages else.
Any better-ideas/suggestions?

Comment 21

[Marco Bonardo [:mak]]

Comment on attachment 8818726 [details] [diff] [review]
v1.patch

Review of attachment 8818726 [details] [diff] [review]:
-----------------------------------------------------------------

::: browser/components/places/content/controller.js
@@ +13,5 @@
> +                                  "resource://gre/modules/PluralForm.jsm");
> +
> +XPCOMUtils.defineLazyGetter(this, "gPlacesBundle", function() {
> +  return Services.strings.createBundle('chrome://browser/locale/places/places.properties');
> +});

You should be able to use PlacesUIUtils.getPluralString that will do some of the work for you and will internally already use these modules.

::: browser/components/places/content/placesOverlay.xul
@@ +160,5 @@
>                hideifnoinsertionpoint="true"/>
>      <menuseparator id="placesContext_newSeparator"/>
>      <menuitem id="placesContext_createBookmark"
>                command="placesCmd_createBookmark"
> +              label=""

I think you can completely remove these empty labels.

Comment 22

[Pavan Karthik [:matrixisreal]]

Attachment: v1.1.patch

I have a small doubt:
Where is the PlacesUIUtils Module Loaded in the javascript?

Comment 23

[Pavan Karthik [:matrixisreal]]

Hi
I think there is a similar bug:
Open Library and goto Downloads,
Now the context menu has the Labels "Clear Download" and "Copy Download Link",
irrespective of the number of items selected.So, may be this has to be fixed too?
May I report it as a bug?
Thanks :D

Comment 24

[Pavan Karthik [:matrixisreal]]

Also, please assign this bug to me, if u forgot:P

Comment 25

[Marco Bonardo [:mak]]

(In reply to Pavan Karthik from comment #23)
> Hi
> I think there is a similar bug:
> Open Library and goto Downloads,
> Now the context menu has the Labels "Clear Download" and "Copy Download
> Link",
> irrespective of the number of items selected.So, may be this has to be fixed
> too?

Sounds like it's a bug, but I'm not sure if the labels are wrong, or rather if we are enabling actions that should not apply to multiple entries.

FWIW, Clear Downloads is plural regardless, cause it's a generic option that applies to all the downloads. So that's correct.
Open Containing Folder, Go to Download Page and Copy Download Link could all be singular/plural.

I'm not sure what's the use case to support multiple selections for these, or in general for this panel. If we care to support it, some of these actions should get plural form, some should be disabled (what's the use case to copy multiple download links to clipboard?).
It's probably worth to file _a separate bug_ to figure out IF we want multi-selection, and if so which actions are expected to apply to multi-selection. Based on the outcome it should be possible to fix the plural forms.

Comment 26

[Marco Bonardo [:mak]]

(In reply to Pavan Karthik from comment #22)
> I have a small doubt:
> Where is the PlacesUIUtils Module Loaded in the javascript?

http://searchfox.org/mozilla-central/rev/a6a339991dbc650bab3aefe939cd0e5ac302274a/browser/components/places/content/placesOverlay.xul#31
This overlay is loaded in all the windows containing a Places view.

Comment 27

[Marco Bonardo [:mak]]

Comment on attachment 8819386 [details] [diff] [review]
v1.1.patch

Review of attachment 8819386 [details] [diff] [review]:
-----------------------------------------------------------------

Sorry, some more changes required. But it looks good overall.

::: browser/components/places/content/controller.js
@@ +654,5 @@
>          }
>        }
>      }
>  
> +    // Make sure correct PluralForms are diplayed when multiple pages are selected in the Histroy Library.

Just a generic:
// Make sure correct PluralForms are diplayed when multiple entries are selected.
only here, don't need to repeat the comment just a few lines below.

@@ +655,5 @@
>        }
>      }
>  
> +    // Make sure correct PluralForms are diplayed when multiple pages are selected in the Histroy Library.
> +    var deleteHistoryItem = document.getElementById("placesContext_delete_history");

s/Item/Elt/. This is just because Places code has too many things called Item (it usually refers to a bookmark)!
nit: new code tends to use let, rather than var, for better scoping.

::: browser/locales/en-US/chrome/browser/places/places.dtd
@@ +44,1 @@
>  <!ENTITY cmd.bookmarkLink.accesskey     "B">

I asked for feedback to flod, he suggested to also move the accesskeys to the .properties, so they are closer to the labels they refer to.
This clearly also means setting the accesskeys from js.

::: browser/locales/en-US/chrome/browser/places/places.properties
@@ +92,5 @@
>  lockPromptInfoButton.accessKey=L
> +
> +# LOCALIZATION NOTE (deletePagesLabel): Semi-colon list of plural forms.
> +# See: http://developer.mozilla.org/en/docs/Localization_and_Plurals
> +cmd.deletePagesLabel=Delete Page;Delete Pages

cmd.deletePages.label (per coding style, and accesskeys will be .accesskey)

@@ +96,5 @@
> +cmd.deletePagesLabel=Delete Page;Delete Pages
> +
> +# LOCALIZATION NOTE (bookmarkPagesLabel): Semi-colon list of plural forms.
> +# See: http://developer.mozilla.org/en/docs/Localization_and_Plurals
> +cmd.bookmarkPagesLabel=Bookmark Page;Bookmark Pages

cmd.bookmarkPages.label

Comment 28

[Pavan Karthik [:matrixisreal]]

Attachment: v1.2.patch

I hope this looks good now?

Comment 29

[Pavan Karthik [:matrixisreal]]

After the v1.2 patch is applied I see that the access keys in "Bookmark pages" and "Delete pages" i.e B and D respectively ,are not underlined, 
what might be the reason and how to fix ?

Comment 30

[Marco Bonardo [:mak]]

Comment on attachment 8819962 [details] [diff] [review]
v1.2.patch

Review of attachment 8819962 [details] [diff] [review]:
-----------------------------------------------------------------

I think I found your problem :)

::: browser/components/places/content/controller.js
@@ +658,5 @@
> +    // Make sure correct PluralForms are diplayed when multiple pages are selected in the Histroy/Bookmarks Library.
> +    var deleteHistoryItem = document.getElementById("placesContext_delete_history");
> +    deleteHistoryItem.label = PlacesUIUtils.getPluralString("cmd.deletePages.label",
> +                                                            metadata.length);
> +    deleteHistoryItem.accesskey = PlacesUIUtils.getString("cmd.deletePages.accesskey");

the property is named accessKey (uppercase k), accesskey doesn't work.

@@ +662,5 @@
> +    deleteHistoryItem.accesskey = PlacesUIUtils.getString("cmd.deletePages.accesskey");
> +    var createBookmarkItem = document.getElementById("placesContext_createBookmark");
> +    createBookmarkItem.label = PlacesUIUtils.getPluralString("cmd.bookmarkPages.label",
> +                                                             metadata.length);
> +    deleteHistoryItem.accesskey = PlacesUIUtils.getString("cmd.bookmarkPages.accesskey");

copy/paste bug, this should be createBookmarkItem

Comment 31

[Pavan Karthik [:matrixisreal]]

Attachment: v1.3.patch

Sorry I missed somethings before,hope its good to go now.

Comment 32

[Marco Bonardo [:mak]]

Comment on attachment 8820349 [details] [diff] [review]
v1.3.patch

Review of attachment 8820349 [details] [diff] [review]:
-----------------------------------------------------------------

thank you.
I'll push this to Try for you.

::: browser/components/places/content/controller.js
@@ +654,5 @@
>          }
>        }
>      }
>  
> +    // Make sure correct PluralForms are diplayed when multiple pages are selected in the Histroy/Bookmarks Library.

please remove "in the Histroy/Bookmarks Library" and stop the comment at "selected."

Comment 33

[Marco Bonardo [:mak]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=109b67e1b7b022c170fd3bfe18cd3e2207831f2d

please attach an updated patch with comment 32 addressed and the reviewer appended to the commit message (like ". r=mak") so that we can ask for check-in.

also, in the future, I'd suggest to have a look at the mozreview system, that while is a bit more complex at the beginning, long term solves a lot of this boilerplate.

Comment 34

[Pavan Karthik [:matrixisreal]]

Attachment: v1.4.patch

Comment 35

[Pavan Karthik [:matrixisreal]]

Thanks Marco for the suggestion.
Could you suggest some bugs for me to work next :D.

Comment 36

[Marco Bonardo [:mak]]

bug 1324571, that you filed, got answered by UX and is actionable.

Comment 37

[Marco Bonardo [:mak]]

in case you don't know about it, you can also find more bugs at https://www.joshmatthews.net/bugsahoy/?

Comment 38

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/620c1f06766b
No plurals form for context menus when multiple pages are selected in Library. r=mak

Comment 39

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/620c1f06766b

Comment 40

[Fahima Zulfath]

[Bugday-20170125]
Operating System: Windows 10
Browser:Firefox Nightly 54.0
  The bug is verfied. Getting singular form while selecting single site to delete. Obtaining plural while deleting multiple sites.

Comment 41

[Sebastian Hengst [:aryx] (needinfo me if it's about an intermittent or backout)]

This bug added a plural form for bookmarking pages from history, but using that command with multiple history items selected does nothing (bug 500090?).

Comment 42

[Marco Bonardo [:mak]]

(In reply to Sebastian Hengst [:aryx][:archaeopteryx] (needinfo on intermittent or backout) from comment #41)
> This bug added a plural form for bookmarking pages from history, but using
> that command with multiple history items selected does nothing (bug 500090?).

that's fine, it's a different bug. If anyone wants to try working on that I can help mentoring/reviewing.