Open Bug 1734217 Opened 1 year ago Updated 17 hours ago

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

Categories

(Firefox :: General, task, P4)

task

Tracking

()

ASSIGNED

People

(Reporter: nordzilla, Assigned: eemeli)

References

(Blocks 8 open bugs)

Details

(Keywords: leave-open)

Attachments

(4 files, 2 obsolete files)

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

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.

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?

Flags: needinfo?(earo)

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.

Depends on: 1628362

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.

Flags: needinfo?(earo)

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

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.

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.

Blocks: 1786543
Assignee: nobody → earo
Status: NEW → ASSIGNED
Attachment #9292210 - Attachment description: WIP: Bug 1734217 - Migrate aboutNetError.xhtml from DTD to Fluent → WIP: Bug 1734217 - Migrate aboutNetError from DTD to Fluent
Attachment #9292210 - Attachment description: WIP: Bug 1734217 - Migrate aboutNetError from DTD to Fluent → Bug 1734217 - Migrate aboutNetError from DTD to Fluent. r=pbz!

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

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

Depends on D156404

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

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},
Flags: needinfo?(mkmelin+mozilla)
Flags: needinfo?(geoff)

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

Flags: needinfo?(fabrice)

(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.

Flags: needinfo?(fabrice)
Blocks: 1722896, 1621895
Depends on: 1543467
Depends on: 1484955
Summary: [meta] Migrate strings from netError.dtd to Fluent → Migrate strings from netError.dtd to Fluent
Keywords: meta
Attachment #9293070 - Attachment is obsolete: true
Attachment #9293069 - Attachment is obsolete: true
Blocks: 1582356
Blocks: 1546625

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

Blocks: 1789406

(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?

Flags: needinfo?(mkmelin+mozilla)
Flags: needinfo?(geoff)

(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.

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.

Keywords: leave-open
Pushed by earo@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/a0ca83029b52
Drop netError.dtd and appstrings.properties from mobile as unused. r=geckoview-reviewers,nalexander,calu

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.

Summary: Migrate strings from netError.dtd to Fluent → Refactor the netError page internals (incl. DTD→Fluent migration) and remove its browser+geckoview overrides
Blocks: 1675814
See Also: → 1725965
No longer depends on: 1484955, 1543467
See Also: → 1484955, 1543467
Blocks: 1790187
See Also: → 1768816

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.

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

Attachment #9295414 - Attachment description: WIP: Bug 1734217 - Clean up neterror element display logic → Bug 1734217 - Clean up neterror element display logic. r=prathiksha!

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.

See Also: → 1742345
Attachment #9295414 - Attachment description: Bug 1734217 - Clean up neterror element display logic. r=prathiksha! → Bug 1734217 - Clean up neterror element display logic and tests. r=prathiksha!
You need to log in before you can comment on or make changes to this bug.