Attachment 183242 Details for Bug 330379 – proposed fix

Some Eclipse Foundation services are deprecated, or will be soon. Please ensure you've read this important communication.

[patch] proposed fix

330379_Shell_scroll_HEAD.patch (text/plain), 17.08 KB, created by Istvan Ballok

on 2010-11-16 11:54:54 EST

(hide)

Description:

Filename:

MIME Type:

Creator: Istvan Ballok

Created: 2010-11-16 11:54:54 EST

Size: 17.08 KB

patch

obsolete

>commit 3e5b48ba66e891d4602fd2fba732ea072ead885a
>tree c4f335c0dac178649a47f65e153829ca751e77b2
>parent d3219a0b3cd5e1bd74d00439a51ca033591c62a2
>author Istvan Ballok <Istvan.Ballok@cas.de> 1289926342 +0100
>committer Istvan Ballok <Istvan.Ballok@cas.de> 1289926342 +0100
>
>    bug 330379 popup shell is not restricted to the page and scrolls the whole page
>    
>    proposed fix to the problem:
>    
>    The Problem
>    ===========
>    Open a new Shell and position it so, that it would not completely fit on
>    the page, e.g. by calling setLocation or making the browser window small.
>    
>    What should happen?
>    -------------------
>    The shell should be moved in such a way, that it is fully (or partially)
>    visible in the browser window.
>    
>    What happens?
>    -------------
>    - the Shell is not restricted to the page
>    - the whole html body is scrolled so that the newly opened shell is
>      fully displayed.
>      this is a serious problem, because the overflow for the body is hidden,
>      and there are no top level scrollbars. i.e., the user can not scroll
>      back to the top
>    
>    How to reproduce?
>    -----------------
>    see the attached patch for the rap demo project.
>    
>    The explanation of the problem
>    ==============================
>    1) For https://bugs.eclipse.org/bugs/show_bug.cgi?id=323730, the mechanism
>    to restrict the Shell to the page, on opening it, was deactivated.
>    I think this mechanism should be reactivated, please preferably
>    try to find a different resolution for bug 323730
>    
>    2) the last statement in the response from the RAP server in this case is:
>    org.eclipse.swt.WidgetManager.getInstance().focus( ... );
>    
>    In fact, it is the Shell that gets focused.
>    This call executes in the end Widget.js#visualizeFocus, that calls
>      this.getElement().focus();
>    
>    As we could test it in this snippet:
>    
>    <html>
>    <head>
>    <script type="text/javascript">
>    function focus() {
>        document.getElementById("d").focus()
>    }
>    </script>
>    </head>
>    <body onload="focus()">
>    <div><input tabindex="1" value="hello" type="button" /></div>
>    <div style="margin-top:2000px"><input tabindex="2" value="world"
>      type="button" id="d" /></div>
>    </body>
>    </html>
>    
>    the browser scrolls the newly focused element in view.
>    
>    The Shell.js indirectly inherits from Popup.js, that overrides the
>    _afterAppear function of Parent (and Widget), to first call base, and
>    then adjust the position of the popup, to restrict it to the page.
>    
>    First, as a result of the base call, the "appear" event is fired,
>    that is handled in the WidgetManager by calling focus.
>    
>    Later in _afterAppear, after some calculations, in a setTimeout call,
>    the new position for the shell is set.
>    
>    This means, 2 rounds of deferred execution!
>    1) setTimeout call
>    2) setLeft -> qooxdoo rendering queue --(deferred)-> DOM updated
>    
>    Only after these deferred calls is the DOM node of the shell in the
>    new position.
>    
>    If we focus it before this happens, the html body is scrolled by the
>    browser to display the shell, and hence we have the bug.
>    
>    In the current implementation, focus() is always called before updating the
>    position because the focus is called directly from _afterAppear,
>    and the position update takes place only after 2 deferred calls.
>    
>    Suggested fix for the problem
>    =============================
>    The single workaround I could think of was to delay focusing the widget,
>    so that it happens after it is repositioned.
>    
>    Problems with the fix
>    ---------------------
>    This is unfortunately not fully reliable, because the needed delay
>    depends on how long it takes to execute the javascript in the deferred
>    execution calls + even activities in other browser tabs.
>    I hope you can find a better solution. : )
>    
>    Perhaps changing the _afterAppear implementation in Popup.js to fire
>    the "appear" event only after the Shell is repositioned (i.e. the changes
>    are rendered)
>
>diff --git a/runtime.rwt/org.eclipse.rap.rwt.q07/js/org/eclipse/swt/WidgetManager.js b/runtime.rwt/org.eclipse.rap.rwt.q07/js/org/eclipse/swt/WidgetManager.js
>index d61a394..b283005 100644
>--- a/runtime.rwt/org.eclipse.rap.rwt.q07/js/org/eclipse/swt/WidgetManager.js
>+++ b/runtime.rwt/org.eclipse.rap.rwt.q07/js/org/eclipse/swt/WidgetManager.js
>@@ -36,11 +36,14 @@ qx.Class.define( "org.eclipse.swt.WidgetManager", {
> 
>     _onAppearFocus : function( evt ) {
>       var widget = this;
>-      widget.focus();
>-      evt.getTarget().removeEventListener( 
>-        "appear", 
>-        org.eclipse.swt.WidgetManager._onAppearFocus, 
>-        widget );
>+      var target = evt.getTarget();
>+      setTimeout(function(){
>+        widget.focus();
>+        target.removeEventListener(
>+            "appear",
>+            org.eclipse.swt.WidgetManager._onAppearFocus,
>+            widget );
>+      }, 100);
>     }
>   },
> 
>diff --git a/runtime.rwt/org.eclipse.rap.rwt.q07/js/org/eclipse/swt/widgets/Shell.js b/runtime.rwt/org.eclipse.rap.rwt.q07/js/org/eclipse/swt/widgets/Shell.js
>index 4c2c221..2cedf19 100644
>--- a/runtime.rwt/org.eclipse.rap.rwt.q07/js/org/eclipse/swt/widgets/Shell.js
>+++ b/runtime.rwt/org.eclipse.rap.rwt.q07/js/org/eclipse/swt/widgets/Shell.js
>@@ -17,7 +17,7 @@ qx.Class.define( "org.eclipse.swt.widgets.Shell", {
>     this.base( arguments );
>     this.setOverflow( qx.constant.Style.OVERFLOW_HIDDEN );
>     // Note: This prevents a laoyut-glitch on the ipad:
>-    this.setRestrictToPageOnOpen( false );
>+    //this.setRestrictToPageOnOpen( false );
>     // TODO [rh] HACK to set mode on Label that shows the caption, _captionTitle
>     //      is a 'protected' field on class Window
>     this._captionTitle.setMode( "html" );
>
>commit d3219a0b3cd5e1bd74d00439a51ca033591c62a2
>tree 406803f431abe46bc96aebca462cd126fc8be24d
>parent 98472489803523cf0e3b3c489e575eea8aa168d4
>author Istvan Ballok <Istvan.Ballok@cas.de> 1289926074 +0100
>committer Istvan Ballok <Istvan.Ballok@cas.de> 1289926074 +0100
>
>    bug 330379 popup shell is not restricted to the page and scrolls the whole page
>    
>    simple change to reproduce the problem:
>    
>    The Problem
>    ===========
>    Open a new Shell and position it so, that it would not completely fit on
>    the page, e.g. by calling setLocation or making the browser window small.
>    
>    What should happen?
>    -------------------
>    The shell should be moved in such a way, that it is fully (or partially)
>    visible in the browser window.
>    
>    What happens?
>    -------------
>    - the Shell is not restricted to the page
>    - the whole html body is scrolled so that the newly opened shell is
>      fully displayed.
>      this is a serious problem, because the overflow for the body is hidden,
>      and there are no top level scrollbars. i.e., the user can not scroll
>      back to the top
>    
>    How to reproduce?
>    -----------------
>    see the attached patch for the rap demo project.
>
>diff --git a/runtime.ui/org.eclipse.rap.demo/src/org/eclipse/rap/demo/controls/ShellTab.java b/runtime.ui/org.eclipse.rap.demo/src/org/eclipse/rap/demo/controls/ShellTab.java
>index 2e63c0d..15054ec 100644
>--- a/runtime.ui/org.eclipse.rap.demo/src/org/eclipse/rap/demo/controls/ShellTab.java
>+++ b/runtime.ui/org.eclipse.rap.demo/src/org/eclipse/rap/demo/controls/ShellTab.java
>@@ -349,7 +349,7 @@ public class ShellTab extends ExampleTab {
>     Point result = getShell().getLocation();
>     int count = getShells().length % 12;
>     result.x += 50 + count * 10;
>-    result.y += 50 + count * 10;
>+    result.y += 650 + count * 10;
>     return result ;
>   }
> 
>
>commit 98472489803523cf0e3b3c489e575eea8aa168d4
>tree 68a3d3e2ddb1a9974c6a9fb7504cef287e9fbacb
>parent d5f6cd8c4d0173db5bda0344c30104d7f2c387e5
>author Istvan Ballok <Istvan.Ballok@cas.de> 1289926020 +0100
>committer Istvan Ballok <Istvan.Ballok@cas.de> 1289926020 +0100
>
>    fix git mirror of the rap cvs repository - add missing Scrollable.js file
>    
>    unfortunately:
>    https://bugs.eclipse.org/bugs/show_bug.cgi?id=330271
>
>diff --git a/runtime.rwt/org.eclipse.rap.rwt.q07/js/org/eclipse/swt/widgets/Scrollable.js b/runtime.rwt/org.eclipse.rap.rwt.q07/js/org/eclipse/swt/widgets/Scrollable.js
>new file mode 100755
>index 0000000..9963306
>--- /dev/null
>+++ b/runtime.rwt/org.eclipse.rap.rwt.q07/js/org/eclipse/swt/widgets/Scrollable.js
>@@ -0,0 +1,245 @@
>+/*******************************************************************************
>+ *  Copyright: 2004-2010 1&1 Internet AG, Germany, http://www.1und1.de,
>+ *                       and EclipseSource
>+ *
>+ * This program and the accompanying materials are made available under the
>+ * terms of the Eclipse Public License v1.0 which accompanies this
>+ * distribution, and is available at http://www.eclipse.org/legal/epl-v10.html
>+ *
>+ *  Contributors:
>+ *    1&1 Internet AG and others - original API and implementation
>+ *    EclipseSource - adaptation for the Eclipse Rich Ajax Platform
>+ ******************************************************************************/
>+
>+qx.Class.define( "org.eclipse.swt.widgets.Scrollable", {
>+  extend : qx.ui.layout.CanvasLayout,
>+
>+  construct : function( clientArea ) {
>+    this.base( arguments );
>+    this._clientArea = clientArea;
>+    this._horzScrollBar = new qx.ui.basic.ScrollBar( true );
>+    this._vertScrollBar = new qx.ui.basic.ScrollBar( false );
>+    this._blockScrolling = false;
>+    this._internalChangeFlag = false;
>+    this.add( this._clientArea );
>+    this.add( this._horzScrollBar );
>+    this.add( this._vertScrollBar );
>+    this._configureScrollBars();
>+    this._configureClientArea();
>+    this.__onscroll = qx.lang.Function.bindEvent( this._onscroll, this );
>+  },
>+  
>+  destruct : function() {
>+    var el = this._clientArea._getTargetNode();
>+    if( el ) {
>+      var eventUtil = qx.html.EventRegistration;
>+      eventUtil.removeEventListener( el, "scroll", this.__onscroll );
>+      delete this.__onscroll;
>+    }
>+    this._clientArea = null;
>+    this._horzScrollBar = null;
>+    this._vertScrollBar = null;
>+  },
>+  
>+  events : {
>+    "userScroll" : "qx.event.type.Event"
>+  },
>+
>+  members : {
>+    
>+    /////////
>+    // Public
>+    
>+    setScrollBarsVisible : function( horizontal, vertical ) {
>+      this._horzScrollBar.setDisplay( horizontal );  
>+      this._vertScrollBar.setDisplay( vertical );
>+      var overflow = "hidden";
>+      if( horizontal && vertical ) {
>+        overflow = "scroll";
>+      } else if( horizontal ) {
>+        overflow = "scrollX";
>+      } else if( vertical ) {
>+        overflow = "scrollY";
>+      }
>+      this._clientArea.setOverflow( overflow );
>+      this._layoutX();
>+      this._layoutY();
>+    },
>+ 
>+    setHBarSelection : function( value ) {
>+      this._internalChangeFlag = true;
>+      if( this._horzScrollBar.getMaximum() < value ) {
>+        // TODO [tb] : The ScrollBar should do that itself
>+        this._horzScrollBar.setMaximum( value );
>+      }
>+      this._horzScrollBar.setValue( value );
>+      this._internalChangeFlag = false;
>+    },
>+
>+    setVBarSelection : function( value ) {
>+      this._internalChangeFlag = true;
>+      if( this._vertScrollBar.getMaximum() < value ) {
>+        // TODO [tb] : The ScrollBar should do that itself
>+        this._vertScrollBar.setMaximum( value );
>+      }
>+      this._vertScrollBar.setValue( value );
>+      this._internalChangeFlag = false;
>+    },
>+    
>+    setBlockScrolling : function( value ) {
>+      this._blockScrolling = value;
>+    },
>+ 
>+    /////////
>+    // Layout
>+
>+    _configureClientArea : function() {
>+      this._clientArea.setOverflow( "scroll" );
>+      this._clientArea.setLeft( 0 );
>+      this._clientArea.setTop( 0 );
>+      this._clientArea.addEventListener( "create", this._onClientCreate, this );
>+      this._clientArea.addEventListener( "appear", this._onClientAppear, this );
>+      // TOOD [tb] : Do this with an eventlistner after fixing Bug 327023 
>+      this._clientArea._layoutPost 
>+        = qx.lang.Function.bindEvent( this._onClientLayout, this );
>+    },
>+    
>+    _configureScrollBars : function() {
>+      var dragBlocker = function( event ) { event.stopPropagation(); };
>+      var preferredWidth = this._vertScrollBar.getPreferredBoxWidth();
>+      var preferredHeight = this._horzScrollBar.getPreferredBoxHeight();
>+      this._horzScrollBar.setLeft( 0 );
>+      this._horzScrollBar.setHeight( preferredHeight );
>+      this._horzScrollBar.addEventListener( "dragstart", dragBlocker );
>+      this._vertScrollBar.setTop( 0 );
>+      this._vertScrollBar.setWidth( preferredWidth );
>+      this._vertScrollBar.addEventListener( "dragstart", dragBlocker );
>+      this._horzScrollBar.addEventListener( "changeValue", 
>+                                            this._onHorzScrollBarChangeValue, 
>+                                            this );
>+      this._vertScrollBar.addEventListener( "changeValue", 
>+                                            this._onVertScrollBarChangeValue, 
>+                                            this );      
>+    },
>+
>+    _applyWidth : function( newValue, oldValue ) {
>+      this.base( arguments, newValue, oldValue );
>+      this._layoutX();
>+    },
>+    
>+    _applyHeight : function( newValue, oldValue ) {
>+      this.base( arguments, newValue, oldValue );
>+      this._layoutY();
>+    },
>+    
>+    _applyBorder : function( newValue, oldValue ) {
>+      this.base( arguments, newValue, oldValue );
>+      this._layoutX();
>+      this._layoutY();
>+    },    
>+
>+    _layoutX : function() {
>+      var clientWidth = this.getWidth() - this.getFrameWidth();
>+      if( this._vertScrollBar.getDisplay() ) {
>+        clientWidth -= this._vertScrollBar.getWidth();
>+      } 
>+      this._clientArea.setWidth( clientWidth );
>+      this._vertScrollBar.setLeft( clientWidth );
>+      this._horzScrollBar.setWidth( clientWidth );
>+    },
>+
>+    _layoutY : function() {        
>+      var clientHeight = this.getHeight() - this.getFrameHeight();
>+      if( this._horzScrollBar.getDisplay() ) {
>+        clientHeight -= this._horzScrollBar.getHeight();
>+      } 
>+      this._clientArea.setHeight( clientHeight );
>+      this._vertScrollBar.setHeight( clientHeight );
>+      this._horzScrollBar.setTop( clientHeight );
>+    },
>+
>+    _onClientCreate : function( evt ) {
>+      this._clientArea.prepareEnhancedBorder();
>+      var el = this._clientArea._getTargetNode();
>+      var eventUtil = qx.html.EventRegistration;
>+      eventUtil.addEventListener( el, "scroll", this.__onscroll );
>+    },
>+
>+    _onClientLayout : function() {
>+      // TODO [tb] : _getScrollBarWidth should be a static function, it has 
>+      // nothing to do with the instance, which here is only used to call it
>+      var barWidth = this._horzScrollBar._getScrollBarWidth();
>+      var node = this._clientArea._getTargetNode();
>+      var el = this._clientArea.getElement();
>+      var overflow = this._clientArea.getOverflow();
>+      var width = parseInt( el.style.width );
>+      var height = parseInt( el.style.height );
>+      if( overflow === "scroll" || overflow === "scrollY" ) {
>+        width += barWidth;
>+      }
>+      if( overflow === "scroll" || overflow === "scrollX" ) {
>+        height += barWidth;
>+      }
>+      node.style.width = width
>+      node.style.height = height;
>+    },
>+    
>+    ////////////
>+    // Scrolling
>+   
>+    _onHorzScrollBarChangeValue : function() {
>+      if( this._isCreated ) {
>+        this._syncClientArea( true, false );
>+      }
>+      if( !this._internalChangeFlag ) {
>+        this.createDispatchEvent( "userScroll" );
>+      }
>+    }, 
>+
>+    _onVertScrollBarChangeValue : function() {
>+      if( this._isCreated ) {
>+        this._syncClientArea( false, true );
>+      }
>+      if( !this._internalChangeFlag ) {
>+        this.createDispatchEvent( "userScroll" );
>+      }
>+    },
>+
>+    _onClientAppear : function() {
>+      this._internalChangeFlag = true;
>+      this._syncClientArea( true, true );
>+      this._internalChangeFlag = false;
>+    },
>+     
>+    _onscroll : function( evt ) {
>+      if( !this._internalChangeFlag ) {
>+        org.eclipse.rwt.EventHandlerUtil.stopDomEvent( evt );
>+        var blockH = this._blockScrolling || !this._horzScrollBar.getDisplay();
>+        var blockV = this._blockScrolling || !this._vertScrollBar.getDisplay();
>+        this._internalChangeFlag = true;
>+        this._syncClientArea( blockH, blockV );        
>+        this._internalChangeFlag = false;
>+        this._syncScrollBars();
>+      }
>+    },
>+
>+    _syncClientArea : function( horz, vert ) {
>+      if( horz ) {
>+        var scrollX = this._horzScrollBar.getValue();
>+        this._clientArea.setScrollLeft( scrollX );
>+      }
>+      if( vert ) {
>+        var scrollY = this._vertScrollBar.getValue();
>+        this._clientArea.setScrollTop( scrollY );
>+      }
>+    },
>+
>+    _syncScrollBars : function() {
>+      var scrollX = this._clientArea.getScrollLeft();
>+      this._horzScrollBar.setValue( scrollX );
>+      var scrollY = this._clientArea.getScrollTop();
>+      this._vertScrollBar.setValue( scrollY );
>+    }
>+
>+  }
>+} );

Actions: View | Diff

Attachments on bug 330379: 183242