1352777 - Clicking on dropmarkers in JSON viewer doesn't collapse long string values

Status: VERIFIED FIXED

(DevTools :: JSON Viewer, defect, P3)

Description

[684sigma]

Attachment: example of JSON file, by reporter.json

I have a problem with Firefox Beta 53. It also happens in Nightly 55. Doesn't happen in Beta 52, ESR 45 (if JSON viewer is enabled from about:config).
Clicking on dropmarkers in JSON viewer doesn't collapse long string values. Here's how to reproduce it:

1. Open attached json file in a new tab
2. Click on dropmarker near a long string's property name.

Result: dropmarkers have no effect
Expected: dropmarkers should collapse/expand the string

Comment 1

[Loic]

https://hg.mozilla.org/integration/autoland/pushloghtml?fromchange=bd11222227ee44fc81d622f3e4530067307e23ac&tochange=fd4b40c65e7e42d51568e30df4547611cd259dcc

Ricky Chien — Bug 1328500 - Support input field in TreeView r=Honza

Comment 2

[Ricky Chien [:rickychien] (inactive)]

Thanks for reporting!

Collapsed button is not so easy to pay attention but it is definitely an existing issue.

Comment 3

[Liz Henry (:lizzard Please n-i to RyanVM, jcristau, or pascal)]

It is late in beta to take a fix here but we could still potentially take a patch for aurora 54.

Comment 4

[Utsav Saha]

HI,i would like to work on this bug.Could you guide me on this?

Comment 5

[Ricky Chien [:rickychien] (inactive)]

Sure!  The regression is caused by this patch https://reviewboard.mozilla.org/r/102066/diff/4#index_header. So you probably want to look through the changes and find out root cause where triggers this regression.

Comment 6

[Matt R [:stylizit] (slow, away until Sep)]

Attachment: modified TreeCell so that the component updates also on 'open' change state

Hi,

From what I understand, the TreeCell component should also update when the row's "open" state is updated, which basically happens when LabelCell is toggled.

Adding an extra check for that state on shouldComponentUpdate in tree-cell.js seems to fix the problem and doesn't seem to break any related tests locally in /devtools.

Comment 7

[Ricky Chien [:rickychien] (inactive)]

Comment on attachment 8860695 [details]
modified TreeCell so that the component updates also on 'open' change state

Hey Matt, thanks for the work!

I guess there are something wrong when submitting patch. I know bugzilla is old and somewhat hard to understand how to use. You can check the "patch" checkbox in "Create New Attachment" when uploading your patch, and then I can start reviewing your patch on bugzilla old review board.

Or how about setup new mozreview [1] (example [2]) to push your patch. It's document very well and pretty convenient. If you have any question, feel free to ask me:)

[1] http://mozilla-version-control-tools.readthedocs.io/en/latest/mozreview.html
[2] https://reviewboard.mozilla.org/r/130850/diff/15#index_header

Comment 8

[Matt R [:stylizit] (slow, away until Sep)]

Attachment: Bug 1352777 - Fix dropmarkers in JSON viewer not collapsing long string

Review commit: https://reviewboard.mozilla.org/r/132690/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/132690/

Comment 9

[Matt R [:stylizit] (slow, away until Sep)]

(In reply to Ricky Chien [:rickychien] from comment #7)

Oh sorry, I thought I had to request feedback first as I saw here: https://developer.mozilla.org/en-US/docs/Tools/Contributing#Asking_for_feedback

Just created review the request. I suppose I don't have the permissions to trigger try builds yet?

Comment 10

[Ricky Chien [:rickychien] (inactive)]

Comment on attachment 8860753 [details]
Bug 1352777 - Fix dropmarkers in JSON viewer not collapsing long string

https://reviewboard.mozilla.org/r/132690/#review135584

Thanks Matt, your patch works for me. Let's ship it.

::: commit-message-7513b:1
(Diff revision 1)
> +Bug 1352777 - Fix dropmarkers in JSON viewer not collapsing long string values: modified tree-cell so that the component updates also on 'open' change state; r?rickychien

This commit message is too long.

Please change it to
```
Bug 1352777 - Fix dropmarkers in JSON viewer not collapsing long string r?rickychien
```

::: devtools/client/shared/components/tree/tree-cell.js:45
(Diff revision 1)
>       * Optimize cell rendering. Rerender cell content only if
>       * the value or expanded state changes.
>       */
>      shouldComponentUpdate: function (nextProps, nextState) {
>        return (this.props.value != nextProps.value) ||
> -        (this.state !== nextState);
> +        (this.state !== nextState) || 

nit: remove tailing whitespace.

Comment 11

[Ricky Chien [:rickychien] (inactive)]

Please fix above two open issues on mozreview and mark them as fixed. You can also trigger a Try Build on our CI testing. Once Try tests are green, you can add a "checkin-needed" keyword in bugzilla field. Release engineer will do the rest for us to merge your commit to mozilla-central thunk branch. And then you will see your fix in Firefox Nightly.

Comment 12

[Matt R [:stylizit] (slow, away until Sep)]

Comment on attachment 8860753 [details]
Bug 1352777 - Fix dropmarkers in JSON viewer not collapsing long string

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/132690/diff/1-2/

Comment 13

[Matt R [:stylizit] (slow, away until Sep)]

Comment on attachment 8860753 [details]
Bug 1352777 - Fix dropmarkers in JSON viewer not collapsing long string

https://reviewboard.mozilla.org/r/132690/#review135684

Fixed commit message + removed whitespace

Comment 14

[Matt R [:stylizit] (slow, away until Sep)]

Thanks for the explanations!

It seems I cannot trigger a Try Build because of permissions. 
I have filed a bug as per: https://www.mozilla.org/en-US/about/governance/policies/commit/ 

I hope that's the correct way to do it. Would you be so kind to vouch for me?
https://bugzilla.mozilla.org/show_bug.cgi?id=1359014

Comment 15

[Ricky Chien [:rickychien] (inactive)]

Vouch!

Nice! so far you've done very well. I've pushed try and let's wait and see.

Comment 16

[Matt R [:stylizit] (slow, away until Sep)]

(In reply to Ricky Chien [:rickychien] from comment #15)
> I've pushed try and let's wait and see.

I'm a bit confused, the reviewboard says:
2 jobs testfailed, 63 jobs success

However when I expand all jobs, all tests seem to be green on treeherder. Is that a bug? An intermittent failure?

Comment 17

[Matt R [:stylizit] (slow, away until Sep)]

Oh, I just realized it's "mochitest clipboard" from the excluded jobs list timing out.

I guess it's unrelated and an occurence of an intermittent failure and we should be good to go?

Comment 18

[Ricky Chien [:rickychien] (inactive)]

Yep, you're right. There are some problems in mochitest clipboard jobs so that release engineer (releng) mark them as excluded jobs. I think they are irrelevant failures and it's ready to go.

btw, I haven't enabled every email notification when getting reply, so feel free to set "Need more information from" flag to me.

Comment 19

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/d7b5ac6d5844
Fix dropmarkers in JSON viewer not collapsing long string r=rickychien

Comment 20

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/d7b5ac6d5844

Comment 21

[Ryan VanderMeulen [:RyanVM]]

Please request Beta approval on this when you get a chance.

Comment 22

[Ricky Chien [:rickychien] (inactive)]

Comment on attachment 8860753 [details]
Bug 1352777 - Fix dropmarkers in JSON viewer not collapsing long string

Approval Request Comment
[Feature/Bug causing the regression]: Bug 1328500
[User impact if declined]: minor, long string can still show property but user might feel a little inconvenient.
[Is this code covered by automated tests?]: no
[Has the fix been verified in Nightly?]: yes
[Needs manual test from QE? If yes, steps to reproduce]: yes, see STR in bug description.
[List of other uplifts needed for the feature/fix]: none
[Is the change risky?]: no
[Why is the change risky/not risky?]: it doesn't affect the real information showing on screen.
[String changes made/needed]: none

Comment 23

[Gerry Chang [:gchang]]

Hi Brindusa, could you help find someone to verify if this issue was fixed as expected on a latest Nightly build? Thanks!

Comment 24

[roxleitan]

I reproduced the issue using Nightly 55.0a1 (2017-04-01) build.

Verified as fixed using the latest Nightly 55.0a1 (2017-05-05) on Ubuntu 16.04 x64, Windows 10 x64 and Mac OS X 10.10.

Comment 25

[Julien Cristau [:jcristau]]

Comment on attachment 8860753 [details]
Bug 1352777 - Fix dropmarkers in JSON viewer not collapsing long string

json viewer fix, verified in nightly, beta54+

Comment 26

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-beta/rev/3d9699ce0b0a

Comment 27

[Cristian Comorasu, QA [:ccomorasu], Releae Desktop QA - Please contact [e-mail]]

I reproduced the issue using Nightly 55.0a1, build ID: 20170401030204, on Windows 10 x64.
I can confirm this issue is verified, I used Fx 54.0b6 on Windows 10 x64, Mac OS X 10.11.6 and Ubuntu 14.04 LTS.

Cheers!