Closed Bug 1394462 Opened 3 years ago Closed 3 years ago

Update illustration and copy for error: server not found

Categories

(Firefox :: General, enhancement)

enhancement
Not set
normal

Tracking

()

RESOLVED FIXED
Firefox 57
Tracking Status
firefox57 --- fixed

People

(Reporter: ewright, Assigned: johannh)

References

(Blocks 1 open bug)

Details

Attachments

(2 files, 1 obsolete file)

Based on copy and mocks found in Bug 1358913
Attached image error-connection-failure.svg (obsolete) —
sorry for just posting this now, but this is the image we'd like to use.
(In reply to Erica Wright [:ewright] from comment #2)
> Created attachment 8909973 [details]
> error-connection-failure.svg
> 
> sorry for just posting this now, but this is the image we'd like to use.

I'll use that one, thanks.
Assignee: nobody → jhofmann
Status: NEW → ASSIGNED
correction, this one :johannh
Attachment #8909973 - Attachment is obsolete: true
(In reply to Erica Wright [:ewright] from comment #4)
> Created attachment 8909975 [details]
> error-server-not-found.svg
> 
> correction, this one :johannh

Heh, thanks, I was just about to ask :)
Comment on attachment 8909969 [details]
Bug 1394462 - Update dnsNotFound error copy and illustration.

https://reviewboard.mozilla.org/r/181460/#review186732

::: browser/base/content/aboutNetError.xhtml:178
(Diff revision 2)
>          if (showCaptivePortalUI) {
>            err = "captivePortal";
>          }
>  
> +        let pageTitle = document.getElementById("ept_" + err);
> +        if (pageTitle) {

We can get rid of most of this if block, see below:

::: browser/base/content/aboutNetError.xhtml:180
(Diff revision 2)
>          }
>  
> +        let pageTitle = document.getElementById("ept_" + err);
> +        if (pageTitle) {
> +          document.title = pageTitle.textContent;
> +        } else if (showCaptivePortalUI) {

err has been set to "captivePortal" in this case, so we if use "ept_captivePortal" as the id, we can get rid of this else if block.

::: browser/base/content/aboutNetError.xhtml:182
(Diff revision 2)
> +        let pageTitle = document.getElementById("ept_" + err);
> +        if (pageTitle) {
> +          document.title = pageTitle.textContent;
> +        } else if (showCaptivePortalUI) {
> +          document.title = document.getElementById("ept_captive").textContent;
> +        } else if (gIsCertError) {

gIsCertError is set to (err == "nssBadCert") so we can just change the id to ept_nssBadCert to avoid this else if block.

::: browser/base/content/illustrations/error-server-not-found.svg:1
(Diff revision 2)
> +<svg xmlns="http://www.w3.org/2000/svg" width="300" height="300" viewBox="0 0 300 300">

Please add the license header to this SVG file.

::: browser/base/jar.mn:42
(Diff revision 2)
>          content/browser/abouthome/sync@2x.png          (content/abouthome/sync@2x.png)
>          content/browser/abouthome/settings@2x.png      (content/abouthome/settings@2x.png)
>          content/browser/abouthome/restore@2x.png       (content/abouthome/restore@2x.png)
>          content/browser/abouthome/restore-large@2x.png (content/abouthome/restore-large@2x.png)
>  
> +        content/browser/illustrations/error-server-not-found.svg (content/illustrations/error-server-not-found.svg)

Note for posterity: bug 1401346 is filed to move these SVGs to browser/themes

::: browser/locales/en-US/chrome/overrides/netError.dtd:25
(Diff revision 2)
>  
> -<!ENTITY dnsNotFound.title "Server not found">
> -<!ENTITY dnsNotFound.longDesc "
> -<ul>
> -  <li>Check the address for typing errors such as
> -    <strong>ww</strong>.example.com instead of
> +<!ENTITY dnsNotFound.pageTitle "Server Not Found">
> +<!-- Localization note (dnsNotFound.title1) - "Hmm" is a sound made when considering or puzzling over something. You don't have to include it in your translation if your language does not have a written word like this. -->
> +<!ENTITY dnsNotFound.title1 "Hmm. We’re having trouble finding that site.">
> +<!ENTITY dnsNotFound.longDesc1 "
> +<strong>If that address is correct, here are three other things you can try:</strong>

Here we say "If that address..." which makes sense in the mock, because it immediately follows "We can’t connect to the server at <URL.com>."

Did you miss this "We can't connect..." line, or are we adding it in from elsewhere somehow?

Also, I'd prefer saying just "three things" instead of "three other things", since we have not suggested anything already, but this is a debate to be had with Michelle, so don't worry about it I guess.

::: docshell/base/nsDocShell.cpp:5021
(Diff revision 2)
>      nsAutoCString host;
>      nsCOMPtr<nsIURI> innermostURI = NS_GetInnermostURI(aURI);
>      innermostURI->GetHost(host);
>      CopyUTF8toUTF16(host, formatStrs[0]);
>      formatStrCount = 1;
> +    errorDescriptionID = "dnsNotFound2";

So, errorDescriptionID is added in bug 1394460, right? It's not showing up in this file in MozReview, but I think the patch should still apply correctly. Just thinking out loud.

::: mobile/locales/en-US/overrides/appstrings.properties:8
(Diff revision 2)
>  # file, You can obtain one at http://mozilla.org/MPL/2.0/.
>  
>  malformedURI2=The URL is not valid and cannot be loaded.
>  fileNotFound=Firefox can't find the file at %S.
>  fileAccessDenied=The file at %S is not readable.
> -dnsNotFound=Firefox can't find the server at %S.
> +dnsNotFound2=Firefox can't find the server at %S.

Shall we file a bug to change all the usages of "Firefox" to &brandShortName; in this file?

::: toolkit/themes/shared/in-content/info-pages.inc.css:69
(Diff revision 2)
>      padding-top: 0;
>    }
>  }
>  
>  ul, ol {
> -  margin: 0;
> +  margin: 1em 0;

I haven't tested these new rules, but I trust your judgement :D
Attachment #8909969 - Flags: review?(nhnt11)
Comment on attachment 8909969 [details]
Bug 1394462 - Update dnsNotFound error copy and illustration.

https://reviewboard.mozilla.org/r/181460/#review186732

> Here we say "If that address..." which makes sense in the mock, because it immediately follows "We can’t connect to the server at <URL.com>."
> 
> Did you miss this "We can't connect..." line, or are we adding it in from elsewhere somehow?
> 
> Also, I'd prefer saying just "three things" instead of "three other things", since we have not suggested anything already, but this is a debate to be had with Michelle, so don't worry about it I guess.

It's in browser/locales/en-US/chrome/overrides/appstrings.properties, no?

> Shall we file a bug to change all the usages of "Firefox" to &brandShortName; in this file?

There's probably a reason why it needs to be hardcoded, though we can certainly try.
Comment on attachment 8909969 [details]
Bug 1394462 - Update dnsNotFound error copy and illustration.

https://reviewboard.mozilla.org/r/181460/#review186732

> There's probably a reason why it needs to be hardcoded, though we can certainly try.

Let's pretend this never happened https://bugzilla.mozilla.org/show_bug.cgi?id=336029
Comment on attachment 8909969 [details]
Bug 1394462 - Update dnsNotFound error copy and illustration.

https://reviewboard.mozilla.org/r/181460/#review186758
Attachment #8909969 - Flags: review?(nhnt11) → review+
Pushed by jhofmann@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/23f0696642c2
Update dnsNotFound error copy and illustration. r=nhnt11
https://hg.mozilla.org/mozilla-central/rev/23f0696642c2
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 57
You need to log in before you can comment on or make changes to this bug.