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

Screenshot preview is overlaying permalink #1274

Open
zoepage opened this issue Sep 14, 2018 · 33 comments

Comments

@zoepage
Copy link
Contributor

@zoepage zoepage commented Sep 14, 2018

screen shot 2018-09-13 at 15 27 49

STR:

  1. Open this profile: https://perfht.ml/2MKT5Hy
  2. Open one of the panels at the top (eg permalink, or publish)
  3. hover the screenshots below

=> as you'll notice, the screenshots are going over the panels. But they should go below, ie, the panels should be on top.

@zoepage zoepage added the polish label Sep 17, 2018
@julienw julienw added the timeline label Sep 17, 2018
@julienw

This comment has been minimized.

Copy link
Contributor

@julienw julienw commented Sep 17, 2018

Here is an example of a profile with screenshots: https://perfht.ml/2MKT5Hy.

This is where the z-index for the permalink is defined:
https://github.com/devtools-html/perf.html/blob/f9fa9309ec19ee0a4718b1d1fe0524b5e725243a/src/components/shared/ArrowPanel.css#L5-L10

This is where the z-index for the screenshot hover is defined:
https://github.com/devtools-html/perf.html/blob/f9fa9309ec19ee0a4718b1d1fe0524b5e725243a/src/components/timeline/TrackScreenshots.css#L30-L35

So it "looks like" that the stacking contexts are well defined.

But the permalink is actually inside the #root element, that also has a z-index defined (and applied as a child of a flex element):
https://github.com/devtools-html/perf.html/blob/f9fa9309ec19ee0a4718b1d1fe0524b5e725243a/res/css/style.css#L30-L32

I tried to manually remove this z-index with the devtools, and this seems to fix the issue. But this z-index has been added as part of the work on tooltips by @gregtatum, so I think that for tooltips it is actually intended that they'd go over the application's "chrome".

So the solution might be that we want to add the screenshots to a separate root element than the one for tooltips (#root-overlay) and that we set a smaller z-index on this element than on #root. I haven't tried it but this should work.

This is where we define which element the screenshots are added to:
https://github.com/devtools-html/perf.html/blob/f9fa9309ec19ee0a4718b1d1fe0524b5e725243a/src/components/timeline/TrackScreenshots.js#L62-L65

I think this could be easily taken by a contributor, so I'm marking it like so. The MDN documentation about stacking contexts could be useful, as this bug is a direct application of it.

@hritvi

This comment has been minimized.

Copy link

@hritvi hritvi commented Sep 19, 2018

@julienw May I take this issue? Thanks 😄

@julienw

This comment has been minimized.

Copy link
Contributor

@julienw julienw commented Sep 19, 2018

@hritvi of course you can, thanks for your help ! Is there anything else I can help with ?

brisad added a commit to brisad/perf.html that referenced this issue Sep 27, 2018
Also use the common root-overlay element as a mount point.

Fixes firefox-devtools#794, fixes firefox-devtools#1274
@edieblu

This comment has been minimized.

Copy link
Contributor

@edieblu edieblu commented Oct 3, 2018

Hi @julienw happy to give this a go

@julienw

This comment has been minimized.

Copy link
Contributor

@julienw julienw commented Oct 3, 2018

@edieblu Hey, I see @hritvi already requested to work on this some days ago. Hey @hritvi could you work on this after all ? Thanks :)

@julienw julienw added the assigned label Oct 4, 2018
@julienw

This comment has been minimized.

Copy link
Contributor

@julienw julienw commented Oct 10, 2018

Hey @hritvi, because I got no answer from you, I'll remove the assigned label and make this issue available for another contributor. Please write another comment if you still work on this :) Thanks!

@julienw julienw removed the assigned label Oct 10, 2018
@Anshika-G

This comment has been minimized.

Copy link

@Anshika-G Anshika-G commented Oct 12, 2018

Hey @julienw I would like to work on this!

@julienw

This comment has been minimized.

Copy link
Contributor

@julienw julienw commented Oct 12, 2018

Hey @Anshika-G, sure ! You can look at the comment #1274 (comment) for some direction, but please ask if you need anything else !
Our contributing documentation has some useful information to start working on the project too.

@julienw julienw added the assigned label Oct 12, 2018
@julienw

This comment has been minimized.

Copy link
Contributor

@julienw julienw commented Oct 17, 2018

Hey @Anshika-G, could you work on this? Do you need some more help getting started? Thanks!

@julienw julienw removed the assigned label Oct 29, 2018
@julienw

This comment has been minimized.

Copy link
Contributor

@julienw julienw commented Oct 29, 2018

Removing the assigned label because no update for more than 2 weeks.

@PhoeniXAbhisheK

This comment has been minimized.

Copy link
Contributor

@PhoeniXAbhisheK PhoeniXAbhisheK commented Feb 26, 2019

Hey @julienw , can I look into this issue ?

@canova

This comment has been minimized.

Copy link
Member

@canova canova commented Feb 26, 2019

Hey @PhoeniXAbhisheK, I just assigned #1371 to you. Let's wait until you fix that one. This issue is up for grabs in the meantime.

@PhoeniXAbhisheK

This comment has been minimized.

Copy link
Contributor

@PhoeniXAbhisheK PhoeniXAbhisheK commented Feb 26, 2019

@canaltinova yes sure 🙂 🙂 🙂 🙂 🙂

@rajmeghpara

This comment has been minimized.

Copy link
Contributor

@rajmeghpara rajmeghpara commented Mar 4, 2019

Hi @julienw , I would like to work on this issue :) CC: @canaltinova

