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

EZP-25546: Pointer events block scrolling after loading is finished in Google Chrome #526

Closed
wants to merge 1 commit into from

Conversation

@sunpietro
Copy link
Contributor

sunpietro commented Mar 8, 2016

https://jira.ez.no/browse/EZP-25546

Explicitly set pointer-events to all when the app is opened but in loading state.

@sunpietro

This comment has been minimized.

Copy link
Contributor Author

sunpietro commented Mar 8, 2016

ping @dpobel

@andrerom

This comment has been minimized.

Copy link
Member

andrerom commented Mar 8, 2016

+1 (bug fix 1.2.x branch)

@lserwatka

This comment has been minimized.

Copy link
Member

lserwatka commented Mar 8, 2016

@sunpietro with this patch we don't have to fix anything on the studio side, correct?

@sunpietro

This comment has been minimized.

Copy link
Contributor Author

sunpietro commented Mar 8, 2016

@lserwatka yes, this patch fixes the blocker EZS-605 in eZ Studio.

@sunpietro

This comment has been minimized.

Copy link
Contributor Author

sunpietro commented Mar 8, 2016

I found that, my proposal doesn't fix the issue on Safari. To fix scrolling issue there, I have to remove the pointer-events: none; property completely.

@sunpietro

This comment has been minimized.

Copy link
Contributor Author

sunpietro commented Mar 9, 2016

Created a different approach only for Studio app: ezsystems/StudioUIBundle#462

@lserwatka

This comment has been minimized.

Copy link
Member

lserwatka commented Mar 9, 2016

@andrerom @dpobel We have to make a decision soon. Either we fix it here or in StudioUI. What I understood, that fixing here (with proposed patch) is not enough for Safari browser. I'm more in favor to have a fix in StudioUI and create 1.2.1 tag there (fix for master and 1.2.x).

@dpobel

This comment has been minimized.

Copy link
Contributor

dpobel commented Mar 9, 2016

so I understand correctly, this patch fixes the issue in Chrome but not in Safari ? Did you try with pointer-events: auto; instead of pointer-events: all; ?

@dpobel

This comment has been minimized.

Copy link
Contributor

dpobel commented Mar 9, 2016

(according to https://developer.mozilla.org/en-US/docs/Web/CSS/pointer-events it seems all is dedicated to SVG element)

@sunpietro

This comment has been minimized.

Copy link
Contributor Author

sunpietro commented Mar 9, 2016

@dpobel yes, I did
we could either remove the pointer-events: none; or override it like I did in the PR for Studio

@lserwatka

This comment has been minimized.

Copy link
Member

lserwatka commented Mar 9, 2016

I can see one risk of not fixing it here. Other plugin devs might experience same issues.

@dpobel

This comment has been minimized.

Copy link
Contributor

dpobel commented Mar 9, 2016

I meant testing in this patch pointer-events: auto; (instead of all).

@@ -29,6 +29,7 @@
opacity: 1;
min-height: 100%;
overflow: auto;
pointer-events: all;

This comment has been minimized.

Copy link
@dpobel

dpobel Mar 9, 2016

Contributor

I would put it on .ez-platformui-app (by doing that, we can keep the .is-app-loading selector)

This comment has been minimized.

Copy link
@sunpietro

sunpietro Mar 9, 2016

Author Contributor

Well, where is it placed doesn't really matter. The is-app-open CSS class is added as soon as the Loading application screen disappears.

We can keep the .is-app-loading selector alone without .is-app-open at the beggining of the selector.
I did it this way to show, that .is-app-loading class is added when the app is opened but switching active views.

To sumarize, I'd keep it here as it is.

This comment has been minimized.

Copy link
@dpobel

dpobel Mar 9, 2016

Contributor

Well, where is it placed doesn't really matter. The is-app-open CSS class is added as soon as the Loading application screen disappears.

for you maybe, for me (as a maintainer of PlatformUI) it matters. The opposite of 'is-app-loading' is not having 'is-app-loading', not 'is-app-open' which is a different concept.

We can keep the .is-app-loading selector alone without .is-app-open at the beggining of the selector.

so please do NOT add unneeded changes. A good bug fix is a fix with the minimum amount of change.

I did it this way to show, that .is-app-loading class is added when the app is opened but switching active views.

which is an implementation detail irrelevant to the problem. open and loading are 2 different concepts which are not really related.

To sumarize, I'd keep it here as it is.

I prefer to not answer to that...

@sunpietro

This comment has been minimized.

Copy link
Contributor Author

sunpietro commented Mar 9, 2016

ping @dpobel

@dpobel

This comment has been minimized.

Copy link
Contributor

dpobel commented Mar 9, 2016

+1 (when Travis agrees)

@sunpietro

This comment has been minimized.

Copy link
Contributor Author

sunpietro commented Mar 9, 2016

@dpobel behat is failing for some reason

@dpobel

This comment has been minimized.

Copy link
Contributor

dpobel commented Mar 9, 2016

@sunpietro let's ignore CI tests, I have absolutely no clue of what's going on there. Since yesterday, Travis runs the BDD tests, and there everything is up to date we can see what really happens.

@dpobel

This comment has been minimized.

Copy link
Contributor

dpobel commented Mar 9, 2016

@sunpietro also failing on Travis but it seems completely unrelated to your patch. I'm restarting the job.

@andrerom @bdunogier any clue on that https://gist.github.com/dpobel/bdfe9769e41d20117511#file-travis-log-L1633 ?

@bdunogier

This comment has been minimized.

Copy link
Member

bdunogier commented Mar 9, 2016

@andrerom @bdunogier any clue on that https://gist.github.com/dpobel/bdfe9769e41d20117511#file-travis-log-L1633 ?

Really not, gotta admit... new one to me :(

As far as I can read this huge stacktrace, it comes from the ObjectManager cleaning stuff:

[31m#21 [internal function]: EzSystems\BehatBundle\Context\EzContext->cleanTestObjects(Object(Behat\Behat\Hook\Scope\AfterScenarioScope))�[39m

In general, unless I've misunderstood, we have planned on not using that anymore (ping @miguelcleverti).

@lserwatka

This comment has been minimized.

Copy link
Member

lserwatka commented Mar 14, 2016

@dpobel @sunpietro is this something we can merge for 1.2.1 tag?

@dpobel

This comment has been minimized.

Copy link
Contributor

dpobel commented Mar 14, 2016

@StephaneDiot

This comment has been minimized.

Copy link
Contributor

StephaneDiot commented Mar 14, 2016

+1

@miguelcleverti

This comment has been minimized.

Copy link

miguelcleverti commented Mar 15, 2016

@bdunogier yes the object manager will be removed

@dpobel

This comment has been minimized.

Copy link
Contributor

dpobel commented Mar 17, 2016

Replaced by #530

@dpobel dpobel closed this Mar 17, 2016
@sunpietro sunpietro deleted the sunpietro:ezp-25546-scrolling-blocked branch May 12, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
7 participants
You can’t perform that action at this time.