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

VERIFIED FIXED in Firefox 54

Status

P3
minor
VERIFIED FIXED
2 years ago
4 months ago

People

(Reporter: 684sigma, Assigned: stylizit, Mentored)

Tracking

({good-first-bug, regression})

53 Branch
Firefox 55
good-first-bug, regression

Firefox Tracking Flags

(firefox52 unaffected, firefox-esr52 unaffected, firefox53- wontfix, firefox54+ verified, firefox55+ verified)

Details

Attachments

(3 attachments)

(Reporter)

Description

2 years ago
Created attachment 8853723 [details]
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
(Reporter)

Updated

2 years ago
Has STR: --- → yes
Keywords: regression

Comment 1

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

Ricky Chien — Bug 1328500 - Support input field in TreeView r=Honza
Blocks: 1328500
Status: UNCONFIRMED → NEW
Has Regression Range: --- → yes
status-firefox52: --- → unaffected
status-firefox53: --- → affected
status-firefox54: --- → affected
status-firefox55: --- → affected
tracking-firefox53: --- → ?
tracking-firefox54: --- → ?
tracking-firefox55: --- → ?
Ever confirmed: true
Flags: needinfo?(rchien)
Thanks for reporting!

Collapsed button is not so easy to pay attention but it is definitely an existing issue.
Mentor: rchien
Flags: needinfo?(rchien)
Keywords: good-first-bug
Priority: -- → P3
It is late in beta to take a fix here but we could still potentially take a patch for aurora 54.
status-firefox53: affected → wontfix
tracking-firefox53: ? → -
tracking-firefox54: ? → +
tracking-firefox55: ? → +

Comment 4

2 years ago
HI,i would like to work on this bug.Could you guide me on this?
Flags: needinfo?(rchien)
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.
Assignee: nobody → utsav.saha
Status: NEW → ASSIGNED
Flags: needinfo?(rchien)
Created attachment 8860695 [details]
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.
Attachment #8860695 - Flags: feedback+
Attachment #8860695 - Flags: feedback+ → feedback?(rchien)
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
Attachment #8860695 - Flags: feedback?(rchien)
Comment hidden (mozreview-request)
(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

a year ago
mozreview-review
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.
Attachment #8860753 - Flags: review?(rchien) → review+
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 hidden (mozreview-request)
(Assignee)

Comment 13

a year ago
mozreview-review
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
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
Vouch!

Nice! so far you've done very well. I've pushed try and let's wait and see.
(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?
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?
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

a year ago
Pushed by rchien@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/d7b5ac6d5844
Fix dropmarkers in JSON viewer not collapsing long string r=rickychien

Comment 20

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/d7b5ac6d5844
Status: ASSIGNED → RESOLVED
Last Resolved: a year ago
status-firefox55: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → Firefox 55
Please request Beta approval on this when you get a chance.
Assignee: utsav.saha → matthieu.rigolot
status-firefox-esr52: --- → unaffected
Flags: needinfo?(rchien)
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
Flags: needinfo?(rchien)
Attachment #8860753 - Flags: approval-mozilla-beta?
Hi Brindusa, could you help find someone to verify if this issue was fixed as expected on a latest Nightly build? Thanks!
Flags: needinfo?(brindusa.tot)
Flags: qe-verify+
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.
Status: RESOLVED → VERIFIED
status-firefox55: fixed → verified
Flags: needinfo?(brindusa.tot)
Comment on attachment 8860753 [details]
Bug 1352777 - Fix dropmarkers in JSON viewer not collapsing long string

json viewer fix, verified in nightly, beta54+
Attachment #8860753 - Flags: approval-mozilla-beta? → approval-mozilla-beta+

Comment 26

a year ago
bugherderuplift
https://hg.mozilla.org/releases/mozilla-beta/rev/3d9699ce0b0a
status-firefox54: affected → fixed
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!
status-firefox54: fixed → verified
Flags: qe-verify+

Updated

4 months ago
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.