Arrange by status arrow has no functionality in the block autoplay exceptions window
Categories
(Firefox :: Settings UI, defect, P3)
Tracking
()
Tracking | Status | |
---|---|---|
firefox-esr60 | --- | unaffected |
firefox65 | --- | disabled |
firefox66 | --- | verified |
firefox67 | --- | verified |
People
(Reporter: dcicas, Assigned: jaws)
References
Details
(Keywords: regression)
Attachments
(2 files)
111.32 KB,
image/gif
|
Details | |
47 bytes,
text/x-phabricator-request
|
jcristau
:
approval-mozilla-beta+
|
Details | Review |
[Affected versions]:
-Fx 66.0b3 Build ID 20190128143734
-Fx 67.0a1 Build ID 20190131093752
[Affected platforms]:
- Win 10 x64
- Ubuntu 18.04 x64
- macOS 10.13
[Steps to reproduce]:
- Reach the about:preferences page.
- Select the Privacy and security tab.
- Scroll down to permissions.
- Click on Exceptions for the "Block websites from automatically playing sounds".
- Add at least 3 random websites with at least 2 different statuses (Allow/Block).
- Click the arrow button to arrange the websites by status.
[Expected result]:
- The websites are arranged by status.
[Actual result]:
- The arrow button does not arrange the websites by status.
[Regression range]:
Found commit message:
Bug 1491676 - Moves permissions strings r=flod,Gijs,zbraniecki
Comment 1•5 years ago
|
||
The regressing bug landed in 65, but this was marked as unaffected. Can you re-test 65?
If we've already shipped this I guess it's not super important, but OTOH this is a recent functional regression. Erring on the side of P3 right now; if this somehow isn't broken in 65 then I'd like to use P1 and get this fixed in 66 so we don't ship the regression. Jared, that sound OK? Do you know if Jack might have time to help fix this given he wrote the regressing patches?
Comment 2•5 years ago
|
||
(In reply to :Gijs (he/him) from comment #1)
The regressing bug landed in 65, but this was marked as unaffected. Can you re-test 65?
+ni for this.
Reporter | ||
Comment 3•5 years ago
|
||
Hello,
Unfortunately, I cannot reproduce this bug on Fx 65.0 (Release) because on Fx 65.0 there is no option to "Block websites from automatically playing sounds".
Assignee | ||
Comment 4•5 years ago
|
||
This is the culprit,
https://hg.mozilla.org/integration/autoland/rev/d364b0b15fa5#l2.139
Assignee | ||
Comment 5•5 years ago
|
||
Updated•5 years ago
|
Pushed by jwein@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/5983bd842b52 Localized strings need to be applied on the permission rows before we can sort them by their values. r=Gijs
Comment 7•5 years ago
|
||
Backed out changeset 5983bd842b52 (bug 1524258) for Browser-chrome failures in browser/components/preferences/in-content/tests/browser_cookies_exceptions.js. CLOSED TREE.
Log:
https://treeherder.mozilla.org/logviewer.html#/jobs?job_id=226030481&repo=autoland&lineNumber=12420
Push with failures:
https://treeherder.mozilla.org/#/jobs?repo=autoland&searchStr=bc&revision=5983bd842b524010654be71b6e31bc52c86a5179
Backout:
https://hg.mozilla.org/integration/autoland/rev/bad9be613a20fd3d91f9155ec1e806a011f3ca8e
Updated•5 years ago
|
Assignee | ||
Updated•5 years ago
|
Pushed by jwein@mozilla.com: https://hg.mozilla.org/integration/autoland/rev/d5758ae506b2 Sort the permission capabilities based on their localization ID so we don't have to wait for the asynchronous localization. r=Gijs
Comment 9•5 years ago
|
||
bugherder |
Comment 10•5 years ago
|
||
Why not just enforce localization here: https://searchfox.org/mozilla-central/rev/e00ea598e52bbb35f8c45abf9c2eade17962bb5e/browser/components/preferences/permissions.js#393 with await document.l10n.translateFragment(frag);
?
Comment 11•5 years ago
|
||
(In reply to Zibi Braniecki [:gandalf][:zibi] from comment #10)
Why not just enforce localization here: https://searchfox.org/mozilla-central/rev/e00ea598e52bbb35f8c45abf9c2eade17962bb5e/browser/components/preferences/permissions.js#393 with
await document.l10n.translateFragment(frag);
?
Careful with the "just" there. The earlier version of this patch tried to do exactly that, the problem is (as always) that it's more complicated than that 1 line, because then buildPermissionsList
becomes an async method, as do its callers, and then we need to update tests and various other things that expect that buildPermissionsList
(and its callers) guarantees that at its (sync) completion, the list is populated and has all the items it should have after an add/remove operation, we need to consider what happens if 2 calls to these methods race, etc.
That was proving problematic (see backout in comment 7) and the current version is more upliftable. The cosmetic problem that the direction of the arrow doesn't necessarily match the alphabetical sorting of "allow" and "block" in some languages doesn't really impact on the usefulness of the dialogue, whereas having sorting being completely broken clearly does impact.
We can investigate making this better in a follow-up, but it's definitely not straightforward. I would actually be tempted to suggest caching the strings somewhere so they can be used synchronously when building the list, but Jared mentioned to me that he tried that and that was also complex. Either way, fodder for a follow-up bug.
Comment 12•5 years ago
|
||
The earlier version of this patch tried to do exactly that, the problem is (as always) that it's more complicated than that 1 line,
Right, of course :) I'm sorry for making it sound as if it was easy. Turning sync chain of code to async is sometimes easy but often hard.
We can investigate making this better in a follow-up, but it's definitely not straightforward. I would actually be tempted to suggest caching the strings somewhere so they can be used synchronously when building the list, but Jared mentioned to me that he tried that and that was also complex. Either way, fodder for a follow-up bug.
I'd prefer to avoid that. Storing strings in both DOM and non-DOM means that you need to invalidate/update etc. on retranslations and generally leads to out-of-sync visuals.
In the ideal world I'd prefer such sorting to be the self-contained component on a menulist or tree column which reacts using mutation observer to changes to text. Imho that would be more reusable and easier to maintain since sorting is exclusively blocked on textual values in the content and should be fired on each text change. The fact that text comes from l10n is not necessarily related.
Updated•5 years ago
|
Assignee | ||
Comment 14•5 years ago
|
||
Comment on attachment 9040787 [details]
Bug 1524258 - Sort the permission capabilities based on their localization ID so we don't have to wait for the asynchronous localization. r?Gijs
Beta/Release Uplift Approval Request
Feature/Bug causing the regression
User impact if declined
Users cannot sort the Block Autoplay exceptions
Is this code covered by automated tests?
No
Has the fix been verified in Nightly?
No
Needs manual test from QE?
No
If yes, steps to reproduce
List of other uplifts needed
None
Risk to taking this patch
Low
Why is the change risky/not risky? (and alternatives if risky)
This is a minimal patch that sorts the exceptions, though with the goal of being minimal the arrow (ascending/descending) may show the wrong direction based on the locale of the user (this happens to work fine for English-US but not necessarily for other languages). However, the grouping of Allowed vs Blocked will still function.
String changes made/needed
none
Comment 15•5 years ago
|
||
Daniel would you mind verifying since you found the issue originally? Thanks!
Comment 16•5 years ago
|
||
(Note: Seems reasonable to uplift and I see you put some thought into making it so, but why not verify - "no test coverage, no manual testing, not actually urgent, just uplift it in mid beta" is hard for me to justify. )
Updated•5 years ago
|
Reporter | ||
Comment 17•5 years ago
|
||
Hello,
I can confirm this issue is fixed, I verified using Fx 67.0a1 BuildID: 20190217214556, on Windows 10 x64, macOS X 10.13 and Ubuntu 18.04 LTS.
Comment 18•5 years ago
|
||
Comment on attachment 9040787 [details]
Bug 1524258 - Sort the permission capabilities based on their localization ID so we don't have to wait for the asynchronous localization. r?Gijs
fix regression in preferences, verified in nightly, approved for 66.0b9
Comment 19•5 years ago
|
||
bugherder uplift |
Updated•5 years ago
|
Updated•5 years ago
|
Reporter | ||
Comment 20•5 years ago
|
||
Hello,
I can confirm this issue is fixed, I verified using Fx 66.0b9 BuildID: 20190218131312, on Windows 10 x64, macOS X 10.13 and Ubuntu 18.04 LTS.
Updated•5 years ago
|
Description
•