Closed Bug 1327947 Opened 7 years ago Closed 7 years ago

Impossible to change sorting in about:preferences#applications

Categories

(Firefox :: Settings UI, defect, P3)

defect

Tracking

()

RESOLVED FIXED
Firefox 56
Tracking Status
firefox-esr52 --- wontfix
firefox54 --- wontfix
firefox55 --- fixed
firefox56 --- fixed

People

(Reporter: arni2033, Assigned: jkt)

References

(Blocks 1 open bug)

Details

(Keywords: regression, Whiteboard: [userContextId][domsecurity-backlog2])

Attachments

(1 file)

>>>   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.
Component: Untriaged → Preferences
No longer blocks: 1277113
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.
OS: Unspecified → All
Priority: -- → P3
Hardware: Unspecified → All
Whiteboard: [userContextId][domsecurity-backlog2]
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
Flags: needinfo?(jkt)
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.
Flags: needinfo?(jkt)
Assignee: nobody → jkt
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?
Attachment #8883777 - Flags: review?(jaws) → review-
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.
Attachment #8883777 - Flags: review?(jaws) → review-
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 :)
Attachment #8883777 - Flags: review?(jaws) → review+
Keywords: checkin-needed
Autoland can't push this until all pending issues in MozReview are marked as resolved.
Keywords: checkin-needed
Ah sorry, I forget every time. I wish there was a warning on that tag.
Thanks.
Keywords: checkin-needed
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
Keywords: checkin-needed
[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)
Flags: needinfo?(jkt)
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,
Flags: needinfo?(jkt) → needinfo?(jaws)
It still has the r+, you might just need to "reopen" the review request then you can re-land it.
Flags: needinfo?(jaws)
Keywords: checkin-needed
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
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/ccfc18b43605
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 56
This grafts cleanly to Beta. Is it worth considering for backport or should we let it ride the 56 train?
Flags: needinfo?(jkt)
Let's backport it to Beta.
Flags: needinfo?(jkt)
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
Attachment #8883777 - Flags: approval-mozilla-beta?
Comment on attachment 8883777 [details]
Bug 1327947 - Remove id's from about:preferences#containers as it breaks sorting for applications.

preferences fix for beta55
Attachment #8883777 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
(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.
Flags: qe-verify-
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: