haxeui / haxeui-openfl

The OpenFL backend of the HaxeUI framework -
http://haxeui.org
MIT License
42 stars 14 forks source link

ListView placement of DropDown incorrect #43

Closed AlexHaxe closed 5 years ago

AlexHaxe commented 5 years ago

When using a DropDown inside a Sprite ListView component does not take into account that Sprite might be offset from screen pos (0/0).

Expected Behavior

ListView opens below DropDown component

Current Behavior

ListView opens displaced by offset of container(s)

Steps to Reproduce (for bugs)

compile for e.g. Neko

import haxe.ui.Toolkit;
import haxe.ui.components.DropDown;
import haxe.ui.data.ArrayDataSource;
import openfl.display.Sprite;

class Main extends Sprite {
    public function new() {
        super();
        Toolkit.init();

        var sprite:Sprite = new Sprite();
        sprite.x = 200;
        sprite.y = 200;
        addChild(sprite);

        var dropDown:DropDown = new DropDown();
        dropDown.dataSource = new ArrayDataSource<String>();
        dropDown.dataSource.add("Text 1");
        dropDown.dataSource.add("Text 2");
        dropDown.dataSource.add("Text 3");
        dropDown.width = 100;
        sprite.addChild(dropDown);
    }
}
<?xml version="1.0" encoding="utf-8"?>
<project>
    <meta title="MyApp" package="" version="1.0.0" company="" />
    <app main="Main" path="out" file="Main" />
    <source path="src" />
    <haxelib name="openfl" />
    <haxelib name="haxeui-core" />
    <haxelib name="haxeui-openfl" />
</project>

Context

my current workaround is to subclass & sneak in a subclassed internal event handler class, so I can override showDropDown and correct left and top with fixed offsets (my app has a graphical header and content starts at a fixed location, so I have two constant offsets)

Your Environment

tested with Haxe 4 on Ubuntu 18.04 Openfl 8.9.1 Lime 7.5.0 haxeui-core git haxeui-openfl git

ianharrigan commented 5 years ago

Sounds like the backend is making assumptions it shouldnt be... ill check it out.

Thanks, Ian

turdparty commented 5 years ago

+1 Please fix. AlexHaxe, would you mind sharing the code please?

AlexHaxe commented 5 years ago

it's basically this:

class SpriteDropDown extends DropDown {
    public function new () {
        super ();
    }

    override private function registerComposite () : Void {
        super.registerComposite ();
        this._internalEventsClass = SpriteDropDownEvents;
    }
}

class SpriteDropDownEvents extends DropDownEvents {
    override public function showDropDown () : Void {
        super.showDropDown ();
        var dropDown : Dynamic = cast _dropdown;
        var handler : IDropDownHandler = dropDown._compositeBuilder.get_handler ();
        if (handler != null) {
            handler.component.top += 155;
            handler.component.left += 10;
        }
    }
}

hardcoded values work for me because of how my application is structured

turdparty commented 5 years ago

Thank you! That helps.

ianharrigan commented 5 years ago

Can you try that again? Latest haxeui-core and haxeui-openfl... should be fixed now,

Thanks for the nice test and thanks for the SpriteDropDownEvents fix which clearly shows the issue :)

ianharrigan commented 5 years ago

Actually its not completely fixed yet. :)

ianharrigan commented 5 years ago

Ok - now it should be fixed

turdparty commented 5 years ago

Sorry, I'm not seeing any change in behavior. I'm testing haxeui-openfl on android. Clicking on an entry also does not select the entry in the dropdown. The list just disappears and the dropdown remains empty. I am using the dropdown in a scrollview.

tested with Haxe 4rc4 Openfl 8.9.4 Lime 7.6.2 haxeui-core git haxeui-openfl git

AlexHaxe commented 5 years ago

I simply commented out most of my SpriteDropDown code to test it and I can confirm it works on Linux (hxcpp and neko targets).

ianharrigan commented 5 years ago

So its not working on android... and android only? Thats weird... :/

turdparty commented 5 years ago

The openfl flash target has been working fine, even before the fix. The android build of openfl is not working. But I haven't tested other platforms sorry. Been crunching for a month..

Oh right, I just tried the flash target again, that is still working. I encountered another issue. You may want to change TableView.fillexistingrenderer with a try catch like so:

var existing=null; try { existing = _tableview.itemRenderer.findComponent(column.id, ItemRenderer, true); } catch (err:Dynamic) {}

In flash the itemrenderer is a label and it crashes. I used this example: https://github.com/AlexHaxe/haxeui-tableview-demo

Thanks again AlexHaxe by the way. Sorry for hijacking the thread, time issues..

turdparty commented 5 years ago

Here is a minimal test case:

Main.hx

`import haxe.ui.Toolkit; import haxe.ui.HaxeUIApp; import haxe.ui.core.Screen; import haxe.ui.core.Component; import haxe.ui.macros.ComponentMacros;

class Main { public static function main() { Toolkit.init();

    var app = new HaxeUIApp();

    app.ready(function() {
        var main:Component = ComponentMacros.buildComponent("Assets/UI.xml");
        app.addComponent(main);
        app.start();
    });
}

}`

UI.xml:

`<?xml version="1.0" encoding="utf-8"?>

ianharrigan commented 5 years ago

FYI - im looking at this now (android)

ianharrigan commented 5 years ago

@turdparty - can you pull latest haxeui-core and haxeui-openfl and try again... there were certainly some scale based issues. Also, ive increased the autoscale dpi threshold. This means that the UI should be bigger. Im not sure if its the "sweet" spot. The device i was testing on was 420dpi and the threshold was 160, so the autoscale ended up as 3x. On my device this seemed a little small. So now its 120, which means the UI is 4x... this seems nicer on my device, but not sure its "right". I can always turn it back off.

If you want to play with it yourself you can set either Toolkit.autoScaleDPIThreshold to something or, Toolkit.scale directly. Id be interested to know what good values are here.

On a separate note, im going to make a few visual enhancements for mobile based UIs so they dont feel so "desktopy, things like having the scrollbars for scrollviews (and sub classes: listview, etc) auto disapearing, etc.

turdparty commented 5 years ago

@ianharrigan Sorry for the late response, I've currently stopped working weekends. I tried the test case above again and the number of issues has increased unfortunately.

  1. The first problem you'll notice is that the background of the vbox does not trigger a scrollevent. And because the scrollbar is now auto-hidden it is not possible to scroll at all.
  2. To circumvent this I set the vbox height from 1000 to 400 so I could test the dropdown, and the list unfortunately still does not popup in the correct place.
  3. Also, the new scrolling makes the issues with triggering mouseevents on components in the scrollview worse. This is also a relatively big issue which I'll get into later. In this test case it means it is very hard to select an item by tapping on it, it takes me about 20 tries. The issue was there with the desktop style scrollbars, but the modern scrollbars make it worse.

For now I would request reverting to the desktop style scrollbars until the issues with the scrollview have been worked out. I'll mention the scrollview issues in a separate github-issue and add a link here later.

ianharrigan commented 5 years ago

Hmmm... that seems strange... do you have a (very) minimal app that shows all this?

Id really like to get this sort of stuff ironed out asap.

Cheers, Ian

turdparty commented 5 years ago

Sure: https://www.dropbox.com/s/x5b4ci3jkd0r64y/dropdowntestcase.zip?dl=0

It's the same code as shown in the test case above.

--Oh right, I did change the vbox height from 1000 to 400 already. So you can see the dropdown right away and test issue 2 & 3. If you set it back to a large height (so the scrollview is needed) you'll notice issue 1.

ianharrigan commented 5 years ago

Ok, so for #1 its an openfl / flash "thing"...

If you dont have anything drawn to a sprite then it doesnt get any events. Ive been pondering this for a while, but a simple fix at the moment is to use this style:

.scrollview-contents {
    background-color: #FF00FF;
    background-opacity: 0;
    width: 100%;
}

This basically makes the scrollview contents 100% and paints and invisible background on it so it will receive events. I dont really like it however, i think another / better way would be to use another sprite for hit testing or something - @intoxopox has mentioned it to me previously, so maybe that is the right path to go. Im looking at the others now

ianharrigan commented 5 years ago

As for the other ones, maybe im missing something? This is the screen i get:

image

Which is correct as of now... ive changed the way it works so that dropdowns show a popup menu in the middle of the screen. If you want to turn off this behaviour do something like:

<dropdown style="mode:normal" />

I just thought that dropdowns on mobiles look / feel wrong.

turdparty commented 5 years ago

Oh you're right, so #2 the listview position of the dropdown is definitely fixed. That's great! Thank you.

In regards to #1, the 100% contents-width I think causes trouble when the scrollview contains a wide tableview for instance, with its internal scrollview disappearing. I'm currently not sure about this, I'll get back to you on that with other scrollview issues.

3 however does prevent the dropdown from currently being practical. That is due to the scrollevents and mouseevents being "in conflict". In the above test-case I have lots of trouble trying to select an item in the dropdown list. Can you confirm this?

It's a difficult generic issue because there are only components with mouseinteraction in the list, and you also need mouseinteraction to scroll. Perhaps the solution would be something like: after 100ms expired { if mousemove>threshold then trigger scrollevent else trigger mouseevent on component } I think you almost never want to trigger both. For instance in case of a scrollview with a vbox with a bunch of editable textfields for instance, the android popup keyboard would constantly get triggered when trying to scroll.

(Also, it would be nice if the autohide scrollbar stays hidden when the Scrollbars have explicitly been set to disabled/hidden.)

ianharrigan commented 5 years ago

Yeah, wrt to #2 you ar right, that is an issue, however, if you know you are going to have a large table view that is bigger than the scrollview can you not just omit the width:100% and it should be fine. I know these arent great solutions, but as work arounds they will have to do until i investigate the "additional hit sprite" route. I wonder if min-width: 100% on the scrollview contents works... not sure.

As for #3 im defo not following you :) Im using your test app above fine... im probably not doing something you are doing when using it.

Cheers, Ian

turdparty commented 5 years ago

Oh that's interesting. Are you perhaps testing on an android emulator? When I run the test app on an android phone I use my finger to tap on the dropdown, the list appears and then I use my finger to select an item from the list. This way I am almost unable to select the item. I do notice the scrollbar appearing briefly.

It's very much possible my finger tap causes a slight mouse move event, probably triggering the scrollbar. But I have no issues whatsoever in the normal android settings menu or something.

You do not experience this issue at all?

What I think happens is that instead of processing a mouseevent on the item component, a scrollevent is processed. I'm guessing this is as soon as a mousemove is detected. And I think this can be resolved by adding a certain threshold.

ianharrigan commented 5 years ago

Hmm, yeah, that is strange... Im using a real device and have no issues what so ever. Do you know what your device DPI is? Or what haxeui is using as a scale?

EDIT: i think you are probably right though, my guess is that its getting a move event which negates any clicking - so to confirm, if you make sure you press verrrry carefully and the scroll doesnt appear... does the selection then work?

turdparty commented 5 years ago

I will look into it and get back to you in about 1 hour. I can confirm this on multiple devices. Weird...

turdparty commented 5 years ago

Exactly, the selection works when I'm able to click without moving. But that's pretty hard using my finger. With a mouse it's not an issue.

The DPI is 320, Toolkit.scale is 2. I did not explicitly set the scale though.

ianharrigan commented 5 years ago

OK, great... so yeah, my guess is thats its easier for me because the DPI was higher (420) and the toolkit scale is 4... ill implement a "only if moved by a certain amount" and see if that fixes things.

turdparty commented 5 years ago

That makes sense. Thank you!

ianharrigan commented 5 years ago

Can you try that again? Ive set the threshold to 3 * scale, so 6 px for you... will be interested in the results.

Cheers, Ian

turdparty commented 5 years ago

Hm, it is a little easier, but still hard to do on this device, feels about twice as accurate. Where can I alter the threshold? I can tweak it and let you know what feels good.

I think it's probably ok to set a large threshold since scrolling tends to be pretty obvious.

Alternatively perhaps a long click to activate a component is an option. In case the component depends on mousemove like with certain charts or signatures or scrollviews in scrollviews for example.

ianharrigan commented 5 years ago

Heres the line: https://github.com/haxeui/haxeui-core/blob/master/haxe/ui/containers/ScrollView.hx#L516

Be good to know what value "feels good"

ianharrigan commented 5 years ago

oh, ive just noticed a bug too: https://github.com/haxeui/haxeui-core/blob/master/haxe/ui/containers/ScrollView.hx#L532-L535

(EDIT: fixed now)

turdparty commented 5 years ago

There are some other issues. Tapping the start of the scrollview responds differently then at the end. And Mouseevents on the components below still trigger after scrolling has been initiated etc. I'm looking into it.

turdparty commented 5 years ago

I think this: hscroll.pos = _offset.x - event.screenX; var distX = Math.abs(_offset.x - event.screenX);

should be: var distX = Math.abs(hscroll.pos - (_offset.x - event.screenX)); hscroll.pos = _offset.x - event.screenX;

So the distance is between the previous and current scrollpos.

ianharrigan commented 5 years ago

Ah, nice one... i just whacked it in without really thinking too much... let me make the change... tell me if its any better.

ianharrigan commented 5 years ago

Ok, done now, let me know if thats better, i think the selection needs to be clearer too... i was selecting and item on the listview and was sure something was broken, but it wasnt. So i think it needs to select, maybe flash, and then remove the popup after a while as it was confusing - different thing, but just something i noticed.

ianharrigan commented 5 years ago

im reverting that.. it doesnt work, and creates problems... instead of this back and forth, maybe lets continue this on #haxeui channel on discord?

turdparty commented 5 years ago

Yes that's working great, thank you! But I see you reverted the code just now..? In any case that worked for the device with 320 dpi & scale 2. I think the threshold is fine at 3, but I think 2 or perhaps even 1 can also work.

--Ah ok, there are some other issues, but it was working great for the dropdown. I'll add it locally and I'm calling it a day. I don't have discord but I think this issue regarding the listview placement can be closed. I'll get back to you with test cases regarding the scrollview.

ianharrigan commented 5 years ago

can you try that latest? Not sure if its right tbh, but feels better...

turdparty commented 5 years ago

It may be slightly better but it's about 1 out of 10 successful selections.

ianharrigan commented 5 years ago

OK, fair enough, can you open other issues regarding this, and we'll close this issue. @AlexHaxe - can you do the honours if you are happy the original issue has been solved (sorry for the spam!)

Cheers, Ian

AlexHaxe commented 5 years ago

DropDowns and their ListViews are properly aligned and work as expected. Thanks!