Closed Bug 1394460 Opened 7 years ago Closed 7 years ago

Update illustration and copy for error: invalid URL

Categories

(Firefox :: General, enhancement)

Product:
Firefox
Component:
General
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
Firefox 57
Tracking Flags:
Tracking Status
firefox57 --- fixed

People

(Reporter: ewright, Assigned: ewright)

References

(Blocks 1 open bug)

Details

Attachments

(1 file)

Bug 1394460 - Update illustration and copy for error: invalid URL. ui-r=shorlander
59 bytes, text/x-review-board-request
johannh
: review+
Details 
Based on copy and mocks found in Bug 1358303
Assignee: nobody → ewright
Note: The switch-case, and the array are being use in preparation for more illustrated error pages in the near future.

And, another image will be coming from :shorlander shortly.
How can I test this in the browser?
Flags: needinfo?(ewright)
Comment on attachment 8903141 [details]
Bug 1394460 - Update illustration and copy for error: invalid URL. ui-r=shorlander

https://reviewboard.mozilla.org/r/174928/#review180726

::: browser/base/content/aboutNetError.xhtml:164
(Diff revision 1)
>          }
>        }
>  
>        function initPage() {
>          var err = getErrorCode();
> +        // List of error pages that have been redesigned 2017

// List of error pages with an illustration

::: browser/base/content/aboutNetError.xhtml:171
(Diff revision 1)
> +        if (illustratedErrors.includes(err)) {
> +          document.body.classList.add("illustrated");
> +          let errorIllustration = document.getElementById('error-illustration');
> +          switch (err) {
> +            case "malformedURI":
> +              errorIllustration.src="chrome://browser/content/illustration/error-malformed-url.svg";

Could you make this a CSS background-image instead of an <img/> and set it in aboutNetError.css?

::: browser/locales/en-US/chrome/overrides/appstrings.properties:5
(Diff revision 1)
>  # 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/.
>  
> -malformedURI=The URL is not valid and cannot be loaded.
> +malformedURI=Please check that the URL is correct and try again.

You need to change the string id when changing the string such that other locales pick this up.

::: browser/locales/en-US/chrome/overrides/netError.dtd:61
(Diff revision 1)
>  <p>You must log in to this network before you can access the Internet.</p>
>  ">
>  
>  <!ENTITY openPortalLoginPage.label2 "Open Network Login Page">
>  
> -<!ENTITY malformedURI.title "The address isn’t valid">
> +<!ENTITY malformedURI.title "Hmm. That address doesn't look right.">

ditto

::: browser/locales/en-US/chrome/overrides/netError.dtd:62
(Diff revision 1)
>  ">
>  
>  <!ENTITY openPortalLoginPage.label2 "Open Network Login Page">
>  
> -<!ENTITY malformedURI.title "The address isn’t valid">
> -<!ENTITY malformedURI.longDesc "
> +<!ENTITY malformedURI.title "Hmm. That address doesn't look right.">
> +<!ENTITY malformedURI.longDesc "">

What's the point of having this empty entity? Can we just remove this?

::: browser/themes/shared/aboutNetError.css:167
(Diff revision 1)
> +}
> +
> +/* New Illustrated Error Pages */
> +
> +.illustrated #errorTryAgain {
> +  display: none;

Hiding this button should explicitly depend on the error type, not on whether the page has an illustration.
Attachment #8903141 - Flags: review?(dao+bmo) → review-
(In reply to Dão Gottwald [::dao] from comment #3)
> How can I test this in the browser?

go to `about:asd`
Flags: needinfo?(ewright)
Comment on attachment 8903141 [details]
Bug 1394460 - Update illustration and copy for error: invalid URL. ui-r=shorlander

https://reviewboard.mozilla.org/r/174928/#review180726

> You need to change the string id when changing the string such that other locales pick this up.

Unfortunately this string id is the error itself, which comes from the C++ files. In order to change this I'll need to change the error name - which does not seem like a good idea, or special case it (and the one below) to fetch this particular string. And every subsequent page that gets a redesign will also need to special-cased. With the current conventions of this shared error page it seems to me that we can never change strings, not without a large overhaul, is this correct?
Flags: needinfo?(dao+bmo)
Added correct illustration and other fixes, waiting still to update the localization issues.
Changing the error name or special-casing it seems like the way to go. AFAIK, changing the string without the id isn't an option.
Flags: needinfo?(dao+bmo)
Comment on attachment 8903141 [details]
Bug 1394460 - Update illustration and copy for error: invalid URL. ui-r=shorlander

https://reviewboard.mozilla.org/r/174928/#review181654
Attachment #8903141 - Flags: review?(dao+bmo) → review-
Comment on attachment 8903141 [details]
Bug 1394460 - Update illustration and copy for error: invalid URL. ui-r=shorlander

https://reviewboard.mozilla.org/r/174928/#review182694

::: browser/locales/en-US/chrome/overrides/netError.dtd:61
(Diff revision 3)
>  <p>You must log in to this network before you can access the Internet.</p>
>  ">
>  
>  <!ENTITY openPortalLoginPage.label2 "Open Network Login Page">
>  
> -<!ENTITY malformedURI.title "The address isn’t valid">
> +<!ENTITY malformedURI.title1 "Hmm. That address doesn't look right.">

Use proper apostrophe instead of straight single quote (’ vs ')

::: mobile/locales/en-US/overrides/appstrings.properties:5
(Diff revision 3)
>  # 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/.
>  
> -malformedURI=The URL is not valid and cannot be loaded.
> +invalidURI=The URL is not valid and cannot be loaded.

How come the message doesn't change for mobile?
Comment on attachment 8903141 [details]
Bug 1394460 - Update illustration and copy for error: invalid URL. ui-r=shorlander

https://reviewboard.mozilla.org/r/174928/#review182694

> How come the message doesn't change for mobile?

We are not redesigning the mobile page just yet. Since I needed to change the way the C++ fetches the entities in order to change the wording for desktop, this name needed to be changed only as a side effect.
(In reply to Erica Wright [:ewright] from comment #13)
> Comment on attachment 8903141 [details]
> Bug 1394460 - Update illustration and copy for error: invalid URL.
> 
> https://reviewboard.mozilla.org/r/174928/#review182694
> 
> > How come the message doesn't change for mobile?
> 
> We are not redesigning the mobile page just yet. Since I needed to change
> the way the C++ fetches the entities in order to change the wording for
> desktop, this name needed to be changed only as a side effect.

And when we're going to update mobile we'll have the same issue, won't we? Since we're changing the ID and forcing people to retranslate the string anyway in /mobile, it seems logic to use the same text for all 3 components.
(In reply to Francesco Lodolo [:flod] from comment #14)
> (In reply to Erica Wright [:ewright] from comment #13)
> > Comment on attachment 8903141 [details]
> > Bug 1394460 - Update illustration and copy for error: invalid URL.
> > 
> > https://reviewboard.mozilla.org/r/174928/#review182694
> > 
> > > How come the message doesn't change for mobile?
> > 
> > We are not redesigning the mobile page just yet. Since I needed to change
> > the way the C++ fetches the entities in order to change the wording for
> > desktop, this name needed to be changed only as a side effect.
> 
> And when we're going to update mobile we'll have the same issue, won't we?
> Since we're changing the ID and forcing people to retranslate the string
> anyway in /mobile, it seems logic to use the same text for all 3 components.

I'm told that we don't have plans to redesign those pages, which means if we ever do, it'll be pretty far in the future. Unfortunately I don't think there is a way around changing that ID, and making it the same string won't necessarily fit with what already exists on mobile.
Comment on attachment 8903141 [details]
Bug 1394460 - Update illustration and copy for error: invalid URL. ui-r=shorlander

https://reviewboard.mozilla.org/r/174928/#review184912

::: browser/themes/shared/aboutNetError.css:176
(Diff revision 5)
> +  background: none;
> +  padding-inline-start: 0;
> +  margin-inline-start: 0;
> +}
> +
> +.illustrated #copy-container {

Like in bug 1394461, I suspect we don't really need the copy-container element. (This id is also rather suboptimal, as "copy" can mean many different things, and there happen to be other elements containing text.)
Attachment #8903141 - Flags: review?(dao+bmo) → review?(jhofmann)
(In reply to Dão Gottwald [::dao] from comment #18)
> Comment on attachment 8903141 [details]
> Bug 1394460 - Update illustration and copy for error: invalid URL. ,
> ui-r=shorlander
> 
> https://reviewboard.mozilla.org/r/174928/#review184912
> 
> ::: browser/themes/shared/aboutNetError.css:176
> (Diff revision 5)
> > +  background: none;
> > +  padding-inline-start: 0;
> > +  margin-inline-start: 0;
> > +}
> > +
> > +.illustrated #copy-container {
> 
> Like in bug 1394461, I suspect we don't really need the copy-container
> element. (This id is also rather suboptimal, as "copy" can mean many
> different things, and there happen to be other elements containing text.)

Initially it was used for flex, but since I am no longer doing that, now it is used so I don't have to add a margin to each element individually.
Comment on attachment 8903141 [details]
Bug 1394460 - Update illustration and copy for error: invalid URL. ui-r=shorlander

https://reviewboard.mozilla.org/r/174928/#review184912

> Like in bug 1394461, I suspect we don't really need the copy-container element. (This id is also rather suboptimal, as "copy" can mean many different things, and there happen to be other elements containing text.)

it is also used to center the text vertically.
Comment on attachment 8903141 [details]
Bug 1394460 - Update illustration and copy for error: invalid URL. ui-r=shorlander

https://reviewboard.mozilla.org/r/174928/#review185182

This looks mostly good to me. I agree that you could probably get rid of the text-container element, but I don't mind it either.

Cancelling review for now since I'd like to take another look at the cpp changes :)

Thanks!

::: browser/themes/shared/aboutNetError.css:160
(Diff revision 7)
>    display: flex;
>    justify-content: end;
>    padding: 10px;
>  }
> +
> +/* New Illustrated Error Pages */

I'm not sure if that comment is useful ;)

::: docshell/base/nsDocShell.cpp:4956
(Diff revision 7)
>  
>    NS_ENSURE_TRUE(stringBundle, NS_ERROR_FAILURE);
>    NS_ENSURE_TRUE(prompter, NS_ERROR_FAILURE);
>  
>    const char* error = nullptr;
> +  const char* displayError = nullptr;

If we go for this solution, we should make it more explicit what displayError is supposed to do. I think it's a sane idea to de-couple frequently changing parts such as localization IDs from error codes and we should be clear about it!

Please rename this variable to something like errorStringID or errorLocalizationID (or a better name if you can think of one) and leave a comment explaining why we're doing this here.

::: docshell/base/nsDocShell.cpp:5202
(Diff revision 7)
>      // Errors requiring simple formatting
>      switch (aError) {
>        case NS_ERROR_MALFORMED_URI:
>          // URI is malformed
>          error = "malformedURI";
> +        displayError = "invalidURI";

Please call this malformedURI2, I think that makes it more clear what we're doing here.
Attachment #8903141 - Flags: review?(jhofmann)
Comment on attachment 8903141 [details]
Bug 1394460 - Update illustration and copy for error: invalid URL. ui-r=shorlander

https://reviewboard.mozilla.org/r/174928/#review185582

This looks good, thank you.

::: docshell/base/nsDocShell.cpp:4956
(Diff revision 8)
>  
>    NS_ENSURE_TRUE(stringBundle, NS_ERROR_FAILURE);
>    NS_ENSURE_TRUE(prompter, NS_ERROR_FAILURE);
>  
>    const char* error = nullptr;
> +  const char* errorDescriptionID = nullptr;

Nit: can you add a short comment what this is used for?
Attachment #8903141 - Flags: review?(jhofmann) → review+
Please add the checkin-needed flag to land commits after getting r+, I nearly forgot about this one :)
Pushed by jhofmann@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/f28b312705df
Update illustration and copy for error: invalid URL. ui-r=shorlander r=johannh
Status: NEW → ASSIGNED
(In reply to Johann Hofmann [:johannh] from comment #27)
> Please add the checkin-needed flag to land commits after getting r+, I
> nearly forgot about this one :)

thanks, I wasn't sure after fixing the nit if you wanted to look again
Depends on: 1401346
https://hg.mozilla.org/mozilla-central/rev/f28b312705df
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
status-firefox57: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 57
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: