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

RESOLVED FIXED in Firefox 53

Status

()

Firefox
Bookmarks & History
--
trivial
RESOLVED FIXED
2 years ago
2 months ago

People

(Reporter: scootergrisen, Assigned: matrixisreal)

Tracking

40 Branch
Firefox 53
Points:
---

Firefox Tracking Flags

(firefox53 fixed, firefox54 verified)

Details

(Whiteboard: [good first bug])

Attachments

(1 attachment, 5 obsolete attachments)

(Reporter)

Description

2 years ago
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...".

Updated

2 years ago
Component: Untriaged → Menus
Summary: Says "Delete This Page" when multiple pages is selected in Library → No plural for context-menu command "Delete This Page" when multiple pages are selected in Library

Updated

a year ago
Severity: normal → trivial
Status: UNCONFIRMED → NEW
Ever confirmed: true
Summary: No plural for context-menu command "Delete This Page" when multiple pages are selected in Library → No plurals form for context menus when multiple pages are selected in Library
Whiteboard: [good first bug]

Updated

a year ago
Component: Menus → Bookmarks & History

Comment 1

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

Comment 2

a year ago
(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

a year ago
Created attachment 8721620 [details] [diff] [review]
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".
Attachment #8721620 - Flags: review?(mak77)
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.
Attachment #8721620 - Flags: review?(mak77)
Attachment #8721620 - Flags: review-
Attachment #8721620 - Flags: feedback?(philipp)
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?
Flags: needinfo?(mheubusch)
Attachment #8721620 - Flags: feedback?(philipp) → feedback-
(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?
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.
---
Flags: needinfo?(mheubusch)

Comment 8

10 months ago
Hi, I would like to work on this bug. Can you please point out the files where I need to make changes.
Flags: needinfo?(mak77)

Comment 9

10 months ago
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
Flags: needinfo?(mak77)

Comment 10

10 months ago
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

10 months ago
(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

6 months ago
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 ?
(Assignee)

Comment 13

5 months ago
Hi, I would like to work on this bug,do assign it to me if you have noone else to.
Thanks.
(Assignee)

Comment 14

5 months ago
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
(Assignee)

Comment 15

5 months ago
I've reproduced the bug,so no need to clarify 1 and 2 in the above comment. :D
(Assignee)

Comment 16

5 months ago
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

5 months ago
(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.
(Assignee)

Comment 18

5 months ago
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
(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.
(Assignee)

Comment 20

4 months ago
Created attachment 8818726 [details] [diff] [review]
v1.patch

Delete/bookmark page if one page is selected
Delete/Bookmark pages else.
Any better-ideas/suggestions?
Flags: needinfo?(scootergrisen)
Attachment #8818726 - Flags: review?(mak77)
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.
Attachment #8818726 - Flags: review?(mak77) → feedback+
(Assignee)

Comment 22

4 months ago
Created attachment 8819386 [details] [diff] [review]
v1.1.patch

I have a small doubt:
Where is the PlacesUIUtils Module Loaded in the javascript?
Attachment #8818726 - Attachment is obsolete: true
Attachment #8819386 - Flags: review?(mak77)
(Assignee)

Comment 23

4 months ago
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
(Assignee)

Comment 24

4 months ago
Also, please assign this bug to me, if u forgot:P
(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.

Updated

4 months ago
Assignee: nobody → pavankarthikboddeda
Status: NEW → ASSIGNED
Flags: needinfo?(scootergrisen)

Updated

4 months ago
Attachment #8721620 - Attachment is obsolete: true
(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 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
Attachment #8819386 - Flags: review?(mak77)
(Assignee)

Comment 28

4 months ago
Created attachment 8819962 [details] [diff] [review]
v1.2.patch

I hope this looks good now?
Attachment #8819386 - Attachment is obsolete: true
Attachment #8819962 - Flags: review?(mak77)
(Assignee)

Comment 29

4 months ago
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 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
Attachment #8819962 - Flags: review?(mak77) → review-
(Assignee)

Comment 31

4 months ago
Created attachment 8820349 [details] [diff] [review]
v1.3.patch

Sorry I missed somethings before,hope its good to go now.
Attachment #8819962 - Attachment is obsolete: true
Attachment #8820349 - Flags: review?(mak77)
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."
Attachment #8820349 - Flags: review?(mak77) → review+
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.
(Assignee)

Comment 34

4 months ago
Created attachment 8820810 [details] [diff] [review]
v1.4.patch
Attachment #8820349 - Attachment is obsolete: true
Attachment #8820810 - Flags: review?(mak77)
(Assignee)

Comment 35

4 months ago
Thanks Marco for the suggestion.
Could you suggest some bugs for me to work next :D.
bug 1324571, that you filed, got answered by UX and is actionable.

Updated

4 months ago
Attachment #8820810 - Flags: review?(mak77) → review+

Updated

4 months ago
Keywords: checkin-needed
in case you don't know about it, you can also find more bugs at https://www.joshmatthews.net/bugsahoy/?

Comment 38

4 months ago
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/620c1f06766b
No plurals form for context menus when multiple pages are selected in Library. r=mak
Keywords: checkin-needed

Comment 39

4 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/620c1f06766b
Status: ASSIGNED → RESOLVED
Last Resolved: 4 months ago
status-firefox53: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 53

Updated

3 months ago
status-firefox54: --- → verified

Comment 40

3 months ago
[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.
This bug added a plural form for bookmarking pages from history, but using that command with multiple history items selected does nothing (bug 500090?).
(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.
QA Whiteboard: [good first verify]
You need to log in before you can comment on or make changes to this bug.