Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fixing PanZoomBar and Panel issues after #164. #167

Merged
merged 3 commits into from
Jan 24, 2012

Conversation

ahocevar
Copy link
Member

For PanZoomBar, this fixes the slider behavior. Now the buttonclick listener argument also includes a buttonXY property, and PanZoomPanel does not need an Events instance for the zoombarDiv any more.

For Panel, this fixes events for panels outside the map. Just setting the element on the Events instance was no longer enough after e70569b. Events::attachToElement is now used, and it needed to be modified to also work if the Events instance had no element previously.

Finally, I renamed the button property of the buttonclick listener argument to buttonElement, to not confuse it with the browser event button property, and added some more tests and documentation.

@jorix
Copy link
Contributor

jorix commented Jan 21, 2012

See this warnings:

..\build\temp\lib\OpenLayers\Control\PanZoomBar.js:216: WARNING - Property doubleClick never defined on OpenLayers.Control.PanZoomBar
            "dblclick": this.doubleClick,
                        ^

..\build\temp\lib\OpenLayers\Control\PanZoomBar.js:217: WARNING - Property doubleClick never defined on OpenLayers.Control.PanZoomBar
            "click": this.doubleClick
                     ^

..\build\temp\lib\OpenLayers\Control\PanZoomBar.js:266: WARNING - Property doubleClick never defined on OpenLayers.Control.PanZoomBar
            "dblclick": this.doubleClick,
                        ^

..\build\temp\lib\OpenLayers\Control\PanZoomBar.js:267: WARNING - Property doubleClick never defined on OpenLayers.Control.PanZoomBar
            "click": this.doubleClick
                     ^

..\build\temp\lib\OpenLayers\Control\PanZoomBar.js:307: WARNING - Property passEventToSlider never defined on OpenLayers.Control.PanZoomBar
            "touchmove": this.passEventToSlider,
                         ^

..\build\temp\lib\OpenLayers\Control\PanZoomBar.js:308: WARNING - Property passEventToSlider never defined on OpenLayers.Control.PanZoomBar
            "mousemove": this.passEventToSlider,
                         ^

..\build\temp\lib\OpenLayers\Control\PanZoomBar.js:309: WARNING - Property passEventToSlider never defined on OpenLayers.Control.PanZoomBar
            "mouseup": this.passEventToSlider,
                       ^

..\build\temp\lib\OpenLayers\Control\PanZoomBar.js:361: WARNING - Property passEventToSlider never defined on OpenLayers.Control.PanZoomBar
                "touchmove": this.passEventToSlider,
                             ^

..\build\temp\lib\OpenLayers\Control\PanZoomBar.js:362: WARNING - Property passEventToSlider never defined on OpenLayers.Control.PanZoomBar
                "mouseup": this.passEventToSlider,
                           ^

..\build\temp\lib\OpenLayers\Control\PanZoomBar.js:363: WARNING - Property passEventToSlider never defined on OpenLayers.Control.PanZoomBar
                "mousemove": this.passEventToSlider,
                             ^

@ahocevar
Copy link
Member Author

Thanks @jorix for your continued testing effort. Does it look better to you now?

@jorix
Copy link
Contributor

jorix commented Jan 22, 2012

@ahocevar:

I have not done a thorough test, I only tried the points they I thought they could be problematic.

Android:

Chrome and FF:

  • 164&167 Allowed drag the map in the body of LayerSwitcher or in the margins of OverviewMap
  • 164&167 It seems strange that starting a drag on a button and completing the other the latter one is the one that works.
  • 167 and on the scroll bar in PanZoomBar prevail initial point of a drag (without touching the slider)

Now "closure_verify" provides no warning.

@ahocevar
Copy link
Member Author

@jorix, thanks for testing again, but are you sure you have pulled the latest? I just tried again, and my results are way more promising than yours:

Android:

  • I cannot reproduce what you describe for the EditingToolbar, OverviewMap and Layerswitcher - Panel clicking and minimizing/maimizing works fine for me.
  • Dragging in the scrollbar of the PanZoomBar is prevented for me.
  • Tapping in the PanZoomBar works fine for me.

Chrome and FF:

  • Dragging in the body of the LayerSwitcher and the border of the OverviewMap is intentional.
  • You are right. When dragging a button, the mouseup position is what counts. I find this natural - in case the user changes their mind after starting to click.
  • I don't understand what you mean with "prevail initial point of a drag (without touching the slider)".

@jorix
Copy link
Contributor

jorix commented Jan 22, 2012

The tests I usually make with VirtualBox+Android x86 2.2, now I've tested with a mobile device Android 2.2 and never fails. So far I had not noticed different behavior, except that my fingers are too big for my little mobile ;-)

I downloaded your branch button-controls-improved again and x86 still fails, is the first time I see these discrepancies.

It seems that except for x86 code behaves well, so I think you can ignore the problem on Android x86.

"prevail initial point of a drag (without touching the slider)" oops!
Using PanZoomBar when it becomes a drag on scrollbar (without touching the slider) the zoom is based on mousedown instead moseup

@ahocevar
Copy link
Member Author

Thanks @jorix for the explanation. I guess the mousedown-mouseup issue on the zoombar is very minor, don't you agree? For what it's worth, I use the Android Emulator for testing if I don't want to use a real mobile device.

@jorix
Copy link
Contributor

jorix commented Jan 22, 2012

Yes agree, mousedown-mouseup issue on the zoombar is minor.

Android x86 is not exactly an emulator is much faster than an emulator because it uses x86 architecture. But I have to find an update and see that the behavior is consistent with the native Android.

For PanZoomBar, this fixes the slider behavior. Now the buttonclick listener argument also includes a buttonXY property, and PanZoomPanel does not need an Events instance for the zoombarDiv any more.
For Panel, this fixes events for panels outside the map. Just setting the element on the Events instance was no longer enough after e70569b. Events::attachToElement is now used, and it needed to be modified to also work if the Events instance had no element previously.
Finally, I renamed the button property of the buttonclick listener argument to buttonElement, to not confuse it with the browser event button property, and added some more tests and documentation.
@elemoine
Copy link
Member

I'm not familiar with the new event extension concept yet, but this particular diff looks sane to me. I can confirm that all tests (but Format.WKT #162 and Strategy.BBOX #161) pass in FireFox 9 and Chromium 15. Relevant tests pass in IE 8 too. Please merge.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants