Ellipsis missing from string

RESOLVED FIXED in Firefox 46

Status

()

Toolkit
Reader Mode
RESOLVED FIXED
3 years ago
3 years ago

People

(Reporter: Dwayne Bailey, Assigned: Shivin, Mentored)

Tracking

unspecified
mozilla46
Points:
---

Firefox Tracking Flags

(firefox46 fixed)

Details

(Whiteboard: [good first bug])

Attachments

(2 attachments, 3 obsolete attachments)

(Reporter)

Description

3 years ago
toolkit/locales/en-US/chrome/global/aboutReader.properties::aboutReader.loading=Loading... 

http://lxr.mozilla.org/mozilla-central/source/toolkit/locales/en-US/chrome/global/aboutReader.properties#5

This should use an ellipsis instead of three fullstops.

Updated

3 years ago
Mentor: margaret.leibovic, gijskruitbosch+bugs
Whiteboard: [good first bug]

Comment 1

3 years ago
Just so I'm clear, all that I need to do to fix this bug is replace the 3 fullstops with
an ellipsis (0x2026)?

Comment 2

3 years ago
(In reply to Param Singh from comment #1)
> Just so I'm clear, all that I need to do to fix this bug is replace the 3
> fullstops with
> an ellipsis (0x2026)?

Yep! You'll also want to update the entity name, so that our localization tools will pick up the change. Something like "aboutReader.loading2" would be fine. For extra clarity, you can also add a comment explaining that the ellipsis character should be used, e.g.:
http://hg.mozilla.org/mozilla-central/file/0b69d304f861/browser/locales/en-US/chrome/browser/browser.properties#l298

Comment 3

3 years ago
Hello, hoping I can pick up this bug? Margaret - after updating the variable so that it gets picked up, the corresponding getter here should be modified as well yes? https://dxr.mozilla.org/mozilla-central/search?q=aboutReader.loading&redirect=true&case=true

Comment 4

3 years ago
(In reply to Andrew from comment #3)
> Hello, hoping I can pick up this bug? Margaret - after updating the variable
> so that it gets picked up, the corresponding getter here should be modified
> as well yes?
> https://dxr.mozilla.org/mozilla-central/search?q=aboutReader.
> loading&redirect=true&case=true

Correct!

Comment 5

3 years ago
(In reply to :Margaret Leibovic from comment #4)
> (In reply to Andrew from comment #3)
> > Hello, hoping I can pick up this bug? Margaret - after updating the variable
> > so that it gets picked up, the corresponding getter here should be modified
> > as well yes?
> > https://dxr.mozilla.org/mozilla-central/search?q=aboutReader.
> > loading&redirect=true&case=true
> 
> Correct!


Great, Margaret. Also, where is this Loading... text supposed to be displayed? I wasn't able to find a Reading List feature in my Nightly build, only an option for viewing saved links to the Pocket app.

Could this be assigned to me? I could submit a patch once I verify my fix.

Comment 6

3 years ago
(In reply to Andrew from comment #5)
> (In reply to :Margaret Leibovic from comment #4)
> > (In reply to Andrew from comment #3)
> > > Hello, hoping I can pick up this bug? Margaret - after updating the variable
> > > so that it gets picked up, the corresponding getter here should be modified
> > > as well yes?
> > > https://dxr.mozilla.org/mozilla-central/search?q=aboutReader.
> > > loading&redirect=true&case=true
> > 
> > Correct!
> 
> 
> Great, Margaret. Also, where is this Loading... text supposed to be
> displayed? I wasn't able to find a Reading List feature in my Nightly build,
> only an option for viewing saved links to the Pocket app.
> 
> Could this be assigned to me? I could submit a patch once I verify my fix.

This is part of the "reader view" feature -- you should see a book icon sometimes appear in the right side of the urlbar, which will take you to reader view.

"Loading..." will only appear if there's a lot of work to be done to parse the page, which can be hard to run into on fast desktop machines. You may want to just hack at the about:reader code to show the string for testing purposes.

In the future, you can also use the needinfo? flag in bugzilla to make sure questions don't slip through the cracks :)

Updated

3 years ago
Assignee: nobody → a.louis

Comment 7

3 years ago
Hello!

I would like to work on this bug, if possible!

Thanks!

Comment 8

3 years ago
Created attachment 8696930 [details] [diff] [review]
Bug1197163.diff

I would like to propose this patch. I hope it helps.

Comment 9

3 years ago
Comment on attachment 8696930 [details] [diff] [review]
Bug1197163.diff

Does this actually work? Can you share a screenshot with this replacement?

Other places in the tree, we literally include the … character:
http://hg.mozilla.org/mozilla-central/file/0b69d304f861/browser/locales/en-US/chrome/browser/browser.properties#l298

As I mentioned in comment 2, you should also add a comment.

Also, in the future, you should make sure you use the feedback? flag to request feedback from a mentor on a bug, to make sure it doesn't fall through the cracks :)
(Assignee)

Comment 10

3 years ago
Hi since nobody has worked on this buug for a while can I work on this . I will upload a patch ASAP
Flags: needinfo?(margaret.leibovic)
(Assignee)

Comment 11

3 years ago
Created attachment 8703572 [details] [diff] [review]
buf-1197163.patch

I changed the requested things and had a successful build but how do I test the changes
Attachment #8703572 - Flags: review?(margaret.leibovic)
(Assignee)

Updated

3 years ago
Attachment #8703572 - Flags: feedback?(margaret.leibovic)

Comment 12

3 years ago
Comment on attachment 8703572 [details] [diff] [review]
buf-1197163.patch

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

::: toolkit/locales/en-US/chrome/global/aboutReader.properties
@@ +2,5 @@
>  # License, v. 2.0. If a copy of the MPL was not distributed with this
>  # file, You can obtain one at http://mozilla.org/MPL/2.0/.
>  
> +# 0x2026 used to display elipsis on the loading screen
> +aboutReader.loading2=Loading & 0x2026

This is not correct. As I mentioned in comment 9, you should include the … character directly, and copy the comment that I linked to as well.

To test this, you could find a very large page to load, or you could add a breakpoint to prevent the rest of the content from loading.
Attachment #8703572 - Flags: review?(margaret.leibovic)
Attachment #8703572 - Flags: review-
Attachment #8703572 - Flags: feedback?(margaret.leibovic)

Updated

3 years ago
Flags: needinfo?(margaret.leibovic)
(Assignee)

Comment 13

3 years ago
Created attachment 8703721 [details] [diff] [review]
bug-1197163.patch

Adding the image of the fixed loading screen tested on a wikipedia article http://imgur.com/mdUEUlZ
Attachment #8703572 - Attachment is obsolete: true
Attachment #8703721 - Flags: review?(margaret.leibovic)
Attachment #8703721 - Flags: feedback?(margaret.leibovic)
(Assignee)

Comment 14

3 years ago
also can you assign the bug to me
Flags: needinfo?(margaret.leibovic)

Comment 15

3 years ago
Comment on attachment 8703721 [details] [diff] [review]
bug-1197163.patch

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

::: toolkit/locales/en-US/chrome/global/aboutReader.properties
@@ +1,5 @@
>  # This Source Code Form is subject to the terms of the Mozilla Public
>  # License, v. 2.0. If a copy of the MPL was not distributed with this
>  # file, You can obtain one at http://mozilla.org/MPL/2.0/.
>  
> +# Use the unicode ellipsis char, \u2026,

Please add a LOCALIZATION NOTE line above this, similar to what we include here:
http://hg.mozilla.org/mozilla-central/file/0b69d304f861/browser/locales/en-US/chrome/browser/browser.properties#l299

@@ +3,5 @@
>  # file, You can obtain one at http://mozilla.org/MPL/2.0/.
>  
> +# Use the unicode ellipsis char, \u2026,
> +# or use "..." if \u2026 doesn't suit traditions in your locale.
> +aboutReader.loading2=Loading \u2026

Please remove the space between the word and the ellipsis.

Thanks for including a screenshot to verify this works, but is this what you see when you look at this line of code here?
http://hg.mozilla.org/mozilla-central/file/0b69d304f861/browser/locales/en-US/chrome/browser/browser.properties#l303

I see the literal … character included, and we should use that, since it's clearer for localizers to read.
Attachment #8703721 - Flags: review?(margaret.leibovic)
Attachment #8703721 - Flags: feedback?(margaret.leibovic)
Attachment #8703721 - Flags: feedback+

Updated

3 years ago
Assignee: a.louis → shivin.yadav
Flags: needinfo?(margaret.leibovic)
(Assignee)

Comment 16

3 years ago
> Thanks for including a screenshot to verify this works, but is this what you
> see when you look at this line of code here?
> http://hg.mozilla.org/mozilla-central/file/0b69d304f861/browser/locales/en-
> US/chrome/browser/browser.properties#l303
> 
> I see the literal … character included, and we should use that, since it's
> clearer for localizers to read.

Can you please clarify on what changes you want me to make do you want me to replace the ... charachter in my file I am a little confused by the last comment
Flags: needinfo?(margaret.leibovic)

Comment 17

3 years ago
(In reply to Shivin from comment #16)
> > Thanks for including a screenshot to verify this works, but is this what you
> > see when you look at this line of code here?
> > http://hg.mozilla.org/mozilla-central/file/0b69d304f861/browser/locales/en-
> > US/chrome/browser/browser.properties#l303
> > 
> > I see the literal … character included, and we should use that, since it's
> > clearer for localizers to read.
> 
> Can you please clarify on what changes you want me to make do you want me to
> replace the ... charachter in my file I am a little confused by the last
> comment

The new entity should be:

aboutReader.loading2=Loading…
Flags: needinfo?(margaret.leibovic)
(Assignee)

Comment 18

3 years ago
(In reply to :Margaret Leibovic from comment #17)
> (In reply to Shivin from comment #16)
> > > Thanks for including a screenshot to verify this works, but is this what you
> > > see when you look at this line of code here?
> > > http://hg.mozilla.org/mozilla-central/file/0b69d304f861/browser/locales/en-
> > > US/chrome/browser/browser.properties#l303
> > > 
> > > I see the literal … character included, and we should use that, since it's
> > > clearer for localizers to read.
> > 
> > Can you please clarify on what changes you want me to make do you want me to
> > replace the ... charachter in my file I am a little confused by the last
> > comment
> 
> The new entity should be:
> 
> aboutReader.loading2=Loading…

But the whole idea of the bug was to use elipsis instead of three fullstops so should I revert back to three fullstops
(Assignee)

Comment 19

3 years ago
Please refer to the above comment
Flags: needinfo?(margaret.leibovic)

Comment 20

3 years ago
(In reply to Shivin from comment #18)
> (In reply to :Margaret Leibovic from comment #17)
> > (In reply to Shivin from comment #16)
> > > > Thanks for including a screenshot to verify this works, but is this what you
> > > > see when you look at this line of code here?
> > > > http://hg.mozilla.org/mozilla-central/file/0b69d304f861/browser/locales/en-
> > > > US/chrome/browser/browser.properties#l303
> > > > 
> > > > I see the literal … character included, and we should use that, since it's
> > > > clearer for localizers to read.
> > > 
> > > Can you please clarify on what changes you want me to make do you want me to
> > > replace the ... charachter in my file I am a little confused by the last
> > > comment
> > 
> > The new entity should be:
> > 
> > aboutReader.loading2=Loading…
> 
> But the whole idea of the bug was to use elipsis instead of three fullstops
> so should I revert back to three fullstops

That *is* the ellipsis character. Try copy/pasting it to see, it is only one character.
Flags: needinfo?(margaret.leibovic)
(Assignee)

Comment 21

3 years ago
Created attachment 8704646 [details] [diff] [review]
bug-1197163.patch
Attachment #8703721 - Attachment is obsolete: true
Attachment #8704646 - Flags: review?(margaret.leibovic)

Comment 22

3 years ago
Comment on attachment 8704646 [details] [diff] [review]
bug-1197163.patch

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

::: toolkit/locales/en-US/chrome/global/aboutReader.properties
@@ +4,5 @@
>  
> +#LOCALIZATION NOTE (aboutReader.loading2):
> +# Use the unicode ellipsis char, \u2026,
> +# or use "..." if \u2026 doesn't suit traditions in your locale.
> +aboutReader.loading2=Loading...

This looks like you went back to using the literal 3 stops...

I'm sorry this is so confusing, I feel like I'm having a hard time communicating, maybe we're just seeing different things on our machines. I'll update just your patch and land it.
Attachment #8704646 - Flags: review?(margaret.leibovic) → review+
(Assignee)

Comment 23

3 years ago
Created attachment 8704649 [details] [diff] [review]
bug-1197163.patch

Sorry got confused fixed it now
Attachment #8704646 - Attachment is obsolete: true
Attachment #8704649 - Flags: review?(margaret.leibovic)

Comment 24

3 years ago
https://hg.mozilla.org/integration/fx-team/rev/d06d7b99fe73efaf0860272ad9723e80bf8e110c
Bug 1197163 - Update reader view "Loading…" string to use ellipsis character. r=margaret"

Updated

3 years ago
Attachment #8704649 - Flags: review?(margaret.leibovic) → review+

Comment 25

3 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/d06d7b99fe73
Status: NEW → RESOLVED
Last Resolved: 3 years ago
status-firefox46: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla46
You need to log in before you can comment on or make changes to this bug.