Open Bug 1728474 Opened 3 years ago Updated 2 years ago

Update About Dialog to use a simpler HTML structure and CSS

Categories

(Thunderbird :: General, enhancement)

Product:
Thunderbird
Component:
General
Type:
enhancement

Tracking

(Not tracked)

Status:
ASSIGNED

People

(Reporter: aleca, Assigned: Paenglab)

References

Details

Attachments

(2 files, 9 obsolete files)

1728474-aboutDialog-html.patch
(deleted), patch
Details | Diff | Splinter Review
1728474-aboutDialog-html-v2.patch
(deleted), patch
aleca
: review+
Paenglab
: review+
henry-x
: review-
Details | Diff | Splinter Review

We should improve the overall structure of the About Dialog to simplify things.
The most glaring things are:

  • Use a simpler HTML structure based on CSS grid + Flexbox.
  • Add the wordmark inline as img instead of a background image of the whole box.
  • Let the dialog resize itself to accommodate the content (if possible).
  • Use fluent.

Note, I recently changed the #updateDeck to html (which is also shared with the preference tab) and I added a method for opening <a> links externally from the dialog https://hg.mozilla.org/comm-central/rev/c1c11744674a#l1.114 . So this method could be used when replacing the other <label is="text-link"> elements as well. Or, we might want to decide that some of the links should open within a thunderbird content tab.

Attached patch 1728474-aboutDialog-html.patch (obsolete) (deleted) — — Splinter Review

This patch converts the dialog to HTML.

To not use the XUL <keyset> I added a event listener to close the dialog. Unfortunately I don't know how to implement the Mac version. Please can you help me with this?

Assignee: nobody → richard.marti
Status: NEW → ASSIGNED
Attachment #9239899 - Flags: review?(alessandro)
Attached patch 1728474-aboutDialog-fluent.patch (obsolete) (deleted) — — Splinter Review

Patch for the Fluent conversion.

Attachment #9239900 - Flags: review?(alessandro)
Attached patch 1728474-aboutDialog-html.patch (obsolete) (deleted) — — Splinter Review

There was another patch applied. This one applies now cleanly.

Attachment #9239899 - Attachment is obsolete: true
Attachment #9239899 - Flags: review?(alessandro)
Attachment #9239901 - Flags: review?(alessandro)
Comment on attachment 9239901 [details] [diff] [review]
1728474-aboutDialog-html.patch

Review of attachment 9239901 [details] [diff] [review]:
-----------------------------------------------------------------

::: mail/base/content/aboutDialog.js
@@ +44,5 @@
>            "distribution.about",
>            Ci.nsISupportsString
>          );
>          var distroField = document.getElementById("distribution");
> +        distroField.innerHTML = distroAbout;

.textContent =

::: mail/base/content/aboutDialog.xhtml
@@ +19,5 @@
> +      xmlns:xul="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul"
> +      xmlns:html="http://www.w3.org/1999/xhtml"
> +      id="aboutDialog"
> +      windowtype="Mail:About"
> +      data-l10n-id="aboutDialog-title"

html uses <title> in <head>

(please move the id to the first line, elsewhere i've put the default namespace there as well, like 

<html id="aboutDialog" xmlns="http://www.w3.org/1999/xhtml"

@@ +42,5 @@
> +      <img id="leftBox" src="chrome://branding/content/about-logo.svg" />
> +      <div id="rightBox">
> +        <img id="wordmark" src="chrome://branding/content/about-wordmark.svg" />
> +        <label id="version" />
> +        <label is="text-link" id="releasenotes" hidden="true">&releaseNotes.link;</label>

<a>

@@ +47,2 @@
>  
> +        <label id="distribution" class="text-blurb" />

These were xul label. Label in html is something else (and not self closing)

@@ +144,2 @@
>  #endif
> +        <div id="experimental" hidden="true">

for html hidden="hidden"
Comment on attachment 9239900 [details] [diff] [review]
1728474-aboutDialog-fluent.patch

Review of attachment 9239900 [details] [diff] [review]:
-----------------------------------------------------------------

::: mail/locales/en-US/messenger/aboutDialog.ftl
@@ +6,5 @@
>      .title = About { -brand-full-name }
>  
> +releaseNotes-link = Release notes
> +
> +update-checkForUpdatesButton = Check for updates

when moving to fluent we really should create a migration recipe so translators don't have to update more than needed.
Comment on attachment 9239900 [details] [diff] [review]
1728474-aboutDialog-fluent.patch

Review of attachment 9239900 [details] [diff] [review]:
-----------------------------------------------------------------

Indeed, we should do a fluent migration here since we're not changing any string.
I can take care of this if you want.
Attachment #9239900 - Flags: review?(alessandro) → review-

(In reply to Alessandro Castellani [:aleca] from comment #7)

Indeed, we should do a fluent migration here since we're not changing any
string.
I can take care of this if you want.

Yes, please.

Comment on attachment 9239901 [details] [diff] [review]
1728474-aboutDialog-html.patch

Review of attachment 9239901 [details] [diff] [review]:
-----------------------------------------------------------------

Magnus already did a first swipe at this.

This is not a complete HTML conversion since there are some fundamental problems that should be solved:
- The `<label>` element shouldn't be used to print text, but only in a FORM context alongside input fields.
- `is="text-link"` elements should be replaced with simple `<a>` tags.
- The HTML structure is not clean, and semantically hard to read. We should avoid using multiple nested `span` or `div` but rather relying on CSS grid and flexbox with proper semantic elements (main, header, footer, section, aside, p). 

Thanks for giving a stab at this, but this needs a deeper reorganization and clean up.

::: mail/base/content/aboutDialog.xhtml
@@ +50,2 @@
>  #ifdef MOZ_UPDATER
> +        <stack id="updateDeck" orient="vertical">

We should also get rid of the `stack` XUL element.

@@ +87,5 @@
> +          </div>
> +          <div id="applying" class="update-deck-container">
> +            <img class="update-throbber" alt=""
> +                 src="chrome://global/skin/icons/loading.png"
> +                 srcset="chrome://global/skin/icons/loading@2x.png 2x" />

This should use the SVG throbber.
Attachment #9239901 - Flags: review?(alessandro) → review-
Attached patch 1728474-aboutDialog-html.patch (obsolete) (deleted) — — Splinter Review

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

Comment on attachment 9239901 [details] [diff] [review]
1728474-aboutDialog-html.patch

Review of attachment 9239901 [details] [diff] [review]:

::: mail/base/content/aboutDialog.js
@@ +44,5 @@

       "distribution.about",
       Ci.nsISupportsString
     );
     var distroField = document.getElementById("distribution");

  •    distroField.innerHTML = distroAbout;

.textContent =

Done

::: mail/base/content/aboutDialog.xhtml
@@ +19,5 @@

  •  xmlns:xul="http://www.mozilla.org/keymaster/gatekeeper/there.is.only.xul"

  •  xmlns:html="http://www.w3.org/1999/xhtml"

  •  id="aboutDialog"

  •  windowtype="Mail:About"

  •  data-l10n-id="aboutDialog-title"

html uses <title> in <head>

Done

(please move the id to the first line, elsewhere i've put the default
namespace there as well, like

<html id="aboutDialog" xmlns="http://www.w3.org/1999/xhtml"

@@ +42,5 @@

  •  <img id="leftBox" src="chrome://branding/content/about-logo.svg" />

  •  <div id="rightBox">

  •    <img id="wordmark" src="chrome://branding/content/about-wordmark.svg" />

  •    <label id="version" />

  •    <label is="text-link" id="releasenotes" hidden="true">&releaseNotes.link;</label>

<a>

Done

@@ +47,2 @@

  •    <label id="distribution" class="text-blurb" />

These were xul label. Label in html is something else (and not self closing)

@@ +144,2 @@

#endif

  •    <div id="experimental" hidden="true">

for html hidden="hidden"

Done

(In reply to Alessandro Castellani [:aleca] from comment #9)

Comment on attachment 9239901 [details] [diff] [review]
1728474-aboutDialog-html.patch

Review of attachment 9239901 [details] [diff] [review]:

Magnus already did a first swipe at this.

This is not a complete HTML conversion since there are some fundamental
problems that should be solved:

  • The <label> element shouldn't be used to print text, but only in a FORM
    context alongside input fields.
  • is="text-link" elements should be replaced with simple <a> tags.
  • The HTML structure is not clean, and semantically hard to read. We should
    avoid using multiple nested span or div but rather relying on CSS grid
    and flexbox with proper semantic elements (main, header, footer, section,
    aside, p).

Thanks for giving a stab at this, but this needs a deeper reorganization and
clean up.

If you like you can do it.

::: mail/base/content/aboutDialog.xhtml
@@ +50,2 @@

#ifdef MOZ_UPDATER

  •    <stack id="updateDeck" orient="vertical">

We should also get rid of the stack XUL element.

This is also used in the General pane of the Prefs and Henry converted it to this <span> blocks. Look after the Fluent patch is applied and it looks simpler.

@@ +87,5 @@

  •      </div>

  •      <div id="applying" class="update-deck-container">

  •        <img class="update-throbber" alt=""

  •             src="chrome://global/skin/icons/loading.png"

  •             srcset="chrome://global/skin/icons/loading@2x.png 2x" />

This should use the SVG throbber.

We don't have a rotating SVG throbber.

Attachment #9239901 - Attachment is obsolete: true
Attachment #9239924 - Flags: review?(alessandro)
Attached patch 1728474-aboutDialog-fluent.patch (obsolete) (deleted) — — Splinter Review

Setting f? so you can do the Fluent conversion script.

Attachment #9239900 - Attachment is obsolete: true
Attachment #9239925 - Flags: feedback?(alessandro)

We don't have a rotating SVG throbber.

We got loading.svg, which we use in various places, most recently is used in the new account setup loading notifications: https://searchfox.org/comm-central/rev/dd1384ed8a2c9dffd153130a3d978d1342422744/mail/themes/shared/mail/message-bar.css#145-153

I agree with alessandro that this is a good opportunity to make a structural change, and an opportunity to use better semantics. Mostly because it would improve accessibility. For example, the current use of <label> (xul or html) is wrong, and their use as text links makes these links inaccessible to both screen reader and keyboard users.

I did make changes to the update stack earlier, but since it was only a partial conversion, and I wasn't touching strings, it required some hacks, like the nested spans. If we're converting strings, then a lot of improvements can be made.

I would also note that xul:stack is just a display: grid, with each child being positioned at 1 / 1. This is basically just a single grid cell. So when you are converting it, if you are using a grid display in the surrounding markup, then this can be replaced by setting all the children to share the same grid area.

Comment on attachment 9239924 [details] [diff] [review]
1728474-aboutDialog-html.patch

I'll look if I can work over the weekend to improve it.

Attachment #9239924 - Flags: review?(alessandro)
Attachment #9239925 - Flags: feedback?(alessandro)
Attached patch 1730365-cui-widget-panel-shadow.patch (obsolete) (deleted) — — Splinter Review

This should address all comments.

I combined the Fluent patch into this one as you never checked the end result of the two previous patches which would have addressed some comments.

I only implemented the bottom box as a grid. The top part makes no sense as grid with only one img on the left and the rest on the right.

Also have in mind that the updateDeck is shared with the general prefs which still uses XUL.

Please can you help me with the Mac close key?

You also offered to do the Fluent conversion script. ;-)

Attachment #9239924 - Attachment is obsolete: true
Attachment #9239925 - Attachment is obsolete: true
Attachment #9240816 - Flags: review?(alessandro)
Comment on attachment 9240816 [details] [diff] [review]
1730365-cui-widget-panel-shadow.patch

Review of attachment 9240816 [details] [diff] [review]:
-----------------------------------------------------------------

aleca is doing the full review, but I noticed a few things.

Besides the in line comments, you also need to give each of the `<a>` links a `href`, otherwise the links cannot be focussed nor activated by the keyboard. As I mentioned in comment 1, I added a method for external links in my patch (https://hg.mozilla.org/comm-central/rev/c1c11744674a#l1.114). You just need a `href` and to give the anchor elements the `download-link` class (feel free to change the name to something more generic and appropriate, just remember it is also used in the preferences). All the links that currently use `openLink` should work like this, and you should get rid of the `openLink` method https://searchfox.org/comm-central/source/mail/base/content/aboutDialog.js#131 The `#releasenotes` element is assigned a `href` link in javascript, but I'm pretty sure this will need fixing because it doesn't currently have the `download-link` class, so won't actually open up the link on click. The rest of the `<a>` links use `openAboutTab`, they should also have a `href` and should be similarly handled.

Also, it would be a good idea to review this in the Toolbox accessibility tree. Some of the sectioning might not be what you expected, with some paragraphs being grouped together (like the `#experimental` paragraphs).

::: mail/base/content/aboutDialog.xhtml
@@ +26,3 @@
>  
> +  <div id="aboutDialogContainer">
> +    <img id="logo" src="chrome://branding/content/about-logo.svg" />

Missing `alt` attribute.

@@ +26,5 @@
>  
> +  <div id="aboutDialogContainer">
> +    <img id="logo" src="chrome://branding/content/about-logo.svg" />
> +    <div id="rightBox">
> +      <img id="wordmark" src="chrome://branding/content/about-wordmark.svg" />

Missing `alt` attribute.

@@ +36,2 @@
>  #ifdef MOZ_UPDATER
> +      <div id="updateDeck">

This is no longer center aligning its content. Plus, it has a fixed height, which does not necessarily fit its content. This used to be `display: grid`, with each child sharing the same `1 / 1` grid area.

@@ +135,5 @@
> +           onclick="openLink('https://www.thunderbird.net/get-involved/')"></a>
> +      </p>
> +    </div>
> +  </div>
> +  <div id="bottomBox">

Maybe this should be a `<footer>`?

::: mail/themes/shared/mail/aboutDialog.css
@@ +95,5 @@
> +
> +#updateDeck {
> +  height: 2.7em;
> +  padding-top: 0.1em;
> +  opacity: 0.6;

I'm not sure this should be opacity 0.6. It makes it harder to read, and it looks like the download buttons are disabled.
Attached patch 1728474-aboutDialog-html.patch (obsolete) (deleted) — — Splinter Review

Thanks, Henry for the feedback. Most of your comments addressed except the height for the #updateDeck. This is needed to not make jump the content below because of the different height of the elements that are alternating shown. And also the #experimental div is still there as it is used in JS to show or hide the part depending of the TB version. Or should I try to add a class to the childs and change getElementById() to getElementsByClassName()?

And Alessandro, about the throbber: because it is a <img> with the icon in src I don't know how to do it with the animation. Something for a follow-up bug?

And still valid:
Please can you help me with the Mac close key?

You also offered to do the Fluent conversion script. ;-)

Attachment #9240816 - Attachment is obsolete: true
Attachment #9240816 - Flags: review?(alessandro)
Attachment #9240924 - Flags: review?(alessandro)
Comment on attachment 9240924 [details] [diff] [review]
1728474-aboutDialog-html.patch

Review of attachment 9240924 [details] [diff] [review]:
-----------------------------------------------------------------

::: mail/base/content/aboutDialog.xhtml
@@ +117,5 @@
> +           data-l10n-id="community-exp">
> +          <a href="https://www.mozilla.org/" class="download-link"
> +             data-l10n-name="community-exp-mozillaLink"></a>
> +          <a onclick="openAboutTab('about:credits');"
> +             data-l10n-name="community-exp-creditsLink"></a>

Missing `href`

@@ +124,5 @@
> +      <p id="communityDesc" data-l10n-id="community-2">
> +        <a href="https://www.mozilla.org/" class="download-link"
> +           data-l10n-name="community-mozillaLink"></a>
> +        <a href="" onclick="openAboutTab('about:credits');"
> +           data-l10n-name="community-creditsLink"></a>

The `href` should probably be "about:credits". A `href` of "" doesn't mean an empty link, but refers to the current page. In this case, you can change `openAboutTab` to use the `href` of the target `<a>` and, to be on the safe side, call `event.preventDefault()`.

Same goes for the other `openAboutTab` anchors.

(In reply to Richard Marti (:Paenglab) from comment #17)

Thanks, Henry for the feedback. Most of your comments addressed except the height for the #updateDeck. This is needed to not make jump the content below because of the different height of the elements that are alternating shown.

This was addressed with the previous rules

/* Emulate the key feature of xul:stack */
#updateDeck {
  display: grid;
}
#updateDeck > * {
  grid-area: 1 / 1;
}
/* Make sure to hide content that is not selected, but ensure it still takes up space in the grid. */
.update-deck-container:not(.selected) {
  visibility: hidden;
}
/* And restore */
.update-deck-container {
  display: flex; /* do not use display: none */
}

This way, each child of #updateDeck will still take up their required space, and the #updateDeck will fit the largest of these.

And also the #experimental div is still there as it is used in JS to show or hide the part depending of the TB version. Or should I try to add a class to the childs and change getElementById() to getElementsByClassName()?

Yes, this would work. I think it may be more common to use querySelectorAll(".className") though.

You also offered to do the Fluent conversion script. ;-)

