Skip to content
This repository was archived by the owner on Sep 5, 2024. It is now read-only.

md-tooltip positioning problem with md-panel change #10543

Closed
fsmeier opened this issue Mar 29, 2017 · 35 comments
Closed

md-tooltip positioning problem with md-panel change #10543

fsmeier opened this issue Mar 29, 2017 · 35 comments
Assignees
Labels
- Lots of Comments has: Pull Request A PR has been created to address this issue P2: required Issues that must be fixed. resolution: fixed severity: regression This issue is related to a regression type: bug
Milestone

Comments

@fsmeier
Copy link
Contributor

fsmeier commented Mar 29, 2017

Actual Behavior:

  • What is the issue? * The second time the tooltip appears it is not positioned correct when $compileProvider.debugInfoEnabled(false)
  • What is the expected behavior? tooltip should stay on top

CodePen (or steps to reproduce the issue): *

AngularJS Versions: *

  • AngularJS Version: 1.5.5
  • AngularJS Material Version: 1.1.3

Additional Information:

  • Browser Type: * Chrome + Firefox
  • Browser Version: * Version 56.0.2924.76 & 52.0.1 (64-bit)
  • OS: * kubuntu
  • Stack Traces:

i was searching for hours. The new "small" version update changed a lot of basic things. i rly was suprised. when md-tooltip switched to use md-panel it stoped working correctly. I found out that it has sth to do with debugInfoEnabled(false). But why should this affect the tooltip/panel ?

is there any hotfix or workaround for it? just simply enabling debugInfo is not a solution.


Shortcut to create a new CodePen Demo.
Note: * indicates required information. Without this information, your issue may be auto-closed.

Do not modify the titles or questions. Simply add your responses to the ends of the questions.
Add more lines if needed.

Best,
Florian

@tyrdavis
Copy link

Thank you for the info about debugInfoEnabled(false)! I had been seeing this issue on my app but couldn't figure out how to replicate it to post a bug ticket.

@fsmeier
Copy link
Contributor Author

fsmeier commented Mar 30, 2017

maybe @bradrich has an idea?

@bradrich
Copy link
Contributor

@IMM0rtalis I am not actively helping with this project at the current moment, but I can ping someone that is: @crisbeto and @EladBezalel

@tipodisimmia
Copy link

+1

@EladBezalel EladBezalel self-assigned this Apr 11, 2017
@fsmeier
Copy link
Contributor Author

fsmeier commented Apr 13, 2017

@EladBezalel whenever you need help or testing on this -> i am available!

@geertjansen
Copy link

+1

@fsmeier
Copy link
Contributor Author

fsmeier commented Apr 25, 2017

@ThomasBurleson Do you know what is the status of this project? How can I help? At least bugfixing... New small versions should just fix bugs and not make new changes with new bugs :/ Atm it seems that nobody have the time to maintain this project anymore. :( Maybe it is time to stop all features and rly just do bugfixes and "finish" this version.

@nurishi
Copy link

nurishi commented May 4, 2017

I'm also the one suffering from this issue since 1.1.2 and had to decide to stay on 1.1.1 for my project.
I also spent quite a lot of time to create a small sample to reproduce it but had no luck.
Great to see the findings by @IMM0rtalis and wishing this issue to be solved.

@EladBezalel
Copy link
Member

@IMM0rtalis sorry for late response, if you can fix it and file a pr that would be good!

@fsmeier
Copy link
Contributor Author

fsmeier commented May 8, 2017

@EladBezalel I can't because i don't know where to start. That's my problem. Otherwise I would have created a PR already :( Maybe you have ideas where the problem could be or where I should look?

@EladBezalel
Copy link
Member

EladBezalel commented May 8, 2017

Well that's some dark magic over there!

I assume you do want to do it on your own therefor i'm guiding you what i'd do -
I would've start with going into the open tooltip method and inspect the panel positions with and without the $compileProvider.debugInfoEnabled(true) and work my way through where it gets the position from and start to figure what's wrong,

I'll let you play with it, ping me here or send me an email at any time - worst case i'd find sometime and we will solve it together!

Thanks again

Love your attitude

@oliversalzburg
Copy link
Contributor

oliversalzburg commented May 9, 2017

So far, I can tell that panelWidth will be 0 the second time around, when the result is a misaligned tooltip: https://github.com/angular/material/blob/master/src/components/panel/panel.js#L3102

Which I find pretty weird, because when I inspect the panel, it seems to be 90px wide:
image

@oliversalzburg
Copy link
Contributor

My guess is that the injection of the Angular debugging information will delay DOM operations just long enough for the width lookup to succeed and return the correct width.

When debug info is disabled, the process is too fast and will fail.

This can be "fixed" by introducing an artificial delay. I wrapped these lines into a setTimeout and that resolved the issue.

Maybe someone can translate that information into a proper fix?

@EladBezalel
Copy link
Member

@oliversalzburg that's definitely what happens, we can use $timeout to force angular to wait 1 digest cycle and i assume that would work, sometimes we have to do those overrides to get the element size.

like we're doing here for instance

@oliversalzburg
Copy link
Contributor

Well, if just waiting one cycle is an acceptable and "clean" solution in this case, then I'd be happy to propose a PR.

I figured there might an event one could (or even should) wait for. Just waiting one cycle often feels arbitrary.

I'd be happy with either way :)

@nurishi
Copy link

nurishi commented May 10, 2017

@oliversalzburg Awesome! Thanks for this fix. I could also confirm the fix worked well with the codepen sample and with my own project.

However, although I see the tooltip always shown in correct position, I sometimes see it re-positioning moving from left to right. Then, I tried removing timeout thing and it's perfectly working without any issues so I believe your commit https://github.com/oliversalzburg/material/commit/05677f001480e58d8c069090bfa938c53f7eaa7b would be the proper and minimal fix and I think we can revert the timeout things. Anyway, thanks for all your time and effort on this!

@oliversalzburg
Copy link
Contributor

@nurishi Thank you for testing :)

