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

[sailfish-browser] Display a handle at the top of the popup menu. Fixes JB#54528 OMP#JOLLA-183 #858

Merged
merged 2 commits into from Jun 23, 2021

Conversation

adenexter
Copy link
Contributor

No description provided.

@adenexter adenexter force-pushed the jb54528 branch 4 times, most recently from e831fcc to 2faaf2d Compare June 7, 2021 22:56
@adenexter adenexter changed the title WIP [sailfish-browser] Display a handle at the top of the popup menu. Fixes JB#54528 OMP#JOLLA-183 [sailfish-browser] Display a handle at the top of the popup menu. Fixes JB#54528 OMP#JOLLA-183 Jun 7, 2021
@adenexter adenexter force-pushed the jb54528 branch 3 times, most recently from 2e33d49 to 6f2a36a Compare June 10, 2021 03:43
@rainemak
Copy link
Member

I think we should review the rest as one meaning that we should "ignore" #857

@llewelld
Copy link
Member

This is really nice behaviour. I'm experiencing a bit of a glitch though, when a page is added to or removed from the pageStack. The horizontal offset seems to mix badly with the shader positioning. These screenshots are from selecting the History item in the menu.

menuslide

@adenexter
Copy link
Contributor Author

This is really nice behaviour. I'm experiencing a bit of a glitch though

I commented here first, #856 (comment)

I guess I will have to use a custom shader for the background after all in order to change the reference coordinates for the texure coordinates. It should be trivial but I just could not make it work the other day.

@adenexter
Copy link
Contributor Author

It should be trivial but I just could not make it work the other day.

Now I know why I couldn't make it work. The scene graph renderer pre-transforms geometry so it can batch together different items into a single render call. So my single item which I create with the geometry (0, 0, 768x1419) gets submitted to the GPU as (200, 32, 768x1419) or something worse if the item is rotated or otherwise transformed. All of which means I can't normalize the position using the item size to get texture coordinates which is what I was trying to do.

So I can add a normalized position coordinates to the Background item's geometry, which I already wanted for something before but also ran into odd issues with at the time. Or do a limited fix by counter transforming some items based on page coordinates, which I'm already realizing won't work because of animator transitions.

@rainemak
Copy link
Member

It should be trivial but I just could not make it work the other day.
So I can add a normalized position coordinates to the Background item's geometry, which I already wanted for something before but also ran into odd issues with at the time. Or do a limited fix by counter transforming some items based on page coordinates, which I'm already realizing won't work because of animator transitions.

Idea / thought: How would it look like if we'd throw away that Background and ShaderEffectSource and just have a Rectangle with a rounded corners (~matching coloring) + a little bit transparency.

@adenexter
Copy link
Contributor Author

Idea / thought: How would it look like if we'd throw away that Background and ShaderEffectSource and just have a Rectangle with a rounded corners (~matching coloring) + a little bit transparency.

The scroll bar and item press highlights won't be clipped that way. And the footer still has a Background item or a large rectangle clipped so only its bottom corners are rounded.

I've found my problem now, it requires a change to Silica but it's something I've wanted before and probably will again.

@adenexter
Copy link
Contributor Author

I've found my problem now, it requires a change to Silica but it's something I've wanted before and probably will again.

A fix for this has been merged to Silica.

Copy link
Member

@llewelld llewelld left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is working nicely, no artefacts when the menu slides on/off the page in any direction. There's some strange behaviour from the scroll decorator though (a minor thing, see my comment elsewhere).

Copy link
Member

@llewelld llewelld left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is working nicely now for me on an XA2. The whole menu approach does now seem quite clear and usable (it's clear if there are more elements in the menu, there's good use of the screen space, good alignment with the other items on the toolbar, etc.).

Copy link
Member

@rainemak rainemak left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM as well.

Copy link
Contributor

@atatarov atatarov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you! I guess we don't have the solution for the tablet, but it would be nice to have ability to close "popup menu" by swiping from top to bottom :)

@rainemak
Copy link
Member

rainemak commented Jun 22, 2021

Thank you! I guess we don't have the solution for the tablet, but it would be nice to have ability to close "popup menu" by swiping from top to bottom :)

I think there are two improvements that we should consider but let's handle both of them separately

  1. Dim the content area when popup menu is opened like we do elsewhere => this should help users to understand that tapping outside of the popup menu will dismiss it

  2. Swipe to close gesture for the popup menu

@okodron
Copy link
Contributor

okodron commented Jun 22, 2021

Thank you! I guess we don't have the solution for the tablet, but it would be nice to have ability to close "popup menu" by swiping from top to bottom :)

I think there are two improvements that we should consider but let's handle both of them separately

1. Dimming the content area when popup menu like we do elsewhere => this should help users to understand that tapping outside of the popup menu will dismiss it

2. Swipe to close gesture for the popup menu

Yes, good idea. Better to file a separate task about this so that it won't be forgotten.

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.

None yet

6 participants