Yup, I'll do the migration recipe, which should land in the same patch, so once this is approved I will add the migration to it before the check in.

Attached patch 1728474-aboutDialog-html.patch (obsolete) (deleted) — — Splinter Review

This should now addressing all comments.

Attachment #9240924 - Attachment is obsolete: true
Attachment #9240924 - Flags: review?(alessandro)
Attachment #9240979 - Flags: review?(alessandro)
Comment on attachment 9240924 [details] [diff] [review]
1728474-aboutDialog-html.patch

Review of attachment 9240924 [details] [diff] [review]:
-----------------------------------------------------------------

Great work here!
Some extra tiny fixes to improve this a bit more.
Hopefully all these changes requests are not too annoying, but they're expected for such a nice and not tiny update.

::: mail/base/content/aboutDialog.js
@@ +12,5 @@
>    "resource://gre/modules/AppConstants.jsm"
>  );
>  
> +window.addEventListener("load", event => {
> +  init(event);

nit: just for consistency, let's call this method `onLoad()`

@@ +29,5 @@
>      return;
>    }
>  
>    try {
>      var distroId = Services.prefs.getCharPref("distribution.id");

nit: please convert all these `var` inside these try catch into `let`

@@ +34,5 @@
>      if (distroId) {
>        var distroVersion = Services.prefs.getCharPref("distribution.version");
>  
>        var distroIdField = document.getElementById("distributionId");
> +      distroIdField.textContent = distroId + " - " + distroVersion;

nit: distroIdField.textContent = `${distroId} - ${distroVersion}`;

@@ +39,1 @@
>        distroIdField.style.display = "block";

Since these are now pure html paragraphs, we can use the `hidden` attribute. You could set `hidden="hidden"` in the HTML and avoid using `display:none` in the CSS, and here use `distroIdField.hidden = false;`

@@ +49,1 @@
>          distroField.style.display = "block";

Same here with the hidden.

@@ +75,5 @@
>      document.getElementById("experimental").hidden = false;
>      document.getElementById("communityDesc").hidden = true;
>    }
>  
> +  // Use Fluent arguments for append version and the architecture of the build

Nit type: Use Fluent arguments to append the build's version and architecture.

::: mail/base/content/aboutDialog.xhtml
@@ +10,5 @@
>  
> +<html id="aboutDialog" xmlns="http://www.w3.org/1999/xhtml"
> +      role="dialog"
> +      aria-describedby="version distribution distributionId currentChannelText communityDesc contributeDesc trademark"
> +      >

nit: let's put the closing tag on the same line.

::: mail/themes/shared/mail/aboutDialog.css
@@ +36,4 @@
>  }
>  
>  #aboutDialog {
> +  appearance: none;

Is this necessary?

@@ +40,4 @@
>    /* Set an explicit line-height to avoid discrepancies in 'auto' spacing
>       across screens with different device DPI, which may cause font metrics
>       to round differently. */
>    line-height: 1.5;

The comment above is not really useful, we can remove it.
Let's specify the `em` unit for the line-height attribute.

@@ +47,5 @@
> +
> +body {
> +  display: flex;
> +  flex-direction: column;
> +  margin: 0;

do we need to set the body to be flex?
Maybe just by resetting the spacing and set the height is necessary.
margin: 0;
padding: 0;
height: 100%;

@@ +58,5 @@
> +}
> +
> +#logo {
> +  width: 210px;
> +  margin-bottom: auto;

No need for the margin-bottom: auto if we set align-items: flex-start; to the aboutDialogContainer.
Attachment #9240924 - Attachment is obsolete: false
Attachment #9240924 - Flags: feedback+

Ah, I just noticed you uploaded an update patch, sorry if we crossed the streams and you already addressed some of my comments.

Comment on attachment 9240979 [details] [diff] [review]
1728474-aboutDialog-html.patch

Review of attachment 9240979 [details] [diff] [review]:
-----------------------------------------------------------------

::: mail/base/content/aboutDialog.js
@@ +154,5 @@
>    m.launchExternalURL(url);
>  }
> +
> +function onKeyPress(aEvent) {
> +  if (aEvent.keyCode == aEvent.DOM_VK_ESCAPE) {

let's use `event` as attribute name.
Also, `keyCode` is deprecated and we shouldn't use it.
We can use this:
`if (event.key == "Escape" || (event.metaKey && event.key == "w"))` which handle the macOS accel + W closing shortcut.
Attachment #9240979 - Flags: review?(alessandro)

I noticed that with this patch the dialog first loads in the top left corner of the monitor and then jumps at the center.
This is more noticeable on macOS.
Anyone else experiencing this?

Another thing missing is to add the windowtype="mail:about" to the <html> tag, and update this method https://searchfox.org/comm-central/rev/6cc04e8018b418764f7eb1346cae4b6953b621f2/mail/base/content/mailCore.js#660-664 to match mail:about.

Comment on attachment 9240979 [details] [diff] [review]
1728474-aboutDialog-html.patch

Review of attachment 9240979 [details] [diff] [review]:
-----------------------------------------------------------------

As a general rule (not written anywhere but we're trying to follow it), we write fluent strings all lowercase with words separated by dashes.
If you can, please update those strings to reflect this.

::: mail/base/content/aboutDialog.xhtml
@@ +2,1 @@
>  

nit: remove empty line

@@ +23,3 @@
>  #endif
> +</head>
> +<body>

nit: blank line between head and body tag, and not blank line after opening the body tag.

@@ +26,2 @@
>  
> +  <div id="aboutDialogContainer">

Let's use semantic HTML5 to make this structure a bit more readable since we have a lot of indented divs.
This element can be `<main>`

@@ +26,4 @@
>  
> +  <div id="aboutDialogContainer">
> +    <img id="logo" src="chrome://branding/content/about-logo.svg" alt="" />
> +    <div id="rightBox">

This can be an `<aside>` element.

@@ +36,2 @@
>  #ifdef MOZ_UPDATER
> +      <div id="updateDeck">

`<section>`.
All its children can remain simple divs as they are now.

@@ +43,5 @@
> +        </div>
> +        <div id="downloadAndInstall" class="update-deck-container">
> +          <button id="downloadAndInstallButton"
> +                  onclick="gAppUpdater.startDownload();">
> +                  <!-- label and accesskey will be filled by JS -->

nit: ...be filled in JS

@@ +72,5 @@
> +               srcset="chrome://global/skin/icons/loading@2x.png 2x" />
> +          <span data-l10n-id="update-applying"></span>
> +        </div>
> +        <div id="downloadFailed" class="update-deck-container"
> +        data-l10n-id="update-failed">

nit: wrong indentation

For the throbber, we can convert those images into <span> containers and use this styling:
<span class="update-throbber"></span>

.update-throbber {
  width: 16px;
  height: 16px;
  margin-inline-end: 3px;
  position: relative;
  display: inline-block;
  overflow: hidden;
  color: SelectedItemText;
}

.update-throbber:after {
  position: absolute;
  content: '';
  background-image: url("chrome://messenger/skin/icons/loading.svg");
  background-position: left center;
  background-repeat: no-repeat;
  width: 480px;
  height: 100%;
  animation: loading-animation 1.05s steps(30) infinite;
}

@keyframes loading-animation {
  0% { transform: translateX(0); }
  100% { transform: translateX(-100%); }
}

As well as adding the rtl version.

I think some more work is needed to for the fluent implementation as I'm not sure adding strings to divs is accessible at all.
I'll later test this with the screen reader on windows, but the accessibility tab in the inspector should give us good insights as a starter.

Attached patch 1728474-aboutDialog-html.patch (obsolete) (deleted) — — Splinter Review

All comments should be addressed now.

(In reply to Alessandro Castellani [:aleca] from comment #22)

::: mail/themes/shared/mail/aboutDialog.css
@@ +36,4 @@

}

#aboutDialog {

  • appearance: none;

Is this necessary?

On Mac with dark TB theme enabled the dialog had still the light grey background. I managed it now with applying the background colour directly to the footer.

Attachment #9240924 - Attachment is obsolete: true
Attachment #9240979 - Attachment is obsolete: true
Attachment #9241110 - Flags: review?(alessandro)

(In reply to Alessandro Castellani [:aleca] from comment #25)

I noticed that with this patch the dialog first loads in the top left corner of the monitor and then jumps at the center.
This is more noticeable on macOS.
Anyone else experiencing this?

Yes I see this too on Mac.

(In reply to Alessandro Castellani [:aleca] from comment #25)

I noticed that with this patch the dialog first loads in the top left corner of the monitor and then jumps at the center.
This is more noticeable on macOS.
Anyone else experiencing this?

I don't notice this on GNOME/wayland. Before this change, where was the window positioned on macOS?

Maybe with the change to a toplevel html, rather than a xul:window, the window dispatcher isn't behaving well. This might be relevant to Bug 1703164, where a lot of dialogs are being converted to html.

(In reply to Henry Wilkes [:henry] from comment #31)

(In reply to Alessandro Castellani [:aleca] from comment #25)

I noticed that with this patch the dialog first loads in the top left corner of the monitor and then jumps at the center.
This is more noticeable on macOS.
Anyone else experiencing this?

I don't notice this on GNOME/wayland. Before this change, where was the window positioned on macOS?

Maybe with the change to a toplevel html, rather than a xul:window, the window dispatcher isn't behaving well. This might be relevant to Bug 1703164, where a lot of dialogs are being converted to html.

With the first patches which had still mixed XUL/HTML it didn't jump. I saw this starting with the latest versions.

Attached patch 1728474-aboutDialog-html.patch (deleted) — — Splinter Review

Updated after landing of bug 1707211 patches.

Attachment #9241110 - Attachment is obsolete: true
Attachment #9241110 - Flags: review?(alessandro)
Attachment #9241620 - Flags: review?(alessandro)

@aleca, I'm not sure it makes semantic sense for #rightBox to be an <aside> because the current structure is now

<main>
  <img />
  <aside id="rightBox">
    <!-- Content -->
  </aside>
</main>
<footer />

i.e. there is no text content that is in <main> but not in <aside>.

The <img /> element is the actual content of the main body, but maybe you're right and it should be the other way around, as the img element should be wrapped around an aside element as that's kind of the left sidebar of the dialog, and the rest of the content should be left outside.

(In reply to Alessandro Castellani [:aleca] from comment #35)

The <img /> element is the actual content of the main body, but maybe you're right and it should be the other way around, as the img element should be wrapped around an aside element as that's kind of the left sidebar of the dialog, and the rest of the content should be left outside.

Can you adapt the patch like you want?

Yes, no worries.
I need a little bit of time to properly review this, troubleshoot the issue with macOS, and add the fluent migration.
Sorry for the delay.

Henry, why did you flag this as a blocker for bug 1707211?
I think this should be independent and kind of a standalone bug.

Flags: needinfo?(henry)

(In reply to Alessandro Castellani [:aleca] from comment #38)

Henry, why did you flag this as a blocker for bug 1707211?
I think this should be independent and kind of a standalone bug.

This bug currently moves the aboutDialog.css from content to themes, which is what the depends on bug is for (see Bug 1707211 comment 40). Maybe its not the best use, but the other bug cannot be closed until this one is because of that. If this is the wrong usage, then you can unset it.

Flags: needinfo?(henry)
Comment on attachment 9241620 [details] [diff] [review]
1728474-aboutDialog-html.patch

Review of attachment 9241620 [details] [diff] [review]:
-----------------------------------------------------------------

I got an updated patch that fixes those few issues.
Attachment #9241620 - Flags: review?(alessandro)

This looks good for me, but I'm asking for extra review from Richard and Henry just to be sure I didn't miss anything.

Attachment #9242763 - Flags: review+
Attachment #9242763 - Flags: review?(richard.marti)
Attachment #9242763 - Flags: review?(henry)

This need a fluent migration recipe to avoid losing translations as we're not changing any string but only moving them to Fluent.
Magnus, is that okay if we work on a migration recipe, will we be able to run it independently?
Or do you think since this is early and we won't uplift this, we can leave it as is and expect proper translations before next ESR?

Flags: needinfo?(mkmelin+mozilla)

Migration recipes need to be run when the relevant code lands, it can't wait.

@flod, if we create a migration recipe, can you run it at your earliest convenience?
(Yes, we should get to a point where Thunderbird can do it independently, but we're not there yet.)

Flags: needinfo?(mkmelin+mozilla) → needinfo?(francesco.lodolo)

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

Migration recipes need to be run when the relevant code lands, it can't wait.

@flod, if we create a migration recipe, can you run it at your earliest convenience?

Yes, just ping me via Bugzilla when landing both patches, at least I know to keep an eye out for them and need to wait before exposing those strings in gecko-strings.

Flags: needinfo?(francesco.lodolo)

Comment on attachment 9242763 [details] [diff] [review]
1728474-aboutDialog-html-v2.patch

Thanks!

Attachment #9242763 - Flags: review?(richard.marti) → review+
Comment on attachment 9242763 [details] [diff] [review]
1728474-aboutDialog-html-v2.patch

Review of attachment 9242763 [details] [diff] [review]:
-----------------------------------------------------------------

::: mail/base/content/aboutDialog.js
@@ +21,5 @@
> +  });
> +}
> +window.addEventListener("keypress", event => {
> +  onKeyPress(event);
> +});

The arrow functions aren't needed. All these addEventListener callback arguments can be shortened to

```
window.addEventListener("load", onLoad);
if (AppConstants.MOZ_UPDATER) {
  window.addEventListener("unload", onUnload);
}
window.addEventListener("keypress", onKeyPress);
```

@@ +51,5 @@
>          // Pref is unset
>          Cu.reportError(ex);
>        }
>      }
>    } catch (e) {

Whilst this area is being tidied up, maybe the `try` blocks should only wrap the `Services.prefs.getCharPref` and `Services.prefs.getComplexValue` calls and nothing else. Currently, if the `distributionId` element was missing, this would silently fail, which I don't think is the intention.

Something like

```
let distroId;
try {
  distroId = Services.prefs.getCharPref("distribution.id");
} catch (e) {
  // Pref is unset
}
if (distroId) {
  // ...
  let distroAbout;
  try {
    distroAbout = Services.prefs.getComplexValue("distribution.about", Ci.nsISupportsString);
  } catch (e) {
    Cu.reportError(ex);
  }
  if (distroAbout) {
    // ...
  }
}
```

@@ +82,5 @@
> +  let versionField = document.getElementById("version");
> +
> +  document.l10n.setAttributes(versionField, versionId, versionAttributes);
> +
> +  await document.l10n.translateElements([versionField]);

I think this can be removed. Isn't `setAttributes` enough?

Also, the `onLoad` method doesn't need to be `async`. Even if you need this method, I don't think you need to `await` before you proceed.

@@ +112,4 @@
>  }
>  
>  // This function is used to open about: tabs. The caller should ensure the url
>  // is only an about: url.

Remove this comment.

::: mail/base/content/aboutDialog.xhtml
@@ +9,5 @@
>  
> +<!DOCTYPE html>
> +
> +<html id="aboutDialog" xmlns="http://www.w3.org/1999/xhtml"
> +      role="dialog"

Do you know if `role="dialog"` is necessary or works? This is already a desktop window with a title, it seems to not contribute to the accessibility tree, and the <html> element is listed as having no permitted roles in the aria specifications (https://w3c.github.io/html-aria/#el-html).

Similar with `aria-describedby` below.

@@ +40,3 @@
>  #ifdef MOZ_UPDATER
> +      <section id="updateDeck">
> +        <div id="checkForUpdates" class="update-deck-container">

We should probably get rid of these wrapper `update-deck-container` `div`s because they are semantically creating a further sub-section. Moreover, the `display: flex` of the container is suppressing the whitespace between text and anchor elements. This patch currently hacks back in the whitespace in the CSS. However, we are doing this regardless of if the localization contains the whitespace before the anchor. The whitespace should be left to the fluent file.

I suggest:

+ The `<button>` elements do not require any wrapper.
+ The other elements should be wrapped in a `<span>` instead of a `<div>`.

In the CSS:

+ Replace the `.update-deck-container { display: flex; align-items: center }` rules with `#updateContainer { display: grid; align-items: center}`.
+ Replace the `.update-deck-container:not(.selected) { visibility: hidden; }` rule with `#updateContainer > :not(.selected) { visibility: hidden; }`.
+ Remove the `#updateDeck .download-link { margin-inline-start: 1ch; }` rule.

The `update-throbber` elements will need some fixing. You can either use `vertical-align: center` (not quite the same), or restore `display: flex` for just these span elements.

::: mail/locales/en-US/messenger/aboutDialog.ftl
@@ +3,5 @@
> +# file, You can obtain one at http://mozilla.org/MPL/2.0/.
> +
> +aboutDialog-title = About { -brand-full-name }
> +
> +releaseNotes-link = Release notes

Should this id be "release-notes-link"?

Similarly, the other ids use a mix of camelCase and hyphens. They should probably just use lowercase and hyphens.

@@ +13,5 @@
> +    .accesskey = R
> +
> +update-checkingForUpdates = Checking for updates…
> +
> +update-downloading = <span data-l10n-name="icon"></span>Downloading update — <span data-l10n-name="download-status"></span>

The download status could be passed as a fluent variable. Also, if you're changing the dom structure, you could even avoid passing in the update-throbber element, unless you expect different localizations to not place it at the inline-start.

@@ +31,5 @@
> +update-unsupported = You can not perform further updates on this system. <a data-l10n-name="unsupported-link">Learn more</a>
> +
> +update-restarting = Restarting…
> +
> +channel-description = You are currently on the <span data-l10n-name="current-channel"></span> update channel.

It might be cleaner to use a fluent variable for the current channel and `setAttributes` in the `onLoad` method.

@@ +35,5 @@
> +channel-description = You are currently on the <span data-l10n-name="current-channel"></span> update channel.
> +
> +warningDesc-version = { -brand-short-name } is experimental and may be unstable.
> +
> +warningDesc-telemetry = It automatically sends information about performance, hardware, usage and customizations back to &vendorShortName; to help make &brandShortName; better.

Use fluent terms for the vendor and brand names.

@@ +39,5 @@
> +warningDesc-telemetry = It automatically sends information about performance, hardware, usage and customizations back to &vendorShortName; to help make &brandShortName; better.
> +
> +community-exp = <a data-l10n-name="community-exp-mozillaLink">{ -vendor-short-name }</a> is a <a data-l10n-name="community-exp-creditsLink">global community</a> working together to keep the Web open, public and accessible to all.
> +
> +community-2 = { -brand-short-name } is designed by <a data-l10n-name="community-mozillaLink">{ -vendor-short-name }</a>, a <a data-l10n-name="community-creditsLink">global community</a> working together to keep the Web open, public and accessible to all.

Maybe use a different id.

@@ +41,5 @@
> +community-exp = <a data-l10n-name="community-exp-mozillaLink">{ -vendor-short-name }</a> is a <a data-l10n-name="community-exp-creditsLink">global community</a> working together to keep the Web open, public and accessible to all.
> +
> +community-2 = { -brand-short-name } is designed by <a data-l10n-name="community-mozillaLink">{ -vendor-short-name }</a>, a <a data-l10n-name="community-creditsLink">global community</a> working together to keep the Web open, public and accessible to all.
> +
> +helpus = Want to help? <a data-l10n-name="helpus-donateLink">Make a donation</a> or <a data-l10n-name="helpus-getInvolvedLink">get involved!</a>

I think the data-l10n-names only need to be named relative to the element they lie within, so the "helpus-" prefix isn't of much use. Plus, "donateLink" and "getInvolvedLink" can be changed to "donate-link" and "get-involved-link" respectively.

@@ +50,5 @@
> +
> +bottomLinks-privacy = Privacy Policy
> +
> +aboutDialog-mac-close =
> +    .key = w

This seems to be unused.

::: mail/themes/shared/mail/aboutDialog.css
@@ +86,5 @@
>  
>  .update-deck-container {
>    display: flex;
>    align-items: center;
> +  font-style: italic;

This makes the button text italic. I don't think this was the case before.
Attachment #9242763 - Flags: review?(henry) → review-

(In reply to Henry Wilkes [:henry] from comment #46)

Whilst this area is being tidied up, maybe the try blocks should only wrap
the Services.prefs.getCharPref and Services.prefs.getComplexValue calls

For getCharPref() just add a second argument to denote default value if the pref is not found.

+helpus = Want to help? <a data-l10n-name="helpus-donateLink">Make a donation</a> or <a data-l10n-name="helpus-getInvolvedLink">get involved!</a>

I think the data-l10n-names only need to be named relative to the element
they lie within, so the "helpus-" prefix isn't of much use. Plus,
"donateLink" and "getInvolvedLink" can be changed to "donate-link" and
"get-involved-link" respectively.

For the things that are from Firefox, lets' not change those. https://searchfox.org/comm-central/source/mozilla/browser/locales/en-US/browser/aboutDialog.ftl#48

This makes the button text italic. I don't think this was the case before.

Will this help Bug 1799072 - release notes link in about dialog is truncated in German build before updates ?

Flags: needinfo?(richard.marti)

I don't think so. The issue is that the button is wide and the link text too.

Flags: needinfo?(richard.marti)
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: