cindyskach commented 13 years ago

new HTML file
drop a DropdownButton onto the canvas
save, close and reopen the file

The outline will now have a div containing a menu.

@jhpedemonte @JonFerraiolo

Edit Nov 3 2011: Changed title from DropdownButton has div associated with it upon reopening.

JonFerraiolo commented 13 years ago

@billreed63, @peller

Bill might know what's going wrong here.

billreed63 commented 13 years ago

Not sure why this stopped working from preview 1, but DropDownButton creates a dijitMenu and places it in a div the div. I added code in the DropDownButtonHelper.create that finds the _dropDown div and marks that widget.hidden=true so it will not be displayed in the outline tree.

July 2011

jhpedemonte commented 13 years ago

I'm very confused by this. I decided to investigate this, since I think Bill's fix may just be a band-aid on top of a larger problem.

First, here's what the HTML code contains:

<body class="claro" data-davinci-ws="collapse" dvFlowLayout="true" id="myapp">
    <span dojoType="dijit.form.DropDownButton" disabled="false" intermediateChanges="false" label="Drop Down" iconClass="dijitNoIcon">
        <span dojoType="dijit.Menu">
            <span dojoType="dijit.MenuItem" label="Option 1"></span>
            <span dojoType="dijit.MenuItem" label="Option 2"></span>
        </span>
    </span>
</body>

Where does the <div> in the Outline come from? I'm assuming it's from when Dojo "saturates" the HTML code to create the actual widget. We need to look at this more to find out exactly what is going on.

jhpedemonte commented 13 years ago

Not sure if I'm looking at the right place, but I'm at Context._attachChildren(). The call stack looks somewhat like this (skipping over unimportant bits):

Context._attachChildren()
Context._attachAll()
Context.activate()
...
Context.setSource()

So this is getting called when loading the file that has the DropDownButton.

_attachChildren() is passed in the DOMNode for <body> and the first line iterates over all the children, calling Context.attach() on each. I'm assuming that attach() is what leads to the <div> appearing in the Outline.

The interesting thing is that this code is working on the DOM, not the Model. So the <body> doesn't look like the source in my previous comment. At this point in the code, it looks like this:

