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

Change layout to fix missing scrollbar highlighting (chromium text search) #635

Merged
merged 1 commit into from
Mar 14, 2020

Conversation

cynecx
Copy link
Contributor

@cynecx cynecx commented Mar 13, 2020

Fixes #623.

This is more of a hack but I am not sure how a proper fix would look like.

@jyn514
Copy link
Member

jyn514 commented Mar 13, 2020

Will this bring back the issue mentioned in #510 (comment) ?

@cynecx
Copy link
Contributor Author

cynecx commented Mar 13, 2020

@jyn514 I can't really reproduce the anchor issue?

@cynecx cynecx force-pushed the fix-chrome-search-highlight branch from 36fa43b to 962e3e1 Compare March 13, 2020 21:00
@jyn514
Copy link
Member

jyn514 commented Mar 13, 2020

It was there a couple months ago, if it's not present in your PR that's a good sign.

r? @GuillaumeGomez

@GuillaumeGomez
Copy link
Member

I'm not sure to understand the connection between the remove code and the theme picker menu...

@cynecx
Copy link
Contributor Author

cynecx commented Mar 13, 2020

@GuillaumeGomez Without it, it seems that the theme-picker is misaligned, why? I don’t really know, hence I have mentioned that this is more like a hack and not a proper fix.

I can’t really take screenshots now because I am not anywhere near a computer, but you can try out these changes directly in your browser and you will probably see what the issue is.

@GuillaumeGomez
Copy link
Member

It's currently working for on my computer without those changes. So I'd definitely like to have screenshots. :)

@cynecx
Copy link
Contributor Author

cynecx commented Mar 14, 2020

Patch without fix for theme-picker:

Screenshot from 2020-03-14 01-02-57

With theme-picker:

Screenshot from 2020-03-14 01-04-50

(On Chromium 80.0.3987.132)

@GuillaumeGomez
Copy link
Member

Ok, I confirm that your change doesn't break things on firefox and chrome. Thanks for the fix!

@GuillaumeGomez GuillaumeGomez merged commit 5bf4321 into rust-lang:master Mar 14, 2020
@cynecx cynecx deleted the fix-chrome-search-highlight branch March 14, 2020 16:13
@cynecx
Copy link
Contributor Author

cynecx commented Mar 14, 2020

Well, I think I broke it. I assume that this went already to production because it seems that I can reproduce the mentioned anchor issue. 🤔

Weird thing is, it worked quite fine when I tested it (Developer Tools). My guess would be that this has to do with modifying the css on-the-fly vs statically.

@GuillaumeGomez
Copy link
Member

Did you try to empty the cache? Check if the CSS is the one from this PR to be sure it's yours too.

@cynecx
Copy link
Contributor Author

cynecx commented Mar 14, 2020

@GuillaumeGomez Yes, in fact I have disabled it xD. And yes again, the css contains this PR.

@GuillaumeGomez
Copy link
Member

The new style is definitely there (tested on https://docs.rs/regex/1.3.5/regex/) and is working for me. You're sure your chromium is "fin"? :p

@cynecx
Copy link
Contributor Author

cynecx commented Mar 14, 2020

Peek 2020-03-14 18-15

Firefox has the same behavior as Chromium. :/

@GuillaumeGomez
Copy link
Member

Oh, that's what you meant! Yes, you definitely broke it. I completely forgot about the anchors...

@cynecx
Copy link
Contributor Author

cynecx commented Mar 14, 2020

How should we proceed? I mean the trade off here doesn't seem that bad compared to losing the search highlighting functionality.

Meanwhile: I am currently trying to find a workaround the fixed position of the sticky header.

@GuillaumeGomez
Copy link
Member

No, the new behavior is worse. We need to find a way to fix the theme picker without breaking the anchors.

@cynecx
Copy link
Contributor Author

cynecx commented Mar 14, 2020

Well, I guess it's a revert then for now. PR is coming right up.

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.

Search result positions not shown in chrome
3 participants