1286864 - Reps: array should show how many items is not displayed.

Status: VERIFIED FIXED

(DevTools :: Shared Components, defect, P1)

Description

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Both the ArrayRep and GripArray rep should show how many items is not displayed:

Example:

let arr = new Array(100).fill(0).map((a,i) => i);


Now (short mode):
Short mode: [0, 1, 2, more...]

But, it should be like:
[0, 1, 2, 97 more...]


The `97` text should have the same style as `more...`

Honza

Comment 1

[Dalimil Hajek [:dalimil]]

Attachment: Bug1286864.patch - rev1

I added the count for array and grip-array. This bug didn't ask to modify object.js, even though we might want to have the same behaviour for Object properties - or maybe it can be a separate bug sometime in the future...

Comment 2

[Jan Honza Odvarko [:Honza] (always need-info? me)]

(In reply to Dalimil Hajek [:dalimil] from comment #1)
> Created attachment 8771986 [details] [diff] [review]
> Bug1286864.patch - rev1
> 
> I added the count for array and grip-array. This bug didn't ask to modify
> object.js, even though we might want to have the same behaviour for Object
> properties - or maybe it can be a separate bug sometime in the future...
Good point, we should do it for Obj rep (object.js) and Grip rep (grip.js)
Both is ok for me, do it as part of this bug or file new one....

Honza

Comment 3

[Dalimil Hajek [:dalimil]]

Attachment: Bug1286864.patch - rev2

I added the count also for object.js and grip.js...
I also noticed some strange behaviour with the object.preview property - the number of items seems to be capped to 10 even though the grip.preview.length shows the correct number (e.g. length=354, items=Array[10]).

Comment 4

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Comment on attachment 8772893 [details] [diff] [review]
Bug1286864.patch - rev2

Review of attachment 8772893 [details] [diff] [review]:
-----------------------------------------------------------------

(In reply to Dalimil Hajek [:dalimil] from comment #3)
> Created attachment 8772893 [details] [diff] [review]
> Bug1286864.patch - rev2
> 
> I added the count also for object.js and grip.js...
Great

> I also noticed some strange behaviour with the object.preview property - the
> number of items seems to be capped to 10 even though the grip.preview.length
> shows the correct number (e.g. length=354, items=Array[10]).
How can I reproduce it?

Honza

::: devtools/client/shared/components/reps/array.js
@@ -60,5 @@
>          }
>        }
>  
>        if (array.length > max) {
> -        items.pop();

Why is this needed?

Comment 5

[Dalimil Hajek [:dalimil]]

(In reply to Jan Honza Odvarko [:Honza] from comment #4)
> Comment on attachment 8772893 [details] [diff] [review]
> Bug1286864.patch - rev2
> 
> Review of attachment 8772893 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> (In reply to Dalimil Hajek [:dalimil] from comment #3)
> > Created attachment 8772893 [details] [diff] [review]
> > Bug1286864.patch - rev2
> > 
> > I added the count also for object.js and grip.js...
> Great
> 
> > I also noticed some strange behaviour with the object.preview property - the
> > number of items seems to be capped to 10 even though the grip.preview.length
> > shows the correct number (e.g. length=354, items=Array[10]).
> How can I reproduce it?
> 

1- Add this: 'console.log(grip.preview)' after line 90 in reps/grip-array.js
2- Open Nightly with my patch loaded (as part of devtools)
3- in client console type: window.aaaa = Array(350).fill("abc")
4- go to DOM panel -> click Refresh
5- In browser console you should see the console log with the grip.preview object -> length set to 350, items: Array[10] <- what I didn't understand was why the property 'items' only contains 10 elements


> 
> ::: devtools/client/shared/components/reps/array.js
> @@ -60,5 @@
> >          }
> >        }
> >  
> >        if (array.length > max) {
> > -        items.pop();
> 
> Why is this needed?

The loop had a condition i <= max and basically always exceeded the max amount and had to pop() the last element out. This makes it cleaner, although it is not part of the real bugfix.

Comment 6

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Comment on attachment 8772893 [details] [diff] [review]
Bug1286864.patch - rev2

Review of attachment 8772893 [details] [diff] [review]:
-----------------------------------------------------------------

(In reply to Dalimil Hajek [:dalimil] from comment #5)
> object -> length set to 350, items: Array[10] <- what I didn't understand
> was why the property 'items' only contains 10 elements
Ah, the preview is limited and shows max 10 items, but grip.preview.lengths
shows the actual length of an array (or number of props).

> The loop had a condition i <= max and basically always exceeded the max
> amount and had to pop() the last element out. This makes it cleaner,
> although it is not part of the real bugfix.
I see, sounds good.

::: devtools/client/shared/components/reps/array.js
@@ +65,5 @@
>          items.push(Caption({
>            key: "more",
>            object: objectLink({
>              object: this.props.object
> +          }, (grip.preview.length - max) + " more…")

The `grip` variable is undefined in this scope.

Comment 7

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Also bug 1282791 is doing changes in grip.js and code in the patch will likely need rebase...

Honza

Comment 8

[Jan Honza Odvarko [:Honza] (always need-info? me)]

Comment on attachment 8772893 [details] [diff] [review]
Bug1286864.patch - rev2

Removing the review flag till comment #6 is resolved.

Honza

Comment 9

[Dalimil Hajek [:dalimil]]

Attachment: Bug1286864.patch - rev3

I addressed the mentioned issues and rebased on master. It passed all mochitests on my local machine - but pushing to try to be 100% sure:

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

(Honza is on PTO, feel free to reassign the review to someone else if appropriate)

Comment 10

[Lin Clark [:linclark]]

Comment on attachment 8774408 [details] [diff] [review]
Bug1286864.patch - rev3

Review of attachment 8774408 [details] [diff] [review]:
-----------------------------------------------------------------

There are some ESLint issues, but other than that this looks good to me. Make sure those are fixed before setting checkin-needed

Comment 11

[Dalimil Hajek [:dalimil]]

Attachment: Bug1286864.patch - rev4

Fixed ESLint issues.

Comment 12

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/fx-team/rev/951aa830b051
Reps: array should show how many items is not displayed. r=Honza

Comment 13

[Iris Hsiao [:ihsiao]]

Sorry had to back out this patch for Mochitest failures, e.g., https://treeherder.mozilla.org/logviewer.html#?job_id=10777237&repo=fx-team#L2497

Comment 14

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/integration/fx-team/rev/820cfffb1fe2
Backed out changeset 951aa830b051 for Mochitest M(oth), M(c1) cases failed

Comment 15

[Dalimil Hajek [:dalimil]]

Attachment: Bug1286864.patch - rev5

Someone must have recently added a new test case, which I missed. I fixed it now.

Comment 16

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/fx-team/rev/685de64439d2
Reps: array should show how many items is not displayed. r=odvarko

Comment 17

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/685de64439d2

Comment 18

[Ada [:adalucinet]]

Dalimil, can you please add STR in order to reproduce and properly verify this report? Or, as in comment 5, do I need to build Firefox locally? Thanks in advance.

Comment 19

[Dalimil Hajek [:dalimil]]

(In reply to Alexandra Lucinet, QA Mentor [:adalucinet] from comment #18)
> Or, as in comment 5, do I need to build Firefox locally?

No you don't have to build anything. Just open Firefox Nightly (latest update) and then go to any page, open Developer Tools and then go to 'DOM Panel' (it is currently using reps). For any entry that is a long array or object, you should see the word 'more...' preceded by a number.

Comment 20

[Ada [:adalucinet]]

Thanks, :dalimil! :)
I found some inconsistencies while verifying this fix with latest 50.0a2 and 51.0a1, across platforms [1], as it follows:
1. DOM search no longer has italics → [2]; I guess this is intended due to bug 1253195?
2. With Dark theme, the scrollbar overlaps the DOM search → [2]

Although, the word 'more...' preceded by a number is properly displayed for long array/object.

[1] Windows 10 64-bit, Ubuntu 16.04 64-bit and Mac OS X 10.11.1
[2] https://i.imgur.com/uXG2226.png

Comment 21

[Dalimil Hajek [:dalimil]]

(In reply to Alexandra Lucinet, QA Mentor [:adalucinet] from comment #20)
> 1. DOM search no longer has italics → [2]; I guess this is intended due to
> bug 1253195?
> 2. With Dark theme, the scrollbar overlaps the DOM search → [2]

Both of these are relevant to other patches, I believe. My bugfix simply prepends the property count number, that's all that it does.

Comment 22

[Ada [:adalucinet]]

(In reply to Dalimil Hajek [:dalimil] from comment #21)
> (In reply to Alexandra Lucinet, QA Mentor [:adalucinet] from comment #20)
> > 1. DOM search no longer has italics → [2]; I guess this is intended due to
> > bug 1253195?
> > 2. With Dark theme, the scrollbar overlaps the DOM search → [2]
> 
> Both of these are relevant to other patches, I believe. My bugfix simply
> prepends the property count number, that's all that it does.

Indeed, the mentioned issues are not dependent on this patch. I thought that you might have an idea if those are under your radar already or not.
Marking here accordingly.