Closed Bug 1524258 Opened 5 years ago Closed 5 years ago

Arrange by status arrow has no functionality in the block autoplay exceptions window

Categories

(Firefox :: Settings UI, defect, P3)

defect

Tracking

()

VERIFIED FIXED
Firefox 67
Tracking Status
firefox-esr60 --- unaffected
firefox65 --- disabled
firefox66 --- verified
firefox67 --- verified

People

(Reporter: dcicas, Assigned: jaws)

References

Details

(Keywords: regression)

Attachments

(2 files)

Attached image bug exceptions.gif

[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]:

  1. Reach the about:preferences page.
  2. Select the Privacy and security tab.
  3. Scroll down to permissions.
  4. Click on Exceptions for the "Block websites from automatically playing sounds".
  5. Add at least 3 random websites with at least 2 different statuses (Allow/Block).
  6. 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

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?

Blocks: 1491676
Flags: needinfo?(jaws)
Priority: -- → P3

(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.

Flags: needinfo?(daniel.cicas)

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".

Flags: needinfo?(daniel.cicas)
Assignee: nobody → jaws
Status: NEW → ASSIGNED
Flags: needinfo?(jaws)
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
Flags: needinfo?(jaws)
Attachment #9040787 - Attachment description: Bug 1524258 - Localized strings need to be applied on the permission rows before we can sort them by their values. r?Gijs → Bug 1524258 - Sort the permission capabilities based on their localization ID so we don't have to wait for the asynchronous localization. r?Gijs
Flags: needinfo?(jaws)
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
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 67

(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.

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.

Is this a wontfix for 66?

Flags: needinfo?(jaws)

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

Bug 1491676

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

Flags: needinfo?(jaws)
Attachment #9040787 - Flags: approval-mozilla-beta?

Daniel would you mind verifying since you found the issue originally? Thanks!

Flags: needinfo?(daniel.cicas)

(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. )

Has Regression Range: --- → yes

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.

Status: RESOLVED → VERIFIED
Flags: needinfo?(daniel.cicas)

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

Attachment #9040787 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Flags: qe-verify+
Whiteboard: [qa-triaged]

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.

Flags: qe-verify+
QA Whiteboard: [qa-triaged]
Whiteboard: [qa-triaged]
You need to log in before you can comment on or make changes to this bug.