'reset filters' doesn't persist filter prefs

VERIFIED FIXED in Firefox 57

Status

defect
P1
normal
VERIFIED FIXED
2 years ago
Last year

People

(Reporter: whittieralaska, Assigned: nchevobbe)

Tracking

57 Branch
Firefox 58
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox57 verified, firefox58 verified)

Details

(Whiteboard: [reserve-console-html])

Attachments

(2 attachments)

User Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:57.0) Gecko/20100101 Firefox/57.0
Build ID: 20171016185129

Steps to reproduce:

- Right click
- Click on "Inspect Element"
- Click on the console tab


Actual results:

Console only shows errors and warnings, so I need to reset the filters to see everything.


Expected results:

I should be able to see everything in console without needing to reset the filters every time I open console.
Component: Untriaged → Developer Tools: Console
Tried restarting in safe mode, issue still exists.
(In reply to Zach from comment #1)
> Tried restarting in safe mode, issue still exists.

Do you see this issue on any page?  Like `data:text/html,<script>console.log("foo")</script>`?  If so, then when you reset the filters and close/reopen the console do they come back again?
Flags: needinfo?(whittieralaska)
Also, if log messages are being filtered out can you check the value of the "devtools.webconsole.filter.log" pref?
(In reply to Brian Grinstead [:bgrins] from comment #2)
> (In reply to Zach from comment #1)
> > Tried restarting in safe mode, issue still exists.
> 
> Do you see this issue on any page?  Like
> `data:text/html,<script>console.log("foo")</script>`?  If so, then when you
> reset the filters and close/reopen the console do they come back again?

It happens on any page and it goes back to only filtering errors and warnings when I close/reopen console.
(In reply to Brian Grinstead [:bgrins] from comment #3)
> Also, if log messages are being filtered out can you check the value of the
> "devtools.webconsole.filter.log" pref?
its set to false
Flags: needinfo?(whittieralaska)
OK thanks for reporting this - I'm seeing it as well. Nicolas, can you confirm these STR:

* Open data:text/html,<script>console.log("foo")</script>
* Open console, disable 'logs' filter
* Click 'reset filters' (message shows back up and logs get enabled again)
* Close and reopen console
* Log filter is still disabled
Flags: needinfo?(nchevobbe)
Summary: Console automatically filters when opened → 'reset filters' doesn't persist filter prefs
Whiteboard: [console-html][triage]
(In reply to Zach from comment #4)
> (In reply to Brian Grinstead [:bgrins] from comment #2)
> > (In reply to Zach from comment #1)
> > > Tried restarting in safe mode, issue still exists.
> > 
> > Do you see this issue on any page?  Like
> > `data:text/html,<script>console.log("foo")</script>`?  If so, then when you
> > reset the filters and close/reopen the console do they come back again?
> 
> It happens on any page and it goes back to only filtering errors and
> warnings when I close/reopen console.

A workaround in the meantime instead of doing reset is to open the filter menu by clicking on the funnel icon to the left of the filter textbox, and then click on 'logs' and whatever other filters that are off to toggle them on permanently.
Assignee: nobody → nchevobbe
Status: NEW → ASSIGNED
Priority: -- → P1
The fix was an easy one, and I added some tests to make sure we handle prefs as expected.
Since it's a small change, I hope we'll be able to uplift it to beta.
Comment on attachment 8920106 [details]
Bug 1409836 - Fix setting filters prefs when filters are reset or cleared; .

https://reviewboard.mozilla.org/r/191096/#review196382

::: devtools/client/webconsole/new-console-output/actions/filters.js:47
(Diff revision 1)
>    return (dispatch, getState) => {
>      dispatch({
>        type: FILTERS_CLEAR,
>      });
>  
> -    const filterState = getAllFilters(getState());
> +    const filterState = getAllFilters(getState()).toJS();

Is this change required? It looks like the fix is below in defaultFiltersReset - I'm not sure what this is doing

::: devtools/client/webconsole/new-console-output/test/store/filters.test.js:230
(Diff revision 1)
> +      "css": true,
>        "net": true,
>        "netxhr": true,
>        "text": "foobar",
>      });
> +    let prefs = ServicesMock.prefs.testHelpers.getAllPrefs();

Nit: could you rewrite these assertion and the ones  below to be analagous to the one above, like:

expect(prefs).toEqual({
  [PREFS.FILTER.WARN]: true,
  [PREFS.FILTER.LOG]: true,
  ...
});

May be easier to compare the two states / copy for new tests that way.

::: devtools/client/webconsole/new-console-output/test/store/filters.test.js:269
(Diff revision 1)
>    });
>  });
>  
>  describe("Resets filters", () => {
>    it("resets default filters value to their original one.", () => {
> +    ServicesMock.prefs.testHelpers.clearPrefs();

Nit: would it be possible to add something like this at the top of the file (i.e. outisde of any `describe`).  And if so, would it make sense to do so (i.e. do we not rely on persisted pref state between individual tests)?

  afterEach(() => {
    ServicesMock.prefs.testHelpers.clearPrefs();
  });
Attachment #8920106 - Flags: review?(bgrinstead) → review+
Whiteboard: [console-html][triage] → [reserve-console-html]
Comment on attachment 8920106 [details]
Bug 1409836 - Fix setting filters prefs when filters are reset or cleared; .

https://reviewboard.mozilla.org/r/191096/#review196382

> Is this change required? It looks like the fix is below in defaultFiltersReset - I'm not sure what this is doing

So this is needed for the `clearFilters` actions. This goes through all filters and clear them.
The action is only used in tests to tidy up things between tests.
Comment on attachment 8920106 [details]
Bug 1409836 - Fix setting filters prefs when filters are reset or cleared; .

https://reviewboard.mozilla.org/r/191096/#review196382

> Nit: could you rewrite these assertion and the ones  below to be analagous to the one above, like:
> 
> expect(prefs).toEqual({
>   [PREFS.FILTER.WARN]: true,
>   [PREFS.FILTER.LOG]: true,
>   ...
> });
> 
> May be easier to compare the two states / copy for new tests that way.

yeah, could be. I initially did not do it like that because prefs hold the persist and filter bar visibility as well. But we can have another helper in ServicesMock to only return filter prefs.

> Nit: would it be possible to add something like this at the top of the file (i.e. outisde of any `describe`).  And if so, would it make sense to do so (i.e. do we not rely on persisted pref state between individual tests)?
> 
>   afterEach(() => {
>     ServicesMock.prefs.testHelpers.clearPrefs();
>   });

I need to check. Ideally each test should have a clean slate, so I'm willing to do this.
(In reply to Nicolas Chevobbe [:nchevobbe] from comment #13)
> > Nit: could you rewrite these assertion and the ones  below to be analagous to the one above, like:
> > 
> > expect(prefs).toEqual({
> >   [PREFS.FILTER.WARN]: true,
> >   [PREFS.FILTER.LOG]: true,
> >   ...
> > });
> > 
> > May be easier to compare the two states / copy for new tests that way.
> 
> yeah, could be. I initially did not do it like that because prefs hold the
> persist and filter bar visibility as well. But we can have another helper in
> ServicesMock to only return filter prefs.

Oh, don't worry about it then
(In reply to Brian Grinstead [:bgrins] from comment #11)
> Nit: would it be possible to add something like this at the top of the file
> (i.e. outisde of any `describe`).  And if so, would it make sense to do so
> (i.e. do we not rely on persisted pref state between individual tests)?
> 
>   afterEach(() => {
>     ServicesMock.prefs.testHelpers.clearPrefs();
>   });

This is already handled by the beforeEach function at the top of the file, in which we call filtersClear (which also resets the prefs). When I added this line, filtersClear was broken, but now that it's fixed, we don't need it anymore, thus I'll remove it.
Pushed by nchevobbe@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/8baf55e85dae
Fix setting filters prefs when filters are reset or cleared; r=bgrins.
https://hg.mozilla.org/mozilla-central/rev/8baf55e85dae
Status: ASSIGNED → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 58
(In reply to Nicolas Chevobbe [:nchevobbe] from comment #10)
> The fix was an easy one, and I added some tests to make sure we handle prefs
> as expected.
> Since it's a small change, I hope we'll be able to uplift it to beta.

Please request uplift ASAP if you still want this in 57.
Flags: needinfo?(nchevobbe)
Comment on attachment 8920106 [details]
Bug 1409836 - Fix setting filters prefs when filters are reset or cleared; .

Approval Request Comment
[Feature/Bug causing the regression]: Always been there in new console frontend (enabled in 57)
[User impact if declined]: Filters state in the console toolbar are not persisted across devtools restart. This would confuse people to see a filter is off when they toggle it before. It can be even more confusing since the toolbar is hidden by default.
[Is this code covered by automated tests?]: Yes
[Has the fix been verified in Nightly?]: Yes
[Needs manual test from QE? If yes, steps to reproduce]: Yes
1. Open the console
2. Click on the filter icon in the toolbar
3. The filter toolbar should be shown
4. Toggle off the "warning" filter
5. evaluate `console.warning("foo")`
6. You should see a "Reset filters" button in the top-right corner
7. Click it
8. You should now see the warning message
9. Close the devtools
10. Open the console again
11. You should still see the warning message (i.e. the warning filter should be on)
[List of other uplifts needed for the feature/fix]: -
[Is the change risky?]: No
[Why is the change risky/not risky?]: It's a really small change (only the order of 2 instructions where changed), and it is well tested.

[String changes made/needed]: -


---

Thanks for the reminder jcristau !
Flags: needinfo?(nchevobbe)
Attachment #8920106 - Flags: approval-mozilla-beta?
Comment on attachment 8920106 [details]
Bug 1409836 - Fix setting filters prefs when filters are reset or cleared; .

Recent problem in 57, low risk fix, Beta57+
Attachment #8920106 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
I managed to reproduce the bug using the steps from comment 20 on an older version of Nightly (2017-10-18) on Windows 10x64 and macOS 10.13.
I retested everything using latest Nightly 58 and beta 57.0b12 on Windows 10, macOS 10.13 and Ubuntu 16.04x64, but the bug is not reproducing anymore. After the console is reopened, the log filters are still enabled (you can still see the warning message).
Status: RESOLVED → VERIFIED
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.