Closed Bug 1317438 Opened 3 years ago Closed 3 years ago

Unnecessary whitespaces added by XHTML code in about:privatebrowsing

Categories

(Firefox :: Private Browsing, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
Firefox 53
Tracking Status
firefox53 --- fixed

People

(Reporter: flod, Assigned: rickychien)

References

Details

Attachments

(1 file)

https://dxr.mozilla.org/mozilla-central/source/browser/components/privatebrowsing/content/aboutPrivateBrowsing.xhtml#57

        <p>
          &aboutPrivateBrowsing.note.before;
          <strong>&aboutPrivateBrowsing.note.emphasize;</strong>
          &aboutPrivateBrowsing.note.after;
        </p>

Unlike other paragraphs this one introduces unnecessary whitespaces. All elements should be on the same line, like https://dxr.mozilla.org/mozilla-central/source/browser/components/privatebrowsing/content/aboutPrivateBrowsing.xhtml#50
Blocks: 1259340
This will require locale changes to all shipping locales, right? Also, why didn't we notice this before? :-(
We've shipped this in 2 (tomorrow, 3) releases...
Flags: needinfo?(francesco.lodolo)
(In reply to :Gijs Kruitbosch from comment #1)
> This will require locale changes to all shipping locales, right? Also, why
> didn't we notice this before? :-(

No, it doesn't require l10n changes. All involved strings already have the requested spaces before and after the <strong> node, so the extra space caused by the newline was collapsed.

We discovered it in bug 1317243, because the translation had a non-breaking space, resulting in 2 spaces displayed.

> We've shipped this in 2 (tomorrow, 3) releases...
It only affects a few locales, and a double space is not exactly easy to spot in a page.
Flags: needinfo?(francesco.lodolo)
Duplicate of this bug: 1317575
And we have another one, this requiring string changes :-\
https://dxr.mozilla.org/mozilla-central/source/browser/components/privatebrowsing/content/aboutPrivateBrowsing.xhtml#77-81

aboutPrivateBrowsing.learnMore2 should have a trailing space (new ID), we need the code on one line and a .after string at this point.
https://hg.mozilla.org/releases/mozilla-aurora/file/default/browser/locales/en-US/chrome/browser/aboutPrivateBrowsing.dtd#l26
Ricky, would you be able to address this before Aurora 53 ships? This can also be an easy-enough-good-first-bug but I am not sure if a contributor can address it timely.
Assignee: nobody → rchien
Status: NEW → ASSIGNED
Flags: needinfo?(rchien)
Flags: needinfo?(rchien)
Sorry for introducing this issue accidentally. The patch has been submitted!
Comment on attachment 8810755 [details]
Bug 1317438 - Remove unnecessary whitespaces in about:privatebrowsing

https://reviewboard.mozilla.org/r/93082/#review93006

::: browser/components/privatebrowsing/content/aboutPrivateBrowsing.xhtml:74
(Diff revision 2)
>            <a id="startTour" class="button">&trackingProtection.startTour1;</a>
>          </p>
>        </section>
>  
>        <section class="section-main">
> -        <p class="about-info">&aboutPrivateBrowsing.learnMore2;
> +        <p class="about-info">&aboutPrivateBrowsing.learnMore2;<a id="learnMore" target="_blank">&aboutPrivateBrowsing.learnMore2.title;</a>.</p>

The period after `</a>` should be a new localizable string.
Comment on attachment 8810755 [details]
Bug 1317438 - Remove unnecessary whitespaces in about:privatebrowsing

https://reviewboard.mozilla.org/r/93082/#review93016

::: browser/locales/en-US/chrome/browser/aboutPrivateBrowsing.dtd:26
(Diff revision 3)
>  <!ENTITY aboutPrivateBrowsing.info.downloads             "downloads">
>  <!ENTITY aboutPrivateBrowsing.info.bookmarks             "bookmarks">
>  <!ENTITY aboutPrivateBrowsing.note.before                "Private Browsing ">
>  <!ENTITY aboutPrivateBrowsing.note.emphasize             "doesn’t make you anonymous">
>  <!ENTITY aboutPrivateBrowsing.note.after                 " on the Internet. Your employer or Internet service provider can still know what page you visit.">
> -<!ENTITY aboutPrivateBrowsing.learnMore2                 "Learn more about">
> +<!ENTITY aboutPrivateBrowsing.learnMore2                 "Learn more about ">

As already explained, you need a new strings ID here.
https://developer.mozilla.org/en-US/docs/Mozilla/Localization/Localization_content_best_practices#Changing_existing_strings

I would go as far as to change also .title, to make sure people recheck their localization.
Attachment #8810755 - Flags: review?(gijskruitbosch+bugs)
(I changed the reviewer to flod, who already used reviewboard to set "r? cleared", which is why the flag got cleared. Please request further reviews from flod.)
Attachment #8810755 - Flags: review?(gijskruitbosch+bugs)
Comment on attachment 8810755 [details]
Bug 1317438 - Remove unnecessary whitespaces in about:privatebrowsing

https://reviewboard.mozilla.org/r/93080/#review93028

::: browser/locales/en-US/chrome/browser/aboutPrivateBrowsing.dtd:26
(Diff revision 4)
>  <!ENTITY aboutPrivateBrowsing.info.downloads             "downloads">
>  <!ENTITY aboutPrivateBrowsing.info.bookmarks             "bookmarks">
>  <!ENTITY aboutPrivateBrowsing.note.before                "Private Browsing ">
>  <!ENTITY aboutPrivateBrowsing.note.emphasize             "doesn’t make you anonymous">
>  <!ENTITY aboutPrivateBrowsing.note.after                 " on the Internet. Your employer or Internet service provider can still know what page you visit.">
> -<!ENTITY aboutPrivateBrowsing.learnMore2                 "Learn more about">
> +<!ENTITY aboutPrivateBrowsing.learnMore2.before          "Learn more about ">

Let's change all these 3 strings to aboutPrivateBrowsing.learnMore3.(before|title|after)

This way we'll force localizers to recheck this page, since the behavior will change.

Also, as suggested, try pushing with r?flod in the commit
Comment on attachment 8810755 [details]
Bug 1317438 - Remove unnecessary whitespaces in about:privatebrowsing

https://reviewboard.mozilla.org/r/93082/#review93034

Thanks, it looks good.
Attachment #8810755 - Flags: review?(francesco.lodolo) → review+
mozreview lists open 2 issues, can you fix this so we can use autoland to land this changes, thanks!
Flags: needinfo?(rchien)
Keywords: checkin-needed
Fixed!
Flags: needinfo?(rchien) → needinfo?(cbook)
Comment on attachment 8810755 [details]
Bug 1317438 - Remove unnecessary whitespaces in about:privatebrowsing

https://reviewboard.mozilla.org/r/93082/#review93818
Comment on attachment 8810755 [details]
Bug 1317438 - Remove unnecessary whitespaces in about:privatebrowsing

https://reviewboard.mozilla.org/r/93082/#review93822

r+ on behalf of flod to get that landed
Attachment #8810755 - Flags: review+
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/09e10ac70397
Remove unnecessary whitespaces in about:privatebrowsing r=flod,Tomcat
Flags: needinfo?(cbook)
https://hg.mozilla.org/mozilla-central/rev/09e10ac70397
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 53
You need to log in before you can comment on or make changes to this bug.