@julienw

This comment has been minimized.

Copy link
Contributor

@julienw julienw commented Mar 5, 2019

hey @rajmeghpara, sure! Thanks for your help :)

@julienw julienw added the assigned label Mar 5, 2019
@julienw

This comment has been minimized.

Copy link
Contributor

@julienw julienw commented Mar 6, 2019

As discussed on Slack, @rajmeghpara will work on another issue, so this one is now up for grabs!

@julienw julienw removed the assigned label Mar 6, 2019
@lloan

This comment has been minimized.

Copy link
Contributor

@lloan lloan commented Mar 10, 2019

@julienw Could I have this bug assigned to me? Thanks!

@julienw

This comment has been minimized.

Copy link
Contributor

@julienw julienw commented Mar 13, 2019

hey @lloan, sure!

@julienw julienw added the assigned label Mar 13, 2019
@canova

This comment has been minimized.

Copy link
Member

@canova canova commented Apr 1, 2019

hey @lloan, are you still working on this? Please let us know and we can help you if you are blocked on anything :)

@lloan

This comment has been minimized.

Copy link
Contributor

@lloan lloan commented Apr 1, 2019

Thanks for asking @canaltinova! Been working on the netmonitor tools project and as that one is on Phabricator, this one completely slipped my mind. If anyone else is interested in working on it, please do assign it to them, otherwise, I can work on it this week. Thanks a ton!

@canova

This comment has been minimized.

Copy link
Member

@canova canova commented Apr 1, 2019

@lloan thanks for letting us know :) I'll unassign it so someone can grab it if they are interested. Let us know if you want to work on it again :)

@canova canova removed the assigned label Apr 1, 2019
@alejandroclose

This comment has been minimized.

Copy link

@alejandroclose alejandroclose commented May 22, 2019

Hi @canaltinova @julienw ! I would like to help with this issue, can I try? :)

@julienw

This comment has been minimized.

Copy link
Contributor

@julienw julienw commented May 23, 2019

Hey @alejandroclose, yes sure, that would be lovely! Assigning to you then :)
You can find information to start with at the start of the thread, but please ask here or on slack if you need anything.

@julienw julienw added the assigned label May 23, 2019
@alejandroclose

This comment has been minimized.

Copy link

@alejandroclose alejandroclose commented May 30, 2019

@julienw I'm sorry but I haven't been able to see this, and I believe I won't have time for the next weeks... Maybe if it's still available I can retake it. Thank you for the opportunity and sorry again for any inconvenience.

@canova canova removed the assigned label Jul 3, 2019
@rbrishabh

This comment has been minimized.

Copy link
Contributor

@rbrishabh rbrishabh commented Jul 3, 2019

I would like to take up this issue as my first contribution! Can I take this up?

@canova

This comment has been minimized.

Copy link
Member

@canova canova commented Jul 4, 2019

@rbrishabh Sure, assigning it to you. Let us know if you need anything!

@julienw

This comment has been minimized.

Copy link
Contributor

@julienw julienw commented Jul 8, 2019

hey @rbrishabh, could you have a look at the issue yet? Do you need anything? Thanks!

@rbrishabh

This comment has been minimized.

Copy link
Contributor

@rbrishabh rbrishabh commented Jul 9, 2019

Yes. I read through everything you posted. I just had some trouble with my machine which is fixed now. I will work on this right now and I'll comment here if I get stuck. Thanks!

@rbrishabh

This comment has been minimized.

Copy link
Contributor

@rbrishabh rbrishabh commented Jul 13, 2019

Hi @julienw! I am sorry for the delay. I have read through this entirely.
I understood stacking contexts, thanks to the wonderful MDN documentation.

So this is what I need to do:

  1. I need to add screenshots to a separate root element. For this I need to change the "#root-overlay" to a different root element in TrackScreenshots.js.
  2. I need to increase z-index in timelineTrackScreenshotHover css class in TrackScreenshots.css to something more than 10, which is the value of z-index in arrowPanelAnchor css class in ArrowPanel.css.

So I think I can change the value of z-index in timelineTrackScreenshotHover css class in TrackScreenshots.css to 12? Would that be okay?

Also, what other root element can I use instead of "root-overlay"?

Thanks a lot. I am just getting started so the questions might be a little off the mark. I hope you don't mind.

@rbrishabh

This comment has been minimized.

Copy link
Contributor

@rbrishabh rbrishabh commented Jul 21, 2019

Hey @julienw ! I am waiting for your response! Please help me get this sorted whenever you can find a little time! Thank you!

@rbrishabh

This comment has been minimized.

Copy link
Contributor

@rbrishabh rbrishabh commented Jul 29, 2019

Hey @canaltinova ! I was wondering if this is still relevant? :)

@julienw

This comment has been minimized.

Copy link
Contributor

@julienw julienw commented Jul 29, 2019

Hey, sorry for the delay, as I was away for holidays! (and as was @canaltinova :) ).

You can use a new root element called root-screenshot for example. Also the new z-index needs to be abled on that new element.
I hope this is clearer!

Also the best way to get started is actually write the code and see if this fixes the problem ;) Then you can ask feedback by pushing the code to a draft pull request so that we can have a look at some actual code.

I hope this is clearer! Please ask if you need anything more :)

@julienw

This comment has been minimized.

Copy link
Contributor

@julienw julienw commented Aug 22, 2019

See the prior PR #2187: this isn't as easy as we initially thought, and will need some investigation to fix this. That's why I've removed the "good first issue" label.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
You can’t perform that action at this time.