Closed Bug 1514495 Opened 4 years ago Closed 1 year ago

Add a button to clear filter input in JSON inspector

Categories

(DevTools :: JSON Viewer, enhancement, P3)

Product:
DevTools
Component:
JSON Viewer
Platform:
Unspecified
All
Type:
enhancement

Tracking

(firefox101 fixed)

Status:
RESOLVED FIXED
Milestone:
101 Branch
Tracking Flags:
Tracking Status
firefox101 --- fixed

People

(Reporter: aakumykov, Assigned: colin.cazabet)

Details

Attachments

(3 files, 3 obsolete files)

Снимок-Firefox Developer Edition.png
175.99 KB, image/png
Details
Bug 1514495 - [devtools] Add a button to clear filter input in JSON inspector r=Honza
48 bytes, text/x-phabricator-request
Details | Review
patch-press_escape_key.diff
8.10 KB, patch
Details | Diff | Splinter Review 
User Agent: Mozilla/5.0 (X11; Linux x86_64; rv:65.0) Gecko/20100101 Firefox/65.0

Steps to reproduce:

1) go to URL that returns pure JSON, for example "https://2ch.hk/mo/res/212281.json"
2) JSON inspector/viewer appears on site response;
3) where is JSON filter text input near top right corner (in JSON tab);
4) enter some text in filter field;


Actual results:

You have to make several manipulations to clear filter field and see full JSON tree again.


Expected results:

Please, add "clear" constol element to filter text input (as it is done in HTML inspector) to clear filter by one click.
I add "cleare" control in graphics editor to show how it should be. There is no "circle with cross" originally.
Hi reporter,

Thank you for taking the time to add this report.  

This seems to be more of an enhancement than an issue in my opinion. The "Clear" button is not displayed on any of the Firefox versions or OS-es. I have edited the title to better reflect the enhancement suggestion. 

However, I am assigning this to DevTools:JSON Viewer component to this issue in order to involve the development team and get an opinion on this.
If this is not the proper component for this report, please move it to a more appropriate one.
Severity: normal → enhancement
Status: UNCONFIRMED → NEW
Component: Untriaged → JSON Viewer
Ever confirmed: true
OS: Unspecified → All
Product: Firefox → DevTools
Summary: Button to clear filter input in JSON inspector → Add a button to clear filter input in JSON inspector
Version: 65 Branch → Trunk
It could use something like the filter in CSS Rules Inspector:

<div class="devtools-searchbox has-clear-btn">
  <input class="searchBox devtools-filterinput" placeholder="...">
  <button class="devtools-searchinput-clear"></button>
</div>

and hide the button when the input is empty.

Hello,

If this is still up to date, I would be happy to do this enhancement

Thank you

Awesome, thank you for the help, Colin!
Assigned to you.

The SearchBox (React) component is implemented here:
https://searchfox.org/mozilla-central/rev/bd25b1ca76dd5d323ffc69557f6cf759ba76ba23/devtools/client/jsonview/components/SearchBox.js#21

Honza

Assignee: nobody → colin.cazabet
Severity: normal → S3
Status: NEW → ASSIGNED
Priority: -- → P3

Hello Jan,

I've added the button but I have two questions:

  • Should I also erase the input when the escape key is pressed ?
  • Should the input still have the focus after clicking on the erase button (currently the searchbox loses focus when i click) ?

Thank you

Flags: needinfo?(odvarko)

Hi Colin, I like the ideas, yes for both.

Flags: needinfo?(odvarko)

Hello Jan Honza,

I have updated the patch with unit tests.

Thank you

Pushed by jodvarko@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/d1e6410ead1d
[devtools] Add a button to clear filter input in JSON inspector r=Honza

Backed out for causing failures on browser_jsonview_filter_clear.js

Backout link: https://hg.mozilla.org/integration/autoland/rev/5e718536225ed4afe6ee1cfd298272a707720a13

Push with failures: https://treeherder.mozilla.org/jobs?repo=autoland&resultStatus=testfailed%2Cbusted%2Cexception%2Crunnable&revision=d1e6410ead1d5022d3fd7aa42427f308989e390e&selectedTaskRun=P8dqRiylTwmzA90He0hrfw.0

Link to failure log: https://treeherder.mozilla.org/logviewer?job_id=369441717&repo=autoland&lineNumber=2901

Failure message : TEST-UNEXPECTED-FAIL | devtools/client/jsonview/test/browser_jsonview_filter_clear.js | Filter input should be empty - Got "honza", expected ""

Flags: needinfo?(colin.cazabet)

Colin, there are two failures (in the new test):

  • TEST-UNEXPECTED-FAIL | devtools/client/jsonview/test/browser_jsonview_filter_clear.js | Filter input should be empty - Got "honza", expected ""
  • TEST-UNEXPECTED-FAIL | devtools/client/jsonview/test/browser_jsonview_filter_clear.js | There must be expected number of rows - Got 4, expected +0

See the raw log: https://treeherder.mozilla.org/logviewer?job_id=369441717&repo=autoland&lineNumber=2901

Hello Jan Honza,

I tried to add a "sleep" after clicking on the clear button, I think the problem comes from here, I cannot reproduce it locally unfortunately.
Can we launch a new test with this little change?

