search

jbms / sphinx-immaterial

Adaptation of the popular mkdocs-material material design theme to the sphinx documentation system
https://jbms.github.io/sphinx-immaterial/
Other
191 stars 29 forks source link

left TOC z-index incorrect in wide viewport with `toc.follow` #189

Closed mhostetter closed 1 year ago

mhostetter commented 1 year ago

Small issue, but I noticed the left TOC from python-apigen generated pages have a display issue. It appears the contents of the TOC appear on top of the page title "API Reference" (in my case) when the TOC is long enough.

Example

https://mhostetter.github.io/galois/latest/api/galois.ReedSolomon.k/

Scroll in the left TOC and observe the overlay on top of "API Reference".

image

2bndy5 commented 1 year ago

At first glance, this seems related to the toc.sticky feature.

2bndy5 commented 1 year ago

well, I was able to fix this in the dev console by adding the following css rule to the <label> element representing the text "API Reference":

z-index: var(--md-nav__sticky-zindex);

The fact that it has an inline style attribute on the element makes me think this was manipulated in JS tough. A quick search for --md-nav__sticky-zindex And I found the rule https://github.com/jbms/sphinx-immaterial/blob/4eb865591fc8b08ac7118d891fb457cceac84b35/src/assets/stylesheets/main/layout/_nav.scss#L185-L190

Sure enough, the style is set in the JS implementation for toc.sticky, but it may be neglecting the toc title. https://github.com/jbms/sphinx-immaterial/blob/4eb865591fc8b08ac7118d891fb457cceac84b35/src/assets/javascripts/components/toc/index.ts#L407

This is tricky because I'm not the best at JS, and the toc is structured as lists of invisible checkboxes in the HTML. I have a feeling it is an easy fix though.

2bndy5 commented 1 year ago

I think this actually calls for a CSS fix concerning the viewport's min-width. Looking again at the dev console and filtering the rules to show anything about z-index, I see the z-index: var(--md-nav__sticky-zindex); is overridden by https://github.com/jbms/sphinx-immaterial/blob/4eb865591fc8b08ac7118d891fb457cceac84b35/src/assets/stylesheets/main/layout/_nav.scss#L678-L681

2bndy5 commented 1 year ago

This was my bad. The last update from upstream introduced this z-index override. I think we'll be ok to simply remove it.

2bndy5 commented 1 year ago

@mhostetter when you get a chance, can you verify if the fix-189 branch resolves this?

mhostetter commented 1 year ago

@2bndy5 yes, it works! Thanks for the speedy response and fix, as always!

jbms commented 1 year ago

Before reverting that upstream change, we should probably check why it was added in the first place and confirm that removing it won't cause any problems.

Also if we do remove it, we should add a sphinx-immaterial comment.

2bndy5 commented 1 year ago

The upstream commit (no PR related) just says "merge features related to scotch bonnet funding goal". I'm inclined to think it has something to do with how the toc.follow feature that was implemented upstream. However, I didn't use the JS related to that upstream implementation (in favor of our own implementation).

There are other z-index rules that refer to safari compatibility (in an similar bug report upstream): https://github.com/jbms/sphinx-immaterial/blob/4eb865591fc8b08ac7118d891fb457cceac84b35/src/assets/stylesheets/main/layout/_nav.scss#L521-L526 https://github.com/jbms/sphinx-immaterial/blob/4eb865591fc8b08ac7118d891fb457cceac84b35/src/assets/stylesheets/main/layout/_nav.scss#L547-L552 both of which also came with #171

I still think we're ok to remove it (and replace with a comment) since our implementation for toc.follow is a bit different. My second approach is to add a special rule to override this regression for the specific viewport width, but I'm not sure what the size difference is ~between break-from-device(screen) and break-from-device(tablet landscape)~ [found the definitions in _config.scss]. I also don't have access to a mac to test changes on the safari browser.

2bndy5 commented 1 year ago

Instead of removing the z-index: 1 rules, I think we can retain the intended functionality by using the var with a default fallback value of 1 (z-index: var(--md-nav__sticky-zindex, 1)). The variable --md-nav__sticky-zindex is only set (via inline style attributes) on elements from the JS that implements the toc.follow feature.

jbms commented 1 year ago

Instead of removing the z-index: 1 rules, I think we can retain the intended functionality by using the var with a default fallback value of 1 (z-index: var(--md-nav__sticky-zindex, 1)). The variable --md-nav__sticky-zindex is only set (via inline style attributes) on elements from the JS that implements the toc.follow feature.

This sounds good to me.

2bndy5 commented 1 year ago

@mhostetter It would help if you could re-test the fix-189 branch since the CSS solution has slightly changed.

mhostetter commented 1 year ago

@2bndy5 It works for me, thanks!