>>> My Info: Win7_64, Nightly 53, 32bit, ID 20161219030207 (2016-12-19) STR_1: 1. Open about:preferences#applications 2. Click on "Action" AR: No visible action ER: Applications should be sorted by "Action" This is regression from bug 1267916. Regression range: > https://hg.mozilla.org/integration/mozilla-inbound/pushloghtml?fromchange=f54a717c6fc663d5ccad40340c4139282b0607a1&tochange=964223d37a2e59a28c4ee1df5a808252442ea94f@ Jonathan Kingston [:jkt]: It seems that this is a regresion caused by your change. Please have a look.

Bug still exists in official 53.0 release: [1] Open about:preferences#applications [2] Click on "Content Type" or "Action" header to sort by said in either ascending or descending order AR: No sorting of either column ER: Sorting should occur when clicking either column header according to column header type in either ascending or descending order Note: Stuck in "Content Type" ascending order.

Jonathan, mind taking a look whenever you have some time? Looks like bug#1267916 regressed this as per comment#0. Builds: * fx54.0, buildid: 20170608105825, changeset: e832ed037a3c - reproduced * fx55.0b3, buildid: 20170619071954, changeset: be4a0ad5d6ca - reproduced * fx56.0a1, buildid: 20170620030208, changeset: 7a6baa6cca32 - reproduced Platforms: * macOS 10.12.5 x64 - reproduced * Ubuntu 16.04.2 LTS x64 VM - reproduced * Win 10 x64 VM - reproduced

This looks like in current nightly it has been fixed... when not in a container?

Ah I see, it doesn't reproduce it you go to about:preferences first.

Comment on attachment 8883777 [details] Bug 1327947 - Remove id's from about:preferences#containers as it breaks sorting for applications. https://reviewboard.mozilla.org/r/154718/#review159862 ::: browser/components/preferences/in-content-new/containers.xul:38 (Diff revision 1) > <listheader equalsize="always"> > - <treecol id="typeColumn" value="type" > + <treecol value="type" > persist="sortDirection" > flex="1" sortDirection="ascending"/> > - <treecol id="actionColumn" value="action" > + <treecol value="action" > persist="sortDirection" > flex="1"/> > </listheader> It doesn't look like anything here inside of the <richlistbox> is needed. I removed it locally and the Containers page worked fine. Can we just remove this?

Comment on attachment 8883777 [details] Bug 1327947 - Remove id's from about:preferences#containers as it breaks sorting for applications. https://reviewboard.mozilla.org/r/154718/#review159866 I forgot to post this in the first review, but thank you for finding this. It makes perfect sense now! :) Also, we should have a test that tries to sort the applications view to make sure that is working and we don't break it silently again.

Good call, added a pretty simple test for both headers and removed that whole listheader section as you mention it's not used.

Comment on attachment 8883777 [details] Bug 1327947 - Remove id's from about:preferences#containers as it breaks sorting for applications. https://reviewboard.mozilla.org/r/154718/#review160488 ::: browser/components/preferences/in-content-new/tests/browser_applications_selection.js:24 (Diff revision 2) > +add_task(async function getFeedItem() { > + win = gBrowser.selectedBrowser.contentWindow; Please duplicate the test changes in /browser/components/preferences/in-content/tests/browser_applications_selection.js ::: browser/components/preferences/in-content-new/tests/browser_applications_selection.js:30 (Diff revision 2) > + const oldDir = typeColumn.getAttribute("sortDirection"); > + Assert.equal(oldDir, > + typeColumn.getAttribute("sortDirection"), > + "Sort direction should change"); This is a good start, but this is missing actually trying to click on the headers to change the sort direction, and then verifying that the sort direction changed.

Whoops yeah for some reason I removed the clicks I added when adding checking for both columns which is odd. Either way I added actual checking of the sorting now. We can remove the sorting code if it doesn't work, I don't really much have much time to perfect it (maybe we could file follow ups if there are issues?) however it seems to work for the existing values.

