Bug in marginal and sidebar

[John_d_3]

Hi,

I found a bug in some of the layout components, this exposes itself on some of my target browsers.

It's got to do with the use of toString.call() in the following modules :

xdk/components/server/ui-builder/ui-builder_repo/resources/toolbar/layout/supporting/
marginal and sidebar

I have a fix which I have applied to my web-site and my local XDK install, what is the best way to submit it back to you guys ?

Cheers
JohnM

[Brandon_K_Intel]

What behavior of the toString.call() function, in those modules, do you believe should be considered a bug? I need more help understanding the issue! Thanks!

[John_d_3]

Hey, OK :-) I can tell you guys target webkit/chrome/nodejs/V8 mostly. Fundamental issue is that toString is part of the Object prototype. Webkit derived engines seem to promote Object.prototype members to the global scope. They really shouldn't really be there, as the correct form would be Object.prototype.toString().call() Either way, it doesn't matter, what makes it a bug is that it'll fail on non webkit browsers like Windows 8, etc ... And it is harder to spot because it is being used to do a fast "typeof" (toString being faster), however, calling the prototypical toString is really only necessary when the value could conceivable be "undefined", which in the context of the code in "marginal" and "sidebar" isn't going to happen ... Anyway, that is what it is, and for my site I spotted the issue in three places (I am happy to send you the patch). You have a whole load of node-webkit code doing this, but it'll work fine as it always runs under V8. However, the browser deployed code needs to be more standards compliant or it'll fail. Let me know if you need more info. Cheers JohnM

[Thepjday_A_]

I found a bug in the sidebar.js. The "perform_open_close_sidebar" function does not work as expected.

1. Code establishes should_toggle on whether the sidebar is visible

2. Code checks to see if the open is true and if so, reverses the should_toggle value - this is a mistake

3. Code checks to see if should_toggle is true and if so, performs a toggle - this is also a mistake

The code should:
1. Determine if the sidebar is_open

2. If the is_open does not match the open value requested (true/false), the toggle should be performed

[Chris_P_Intel]

@John D.

Thanks for the report. I've logged this as an issue and we'll investigate it.

@Thepdjd

I'm not sure that yours is a bug. What is it you are trying to do that is not working?

The code should definitely be clearer. Anytime the reader has to start doing boolean logic in their head, that is a failure.

But the logic of the code is:

1 - set "should_toggle" to be true if the sidebar is visible. Otherwise false.  This is the correct choice for a close directive (open == false).  Because  if the sidebaris open now, close it.   If the sidebar is closed, do nothing.  

2. But for the open==true directive, it's the opposite. So we reverse it.   Now, if the sidebar is presently open, do nothing.  Otherwise, close it.

3. Check should_toggle, and toggle accordingly.