My favorites | Sign in
Project Home Wiki Issues
New issue   Search
for
  Advanced search   Search tips   Subscriptions
Issue 5786: Show array-like objects differently than actual arrays
1 person starred this issue and may be notified of changes. Back to list
 
Project Member Reported by sroussey, Aug 4, 2012
NodeLists and HTMLCollection are not actually arrays, and we should demonstrate that to users via their rep. Same with objects trying to fool Firebug into displaying themselves as arrays (like jQuery).

Normal arrays: [item1,item2,item3]
NodeList: <[item1,item2,item3]>
Arrayish objects like jQuery: {[item1,item2,item3]}


Aug 4, 2012
Project Member #1 sroussey
OK, I put this into the console-dollar branch.

https://github.com/firebug/firebug/commit/ca56207b8cc1084ad4cdb184f257b39444a6b86d
Status: Commit
Aug 4, 2012
Project Member #2 sroussey
Example on a page with jQuery is to do:
  jQuery("div")

For html collections:
  document.scripts

Aug 4, 2012
Project Member #3 sebastia...@gmail.com
Didn't test your patch yet but I'm not happy with the syntax suggestion described above. Nobody will know what <[]> means and {[]} is even worse because it can be mistaken as an object with incorrect syntax.
I'd prefer if we just wrote this instead:

NodeList [item1, item2, item3]
Object [item1, item2, item3]

Btw. that's how Dragonfly is doing it.

Please also don't set an issue to 'Commit' when it wasn't committed to the master branch. Otherwise it will be marked as 'Fixed' with the next release even when it wasn't included.

Sebastian
Status: Started
Cc: sebastia...@gmail.com
Labels: -ui console Test-case-needed
Aug 5, 2012
Project Member #4 sroussey
"because it can be mistaken as an object with incorrect syntax"

It *is* an object. That is the point. It is not an array.

I wanted the change to be subtle, but I'm OK with using names like that.

BTW: console isn't even a great label (though better than ui, I agree), since it is a "rep" issue and affects all panels, like dom and script. But console will do!
Aug 5, 2012
Project Member #5 sebastia...@gmail.com
> "because it can be mistaken as an object with incorrect syntax"
> 
> It *is* an object. That is the point. It is not an array.
I know, but not an object with incorrect syntax. ;-) That's why I posted "Object [item1, item2, item3]" in comment 3.

> BTW: console isn't even a great label (though better than ui, I agree), since it is 
> a "rep" issue and affects all panels, like dom and script. But console will do!
I added "dom". Covers this better I think.

Sebastian
Labels: dom
Sep 11, 2012
Project Member #6 sroussey
See https://github.com/firebug/firebug/pull/37
Sep 11, 2012
Project Member #7 sroussey
BTW: I no longer use {[ ]} instead just use []
Sep 12, 2012
Project Member #8 sebastia...@gmail.com
Didn't we say we'll use:

NodeList [item1, item2, item3]
Object [item1, item2, item3]

?

Instead I still see <[]> and {[]}.

But let's hear another opinion. Honza, what would you prefer? <[]> and {[]} or NodeList[] and Object[]?

Sebastian
Cc: odva...@gmail.com
Sep 12, 2012
Project Member #9 sebastia...@gmail.com
> Instead I still see <[]> and {[]}.
Ah, I missed the other commit you made for this in the pull request.

So what we have now is a mix of both:

HTMLCollection {[]}
Object {[]}

Sebastian
Sep 12, 2012
Project Member #10 sroussey
Huh? Example:

>>> [1,2,3]

[1,2,3]

>>> document.scripts

HTMLCollection [script jquery.min.js, script ga.js, script, script]

>> jQuery("div")

Object [div#wrapper, div#banner, div#dock, div.left, div, div.right, div#navigation, div.left, div.right, div#content-wrapper, div#content, div.content, div.content-body, div#home-content-wrapper, div#home-content-header, div#home-download, div#home-content, div#home-nav, div.home-nav-section, div.home-nav-section, div.home-nav-section, div#home-sidebar, div.home-sidebar-section, div.home-sidebar-section, div#jq-books.home-sidebar-section, div.jq-author, div.jq-author, div#footer, div.bg, div.inner]

I went with your suggestion.
Sep 12, 2012
Project Member #11 sroussey
Ah... I have origin set to sroussey/firebug not firebug/firebug. I should really simplify my github repos... sorry about that.
Sep 12, 2012
Project Member #12 sebastia...@gmail.com
> Huh?
> I went with your suggestion.
Ok, it seems I just didn't apply your changes correctly then.

Note that being on your console-changes branch I see different results:

>>> [1,2,3]
[1, 2, 3]

>>> document.scripts
[7] [script jquery.min.js, script, script custom.js, script ga.js, script, script donate.js, script]

>>> jQuery("ul")
[8] [ul, ul, ul.jq-checkpoints, ul.jq-whosUsing, ul, ul, ul, ul]

So I assume you need to replace your FirebugReps.ArrBase.getTitle() function by this:

getTitle: function(object, context)
{
    const re =/\[object ([^\]]*)/;
    var label = Str.safeToString(object);
    var m = re.exec(label);
    return m[1] || label;
} 

Sebastian
Sep 13, 2012
Project Member #13 odva...@gmail.com
> NodeList [item1, item2, item3]
> Object [item1, item2, item3]
Yep, I agree this is better 

@Sebastian: tested your getTitle suggestion (comment #12) and works well for me in the 
@Steven: please create separate pull request for this (do note merge with  issue 5764 ).

When testing Steven's branch + Sebastian's patch I don't see the prefix (Node List, Object) in the DOM panel (and so not even in the Watch panel). It would be great if this new rep is used everywhere thorough the Firebug UI.

In general, I like the improvement, I am just worried if this breaks FireQuery extension.

Honza

Sep 13, 2012
Project Member #14 odva...@gmail.com
I just sent Antonin (FireQuery author) an email mentioning this issue report.

Honza
Oct 3, 2012
Project Member #16 simon.lindholm10
Can we make the "Object" in "Object [...]" into a link?
Oct 3, 2012
Project Member #17 odva...@gmail.com
> Can we make the "Object" in "Object [...]" into a link?
I like that idea! Please create a separate issue for it.
Honza
Oct 3, 2012
Project Member #18 odva...@gmail.com
I also suppose that clicking would navigate the user to the DOM panel, right?
Honza
Oct 3, 2012
Project Member #19 odva...@gmail.com
 Issue 5951 

Honza
Oct 4, 2012
Project Member #22 simon.lindholm10
> but it seems that we need to unwrap the object to make instanceof working properly.
I can't reproduce that. Are you sure? Which Firefox version? (I'm on Nightly.)
Oct 5, 2012
Project Member #23 odva...@gmail.com
> I can't reproduce that. Are you sure? Which Firefox version? (I'm on Nightly.)
Well, you pointed out that the following test is failing, no?
https://getfirebug.com/tests/head/console/reps/console_array.html
And I believe that the reason is the wrapper. Or am I wrong?

Honza

Oct 5, 2012
Project Member #24 odva...@gmail.com
Btw. I am also on Nightly.
Honza
Oct 5, 2012
Project Member #25 simon.lindholm10
By saying it failed I was just going by the test-bot. I didn't test very thoroughly, but did I copy over 'view', 'arr', 'obj', 'win' into global scope and from what I saw in the FBTrace console
'obj instanceof view.NodeList' and
'arr instanceof view.NodeList'
were equivalent. (also with HTMLCollection, and tested only for NodeList's and HTMLCollection's from the page)

I suppose I could look at the actual test later...
Oct 5, 2012
Project Member #26 odva...@gmail.com
This issue has been fixed in Firebug 1.11a4
https://getfirebug.com/releases/firebug/1.11/firebug-1.11.0a4.xpi

Please try it and let us know if it works for you

Thanks for the help!
Honza
Status: Fixed
Labels: fixed-1.11-a4
Oct 5, 2012
Project Member #27 sroussey
Just a note: The original code unwrapped things. A comment on Github suggested a cleaner way with unwrapping, and it worked for me, so I went ahead and did that. Likely should have done that as a separate issue. So I think unwrapping is OK, since that is how we have done it for a while.
Nov 30, 2012
Project Member #28 odva...@gmail.com
FBTest at:
https://github.com/firebug/firebug/commit/2e6730c3d31eafc864e319c680f63e2c250fed0a

Honza
Labels: FBTest-available
Sign in to add a comment

Powered by Google Project Hosting