Closed Bug 1608571 Opened 1 year ago Closed 1 month ago

Expanding objects makes them jump around

Categories

(DevTools :: Shared Components, defect, P3)

defect

Tracking

(firefox88 fixed)

RESOLVED FIXED
88 Branch
Tracking Status
firefox88 --- fixed

People

(Reporter: Harald, Assigned: karthiksundar30092002)

Details

(Keywords: good-first-bug)

Attachments

(8 files)

Attached image ScreenFlow.gif

Expanding objects and arrays changes their representation, often to something shorter. visual jitter.

Inspection should aim to keep objects in place.

Nicolas, is there some prior arguments on why expanded objects change shape?

Flags: needinfo?(nchevobbe)

The argument was made in the very beginning of the ObjectInspector, and I can't find the discussion anymore .
I agree that it can be weird sometimes.

This should be a simple fix if we want to do it: remove this block devtools/client/debugger/packages/devtools-reps/src/object-inspector/components/ObjectInspectorItem.js#168-170 and fix the tests that would fail because of this change.

Flags: needinfo?(nchevobbe)

This will probably need some hands-on experimentation to get right. Opening up for good-first-bug in case somebody who likes to tinker in UI wants to play around with this.

Different use cases should be considered (single entries, multiple entries in one log, narrow and wide viewport) and the amount of content jumping erratically reduced when expanding arrays/objects/etc.

Keywords: good-first-bug

I would like to work on this

Thank you Samarjeet. Nicolas pointed to the right files that should get you started. As this might not be straightforward, feel free to share video (like Loom) recordings of any ideas you want to bounce off.

Assignee: nobody → thelehhman
Status: NEW → ASSIGNED

I can't find an object to reproduce it. Tried many levels of nesting. Could you explain how to reproduce this bug?

Attached image array and object.gif

In this example, clicking the Array pushes the Object around. Not shown, a smaller width expanding the Object pushes it down one line.

Other STR:

  • Log console.log(window, document)
  • Resize the console so both elements fit on the same line
  • Expand one

AR: Clicked Objects jump around.
ER: Layout stays more stable

Attached image jumping.gif

Is this the undesired behaviour?

Attached image case2.gif

Or is this?

I thought that it would be easiest if the Array(3) [ 1, 2, 3 ] would stay in place while additional content expands below.

The forced line break from comment 9 also makes sense, ensuring expanded objects get the space they need at all times.

Samarjeet, are you still interested in fixing this bug?

Honza

Flags: needinfo?(thelehhman)

Oh yes, I am. This had slipped earlier.

Flags: needinfo?(thelehhman)

Hi, actually I am working on another bug at the moment. I don't think it's ethical for me to be working on two bugs at a time, it'll just delay it.
Let's open this up for contributors. I'll try getting back to this once the other bug is done.

Thanks for the heads-up Samarjeet, I unassigned the bug.

Assignee: thelehhman → nobody
Status: ASSIGNED → NEW

Hi, can I be assigned to this bug?

Flags: needinfo?(hkirschner)

Hello Mary, the bug is now yours

If it's your first bug, you might want to read this https://firefox-source-docs.mozilla.org/devtools/getting-started/README.html to setup the work environment.
Make sure to select "Artifact builds" when asked as it speeds up the workflow quite a bit :)
Also, you can come and chat on our Slack https://devtools-html-slack.herokuapp.com/

When doing changes for that patch, you'll need to re-generate the bundle we have each time you're testing a change:

cd devtools/client/debugger
node bin/bundle

don't hesitate to ask any question here or on Slack

Assignee: nobody → csfj87
Status: NEW → ASSIGNED
Flags: needinfo?(hkirschner)

Thanks for the info Nick!

Hey Nick,

I ran into this error when I used the re-generate bundle code. I don't know where to find devtools-launchpad/index.

Flags: needinfo?(nchevobbe)

Hello Mary,

Could you try to do yarn install before? We need to pull those dependencies from npm :)

Flags: needinfo?(nchevobbe)

Hey Nick,

yarn install worked! I pushed a commit and added you as the reviewer. My only question now is, is there a revision ID for this bug?

Flags: needinfo?(nchevobbe)

Hello Mary,

I don't see the patch anywhere? What did you use to push?
Is the commit message like Bug 1608571 - Don't shorten object when expanding. r=nchevobbe. ?

Flags: needinfo?(nchevobbe)
Attached image TerminalImage2.png

I used hg commit and the message is Bug 1608571 - Fixes expanding Object issue. r=nchevobbe. I realized I forgot to use moz-phab submit, but now when I try to use it I get an API error.

Flags: needinfo?(nchevobbe)

can you try to run moz-phab install-certificate again?

Flags: needinfo?(nchevobbe)

This good-first-bug hasn't had any activity for 6 months, it is automatically unassigned.
For more information, please visit auto_nag documentation.

Assignee: csfj87 → nobody
Status: ASSIGNED → NEW

Can I take this bug??, If it is still open

Flags: needinfo?(nchevobbe)

(In reply to Nicolas Chevobbe [:nchevobbe] from comment #2)

The argument was made in the very beginning of the ObjectInspector, and I can't find the discussion anymore .
I agree that it can be weird sometimes.

This should be a simple fix if we want to do it: remove this block devtools/client/debugger/packages/devtools-reps/src/object-inspector/components/ObjectInspectorItem.js#168-170 and fix the tests that would fail because of this change.

I tried to look into the file mentioned. However, I was not able to locate the folder devtools-reps.
I have only the following folders in the packages directory.

  1. devtools-source-map
  2. devtools-splitter
  3. devtools-utils
  4. devtools-wasm-dwarf
Assignee: nobody → karthiksundar30092002
Status: NEW → ASSIGNED
Component: Console → Shared Components
Flags: needinfo?(nchevobbe)

In Comment 8 and 9, two cases are given, which is the undesired behaviour? Also, it will be really helpful, if I get some testcases to test my code, for instance the code in the Screenflow.gif.

Flags: needinfo?(nchevobbe)

None of them seem to picture what's wanted.
Let me recap:

  1. Open the console
  2. Evaluate [1,2,3] (
  3. We now have ▶︎ Array(3) [ 1, 2, 3 ] printed in the console
  4. Click on the arrow to expand the object
  5. We now have
▼ (3) […]
|   0: 1
|   1: 2
|   2: 3
|   length: 3
| ▶︎ <prototype>: Array []

the purpose of this bug is to have

▼ (3) Array(3) [ 1, 2, 3 ]
|   0: 1
|   1: 2
|   2: 3
|   length: 3
| ▶︎ <prototype>: Array []

instead (so the first line stays the same, we only expand the arrow)
It should limit some of the jumping we see.

Flags: needinfo?(nchevobbe)
Attached image Result.png

I have been able to achieve this result. Shall I submit my code for review???

Flags: needinfo?(nchevobbe)

I just want to notify that I have submitted my patch. I hope I didn't do anything wrong in submitting my patch. If any issues kindly contact me.

Flags: needinfo?(nchevobbe)
Flags: needinfo?(nchevobbe)
Pushed by nchevobbe@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/22ea220f8502
Removed the code responsible for shortening to remove the jumping around of objects when expanding. r=nchevobbe.
Status: ASSIGNED → RESOLVED
Closed: 1 month ago
Resolution: --- → FIXED
Target Milestone: --- → 88 Branch
You need to log in before you can comment on or make changes to this bug.