P T Withington wrote: > Questions: > > 1. Do we need to hide the root context div too if > 'css_hide_canvas_during_init'?
This is probably a good idea for performance... > 2. This > >> this.quirks.fix_contextmenu && this.__LZcontextcontainerdiv > > > seems redundant, since you will only have a context containerdiv if the > quirk is set. True - it's more for clarity's sake, so it's easy to decipher which blocks are tied to the quirk... > 3. Remind me again why we need a contextcontainerdiv _and_ a contextdiv? The contextcontainerdivs are for placement, so if a parent view changes its position/clip/visibility/zIndex the contextdiv ends up in the right place. > 4. Rather than unsetting clip by setting it to 'rect(auto auto auto > auto)', why not just set it to ''? Good point! Are you sure that does the same thing? I guess I can be sure by applying that style to the CSS... > 5. If you are copying _all_ of the local styles from one div to another, > the fastest way is to say: > >> newdiv.style = olddiv.style.cssText I'm explicitly copying the styles that matter... > 6. You should just remove this else clause: > >> cxdiv.id = 'lzcontextdiv'; > > because it 'is an error' to have more than one div with the same ID in a > DOM (even if all browsers will let you get away with it). Good point! I guess the classname is good enough for identification. > 7. Should setID also set the id on the context div if it exists? Probably so... Instead, it caches the ID under the assumption that a context menu will be applied later - but you know what they say about assume :) > 8. Why do we need 'istextsprite'? Can't you just say `foo instanceof > LzTextSprite`? That wasn't working for me. I'll try it again. > Noticed in passing: > > 1. Is 'safari_visibility_instead_of_display' still necessary? If it is, > how come we only use it on the div and not the clickdiv? Probably bit rot. I'm not sure if the quirk is still needed in Safari 4 or not, but I'll fix to be consistent just in case. We really need to go through and test all the quirks in modern browsers - I'm guessing a lot of them are no longer required... While we're at it, we might as well remove/comment out conditionals for quirks that are locked on - like fix_clickable... > On 2009-07-28, at 21:22EDT, Max Carlson wrote: > >> Change 20090728-maxcarlson-L by maxcarl...@bank on 2009-07-28 17:53:48 >> PDT >> in /Users/maxcarlson/openlaszlo/trunk-clean >> for http://svn.openlaszlo.org/openlaszlo/trunk >> >> Summary: Size views with no bgcolor or resource to 0x0, add separate >> tree for context menus >> >> Bugs Fixed: LPP-5447 - DHTML: inputtext and clickable (partial) >> >> Technical Reviewer: ptw >> QA Reviewer: [email protected],hminsky >> >> Details: LzSprite - (from ptw's change - Move the canvas hiding from >> the CSS class style to the canvas div, so removing it just removes the >> div style (and the div reverts to the class style default). Similarly >> for controlling visibility on all divs.) Add quirks property for >> sprite constructor. Add fix_contextmenu and size_blank_to_zero >> quirks, default to on. When fix_contextmenu quirk is on, build >> context menu container div called lzcanvascontextdiv that lives under >> the lzcanvasdiv and lzcanvasclickdiv. Set x and y position, >> visibility, clipping and z-index of context container, if it exists. >> Lazily build up context menu div tree when context menu is set. Base >> __LZclick div on cached width and height values. When >> size_blank_to_zero quirk is on and there's no bgcolor or source >> property set (and we're not a textsprite), set the width/height to 0 >> and set __sizedtozero flag so size can be restored as needed. Set the >> context menu height/width if needed. Restore div size when bgcolor or >> source is set to a non-null value. Modify __findParents() to accept >> an optional second argument - when true, look for parents with a null >> value. Change __updateClip() to update contextmenu and click >> container div clip values. Clean up context menu and context menu >> container divs in destroy(). Cache value passed to setID() so it can >> be used later. >> >> LzTextSprite - Add istextsprite flag to test for text sprite classes >> more easily. >> >> LzMouseKernel - If fix_contextmenu quirk is on, hide visible and click >> div trees so context menu tree can be checked to find the correct >> context menu to show. >> >> Tests: Testcase attached to LPP-7661 works as before, as does the >> testcase from Maynard on 23/Feb/09 12:12 PM. This change will make it >> possible for my recent changeset for LPP-5447 to work with context >> menus... >> >> Files: >> M WEB-INF/lps/lfc/kernel/dhtml/LzSprite.js >> M WEB-INF/lps/lfc/kernel/dhtml/LzTextSprite.js >> M WEB-INF/lps/lfc/kernel/dhtml/LzMouseKernel.js >> >> Changeset: >> http://svn.openlaszlo.org/openlaszlo/patches/20090728-maxcarlson-L.tar >> _______________________________________________ >> Laszlo-reviews mailing list >> [email protected] >> http://www.openlaszlo.org/mailman/listinfo/laszlo-reviews > -- Regards, Max Carlson OpenLaszlo.org _______________________________________________ Laszlo-reviews mailing list [email protected] http://www.openlaszlo.org/mailman/listinfo/laszlo-reviews
