Closed
Bug 1453486
Opened 6 years ago
Closed 6 years ago
Remove container alert has no text
Categories
(Firefox :: Settings UI, defect, P1)
Firefox
Settings UI
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)
59 bytes,
text/x-review-board-request
|
Gijs
:
review+
jcristau
:
approval-mozilla-beta+
|
Details |
59 bytes,
text/x-review-board-request
|
Gijs
:
review+
|
Details |
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
Reporter | ||
Comment 1•6 years ago
|
||
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
Comment 2•6 years ago
|
||
Adam, I take it this is reproducible on beta as well, then? That's not good... Gandalf, can you take a look?
status-firefox60:
--- → ?
status-firefox61:
--- → affected
Flags: needinfo?(gandalf)
Flags: needinfo?(agashlin)
Reporter | ||
Comment 3•6 years ago
|
||
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)
Assignee | ||
Comment 4•6 years ago
|
||
Taking!
Assignee: nobody → gandalf
Status: NEW → ASSIGNED
Flags: needinfo?(gandalf)
Comment 5•6 years ago
|
||
[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...
Assignee | ||
Comment 6•6 years ago
|
||
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.
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 9•6 years ago
|
||
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
Assignee | ||
Comment 10•6 years ago
|
||
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.
Assignee | ||
Comment 11•6 years ago
|
||
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)
Assignee | ||
Comment 12•6 years ago
|
||
Stefan - also something for you. Seems like that slipped through the 60 and 61 QA.
Flags: needinfo?(stefan.georgiev)
Comment 13•6 years ago
|
||
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)
Comment 14•6 years ago
|
||
(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 15•6 years ago
|
||
mozreview-review |
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 16•6 years ago
|
||
mozreview-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+
Comment 17•6 years ago
|
||
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)
Comment 18•6 years ago
|
||
(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)
Comment 19•6 years ago
|
||
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)
Updated•6 years ago
|
Comment 20•6 years ago
|
||
triage: giving this a priority. Looks like we intend to land this in this cycle, so p1.
Priority: -- → P1
Comment 21•6 years ago
|
||
Added test case to the Pre-Beta test suite for Fx61.
Flags: needinfo?(stefan.georgiev)
Comment hidden (mozreview-request) |
Comment hidden (mozreview-request) |
Assignee | ||
Comment 24•6 years ago
|
||
(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?
Assignee | ||
Comment 25•6 years ago
|
||
I filed bug 1453765 to discuss the exact error reporting strategy.
Comment 26•6 years ago
|
||
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
Comment 27•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/6b53ecf07367 https://hg.mozilla.org/mozilla-central/rev/e19ab548cb37
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 61
Assignee | ||
Comment 28•6 years ago
|
||
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 29•6 years ago
|
||
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+
Comment 30•6 years ago
|
||
bugherder uplift |
https://hg.mozilla.org/releases/mozilla-beta/rev/1210b05b97b0 https://hg.mozilla.org/releases/mozilla-beta/rev/7a38d42b6b69
Assignee | ||
Comment 31•6 years ago
|
||
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)
Comment 32•6 years ago
|
||
Backed out, sorry. Backout link: https://hg.mozilla.org/releases/mozilla-beta/rev/18f776b5b08011b84a75903883e961c1cd69249c
Flags: needinfo?(btara)
Updated•6 years ago
|
Flags: qe-verify+
Updated•6 years ago
|
Flags: qe-verify+
You need to log in
before you can comment on or make changes to this bug.
Description
•