1197163 - Ellipsis missing from string

Status: RESOLVED FIXED

(Toolkit :: Reader Mode, defect)

Description

[Dwayne Bailey]

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.

Comment 1

[Param Singh]

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

[:Margaret Leibovic]

(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

[Andrew]

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

[:Margaret Leibovic]

(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

[Andrew]

(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

[:Margaret Leibovic]

(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 :)

Comment 7

[Roland Demeter]

Hello!

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

Thanks!

Comment 8

[Roland Demeter]

Attachment: Bug1197163.diff

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

Comment 9

[:Margaret Leibovic]

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 :)

Comment 10

[Shivin]

Hi since nobody has worked on this buug for a while can I work on this . I will upload a patch ASAP

Comment 11

[Shivin]

Attachment: buf-1197163.patch

I changed the requested things and had a successful build but how do I test the changes

Comment 12

[: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.

Comment 13

[Shivin]

Attachment: bug-1197163.patch

Adding the image of the fixed loading screen tested on a wikipedia article http://imgur.com/mdUEUlZ

Comment 14

[Shivin]

also can you assign the bug to me

Comment 15

[: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.

Comment 16

[Shivin]

> 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

Comment 17

[: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…

Comment 18

[Shivin]

(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

Comment 19

[Shivin]

Please refer to the above comment

Comment 20

[: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.

Comment 21

[Shivin]

Attachment: bug-1197163.patch

Comment 22

[: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.

Comment 23

[Shivin]

Attachment: bug-1197163.patch

Sorry got confused fixed it now

Comment 24

[:Margaret Leibovic]

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

Comment 25

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/d06d7b99fe73