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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improvements for use with ShadowRoot and Websites with white text #72

Conversation

FabianWilms
Copy link

Hello there,

With your recent commit 523519f you already made a big step to make this tool compatible with shadow roots and it works like a charm with polymer. But the element that is highlighted won't be highlighted correctly 馃槈

Screenshot of the described behaviour:

shadow-root_before

While inspecting the way the overlay works I also noticed that Websites that use white text will head into problems when using this tool:

white_text_before

So I tried another way of creating an overlay with a cutout for a element. This PR contains some changes in the css so that the overlay div won't be needed anymore. These changes would make driver.js compatible for use with shadow doms and independent of text colors in the highlighted elements.

As I didn't want to dive in deeper into the internal workings of the code the overlay div is still created but doesn't serve any purpose anymore.

Some "after"-Screenshots:

shadow-root_after
white_text_after

I think that this is an easier way of doing things which also enhances compatiblity for different projects. I hope that you will think the same!

Kind regards
Fabian

@kamranahmedse
Copy link
Owner

Hey @FabianWilms, thanks for the PR. It seems to be a clever solution that could potentially fix the z-index issues but it seems to have a flaky performance with animation in Firefox because of the higher box-shadow.

@FabianWilms
Copy link
Author

Hi @kamranahmedse - thanks for looking into it! I just tried it with Firefox 59 on Ubuntu 17 but didn't notice a performance degredation. With which version did you try it?

@kamranahmedse
Copy link
Owner

kamranahmedse commented Apr 15, 2018

Firefox 59 on Mac.

I was just working on this PR, thinking that we can use it for non-animated version but as it turns out Safari doesn't like the box-shadow this high and ends up not showing any overlay. Also, look at this article from airbnb about the performance issues with it.

Box-shadow is going to introduce more issues than the fix itself. I will have to close this PR. But I will look into it further and see if we can fix it in any other way.

@kamranahmedse
Copy link
Owner

Wait, I just tried it with outline which seems to be working well for non-animated version on safari, firefox and chrome. This could be the fix. Can you try the overlay-shadow branch and see if it works?

@FabianWilms
Copy link
Author

Hello @kamranahmedse Yes, that does indeed work, too. I also tried using this approach with animations enabled and at least in Firefox and Chrome it works without any hiccups. Maybe it would be good to make this a configurable option?

@kamranahmedse
Copy link
Owner

Cool. I pushed it in the 0.5.0 release yesterday. For the animation, actually, I tried that on mobile and it was much much laggy, I think it makes sense to not use this approach in the animation. But anyways, I will look into it further and see if we can figure something out for the animated version.

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

2 participants