Comment on attachment 8883777 [details] Bug 1327947 - Remove id's from about:preferences#containers as it breaks sorting for applications. https://reviewboard.mozilla.org/r/154718/#review160690 Thank you! Looks good :)

Autoland can't push this until all pending issues in MozReview are marked as resolved.

Ah sorry, I forget every time. I wish there was a warning on that tag. Thanks.

Pushed by ryanvm@gmail.com: https://hg.mozilla.org/integration/autoland/rev/a8d854de191c Remove id's from about:preferences#containers as it breaks sorting for applications. r=jaws

[Tracking Requested - why for this release]: regression introduced in Firefox 52, would be good to uplift the fix to beta if possible.

Backed out for eslint failures in browser_applications_selection.js: https://treeherder.mozilla.org/logviewer.html#?job_id=113036658&repo=autoland Push with failure: https://treeherder.mozilla.org/#/jobs?repo=autoland&revision=a8d854de191c2403e82f4e55d3d6f08c47dfcc2a&filter-resultStatus=testfailed&filter-resultStatus=busted&filter-resultStatus=exception&filter-resultStatus=retry&filter-resultStatus=usercancel&filter-resultStatus=runnable Failure log: https://treeherder.mozilla.org/logviewer.html#?job_id=113036658&repo=autoland TEST-UNEXPECTED-ERROR | /home/worker/checkouts/gecko/browser/components/preferences/in-content-new/tests/browser_applications_selection.js:84:9 | 'container' is already declared in the upper scope. (no-shadow) TEST-UNEXPECTED-ERROR | /home/worker/checkouts/gecko/browser/components/preferences/in-content-new/tests/browser_applications_selection.js:85:9 | 'firstItem' is assigned a value but never used. (no-unused-vars) TEST-UNEXPECTED-ERROR | /home/worker/checkouts/gecko/browser/components/preferences/in-content/tests/browser_applications_selection.js:84:9 | 'container' is already declared in the upper scope. (no-shadow) TEST-UNEXPECTED-ERROR | /home/worker/checkouts/gecko/browser/components/preferences/in-content/tests/browser_applications_selection.js:85:9 | 'firstItem' is assigned a value but never used. (no-unused-vars)

Backout by archaeopteryx@coole-files.de: https://hg.mozilla.org/integration/autoland/rev/d34a3e340947 Backed out changeset a8d854de191c for eslint failures in browser_applications_selection.js. r=backout

Hmm my setup doesn't even run that linting... will fix. Sorry about that.

Hey could I get an r+ on this again, sorry,

It still has the r+, you might just need to "reopen" the review request then you can re-land it.

Pushed by ryanvm@gmail.com: https://hg.mozilla.org/integration/autoland/rev/ccfc18b43605 Remove id's from about:preferences#containers as it breaks sorting for applications. r=jaws

This grafts cleanly to Beta. Is it worth considering for backport or should we let it ride the 56 train?

Let's backport it to Beta.

Comment on attachment 8883777 [details] Bug 1327947 - Remove id's from about:preferences#containers as it breaks sorting for applications. Approval Request Comment [Feature/Bug causing the regression]: regressed by bug 1267916 [User impact if declined]: sorting the applications view of preferences is broken [Is this code covered by automated tests?]: yes [Has the fix been verified in Nightly?]: no [Needs manual test from QE? If yes, steps to reproduce]: no [List of other uplifts needed for the feature/fix]: none [Is the change risky?]: no [Why is the change risky/not risky?]: code removal isolated to preferences. the bug was that containers introduced new (dead) code that re-used the same IDs which caused a conflict when the applications pane tried to retrieve elements based on their ID [String changes made/needed]: none

Comment on attachment 8883777 [details] Bug 1327947 - Remove id's from about:preferences#containers as it breaks sorting for applications. preferences fix for beta55

(In reply to Jared Wein [:jaws] (please needinfo? me) from comment #28) > [Is this code covered by automated tests?]: yes > [Has the fix been verified in Nightly?]: no > [Needs manual test from QE? If yes, steps to reproduce]: no Setting qe-verify- based on Jared's assessment on manual testing needs and the fact that this fix has automated coverage.