cjolif / dojo-todo-app

Dojo ToDo App
Other
28 stars 15 forks source link

Visual glitch when touching some of todoApp's Back buttons on iOS devices #70

Open AdrianVasiliu opened 12 years ago

AdrianVasiliu commented 12 years ago

This purpose of this ticket is to keep track of a visual glitch that has been reported by ChrisM and that I reproduced so far only with the "todoApp" (demos/todoApp, fresh new in Dojo trunk): when running the todoApp demo from trunk (22 May 2012) on an iOS device, touching some of its dojox.mobile "Back" buttons in the headings leads to an unpleasant two-steps highlighting of the button: first the rectangular area, then the arrow. While being specific to iOS devices, it is not specific to a given theme (it can be reproduced on iOS using the iPhone, Android, and BlackBerry themes). It is currently unknown whether the cause is in dojox.mobile, dojox.app, dojox.mvc, demos/todoApp, or elsewhere.

Explanation: the expectation is that, when touching the button, it goes temporarily in "selected" state, which visual effect is determined by the style sheet. Usually, this happens at the same time (visually) for the two parts of the Back button, the rectangular area and the arrow. But, for some unknown reason, in the todoApp there is slight delay between the highlighting of these parts for some (but not all...) of the Back buttons.

How to reproduce:

  1. Launch demos/todoApp/demo.html on a iOS device. To experiment with different themes, test successively demos/todoApp/demo.html?theme=iPhone, demos/todoApp/demo.html?theme=Android, and demos/todoApp/demo.html?theme=BlackBerry.
  2. In the right-side pane, touch for instance "Reminder 1 to do buy milk".
  3. In the heading of the right-side pane, touch the "Back" button. ==> The rectangular part and the arrow part of the button are not highlighted simultaneously. There is a slight delay (first the rectangle, then the arrow).

Notes: a) Reproduced with iPhone, Android and BlackBerry themes on the following devices: iPhone 4S iOS 5.0.1 and iPad 2 iOS 5.0.1. b) Not reproduced with any of these themes on: Galaxy Tab Android 2.2, Galaxy Nexus Android 4.0.4, BlackBerry Torch 9800 6.0.0.246. c) Not reproduced with another Back button of the same todoApp on the same iOS devices: after touching "Reminder 1 to do buy milk", touch "Repeat" in the list, then touch the Back button. For this Back button (from todoApp/templates/details/repeat.html) the problem does not occur. d) Not reproduced either by testing on the same iOS devices various dojox.mobile apps such as test_GridLayout-demo.html or test_ListItem-actions.html from dojox/mobile/tests, or demos/mobileGallery/demo.html. e) Also not reproduced using dojox/app/tests/multiSceneApp/index.html, nor using dojox/mvc/tests/mobile/demo/demo.html. f) Inside todoApp, the comparison betw. a Back button which has the problem and another which doesn't have it shows that the first is in a page (todoApp/templates/details/detail.html) which includes dojox.app.widgets.Container and dojox.mvc.Group widgets. Now, at least the replacement of dojox.app.widgets.Container by a dojox.mobile.ScrollableView and other attempts to simplify this page do not avoid the problem. g) The cause might also be inside dojox.mobile. Still, so far I only reproduce in todoApp, and there is a bunch of heavy and complex actions in todoApp, such as the "refreshData" called by "beforeActivate" in todoApp's details/details.js and/or in dojox.app/dojox.mvc, which are possible causes of interference with the selection/deselection done by dojox.mobile.ToolBarButton (_setSelectedAttr and _updateArrowColor) and its base class dojox.mobile._ItemBase. Another possible cause would be the interference between the transition mechanism in dojox.app (see dojox.app.controllers.History and dojox.app.controllers.Transition) and dojox.mobile's own transition (both being active in todoApp), but at a first glance deactivating dojox.app's history (if correctly done - just commented it out in config-*.json) doesn't solve the issue. dojox.mvc._atBindingExtension might also come into the play. Nothing clearly identified as being the faulty code so far.

cjolif commented 12 years ago

If I modify ToolBarButton.js to implement _updateArrowColor as this:

    _updateArrowColor: function(selected){
        if(this.arrowNode && !has("ie")){
            if(selected){
                domClass.replace(this.arrowNode, this.selColor, this.defaultColor);
            }else{
                domClass.replace(this.arrowNode, this.defaultColor, this.selColor);
            }
            /*
            var s = domStyle.get(this.bodyNode, "backgroundImage");
            if(s === "none"){ return false; }                   
            domStyle.set(this.arrowNode, "backgroundImage",
                         s.replace(/\(top,/, "(top left,") // webkit new
                         .replace(/0% 0%, 0% 100%/, "0% 0%, 100% 100%") 
                         .replace(/50% 0%/, "0% 0%")); // moz
                         */
        }
        return true;
    },

This "solves" the issue. However it seems (and this sounds logical) that after that the arrow part is kind of missing a background when not selected.

AdrianVasiliu commented 12 years ago

Thanks Christophe. I will further dig into it.

eric-wang commented 12 years ago

I test some applications(todoApp, test applications in dojox/app/tests) on iPhone(5.1) and itouch(5.0.1) and only todoApp has this problem.

AdrianVasiliu commented 12 years ago

Yes, same for me. ("Also not reproduced using dojox/app/tests/multiSceneApp/index.html, nor using dojox/mvc/tests/mobile/demo/demo.html.") I still work on it.

AdrianVasiliu commented 12 years ago

Some findings. The "how to reproduce" is about the Back button from todoApp/templates/details/detail.html which contains this dojox.app widget:

<div data-dojo-type="dojox.app.widgets.Container" id="details" scrollable="true">

Now, the Back button issue goes away by changing the value of "scrollable" into "false". However, in this case the app misbehaves differently (the detail view remains empty except for the heading with the Back button). But if, additionally, I apply eric-wang's very recent patch (not yet committed) for dojox/css3/transition.js: http://bugs.dojotoolkit.org/attachment/ticket/15386/transition.patch the result is that, with scrollable=false in todoApp's detail.html, the trouble of the Back button is gone AND the global behavior looks good, including (surprisingly...) the ability to scroll the detail view... A second way to fix the trouble is to keep scrollable at true while modifying dojox.app.widgets.Container as detailed below.

Details: The class dojox.app.widgets.Container includes (directly or via its parent classes) a mix of dojox.app's specifically modified variants of dojox.mobile's ScrollableView, _ScrollableMixin and scrollable. (Note that in the meantime the code in dojox.mobile on which was based the modified variant in dojox.app has changed quite a bit. I have experimented a partial resync to apply a few of these changes from dojox.mobile's code, with no succes.) What is the usage of "scrollable" in dojox.app.widgets.Container use the value of "scrollable"? There are two usages:

buildRendering: function(){
  ... 
  if(this.scrollable && this.scrollable == "true"){
    this.inherited(arguments);
    this.domNode.style.position = "absolute";
    this.domNode.style.width = "100%";
    this.domNode.style.height = "100%"; 
  }
},
startup: function(){
  ...           
  if(this.scrollable && (this.scrollable == "true")){
    this.inherited(arguments);
  }
}

As far as I can tell from the testing, a second way to fix the misbehavior of the Back button is to keep scrollable at true in detail.html and comment out the call of "inherited()" in the second usage (in startup() of Container). In both ways (scrollable at false, or scrollable at true plus this call removed), todoApp behaves normally, including the scrolling ability (at the level of the container, not browser scrolling), provided that the patch for css3/transition is applied.

All in one, I think there is something fragile in dojox.mobile which breaks because of a particular usage in the context of todoApp. For sure, it would be nice to find a good way to make dojox.mobile's back button more "resistent" to such complex situations, the todoApp case being kind of an extreme case, due to the stack of code and intermixed complex and fragile mechanisms from dojox.mobile, dojox.app, and dojox.mvc (not speaking of dijit.widget). That said, I think it would be helpful to revisit the code of dojox.app's Container to understand how it goes with this "scrollable" parameter which still allows scrolling even if at false... (thus fixing the Back button trouble).

cjolif commented 12 years ago

@eric-wang can you analyze @AdrianVasiliu answer please? In particular why when scrollable=false we can still scroll the view? And if that is expected, then why do we need that flag? Thanks.

eric-wang commented 12 years ago

set scrollable=false will not do scrollable, I test on dojox/app/tests/layoutApp, Chrome19, Win7. What's you test environment and steps? I can try it.

<div data-dojo-type="dojox.app.widgets.Container" region="center" data-dojo-props="scrollable:'false'">
AdrianVasiliu commented 12 years ago

@eric-wang First of all, I said that using scrollable=false I can still scroll in todoApp's "detail" view, that is where the Back button issue was hurting. I didn't say that for dojox/app/tests/layoutApp. And, as I said, in todoApp's "detail" view I was still able to scroll despite scrollable=false provided that your patch for dojox/css3/transition.js (http://bugs.dojotoolkit.org/attachment/ticket/15386/transition.patch ) is applied.

Now, in the meantime, there are lots of code changes in todoApp, and also in dojox.app. With a fresh update of everything, the situation looks as follows:

  1. In dojox/app/tests/layoutApp, as you say, setting scrollable=false disables indeed the scrolling (but, again, I never said the contrary for this test).
  2. In todoApp, the old templates/details/detail.html has been renamed into templates/details/EditTodoItem.html and has a couple of code changes, such as the back button now being a ToolBarButton as child of the Heading instead of simply setting the "back" property of the Heading as before. With this new version of the code, for me this view does NOT scroll despite the fact it still has scrollable=true for the dojox.app.widgets.Container (I now get scroll at the level of the browser page, altogether with the heading, not at the level of the container as before). On the other side, for me the Back button in this view doesn't work anymore at all (it has no effect). This holds for both cases, with your patch in transition.js applied, or without.
AdrianVasiliu commented 12 years ago

The behavior I described in my previous comment holds for the result of a fresh svn-update of my checkout of trunk. Now, doing a new checkout instead of updating my old checkout, the behavior is different. So I'm sorry for the previous description. My svn-update didn't go well for todoApp itself, I guess due to a git-svn bridge trouble. Okay, so here is how it goes for me using the new (correct) checkout:

  1. The view does scroll, and
  2. The back button issue seems basically gone! Obviously, if the original problem is gone, there's no need anymore to set scrollable at false... Of course, it would be good that we understand the deep reason of the trouble that we used to have, and fix it if there's something fragile. But for now we are all good, apparently.
AdrianVasiliu commented 12 years ago

Per our chat, here are the testing results when setting scrollable=false on dojox.app.widgets.Container in details/EditTodoItem.html: A. iPhone iOS 5.0.1: 1/ without Eric's patch in css3/transition : the detail view is empty, and the Back button does nothing 2/ with Eric's patch in css3/transition : all good, the back button works, but I can scroll despite scrollable=false. B. Samsung Galaxy Tab Android 2.2: 1/ without Eric's patch in css3/transition : the detail view is incompletely drawn; the Back button works. 2/ with Eric's patch in css3/transition : all good, the back button works, but I can scroll despite scrollable=false (same behavior as on iOS).

eric-wang commented 12 years ago

set scrollable="false" in details/EditTodoItem.html, patch css3/transition

<div data-dojo-type="dojox.app.widgets.Container" id="details" scrollable="false">

The test result is: A. Samsung Galaxy S plus(i9001), Android2.3.6 detail view cannot scrollable (scrollable=false), "Back" button css style is good

B. iPad 2, iOS 5.0.1 detail view cannot scrollable (scrollable=false), "Back" button on details view css style is good. But the "Back" button on other views (scrollable=true) css style is not good

eric-wang commented 12 years ago

test on iPod touch, iOS5.0.1 detail view scrollable (scrollable=false), "Back" button on details view css style is good. But the "Back" button on other views (scrollable=true) css style is not good

The scrollable more like a browser's feature on iOS, but there's no scrollable bar display. Need to dig the root cause of this problem.

AdrianVasiliu commented 12 years ago

@eric-wang I confirm that, when using your patch for css3/transition, the "details" view is scrollable despite scrollable=false only on iPhone 4S iOS 5.0.1, while on iPad 2 iOS 5.0.1 (and on Android) it is not scrollable... I tested again on both devices with the very same code, and the behavior is different... Might be a due to a subtle timing/speed issue. To clarify, by "scrollable" I understand scroll at the level of the container, that is I scroll the content of the detail view but its header does not move. Differently, when browser scroll occurs, the entire page moves.

Anyway, maybe more important, @eric-wang , do you reproduce (as myself) the fact that, using todoApp as-is (scrollable=true) and the repository version (unpatched) of dojox/css3/transition, the Back button behavior seems improved (for all themes)? That is, the delay between the unhighlighting of the rectangle part of the Back button and of its arrow part is now smaller than before.

I have also tested with the newer css3/transition patch variant by Eric Durocher (http://trac.dojotoolkit.org/attachment/ticket/15386/15386.patch), with a result which looks similar (well, it's not easy to judge/measure how much delay for the arrow...).