Closed Bug 1197163 Opened 9 years ago Closed 8 years ago

Ellipsis missing from string

Categories

(Toolkit :: Reader Mode, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla46
Tracking Status
firefox46 --- fixed

People

(Reporter: dwayne, Assigned: shivin.yadav, Mentored)

Details

(Whiteboard: [good first bug])

Attachments

(2 files, 3 obsolete files)

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.
Mentor: margaret.leibovic, gijskruitbosch+bugs
Whiteboard: [good first bug]
Just so I'm clear, all that I need to do to fix this bug is replace the 3 fullstops with
an ellipsis (0x2026)?
(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
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
(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!
(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.
(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 :)
Assignee: nobody → a.louis
Hello!

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

Thanks!
Attached patch Bug1197163.diffSplinter Review
I would like to propose this patch. I hope it helps.
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 :)
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)
Attached patch buf-1197163.patch (obsolete) — Splinter Review
I changed the requested things and had a successful build but how do I test the changes
Attachment #8703572 - Flags: review?(margaret.leibovic)
Attachment #8703572 - Flags: feedback?(margaret.leibovic)
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)
Flags: needinfo?(margaret.leibovic)
Attached patch bug-1197163.patch (obsolete) — Splinter Review
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)
also can you assign the bug to me
Flags: needinfo?(margaret.leibovic)
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+
Assignee: a.louis → shivin.yadav
Flags: needinfo?(margaret.leibovic)
> 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)
(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)
(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
Please refer to the above comment
Flags: needinfo?(margaret.leibovic)
(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)
Attached patch bug-1197163.patch (obsolete) — Splinter Review
Attachment #8703721 - Attachment is obsolete: true
Attachment #8704646 - Flags: review?(margaret.leibovic)
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+
Sorry got confused fixed it now
Attachment #8704646 - Attachment is obsolete: true
Attachment #8704649 - Flags: review?(margaret.leibovic)
https://hg.mozilla.org/integration/fx-team/rev/d06d7b99fe73efaf0860272ad9723e80bf8e110c
Bug 1197163 - Update reader view "Loading…" string to use ellipsis character. r=margaret"
Attachment #8704649 - Flags: review?(margaret.leibovic) → review+
https://hg.mozilla.org/mozilla-central/rev/d06d7b99fe73
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla46
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: