maqetta / maqetta

Maqetta Designer
http://maqetta.org
Other
1.04k stars 338 forks source link

Replace iPad check in MobileCreateTool.js with more general check #1011

Closed JonFerraiolo closed 12 years ago

JonFerraiolo commented 12 years ago

@jhpedemonte, @billreed63

Right now MobileCreateTool.js has this line of code:

Maybe with cleanups the ipad check will go away, but if not, we should generalize the ipad check to deal with devices that might have multiple CSS files.

peller commented 12 years ago

no, it's got to go away. The existence of this logic is also hardcoded to remove iphone.css... we can't generalize that, too.

billreed63 commented 12 years ago

dojo.require('dojox.mobile.deviceTheme'); uses the location and the user agent to determine which css files to add to the head.

    var ua = (location.search.match(/theme=(\w+)/)) ? RegExp.$1 : navigator.userAgent;

mobile assumes this is only done once when the deviceTheme is loaded, they did not anticipate that anyone would want to change the theme without reloading the page.

We currently use the theme param on the preview, but in this case the frame has a location. In the case of page designer the frame is created with no location and the document contents are dynamically added.

I only see two option:

1) Add a src to the iframe when the frame is created so that the location can contain the theme param, This seems like a major change to maqetta.

2) dojo.mobile.deviceTheme add an API for dynamic theme switching.. This would basiclly require all our theme mobileTheme switching code move to dojo.mobile.deviceTheme. Remove the old theme css files from the head before added the new ones.

Any other options?

peller commented 12 years ago

we do set HTML BASE on our IFRAME which may or may not have a search property detectable by browsers. I'd prefer option 2, encapsulating as much as possible in dojox.mobile. I think most of the logic is already in dojox.mobile, we'd just need to refactor it slightly and find a way to remove the old stylesheets. Most of the code we've added to handle dojox.mobile themes should go away. It's probably too much to ask dojox.mobile to remove the old stylesheets, but it could easily keep track of the stylesheet urls for us. Otherwise, we shouldn't know anything about the internals of were dojox.mobile puts its stylesheets.

I'll put together a proposal on a branch and use this ticket for reference.

billreed63 commented 12 years ago

Removing the mobile style sheets from the head can be problematic, I have seen iphone style sheets added multiple times to the head in the designer. That was the reason for the following code in setMobileTheme

               // remove the old iPhone files
            var iphone_qStr = 'link'; // iphone gets added by require, so we need to remove it
            var iphone_links = head.querySelectorAll(iphone_qStr);
            // remove the old css files
            for(var ip = 0; ip < iphone_links.length; ip++){
                var href = iphone_links[ip].href;
                if (href.indexOf('iphone/iphone.css') > 0 || href.indexOf('iphone/iphone-compat.css') > 0){
                    head.removeChild(iphone_links[ip]); 
                }

            }

it was not the most efficient approach but it was the most robust

peller commented 12 years ago

if dojox.mobile provides us a list of exactly what it added, the solution should be simple.

billreed63 commented 12 years ago

dojox.moble.deviceTheme generates the css names based on the theme, Android, iphone, blackberry....

var ua = (location.search.match(/theme=(\w+)/)) ? RegExp.$1 : navigator.userAgent;
        for(i = 0; i < m.length; i++){
            if(ua.match(new RegExp(m[i][0]))){
                var theme = m[i][1];
                var files = m[i][2];
                for(j = t.length - 1; j >= 0; j--){
                    var pkg = lang.isArray(t[j]) ? (t[j][0]||"").replace(/\./g, '/') : "dojox/mobile";
                    var name = lang.isArray(t[j]) ? t[j][1] : t[j];
                    var f = "themes/" + theme + "/" +
                        (name === "@theme" ? theme : name) + ".css";
                    files.unshift(require.toUrl(pkg+"/"+f));
                }
                for(j = 0; j < files.length; j++){
                    dm.loadCssFile(files[j].toString());
                }
                break;
            }

dojo.mobile.compat adds a require('dojox.mobile._compat') for none webkit browsers, _conpat is what adds the theme -compat.css stylesheet

         //            This module also loads compatibility CSS files, which has -compat.css
    //      suffix. You can use either the <link> tag or @import to load theme
    //      CSS files. Then, this module searches for the loaded CSS files and loads
    //      compatibility CSS files. For example, if you load iphone.css in a page,
    //      this module automatically loads iphone-compat.css.
peller commented 12 years ago

I proposed a dojox.mobile patch here: http://bugs.dojotoolkit.org/ticket/13916

peller commented 12 years ago

and I made a branch with the relevant Maqetta changes here: af670a4ce77e68cc3dc572a3a5f77e9542697eab

jhpedemonte commented 12 years ago

This may not be the only scenario where reloading the iframe will help us. Jon also pointed out an issue with the fixed property in Dojox Mobile -- if you set that property on a Heading and also have a ScrollableView sibling, the ScrollableView code reorders the DOM, which causes some issues for the rest of the Maqetta code. In that bug, I suggested that reloading the iframe in such a situation might be the best way to put things in order. But that requires more investigation.

Also, I'm not a big fan of forcing this change in the Dojo code for something that is only of use to us; it won't be used by any actual users of the Dojo library.

We also need to consider how to get this code out of core. This needs to reside in the Dojo bundle. I'm thinking of adding an onDeviceChange callback. As arguments, it would take in any of the device names which we already specify, plus "none"; and the Context instance. Then this Dojo bundle code could force an iframe refresh or call the deviceTheme API, which ever we decide on.

peller commented 12 years ago

Reloading the iframe, and constructing it in a way that carries a url, was non-obvious, but is certainly worth pursuing.

The patch requested of Dojo is pretty minimal and IMO not an unreasonable request. There are options to override other things, why not a simple property to override the user agent detection? I feel good about making that request whether or not we use it.

billreed63 commented 12 years ago

Issues to keep in mind when refreshing the iframe:

1) We create the iframe and then dynamically load the page contents, the last device set by the user is store in the page contents as an attribute on the body tag. So we need to find the body element before the setContent/model parsing to put it on the url.

2) When a new page is created we do not add the mobile.deviceTheme until the first mobile widget is added to the page. I think this will not be an issue if we can get the theme on the url Although we do not want to refresh the iframe on every widget drop.

3) Refreshing the iframe does not maintain the selected widget. Currently if we change from one device to another the selected widget stays selected and the theme change is very fast. If we refresh the iframe we loses the selected widget and for a heavily populated page the performance is not that great. Firefox for sure!

jhpedemonte commented 12 years ago

All good points, and for the most part, we can work around them (except maybe [3]). As an aside, I'd like to redo the way we initially put the contents in the iframe. I think doing dynamically, piece-meal is inefficient and causes us issues. Best to just load the actual file (or a template for new pages) and then create the structures we need.

billreed63 commented 12 years ago

I think one of the issues with have the frame load the page content is javascript, we don't want the javascipt in the page to be running in page designer.

jhpedemonte commented 12 years ago

Yes, we do; we need the JS to run. All of the widget libraries that we currently "support" (Dojo, jQuery, YUI) involve a JS parsing step in order to create the widget.

One of the issues is that part of our code assumes Dojo and does the parsing step in the core, rather than letting the Dojo in the iframe handle that. Need to get rid of this and move any Dojo-specific stuff out to that library bundle.

As long as we have a way to map widgets from the source to the DOM of the iframe (usually IDs) then we're fine.

JonFerraiolo commented 12 years ago

That's right. We run all of the JS referenced by dojo.require() but don't run any other SCRIPT tag and strip out all of the on* event listeners so that when we do the innerHTML assignment the document in the IFRAME is a static DOM (i.e., no active JS)

jhpedemonte commented 12 years ago

Jon, what is the exact reason for this? We obviously run some JS (scripts that get added to <head>). Granted we need to disable some things that would interfere with the Visual Editor. I guess we need to catalogue exactly what we need to disable.

JonFerraiolo commented 12 years ago

This approach was inherited from the original SMash code, but makes sense when you think about it. Any SCRIPT tag in the doc might include event listeners (e.g., onload) which would cause various things to happen to the DOM which would conflict with Maqetta, which needs to be in control of the DOM that appears in the IFRAME. Really, I look at us running dojo.require() or the equivalent in other libraries as "special case" in that we only eval that logic because that logic is well-behaved and only defines functions and is only called to do layout and rendering local to the given widget.

jhpedemonte commented 12 years ago

Hmm, well I recently added code that invoked the inline scripts. This was needed by the jQuery and YUI libs, so that their "parsing" step could take place. So we'll need to rethink how this works.

peller commented 12 years ago

89da5b95cb092d7aae3ac22eec2d58c88abf2427 backported changes from http://bugs.dojotoolkit.org/ticket/13916

billreed63 commented 12 years ago

Issue #1024 create theme editor for mobile widget to allow uses to customize mobile theme.

billreed63 commented 12 years ago

I think we should add some code that removes the old css files, we do not need it for context but if anyone else uses it... I found it by doing this bit of code

 function setMobileTheme(){

var dm = dojo.getObject("dojox.mobile", true);
dm.themeMap=[["Android","android",[]],["BlackBerry","blackberry",[]],["iPad","iphone",[require.toUrl("dojox/mobile/themes/iphone/ipad.css")]],["Custom","custom",[]],["mobile3","",["themes/mobile3/mobile3.css"]],[".*","iphone",[]]];
dm.loadDeviceTheme('mobile3');
}
dojo.ready(setMobileTheme);
billreed63 commented 12 years ago

Turns out the code in dojox.mobile._compat that adds the -compat.css files for non webkit browsers only gets run once at _compat load time..

dojox.mobile._compat


ready(function(){
            if(config["mblLoadCompatCssFiles"] !== false){
                setTimeout(function(){ // IE needs setTimeout
                    dm.loadCompatCssFiles();
                }, 0);
            }
            if(dm.applyPngFilter){
                dm.applyPngFilter();
            }
        });

So when we call

dm.loadDeviceTheme('Android');

The correct themes get added but the corisponding -compat.css is missing.

I added the following code to dojox.mobile.deviceTheme which seems to fix the issue The code below also contains code for removing old css theme files. Which is not need for maqetta, but is needed outside of maqetta.

    dm.loadDeviceTheme = function(/*String?*/userAgent){
        // summary:
        //      Loads a device-specific theme according to the user-agent
        //      string.
        // description:
        //      This function is automatically called when this module is
        //      evaluated.
////////////////////////////////////////////////////////////////////////////////////////////////////
        var simulate = userAgent;   // added to fix compat loading when userAgent passed in
////////////////////////////////////////////////////////////////////////////////////////////////////             
        var t = config["mblThemeFiles"] || dm.themeFiles || ["@theme"];
        if(!lang.isArray(t)){ console.log("loadDeviceTheme: array is expected but found: "+t); }
        var i, j;
        var m = dm.themeMap;
        userAgent = userAgent || config["mblUserAgent"] || (location.search.match(/theme=(\w+)/) ? RegExp.$1 : navigator.userAgent);
        for(i = 0; i < m.length; i++){
            if(userAgent.match(new RegExp(m[i][0]))){
                var theme = dm.currentTheme = m[i][1];
                var files = m[i][2];
                for(j = t.length - 1; j >= 0; j--){
                    var pkg = lang.isArray(t[j]) ? (t[j][0]||"").replace(/\./g, '/') : "dojox/mobile";
                    var name = lang.isArray(t[j]) ? t[j][1] : t[j];
                    var f = "themes/" + theme + "/" +
                        (name === "@theme" ? theme : name) + ".css";
                    files.unshift(require.toUrl(pkg+"/"+f));
                }
///////////////////////////remove old css files/////////////////////////////////////////////////////////////////////////////////////
                if (dojox.mobile.loadedCssFiles){
                    var lcss = dojox.mobile.loadedCssFiles;
                    for(j=0;j<lcss.length;j++){
                        dojo.query('link[href="' + lcss[j].toString() + '"]').orphan();
                    }
                }
                dojox.mobile.loadedCssFiles = files;
///////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////
                for(j = 0; j < files.length; j++){
                    dm.loadCssFile(files[j].toString());
                }
                domClass.add(win.doc.documentElement, theme + "_theme");
///////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////
                if(simulate && dm.loadCompatCssFiles){ // we will assume compat is loaded and ready..
                    dm.loadCompatCssFiles();
                }
////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////
                break;
            }
        }
    };
billreed63 commented 12 years ago

Open a ticket at dojo http://bugs.dojotoolkit.org/ticket/13986

peller commented 12 years ago

13986 is fixed. Will bring in with next 1.7 tag (see #829)