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

VERIFIED FIXED in Firefox 50

Status

defect
P1
normal
VERIFIED FIXED
3 years ago
Last year

People

(Reporter: Honza, Assigned: dalimil)

Tracking

unspecified
Firefox 50
Dependency tree / graph

Firefox Tracking Flags

(firefox50 verified)

Details

(Whiteboard: [reserve-html] [good taipei bug])

Attachments

(1 attachment, 4 obsolete attachments)

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
Posted patch Bug1286864.patch - rev1 (obsolete) — Splinter Review
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...
Assignee: nobody → dalimilhajek
Status: NEW → ASSIGNED
Flags: needinfo?(odvarko)
Attachment #8771986 - Flags: review?(odvarko)
(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
Flags: needinfo?(odvarko)
Posted patch Bug1286864.patch - rev2 (obsolete) — Splinter Review
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]).
Attachment #8771986 - Attachment is obsolete: true
Attachment #8771986 - Flags: review?(odvarko)
Attachment #8772893 - Flags: review?(odvarko)
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?
(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 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.
Also bug 1282791 is doing changes in grip.js and code in the patch will likely need rebase...

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

Removing the review flag till comment #6 is resolved.

Honza
Attachment #8772893 - Flags: review?(odvarko)
Flags: qe-verify+
Priority: -- → P3
QA Contact: alexandra.lucinet
Whiteboard: [good taipei bug] → [reserve-html] [good taipei bug]
Posted patch Bug1286864.patch - rev3 (obsolete) — Splinter Review
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)
Attachment #8772893 - Attachment is obsolete: true
Attachment #8774408 - Flags: review?(lclark)
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
Attachment #8774408 - Flags: review?(lclark) → review+
Posted patch Bug1286864.patch - rev4 (obsolete) — Splinter Review
Fixed ESLint issues.
Attachment #8774408 - Attachment is obsolete: true
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/fx-team/rev/951aa830b051
Reps: array should show how many items is not displayed. r=Honza
Keywords: checkin-needed
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
Flags: needinfo?(dalimilhajek)
Backout by ihsiao@mozilla.com:
https://hg.mozilla.org/integration/fx-team/rev/820cfffb1fe2
Backed out changeset 951aa830b051 for Mochitest M(oth), M(c1) cases failed
Someone must have recently added a new test case, which I missed. I fixed it now.
Attachment #8774881 - Attachment is obsolete: true
Flags: needinfo?(dalimilhajek)
Keywords: checkin-needed
Pushed by cbook@mozilla.com:
https://hg.mozilla.org/integration/fx-team/rev/685de64439d2
Reps: array should show how many items is not displayed. r=odvarko
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/685de64439d2
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 50
Iteration: --- → 50.4 - Aug 1
Priority: P3 → P1
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.
QA Whiteboard: [qe-dthtml]
Flags: needinfo?(dalimilhajek)
(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.
Flags: needinfo?(dalimilhajek)
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
Flags: needinfo?(dalimilhajek)
(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.
Flags: needinfo?(dalimilhajek)
(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.
Status: RESOLVED → VERIFIED
Flags: qe-verify+
Product: Firefox → DevTools
You need to log in before you can comment on or make changes to this bug.