2061642 - Missing space between the two sentences in the calendar conflict dialog

Status: RESOLVED FIXED

(Calendar :: Dialogs, task)

Description

[Nico D.]

The conflict dialog builds its text from two Fluent strings and glues them together without a separator, so it reads "...changed on the server.Submitting your changes will overwrite..." - no space after the full stop.

calendar-conflicts-dialog.js:

  descr.textContent = lazy.l10n.formatValueSync("item-modified-on-server");
  ...
  descr.textContent += lazy.l10n.formatValueSync("modify-will-lose-data");

Both branches are affected, the modify one and the delete one.

Link to the code:
https://searchfox.org/comm-central/source/calendar/base/content/dialogs/calendar-conflicts-dialog.js#23-30

There is no automated check for this one. The dialog only shows up when a cached calendar syncs an offline change the server has moved on from, which is awkward to reproduce, so reading the code is the practical way to see it.

Comment 1

[Nico D.]

The concatenation goes back to bug 380060 (2011), which added the dialog. The comment above it originally read "These strings should move to DTD files, but we don't want to disrupt string freeze right now" - bug 1893758 did move them, to Fluent, without changing how they are joined.

Comment 2

[Nico D.]

Attachment: Before: Dialog with the two sentences run together after "on the server."

Comment 3

[Nico D.]

Correction on the direction: inserting a space would be the wrong fix. Joining two localized sentences in JS is the actual defect - the separator
between them is a property of the language, not of the code, and some locales would want to merge or reorder the two sentences, which concatenation makes impossible.

The fix is to give each sentence its own element with its own data-l10n-id instead of building the text with textContent +=.

There is also a long-standing // TODO This dialog should be reworked! right above that code. This bug is not that rework, just the string handling.

Comment 4

[Magnus Melin [:mkmelin]]

Well... I think just joining with space should work alright. We shouldn't overthink localization needs.

Comment 5

[Nico D.]

Attachment: Bug 2061642 - Add the missing space between the two sentences in the calendar conflict dialog. r=mkmelin

The second sentence was appended directly to the first, so the dialog read
"changed on the server.Submitting your changes will overwrite ...".

Joining the two sentences in JS fixes the separator for every locale. In ja
and zh-CN both end with an ideographic full stop, which already carries the
spacing, so the added space is superfluous there. Giving each sentence its
own element would leave the separator to the locale, but that is more than
this bug is about.

Comment 6

[Nico D.]

Attachment: After fix: Dialog - modify branch

Comment 7

[Nico D.]

Attachment: After fix: Dialog - delete branch

Comment 8

[Nico D.]

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

Well... I think just joining with space should work alright. We shouldn't overthink localization needs.

Agreed - the patch just joins the two sentences with a space. Comment #3 was overthinking it.

One thing I checked before deciding, for the record: in ja and zh-CN both sentences end with the ideographic full stop, which already carries the spacing, so the added space is superfluous there. That note is in the commit message too. It is not a reason to restructure the dialog, and the // TODO This dialog should be reworked! right above that code is where such a change would belong anyway.

Patch is up as D318686. I triggered both branches by hand in a debug build, the offline modify and the offline delete of the same item; the screenshots show the dialog before and after the change.

Comment 9

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/comm-central/rev/9c957336b638
Add the missing space between the two sentences in the calendar conflict dialog. r=mkmelin