Flags: needinfo?(colin.cazabet) → needinfo?(odvarko)

I am still seeing failures related to the test (in the Try push)

TEST-UNEXPECTED-FAIL | devtools/client/jsonview/test/browser_jsonview_filter_clear.js | Filter input should be empty - Got "honza", expected ""
TEST-UNEXPECTED-FAIL | devtools/client/jsonview/test/browser_jsonview_filter_clear.js | There must be expected number of rows - Got 4, expected +0

I couldn't repro locally (Win10, Ubuntu)

Feel free to ping me anytime you'd like to push your changes to Try.

Honza

Hello Jan Honza ,

Unfortunately I did not find anything interesting in the log files I don't know how i could debug this.
Is there a way to take a screenshot like this one: https://firefox-ci-tc.services.mozilla.com/api/queue/v1/task/SLdvkYM1RWysXO5cCdaB-Q/runs/0/artifacts/public/test_info/mozilla-test-fail-screenshot_dfwml1n2.png but at a specific line in the test file ?

Thank you

Flags: needinfo?(odvarko)

You might try to look at this code and get some inspiration (not perfect, but should work):
https://hg.mozilla.org/try/file/bd06124d04f68ce67f8154c836678133f86c5916/devtools/client/shared/test/shared-head.js#l1974

Flags: needinfo?(odvarko) → needinfo?(colin.cazabet)
Attached patch patch-with-screenshots (obsolete) — — Splinter Review

Hello,

I added some logs and I now take screenshots to help debug, can we launch a new test on try ?

Thank you

Flags: needinfo?(colin.cazabet) → needinfo?(odvarko)

Hello,

The bug disappeared probably because I increased the sleep value (I see in the logs that the test takes 2 seconds on OSX and 4 seconds on linux).
Instead of doing that I modified the patch to wait for mutations on the screen.

Instead of waiting for X seconds, I wait until the jsonview is back into its initial state before going to the next assertion, can we please try this new patch on TRY ?

Thank you

Attachment #9268976 - Attachment is obsolete: true
Flags: needinfo?(odvarko)

Thank you!

Try push:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=2ddc7020c5e2a82967ccf6859bc310f877bd5b3c

Btw. Id you'd like to get Level 1 Access (used to push to Try), you can follow these instructions:
https://www.mozilla.org/en-US/about/governance/policies/commit/access-policy/
I'll vouch for you.

Honza

Flags: needinfo?(odvarko)

Hello,

Thank you, I asked for level access 1 here: https://bugzilla.mozilla.org/show_bug.cgi?id=1761574
I still don't understand why it times out on linux, I may have to push multiples times to TRY

Colin

Flags: needinfo?(odvarko)

Perfect, just vouched for you!

I still don't understand why it times out on linux, I may have to push multiples times to TRY

Yes, feel free to do it (not the first time when we could repro an issue only on the Try)

Honza

Flags: needinfo?(odvarko)

Hello,

Thank you for the access, when I try to login on try I get this error:
Access is not allowed because you are not enrolled in Duo. Please contact your organization's IT help desk.

What can I do to fix it ?

Flags: needinfo?(odvarko)

(In reply to colin.cazabet from comment #24)

Access is not allowed because you are not enrolled in Duo. Please contact your organization's IT help desk.
What can I do to fix it ?

Please, ask in the bug where you requested the Commit #1 level. They should be able to help you.
(it's related to two-factor authenticator that you should set up)

Flags: needinfo?(odvarko)

Hello,

For some reason, in some cases the BrowserTestUtils.synthesizeMouseAtCenter was not triggering the click on the button, I was able to fix this part by synthesizing a press on the escape key instead of a click on the clear button (both events trigger the same function).

All jobs are now error-free on TRY, do you think that's enough?

https://treeherder.mozilla.org/jobs?revision=f2d4d5beccd82eb9420224cd05445ea3a23dd8b2&repo=try

Thank you

Attachment #9269149 - Attachment is obsolete: true
Flags: needinfo?(odvarko)

(In reply to colin.cazabet from comment #26)

For some reason, in some cases the BrowserTestUtils.synthesizeMouseAtCenter was not triggering the click on the button, I was able to fix this part by synthesizing a press on the escape key instead of a click on the clear button (both events trigger the same function).

This method seems utterly broken, I also had problems as explained in bug 1720898 comment 1. If you want to specifically test the button, you can use .click() in the content process like in https://phabricator.services.mozilla.com/D134660

Agree with Oriol.

Please see my comments in Phab.

Thank you!

Flags: needinfo?(odvarko)
Attachment #9270737 - Attachment is obsolete: true

Hello,

I put back the click on the button but with the right function and it now works.
Here is the TRY https://treeherder.mozilla.org/push-health/push?repo=try&revision=58b78eec319ec6abacee40cdc64c2e22aa1320f4&tab=tests&testGroup=pr

Thank you

Pushed by jodvarko@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/03247a8340da
[devtools] Add a button to clear filter input in JSON inspector r=Honza,Oriol

https://hg.mozilla.org/mozilla-central/rev/03247a8340da

Status: ASSIGNED → RESOLVED
Closed: 1 year ago
status-firefox101: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → 101 Branch
You need to log in before you can comment on or make changes to this bug.