1734217 - Refactor the netError page internals (incl. DTD→Fluent migration) and remove its browser+geckoview overrides

Status: RESOLVED FIXED

(Firefox :: General, task, P4)

Description

[Erik Nordin [:nordzilla]]

As part of the M3 milestone, there are two netError.dtd files containing strings that should be move to Fluent:

• browser/chrome/overrides/netError.dtd
• dom/chrome/netError.dtd

At the time of creating this bug, these two files contain 126 strings that should be migrated to Fluent.

These strings don't block the startup path or anything else critical, but should be moved eventually.

This will serve as a meta bug for the migration.

Comment 1

[Zibi Braniecki [:zbraniecki][:gandalf]]

Those strings surface a limitation of Fluent DOM at the moment in form of complex DOM Overlay trees.

Example: https://searchfox.org/mozilla-central/source/browser/locales/en-US/chrome/overrides/netError.dtd#154-163

Currently, fluent cannot directly take a nested list. We could solve it with something like https://github.com/zbraniecki/fluent-domoverlays-js/wiki/New-Features-(rev-3)#1-allow-all-elements-as-children-in-the-source or we'd need to think of alternative migration.

@eemeli - thoughts?

Comment 2

[Francesco Lodolo [:flod]]

This might be a dupe of bug 1484955, definitely blocked by bug 1628362.

We should split up those strings, and get rid of most (if not all) markup.

Comment 3

[Eemeli Aro [:eemeli]]

I think a fundamental question we need to answer here is, "Do these messages present a need to allow for localisation to affect the DOM structure of the message?" In other words, these netError.dtd files currently include a number of messages with this kind of structure:

<p>foo</p>
<ul>
<li>one</li>
<li>two</li>
<li>three</li>
</ul>

Should a localisation be able to e.g. incorporate the foo in each of its bullet points and therefy leave the <p> empty, or merge two and three such that the list only has two items? If the answer here is "yes", then we do need to consider changes to how Fluent DOM localisation happens. On the other hand, if the answer is "no", then I agree with @flod that the right action is to split up the strings.

Separately from this issue, I do agree that the current Fluent DOM restrictions are a bit arbitrary, but I don't think that these messages should necessarily have such complex structure in Fluent, even if that were possible.

Comment 4

[Francesco Lodolo [:flod]]

We currently have 191 DTD strings left in mozilla-central. netError.dtd(s) represents 121 of these strings (63%).

If we want to take a stab of getting rid of DTDs, I think this should be the first step (and likely the most complicated).

Comment 5

[Francesco Lodolo [:flod]]

One more note: we need to figure out how to manage overrides. Right now, we have the same DTDs in different parts of the tree, and browser can override strings from DOM.

Comment 6

[Zibi Braniecki [:zbraniecki][:gandalf]]

We have two sources - toolkit and browser. Browser overrides toolkit, so if we're ok with DOM neterror being in toolkit and browser in browser, we should be good.

Comment 7

[Eemeli Aro [:eemeli]]

Attachment: Bug 1734217 - Migrate aboutNetError from DTD to Fluent. r=pbz!

Comment 8

[Eemeli Aro [:eemeli]]

Attachment: Bug 1734217 - Drop netError.dtd and appstrings.properties from mobile as unused. r=#geckoview-reviewers!

These files were made available as:

• chrome://browser/locale/netError.dtd
• chrome://browser/locale/appstrings.properties

For desktop, overrides are defined in browser/base/jar.mn that map the corresponding global/ paths to the above:

 % override chrome://global/locale/appstrings.properties chrome://browser/locale/appstrings.properties
 % override chrome://global/locale/netError.dtd chrome://browser/locale/netError.dtd

For mobile, similar overrides were earlier defined in mobile/android/chrome/jar.mn, but that file was removed in bug 1589182 three years ago.

Consequently, the global/ paths for these files that are used under docshell/ and dom/ have resolved to the non-overridden dom/ files since Firefox 72.

It should therefore be safe to remove them.

Depends on D155951

Comment 9

[Eemeli Aro [:eemeli]]

Attachment: WIP: Bug 1734217 - Drop netErrorApp.dtd as unused

The original intent with this file was to allow for partially overriding strings from netError.dtd, but at least currently this is functionality appears unused within and withoutm-c. With the move to Fluent, it doesn't make sense to recreate this structure just because it once made sense.

Depends on D156403

Comment 10

[Eemeli Aro [:eemeli]]

Attachment: WIP: Bug 1734217 - Migrate DOM netError from DTD to Fluent

Depends on D156404

Comment 11

[Eemeli Aro [:eemeli]]

Having spent some time on this, here's a brief overview of how error page localisation currently works:

1. The only place that actually builds about:neterror and about:certerror URIs is nsDocShell::DisplayLoadError().
2. To do so, it includes a couple of search parameters. One of these (d) is a localised string that's formatted from an appstrings.properties file (dom, browser) that may be overridden by an application.
3. An AboutRedirector (docshell, browser) identifies an XHTML file that's used to render the page (docshell, browser).
4. For further strings, the browser version is completely self-reliant. The docshell error page may be customised by an application defining and overriding a netError.dtd file.

Now, while it might appear that GeckoView also contains some such overrides, those don't actually do anything and the docshell versions are used. The error pages for Android and iOS are handled completely separately, independently of any of the above.

Thunderbird, on the other hand, does override the netError.dtd file used by the docshell netError.xhtml file, customising some of the error strings.

The patches currently submitted for this bug would do the following:

1. Drop the unused mobile files from the repo
2. Drop the docshell's customisability using a netErrorApp.dtd file. This is not mentioned above because it's not used by anyone at all.
3. Migrate the browser to use Fluent where it currently uses DTD files.
4. Migrate the docshell to use Fluent where it currently uses DTD files.

The localisation done by nsDocShell::DisplayLoadError() is untouched here, and may be handled as a separate subsequent change.

Now, having done all that, what I'd actually like to do is to replace in step 4 above the docshell version of the error page with something really minimal that doesn't use any localisation. This would correspond better with the way that it does not appear to be actually used in its uncustomised form anywhere.

Geoff, Magnus, this would require including a bit more custom code in Thunderbird than currently. It could be a direct copy of the current netError.xhtml and netError.js, or it could be the Fluent port of that which I've already done in D156405. Would that work for you?

---

According to codecoverage, at least some test manages to hit the docshell error page, but I've not managed to identify it yet. To see it manually in a browser, you should apply the following changes. You may need to also mach clobber, as I at least wasn't able to get the override change applied otherwise.

It's also possible to view the docshell error page in Fenix by manually entering an about:neterror URI, but I'm not sure if that's intentional.

diff --git a/browser/base/jar.mn b/browser/base/jar.mn
index e1d9307ac08ad..284b95d63a507 100644
--- a/browser/base/jar.mn
+++ b/browser/base/jar.mn
@@ -106,7 +106,5 @@ browser.jar:
         content/browser/spotlight.js                  (content/spotlight.js)
 *       content/browser/default-bookmarks.html        (content/default-bookmarks.html)

-% override chrome://global/content/netError.xhtml chrome://browser/content/certerror/aboutNetError.xhtml
-
 # L10n resources and overrides.
 % override chrome://global/locale/appstrings.properties chrome://browser/locale/appstrings.properties
diff --git a/browser/components/about/AboutRedirector.cpp b/browser/components/about/AboutRedirector.cpp
index ce25ab082af71..1926e0d46b29d 100644
--- a/browser/components/about/AboutRedirector.cpp
+++ b/browser/components/about/AboutRedirector.cpp
@@ -49,7 +49,7 @@ static const RedirEntry kRedirMap[] = {
      nsIAboutModule::URI_SAFE_FOR_UNTRUSTED_CONTENT |
          nsIAboutModule::URI_CAN_LOAD_IN_CHILD | nsIAboutModule::ALLOW_SCRIPT |
          nsIAboutModule::HIDE_FROM_ABOUTABOUT},