<body id="myapp" style="width: 100%; height: 100%; visibility: visible; margin-top: 0px; margin-right: 0px; margin-bottom: 0px; margin-left: 0px;" class="claro HtmlWidget" data-davinci-ws="collapse">
    <span class="dijit dijitReset dijitInline dijitDropDownButton" widgetid="span_0">
        <span class="dijitReset dijitInline dijitButtonNode" dojoattachevent="ondijitclick:_onClick" dojoattachpoint="_buttonNode">
            <span class="dijitReset dijitStretch dijitButtonContents dijitDownArrowButton" dojoattachpoint="focusNode,titleNode,_arrowWrapperNode" role="button" aria-haspopup="true" aria-labelledby="span_0_label" tabindex="0" aria-disabled="false" id="span_0" style="-webkit-user-select: none;">
                <span class="dijitReset dijitInline dijitButtonText" dojoattachpoint="containerNode,_popupStateNode" id="span_0_label">Drop Down</span>
                <span class="dijitReset dijitInline dijitArrowButtonChar">▼</span>
            </span>
        </span>
        <input type="button" value="" class="dijitOffScreen" tabindex="-1" dojoattachpoint="valueNode">
    </span>
    <div class="dijitPopup" style="display: none;" role="presentation">
        <table class="dijit dijitMenu dijitMenuPassive dijitReset dijitMenuTable" role="menu" tabindex="0" dojoattachevent="onkeypress:_onKeyPress" cellspacing="0" id="span_1" widgetid="span_1" style="top: 0px;">
            <tbody class="dijitReset" dojoattachpoint="containerNode">
                <tr class="dijitReset dijitMenuItem" dojoattachpoint="focusNode" role="menuitem" tabindex="-1" dojoattachevent="onmouseenter:_onHover,onmouseleave:_onUnhover,ondijitclick:_onClick" aria-labelledby="span_2_text span_2_accel" style="-webkit-user-select: none;" id="span_2" widgetid="span_2">
                    <td class="dijitReset dijitMenuItemIconCell" role="presentation">
                        <img src="lib/dojo/dojo/resources/blank.gif" alt="" class="dijitIcon dijitMenuItemIcon dijitNoIcon" dojoattachpoint="iconNode">
                    </td>
                    <td class="dijitReset dijitMenuItemLabel" colspan="2" dojoattachpoint="containerNode" id="span_2_text">
                        Option 1
                    </td>
                    <td class="dijitReset dijitMenuItemAccelKey" style="display: none" dojoattachpoint="accelKeyNode" id="span_2_accel"></td>
                    <td class="dijitReset dijitMenuArrowCell" role="presentation">
                        <div dojoattachpoint="arrowWrapper" style="visibility: hidden">
                            <img src="lib/dojo/dojo/resources/blank.gif" alt="" class="dijitMenuExpand"> <span class="dijitMenuExpandA11y">+</span>
                        </div>
                    </td>
                </tr>
                <tr class="dijitReset dijitMenuItem" dojoattachpoint="focusNode" role="menuitem" tabindex="-1" dojoattachevent="onmouseenter:_onHover,onmouseleave:_onUnhover,ondijitclick:_onClick" aria-labelledby="span_3_text span_3_accel" style="-webkit-user-select: none;" id="span_3" widgetid="span_3">
                    <td class="dijitReset dijitMenuItemIconCell" role="presentation">
                        <img src="lib/dojo/dojo/resources/blank.gif" alt="" class="dijitIcon dijitMenuItemIcon dijitNoIcon" dojoattachpoint="iconNode">
                    </td>
                    <td class="dijitReset dijitMenuItemLabel" colspan="2" dojoattachpoint="containerNode" id="span_3_text">
                        Option 2
                    </td>
                    <td class="dijitReset dijitMenuItemAccelKey" style="display: none" dojoattachpoint="accelKeyNode" id="span_3_accel"></td>
                    <td class="dijitReset dijitMenuArrowCell" role="presentation">
                        <div dojoattachpoint="arrowWrapper" style="visibility: hidden">
                            <img src="lib/dojo/dojo/resources/blank.gif" alt="" class="dijitMenuExpand"> <span class="dijitMenuExpandA11y">+</span>
                        </div>
                    </td>
                </tr>
            </tbody>
        </table>
    </div>
</body>

This is the saturated Dojo widget. Now, _attachChildren iterates over the children (the <span> and <div>) and calls davinci.ve.widget.getWidget(), which creates a Maqetta widget object (DijitWidget and HTMLWidget, respectively) for each child, and then calls Context.attach() on each of the widget objects.

First, maybe we need to make davinci.ve.widget.getWidget() smarter, somehow, so in this case it would only return something for the DropDownButton and not for the associated <div>.

Second, should this code be working on the DOM? Maybe it needs to function on the Model, since that is the truer representation of the HTML source. If we iterate over the DOM, we always have this situation where we are working on code that may not match the source/Model.

@peller @childsb

billreed63 commented 13 years ago

VisualEditorOutline.js uses widget.hidden to filter widgets when displaying the tree. I am not sure why it was add, possibly other widgets have this same issue. Changing the getWidget this late in M2 would be to risky, maybe we should push this issue to M3.

JonFerraiolo commented 13 years ago

I'm a little confused by the thread above in terms of what is being proposed short-term and long-term, but I'll say that most of our widget.js logic traverses the iframe's DOM and does various filtering to ignore various things. Most of the filtering is checking _dvWidget property on DOM nodes. If no _dvWidget, then that DOM node shouldn't show in the Outline. There is other filtering going on, too, some of which I understand, and others which I haven't run into yet.

We certainly don't want to change getChildren() to use the model instead of the DOM at this point of the cycle. Interesting idea about maybe doing that long-term, though. We use the DOM mostly for historical reasons because the original code didn't have a model. My instincts are that life would be much easier if we used the model instead of the DOM, but that's probably a multiple man-month transition.

jhpedemonte commented 13 years ago

Oh, yeah, my proposals weren't for M2, but longer-term.

Regarding _dvWidget, that isn't an option at the loading stage. This is when the code is first being parsed. However, the way that is done is by adding the code to the DOM (resulting in Dojo saturation of widgets) and then iterating over the DOM doing setup things.

So yes, I think this is something that we should look at using the Model instead of the DOM.

Keep this open for now, so we can hear from @peller or @childsb.

billreed63 commented 13 years ago

751 Is a similar issue.

jhpedemonte commented 13 years ago

Some more discussion on getWidget() took place in #717.

jhpedemonte commented 13 years ago

I think this is too risky for M4. Pushing to M5 and raising priority to high.

peller commented 13 years ago

What's the motivation for doing this, again? If it's just the popup wrapper div on dropdownbutton, I think we ought to pursue bill's workaround (which seems to fail on reload for some reason) see #1137

jhpedemonte commented 13 years ago

We've run into several issues now which are caused by code depending on the VE DOM rather than the Model. Instead of continuing to add workarounds, we should just do the right thing.

JonFerraiolo commented 13 years ago

@jhpedemonte: what are you thinking exactly in this case? Right now we use dvwidget.getChildren() as the main API for traversing the widgets that are listed in the Outline palette and used elsewhere in the application (e.g., the dynamic grid lines also uses that API). Are you thinking that we rewrite the logic in dvwidget.getChildren() to use the model instead of the iframe's DOM, or are you thinking we create a new APIs that we call instead of dvwidget.getChildren()?

jhpedemonte commented 13 years ago

Not sure, haven't studied it in depth. Does dvWidget have a property that points to the model? If not, we should at least create a helper method. And the model should be made easily traversable, if it is not already.

(cc @waynevicknair, since this is model related)

JonFerraiolo commented 13 years ago

Yes, if you go into Chome debugger, in Elements view, click on hourglass and then click on a dijit Button or whatever on the canvas. Then in styling sections on right, go to Properties and open up HTMLDivElement at top. There you'll see property _dvWidget, and underneath that you'll see _srcElement.

peller commented 12 years ago

With #1137, the initial concern in this ticket is fixed. I still don't see the need to change what we're doing.

JonFerraiolo commented 12 years ago

I still don't see the need to change what we're doing.

@peller, you are talking about the original issue here (switch to model instead of VE data structures)? Personally, I would like to see a detailed proposal for the proposed coding changes and which parts of our code would switch over. It might be a good thing to make this change (due to cleaner and more reliable code), but I worry that pulling on the string will reveal that the string is very long.

jhpedemonte commented 12 years ago

The reason is that the model should reflect the source (the model). By generating the Outline from the VE DOM, we have to put in special checks and workarounds to exclude the things that shouldn't be there. By generating from the model, we can avoid that.

jhpedemonte commented 12 years ago

-> M6

jhpedemonte commented 12 years ago

-> M7

peller commented 12 years ago

reading my earlier comments, and I've flip-flopped completely on this one. Implementing a real model (perhaps a dojo.store one) would mean we could get rid of the refresh and have the tree update based on real data.

jhpedemonte commented 12 years ago

Are you saying recreate our main Model as a dojo.store? Or create a dojo.store facade to the Model, for use by the Outline?

peller commented 12 years ago

I'd think running off a single model would be optimal, rather than working to keep multiple models in synch, but that's exactly what we're going to have to figure out.

jhpedemonte commented 12 years ago

I think it makes sense to use something that integrates with the Dojo widgets we use -- something like dojo.store (or dojo.mvc?). To start with, we can create a facade and slowly convert the users of the Model to the new way of doing things.

peller commented 12 years ago

It's got to be dojo.store (or dojo.data) to integrate with the outline (a dijit.Tree)

jhpedemonte commented 12 years ago

-> M8

JonFerraiolo commented 12 years ago

-> M8 Prio-Med

maqetta / maqetta

Generate Outline from Model (instead of VE DOM) #750

751 Is a similar issue.