@EladBezalel
Copy link
Member

@nurishi @oliversalzburg thanks so much

I ❤️ our community

@fsmeier
Copy link
Contributor Author

fsmeier commented Jun 1, 2017

@oliversalzburg thank you for your fix 👍

does anybody know when it will be deployed to the next version? we can not update from 1.1.1 since this bug is still there... :/

@jscti
Copy link

jscti commented Jun 8, 2017

+1

1 similar comment
@geertjansen
Copy link

+1

@oliversalzburg
Copy link
Contributor

From what I can tell, this will be included in 1.1.5

@fsmeier
Copy link
Contributor Author

fsmeier commented Jun 26, 2017

But when will the fix be released? It is still a bugfix.

@fsmeier
Copy link
Contributor Author

fsmeier commented Sep 5, 2017

1.1.5 will be released this week regarding to @ThomasBurleson , but the pullrequest is changed to 1.1.6 now. @oliversalzburg

@oliversalzburg
Copy link
Contributor

So this fix won't be included? :(

@fsmeier
Copy link
Contributor Author

fsmeier commented Sep 6, 2017

@crisbeto , @EladBezalel : can you do sth about this? or push it forward to include this in 1.1.5?
#10651

@FrozenDroid
Copy link

How has this not been merged yet?

@EladBezalel
Copy link
Member

@FrozenDroid we're facing some issues with merging ATM.. sorry for the trouble..

@oliversalzburg
Copy link
Contributor

@EladBezalel I've read about internal issues before. Is this process intentionally not transparent or do I just not know where to look or how to involve myself? It would be a lot easier to be patient if the process and problems are well understood.

These problems are holding back development on applications that depend on angular-material and forking the project would be a very undesirable move.

@FrozenDroid
Copy link

I've quite literally made the change manually in my node_modules... which is definitely not how you'd want to do it. What are the issues with merging?

@fsmeier
Copy link
Contributor Author

fsmeier commented Nov 1, 2017

Here is already a small discussion: https://groups.google.com/d/msg/angular/_gWnAcupJTk/vHfpmrSZBwAJ

I also got an answer from @StephenFluin from google who is mainly saying the same like what is written in the thread i linked.

My conclusion: the wheels are rolling but way to slow for the fast moving world.

@Splaktar Splaktar added has: Pull Request A PR has been created to address this issue P3: important Important issues that really should be fixed when possible. type: bug labels Dec 4, 2017
@Splaktar Splaktar added this to the 1.1.6 milestone Dec 4, 2017
@Splaktar Splaktar added severity: regression This issue is related to a regression - Lots of Comments labels Dec 4, 2017
@Splaktar
Copy link
Member

Splaktar commented Dec 4, 2017

Sorry for the delays. We're trying to get the associated PR merged atm but it's causing failures when running against Google internal tests. We're continuing to try to get this PR merged and released soon.

@fsmeier
Copy link
Contributor Author

fsmeier commented Dec 11, 2017

@Splaktar can you get us more information what causes the failures? - And how we can support it? - In 4 days the version 1.1.6 is scheduled and I hope THIS TIME we can get this MR in there as well. Finally.

@FrozenDroid
Copy link

What's the status on this? A bit peculiar on why this could take so long.

@Splaktar Splaktar added P2: required Issues that must be fixed. and removed P3: important Important issues that really should be fixed when possible. labels Jan 11, 2018
@Splaktar
Copy link
Member

The PR caused screenshot tests to fail on an unknown set of Google production applications. I don't have any further information at this time, but I have requested guidance on what we can do to get this PR to pass the presubmit tests.

Obviously if a test is expecting for the tooltip to be in an incorrect position the second time it is opened, it would be wise to fix the test. However, we don't have that kind of information at this time. It may be due to a slight re-positioning of tooltips in some edge cases. Then the tooltips, while still positioned in a readable and correct way, would not be in the same position and may cause screenshot tests to fail. I am unaware of the sensitivity of these tests (i.e. are there a few pixels margin for error or not).

Note that some of these tests may be running against applications that are running in some kind of hybrid ngUpgrade mode as well. We don't currently have any tests within the AngularJS Material repository to test that kind of integration.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
- Lots of Comments has: Pull Request A PR has been created to address this issue P2: required Issues that must be fixed. resolution: fixed severity: regression This issue is related to a regression type: bug
Projects
None yet
Development

No branches or pull requests