-    {"certerror", "chrome://browser/content/certerror/aboutNetError.xhtml",
+    {"certerror", "chrome://global/content/netError.xhtml",
      nsIAboutModule::URI_SAFE_FOR_UNTRUSTED_CONTENT |
          nsIAboutModule::URI_CAN_LOAD_IN_CHILD | nsIAboutModule::ALLOW_SCRIPT |
          nsIAboutModule::HIDE_FROM_ABOUTABOUT},

Comment 12

[Eemeli Aro [:eemeli]]

Fabrice, it looks like KaiOS does not depend on the docshell XHTML or DTD files, based on the routing done here?

Comment 13

[[:fabrice] Fabrice Desré]

(In reply to Eemeli Aro [:eemeli] from comment #12)

Fabrice, it looks like KaiOS does not depend on the docshell XHTML or DTD files, based on the routing done here?

Hi Eemeli, thanks for checking that! You're correct, we apply our own localization so this patch should not impact us.

Comment 14

[Eemeli Aro [:eemeli]]

I've added a WIP patch for bug 1543467 that applies the docshell netError.xhtml simplification mentioned above: https://phabricator.services.mozilla.com/D156478

Comment 15

[Magnus Melin [:mkmelin]]

(In reply to Eemeli Aro [:eemeli] from comment #11)
I took a look at what we have. The Thunderbird page really hasn't been very well kept up to date. Showing the page in Thunderbird is not something that would happen very often though. All in all, it would be much better for Thunderbird if we just drop the override, and the browser specific page now being converted to Fluent would be the source of truth (i.e. use that for everyone). The canonical version should live in docshell (or e.g. toolkit) so Thunderbird can use it. What do you think?

Comment 16

[Eemeli Aro [:eemeli]]

(In reply to Magnus Melin [:mkmelin] from comment #15)

[...] All in all, it would be much better for Thunderbird if we just drop the override, and the browser specific page now being converted to Fluent would be the source of truth (i.e. use that for everyone). The canonical version should live in docshell (or e.g. toolkit) so Thunderbird can use it. What do you think?

You know, that might work. It'll mean also moving NetErrorParent and NetErrorChild, but besides that there don't seem to be too deep dependencies to browser things. I'll give it a shot and see if I can find a reason why this might be a bad idea.

Comment 17

[Eemeli Aro [:eemeli]]

Attachment: Bug 1734217 - Merge all about:neterror front-end handling under toolkit. r=smaug!,nalexander!,mkmelin

Following a suggestion from :mkmelin, this seems like an optimal solution: the overriding/duplication in m-c is removed, and all users get a more powerful default choice that they're still able to override with their own, should they so wish.

For clarity and to match other about: pages, the shared code is placed under toolkit/content/, and all content under docshell/resources/ is removed.

Comment 18

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/a0ca83029b52
Drop netError.dtd and appstrings.properties from mobile as unused. r=geckoview-reviewers,nalexander,calu

Comment 19

[Eemeli Aro [:eemeli]]

I've updated the stack of patches so that it also moves the updated aboutNetError.xhtml from browser/ to toolkit/, and renamed this bug to more appropriately describe what it's about.

Comment 20

[Marian-Vasile Laza]

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

Comment 21

[Eemeli Aro [:eemeli]]

Attachment: Bug 1734217 - Clean up neterror element display logic and tests. r=prathiksha!

The neterror page serves a number of different use cases, which effectivly pick and choose elements from the page's DOM to display. Previously this logic was partly defined in the HTML, partly in the CSS, and partly in JS, using a couple of different methods. This change normalises all of that such that:

• All optional elements are initially hidden.
• Hiding is always controlled by an element's hidden attribute.
• Setting a CSS display style does not override the hidden attribute.

In addition to making the page easier to reason about, this allows for the CSS styles of the page to not be considered essential for its display, which means that they (and their dependencies) do not need to be included in toolkit/themes/shared/minimal-toolkit.jar.inc.mn.

The HTML and CSS of the page are also somewhat simplified, leaving out unused or unnecessary elements and styles.

Comment 22

[Eemeli Aro [:eemeli]]

Added one more patch that makes the styles non-critical, so that they don’t need to be (effectively uselessly) included in all our mobile builds as well: https://phabricator.services.mozilla.com/D157726

Comment 23

[Francesco Lodolo [:flod]]

Operational note: this requires running migrations on all l10n repositories. Given All Hands and personal time off, this means that the landing window closes this week and reopens after October 7.

Comment 24

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/2e7d726899c1
Migrate aboutNetError from DTD to Fluent. r=fluent-reviewers,prathiksha,flod
https://hg.mozilla.org/integration/autoland/rev/ad9d182d822b
Merge all about:neterror front-end handling under toolkit. r=mkmelin,smaug,nalexander,flod,Gijs
https://hg.mozilla.org/integration/autoland/rev/1258b03e6b88
Clean up neterror element display logic and tests. r=prathiksha

Comment 25

[Natalia Csoregi [:nataliaCs]]

https://hg.mozilla.org/mozilla-central/rev/2e7d726899c1
https://hg.mozilla.org/mozilla-central/rev/ad9d182d822b
https://hg.mozilla.org/mozilla-central/rev/1258b03e6b88

Comment 26

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/8470eb47f785
Adjust Thunderbird for netError changes. rs=bustage-fix

Comment 28

[:Gijs (out for now; he/him)]

Looking at this page again for unrelated reasons, something I missed in review: the main error page script was turned into a module. That means it loads asynchronously. But the script has an event listener for DOMContentLoaded. AFAIK there is no guarantee that that event hasn't already fired by the time the script loads, so I think that there is now a race condition in these pages. Eemeli, am I missing something?

Comment 29

[Eemeli Aro [:eemeli]]

I think you're right. Am I right in presuming that the listener isn't actually needed for anything, as the <script> tag is at the end of the page, and we could just run its contents at the top level?

Comment 30

[:Gijs (out for now; he/him)]

(In reply to Eemeli Aro [:eemeli] from comment #29)

I think you're right. Am I right in presuming that the listener isn't actually needed for anything, as the <script> tag is at the end of the page, and we could just run its contents at the top level?

In terms of the DOM being ready, I think you're right. However, it fires a custom event tests rely upon, which presumes all the other processing has run, so we should check that is still true.

Given this bug is closed, I'd suggest spinning off a separate bug to work out what the initialization requirements here really are. More generally for perf/flicker reasons it'd be better if some of the processing (esp/also fluent string id assignments!) in that file happened sync, not async, so that they'd be in place by first paint - and I'm not entirely sure why it was changed to be a module rather than a normal (sync) script. Does that sound OK?

Comment 31

[Eemeli Aro [:eemeli]]

(In reply to :Gijs (he/him) from comment #30)

Given this bug is closed, I'd suggest spinning off a separate bug to work out what the initialization requirements here really are. More generally for perf/flicker reasons it'd be better if some of the processing (esp/also fluent string id assignments!) in that file happened sync, not async, so that they'd be in place by first paint - and I'm not entirely sure why it was changed to be a module rather than a normal (sync) script. Does that sound OK?

Yeah. Added bug 1794423 for that; let's continue this thread there.