Closed Bug 1453486 Opened 2 years ago Closed 2 years ago

Remove container alert has no text

Categories

(Firefox :: Preferences, defect, P1)

defect

Tracking

()

RESOLVED FIXED
Firefox 61
Tracking Status
firefox-esr52 --- unaffected
firefox59 --- unaffected
firefox60 + fixed
firefox61 --- fixed

People

(Reporter: agashlin, Assigned: zbraniecki)

References

Details

(Keywords: regression)

Attachments

(2 files)

Steps to reproduce
1. Create a tab in a container (File -> New Container Tab -> Personal)
2. Attempt to remove that container (File -> New Container Tab -> Manage containers, Remove button for Personal)

Expected result:
Message "If you remove this Container now..." etc, buttons "Remove this Container" and "Don't remove this Container"

Actual result:
Alert box with no message or title bar text, Ok/Cancel buttons


Regression range
https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=a26ef088407c9f9e671b3ef820ddc64af8b6c3ee&tochange=b6b8e5e12ca51f5f971bde890a62523632771da3
Also noticed that this happens with the following steps:
1. Create a tab in a container
2. Open Options
3. Uncheck "Enable Container Tabs"

Another box with no message or title bar shows up.

I got a regression range of https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=c7cab5776a7d5769319db2dfa79928c156059593&tochange=95eb6ee7690b0c326bfb1db0c12d37123fdc3fff

but I think https://hg.mozilla.org/integration/autoland/rev/9b1228043619
is the one that stuck.
Blocks: 1435912
Adam, I take it this is reproducible on beta as well, then? That's not good...

Gandalf, can you take a look?
Flags: needinfo?(gandalf)
Flags: needinfo?(agashlin)
Yes, in beta 60.0b11 I see the second issue (disabling Container tabs altogether has no text), but not the first (removing a Container with active tabs has no text).
Flags: needinfo?(agashlin)
Taking!
Assignee: nobody → gandalf
Status: NEW → ASSIGNED
Flags: needinfo?(gandalf)
[Tracking Requested - why for this release]:
User-visible UI regression.


Really hoping we can fix this without any actual l10n changes given that it's beta...
Fortunately, both are! Simple programmers mistake (mine) in both cases. I'll attach the fix in form of two patches so that we can uplift one of them to beta.
The `containers.ftl` inclusion will be upstreamed to beta. Fortunately the patch is very small.

It also triggered me to file two issues against Fluent-DOM API:
 - https://github.com/projectfluent/fluent.js/issues/180
 - https://github.com/projectfluent/fluent.js/issues/181
btw. both errors trigger console.warns. Is there any way to collect the list of warnings from Nightly users? It should help us catch any case where the request for l10n-ids comes back emptyhanded.

Another idea is that it would be nice to hook up Fluent to our test infrastructure to catch any case where fluent fires console.warn during testing.
Flod, Stas - Flagging you because while we likely want to solve it by just including `containers.ftl` in preferences.xul (to keep the fix simple for upstream to beta), the optimal solution would be to move the 4 strings from containers.ftl (which is the file we use in containers subdialog) to preferences.ftl (because they're only used in the in-content/containers.xul).

A good example of where ftl->ftl migration will be necessary to remove l10n from being a reason for technical debt accrue.
Flags: needinfo?(stas)
Flags: needinfo?(francesco.lodolo)
Stefan - also something for you. Seems like that slipped through the 60 and 61 QA.
Flags: needinfo?(stefan.georgiev)
With x-channel those strings will exist in two files for 4 cycles, more if we land them in 60. So, for 60, it's definitely good to include containers.ftl

I thought we had a bug tracking FTL to FTL migrations, but couldn't find it. Filed bug 1453557.

For >61 we have two ways to move those strings:
* File a bug to not lose track of it, blocked by bug 1453557.
* Create a new migration from DTD/properties (we still have those files in l10n repos). The risk is that we might lose changes done by localizers only to containers.ftl in the meantime: it should be low, and can be actually checked if needed via Transvision API.
Flags: needinfo?(francesco.lodolo)
(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #10)
> btw. both errors trigger console.warns. Is there any way to collect the list
> of warnings from Nightly users? It should help us catch any case where the
> request for l10n-ids comes back emptyhanded.

No. We have an experimental project that collects actual JS *errors*, but I don't think it'd get console.warn calls. Osmose, can you confirm?

TBH, for this type of failure I think "real" errors would be a fine way of indicating to the caller that stuff is broken. That would likely break enough that it'd fail tests if we got it wrong somehow.
Flags: needinfo?(mkelly)
Comment on attachment 8967203 [details]
Bug 1453486 - Fix Fluent API use when building a dialog to disable containers.

https://reviewboard.mozilla.org/r/235868/#review241708
Attachment #8967203 - Flags: review?(gijskruitbosch+bugs) → review+
Comment on attachment 8967204 [details]
Bug 1453486 - Include containers.ftl in main preferences.xul to fix the containers removal dialog.

https://reviewboard.mozilla.org/r/235870/#review241710
Attachment #8967204 - Flags: review?(gijskruitbosch+bugs) → review+
We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.

hg error in cmd: hg rebase -s 471b28cdc8dc5805a1cef1fe49b26973b4ac9740 -d 860e07a126c0: rebasing 457627:471b28cdc8dc "Bug 1453486 - Fix Fluent API use when building a dialog to disable containers. r=Gijs"
rebasing 457628:a37029a2c816 "Bug 1453486 - Include containers.ftl in main preferences.xul to fix the containers removal dialog. r=Gijs" (tip)
merging browser/components/preferences/in-content/preferences.xul
warning: conflicts while merging browser/components/preferences/in-content/preferences.xul! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)
(In reply to :Gijs (he/him) from comment #14)
> (In reply to Zibi Braniecki [:gandalf][:zibi] from comment #10)
> > btw. both errors trigger console.warns. Is there any way to collect the list
> > of warnings from Nightly users? It should help us catch any case where the
> > request for l10n-ids comes back emptyhanded.
> 
> No. We have an experimental project that collects actual JS *errors*, but I
> don't think it'd get console.warn calls. Osmose, can you confirm?

Correct, I don't think console.warn calls get collected, since they probably don't get nsScriptError instances built for them.
Flags: needinfo?(mkelly)
Thanks for flagging this. Clearing the needinfo because I don't think there's actually any info I can provide right now. Let's discuss FTL->FTL moves in bug 1453557.
Flags: needinfo?(stas)
triage: giving this a priority. Looks like we intend to land this in this cycle, so p1.
Priority: -- → P1
Added test case to the Pre-Beta test suite for Fx61.
Flags: needinfo?(stefan.georgiev)
(In reply to :Gijs (he/him) from comment #14)
> TBH, for this type of failure I think "real" errors would be a fine way of
> indicating to the caller that stuff is broken. That would likely break
> enough that it'd fail tests if we got it wrong somehow.

That's a great feedback. We have a lot of room to fine tune how severely we want to report things.

I'm wondering if it should be a channel dependent behavior or debug/optimize diff. Breaking with an Error is very useful for devs and test env., but useless for the user.

For the user I'd prefer to salvage as much of the UI as possible. What do you think?
I filed bug 1453765 to discuss the exact error reporting strategy.
Pushed by zbraniecki@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/6b53ecf07367
Fix Fluent API use when building a dialog to disable containers. r=Gijs
https://hg.mozilla.org/integration/autoland/rev/e19ab548cb37
Include containers.ftl in main preferences.xul to fix the containers removal dialog. r=Gijs
https://hg.mozilla.org/mozilla-central/rev/6b53ecf07367
https://hg.mozilla.org/mozilla-central/rev/e19ab548cb37
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 61
Comment on attachment 8967203 [details]
Bug 1453486 - Fix Fluent API use when building a dialog to disable containers.

Approval Request Comment
[Feature/Bug causing the regression]: Bug 1435912
[User impact if declined]: Disabling containers warning alert in Preferences is empty
[Is this code covered by automated tests?]: no
[Has the fix been verified in Nightly?]: yes
[Needs manual test from QE? If yes, steps to reproduce]: yes
1) Launch Nightly
2) Open Preferences
3) Add a container
4) Open a tab with the new container
5) Try to disable containers in Preferences

Expected result:

The warning subdialog shows up with text
[List of other uplifts needed for the feature/fix]: none
[Is the change risky?]: no
[Why is the change risky/not risky?]: It's a 4 line change which was verified to work in Nightly
[String changes made/needed]: none
Attachment #8967203 - Flags: approval-mozilla-beta?
Comment on attachment 8967203 [details]
Bug 1453486 - Fix Fluent API use when building a dialog to disable containers.

regression fix in preferences, approved for 60.0b13
Attachment #8967203 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Bogdan, I'm afraid you landed two changes, while only one was requested and intended for beta. Can you please back out https://hg.mozilla.org/releases/mozilla-beta/rev/7a38d42b6b69 from beta?
Flags: needinfo?(btara)
You need to log in before you can comment on or make changes to this bug.