fix #1213

[mohammedsahl]

Summary
For devices having a width of 768px or less the sidebar is overlapped by the cover page
This PR fixes that by sliding the cover page over, similar to the content page

The bug is reproduced by loading the page on a device browser having a screen size of 786px or less
and clicking the sidebar-toggle button

What kind of change does this PR introduce? (check at least one)

• Bugfix
• Feature
• Code style update
• Refactor
• Docs
• Build-related changes
• Other, please describe:

Before

After

Does this PR introduce a breaking change? (check one)

• Yes
• No

The PR fulfills these requirements:

• Fix Sidebar: Overlapped by cover page, toggled by screen width shortening #1213

You have tested in the following browsers: (Providing a detailed version will be better.)

• Chrome
• Firefox
• Safari
• Edge
• IE

• DO NOT include files inside lib directory.

[vercel]

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/docsify-core/docsify-preview/r40w71b80
✅ Preview: https://docsify-preview-git-fork-mlh-fellowship-1213-fix-sidebar-toggle.docsify-core.vercel.app

[codesandbox-ci]

This pull request is automatically built and testable in CodeSandbox.

To see build info of the built libraries, click here or the icon next to each commit SHA.

Latest deployment of this branch, based on commit ca460b7:

Sandbox	Source
funny-bassi-65zmj	Configuration

[trusktr]

Awesome! It turned out to be a simple fix.

The added transition seems to make the menu delay before closing. Let's remove the transition just for this fix.

Thank you for finding this fix!

Also be sure to update the PR with the latest from develop (there's an "Update" button below).

[jhildenbiddle]

Fix looks good!

[trusktr]

@jhildenbiddle Did you check the menu animation (considering there's a new transition property)? For me the menu lags now when closing it (tested in Chrome).

I would love to get your fix in @mohammedsahl! I think it is better to remove the animation tweak so we can merge the fix, then we can make animation tweaks in a new PR (happy for that to be improved too!).

By the way, why did you add the transition? What browser are you in? Does it make it better for you on your end?

[jhildenbiddle]

@trusktr --

Very strange. I'm not seeing any lag on the latest versions of Safari (Mac, iPhone, iPad), Chrome (Mac), or Firefox (Mac). I'm previewing the latest deployment here: https://docsify-preview-qbuv6mk3f.vercel.app/

Have you tried other browsers in hopes of isolating the issue to Chrome? If it is Chrome-specific, verify that Chrome is hardware accelerated by typing in chrome://gpu/ in the URL bar. It's unlikely that this is the issue, but it's worth checking.

[mohammedsahl]

@trusktr The transition was so that the cover "slides" into place rather than "snap". I've tried it on chrome for now.
@jhildenbiddle https://pastebin.com/Ec5ZyBcM. This is what I get from chrome://gpu. I do agree with @trusktr that there is a lag when closing the menu

EDIT: I seem to have found the source of the lag

  body.close
    .sidebar, .cover
      transform translateX($sidebar-width)

    .sidebar-toggle
      background-color rgba($color-bg, 0.8)
      transition 1s background-color
      width $sidebar-width - 16px
      padding 10px

    .content
      transform translateX($sidebar-width)

This around lines 440-452 in _layout.styl

Adding transform translateX($sidebar-width) to .cover seems to cause the lag. Remove the attribute from the .cover removes the lag entirely

[jhildenbiddle]

Nice work, @mohammedsahl!

Still confused as to why I don't see the lag, but it's a moot point if you've solved the issue.

[mohammedsahl]

I think there's a misunderstanding here, the lag issue is not entirely solved 😅
As it currently stands, we either have

• The cover page shift when the sidebar is toggled, done by adding .cover translation
• The sidebar overlaps as before, done by removing the .cover translation (the buggy landing page)

There is a third option where the user is not allowed to toggle the side bar when the cover page is displayed but that would, in my opinion, not be in line with the current design

The version I've pushed currently is the first, but the downside is that there will be a lag when the side bar is toggled.
I hope this clears it up a bit 😄

@jhildenbiddle

[sy-records]

#1173 I also feel that if the mobile terminal chooses to display the cover, it can not need the sidebar.

[jhildenbiddle]

Apologies for the misunderstanding, @mohammedsahl.

So I dug into this a little bit...

The original issue (not the lag issue) is caused the docsify-dark-mode plugin's style sheet. Specifically, this ruleset:

html,
body,
main,
.sidebar-toggle,
.sidebar,
aside {
  background: var(--docsify_dark_mode_bg);
}

The issue is that the CSS custom property --docsify_dark_mode_bg is undefined in normal mode, which is essentially the same as setting background to none. You can see this rule overriding the background color of the sidebar in using dev tools:

An easy way to verify this is to visit https://docsify.js.org and block the docsify-dark-mode stylesheet (see <link> below) using Chrome dev tools ("Network tab", select the stylesheet, select "Block request URL", then refresh).

<link rel="stylesheet" href="//cdn.jsdelivr.net/npm/[email protected]/dist/style.css">

Three ways to fix:

1. Remove the docsify-dark-mode plugin and stylesheet. This is my recommendation for the same reasons I listed in Updated docs site dark and light mode with switch and redesigned search bar using docsify-darklight-theme #1182:

   Generally speaking, we should be cautious about which (if any) third-party docsify-related addons we use on the public docsify site. Most visitors will view the site as an example of what they will get with an "out of the box" docsify installation. If after following our installation guide their site looks or behaves differently (which will be the case if docsify.js.org uses this theme), confusion can lead to issues being filed (which we have to manage) and an overall frustrating experience. We could try to address this by listing all of the plugins we use on the site at the end of our installation guide, but then we're essentially endorsing some addons over others which isn't great for the community. Unless there is legitimate need, I believe it's best to let docsify.js.org be a showcase primarily for code from the docsify team.

   There are also issues with this plugin+theme combo (poor readability due to low text/background color contrast of text, non-dark-mode search box and code blocks, breaks in IE11, possibly others but I haven't tested extensively). Even if these issue are addressed, I still think the above statement justifies removal of the plugin+theme on docsify.js.org.

   

2. Create a docsify-dark-mode issue and wait for the author to fix the bug.

3. Modify our CSS (as we have been doing in this PR) to try and work around the issue with docsify-dark-mode. I recommend against this in favor of either of the options above.

Any opposition to just removing the docsify-dark-theme plugin and stylesheet?

(@trusktr, @anikethsaha)

[anikethsaha]

Any opposition to just removing the docsify-dark-theme plugin and stylesheet?

Removed already, #1207, need a release

also, the preview link should not have the docsify-dark-mode plugin in it.

[mohammedsahl]

Thank you @jhildenbiddle for the investigative work 😄
Are there any places I need to look into to make the appropriate changes? I'd really like to increase my familiarity with the code base and hence make working with issues a little easier. I can get started on either of the three fixes mentioned above but I'd need to know which one to work on first 😅.

[jhildenbiddle]

Apologies for the confusion, everyone.

There are actually two completely separate issues, both of which coincidentally present themselves in similar ways. To make things even more confusing, I was debugging the code on https://docsify.js.org/ (master branch) when I should have been debugging the previews generated for this PR. Sorry about that. Hopefully the following screenshots and explanations will help clarify both issues and my recommendation on how to wrap up this PR.

1. The first issue (as stated above) is/was the docsify-dark-mode theme which set the sidebar's background color to undefined (which is the same as setting it to none). Since this plugin was removed in Revert "Updated docs site dark and light mode with switch and redesigned search bar using docsify-darklight-theme" #1207 we just need to publish these changes to address this issue.

   Here is a screenshot from https://docsify.js.org/, which is still using the plugin and therefore exhibits the issue. Note that this behavior matches the "before" screenshot listed at the top of this PR, which is why I stated this was the original issue:

   

   Removing the plugin addresses the missing background color as seen here:

   

   Good news? The background color is fixed. Bad news? The stacking order of the sidebar and coverpage content is wrong. On to the second issue...

2. The second issue is related to stacking contexts. When certain CSS properties (like transform or opacity) are applied to an element, the element is rendered as a "composite layer" that is accelerated by the GPU. These layers exist in a completely different stacking context than non-accelerated layers with a z-index applied. In this particular case, the sidebar's translateX value and the <div class="cover-main"> element's z-index value cause them to be rendered in separate stacking contexts, with the sidebar being rendered below the <div class="cover-main"> element. Because these elements are in separate stacking contexts, the order can't be fixed by simply adjusting z-index values.

   The easiest fix is to simply remove the z-index value from the <div class="cover-main"> because it isn't needed.
   

   docsify/src/themes/basic/_coverpage.styl

   Line 25 in da9af18

   z-index 1

   This allows the sidebar element to render above the coverpage content as intended.

   

Summary: I believe if @mohammedsahl simply removes changes made thus far along with the z-index declaration listed above, both issues will be resolved.

If your curious about the stacking context issue, here is an excellent article that helps explain in more detail: What No One Told You About Z-Index

Whew. :)

[anikethsaha]

Thanks for the detailed explanation @jhildenbiddle .
I believe as this is a small change, we can include that in this PR itself.

@mohammedsahl can you add the fix here as mention above ?

[trusktr]

I like the cover sliding to the right when the menu opens because it matches the same pattern as with the main content. I think it would be great to go with that solution (apart from fixing the background color).

The lag problem only happens in mobile; I can reproduce while in mobile mode in devtools or on my phone.

But guess what! I just remembered about the touch-action CSS property. The browser is the one making the delay. It does this by default to see if the user will be performing a double tap (in which case it can perform a zoom action).

I set touch-action: none; on the button for the menu, and that fixes the issue.

[trusktr]

Turns out https://docsify.js.org has the same issue on mobile. I am happy to merge this then, and then we should fix the following next (new PRs would be fine):

• missing background
• add touch-action: none (wdyt @jhildenbiddle?)

[jhildenbiddle]

@trusktr --

I don't mind the sliding coverpage. I'd still recommend removing the z-indexdeclaration on .cover-main for two reasons: 1) it isn't necessary and 2) the stacking context bug would be a hard one to track down if for some reason it became an issue again in the future.

One other thing to consider is that touch-action:none is only supported by iOS >= 13 (the latest version). We should try touch-action: manipulation to see if it accomplishes the same thing since this is supported all the way back to iOS 9.3 (details on caniuse.com)

@mohammedsahl --

Thanks for your patience on this one. I know there's been a lot of back and forth. Here's what I understand the next steps to be:

1. Leave current changes in place (coverpage slide behavior remains).
2. Delete the following line:
   

   docsify/src/themes/basic/_coverpage.styl

   Line 25 in da9af18

   z-index 1

3. Add a touch-action: manipulation declaration to the .sidebar-toggle element and see if this addresses the delay. If not, set it to touch-action: none and we'll just have to be okay with the lag in iOS < 13.

[trusktr]

Feel free to make the z-index change here or in a new PR.

[jhildenbiddle]

Not sure what's going on @trusktr. No issues on my end.

Let's get the changes I listed in my comment above made to this PR before spending more time troubleshooting previews. I believe those changes take everything we've discussed into account. Ideally those changes address all issues so we can close the PR and move on to the next thing.

[trusktr]

Oh, wait a second! I discovered why the lag is actually introduced. To see why,

• open the Vercel preview on mobile (i tested on Android Chrome)
• double tap anywhere (without tapping a link) and notice it does not zoom
• now open the menu. Notice there is no delay.
• now double tap anywhere (avoid links) and notice that it zooms out.
• zoom back in a tap the menu button and notice the lag

This lag is because the site has become zoomable due to the translation of the cover and causing it to render beyond the edge of the display. But notice that the main content does not render beyond the edge of the display.

So to solve the issue fully, I think we should ensure that we do not draw outside of the viewport. Let's see what the main content area does and see if we can copy that behavior.

The other option is we can disable zoom.

Applying touch-action:none to the button isn't good because it doesn't solve the issue that when the menu is open the user can scroll the whole site to the left. Try opening the menu on mobile then scroll the main content to the left while the menu is open.

[mohammedsahl]

@trusktr Here's something I'm not able to understand.

Suppose we're scrolling through contents (not the cover page) and we click the sidebar-toggle. Why doesn't the web page zoom out when I double-tap a non-link area of the contents. Since .content is also being translated by $sidebar-width, shouldn't it also lie outside the view-port?

The current behavior (observed on the develop branch) is that clicking anywhere outside the sidebar closes the sidebar, which I think is what we're going for. Meaning, we'd want the cover-page to shift and clicking anywhere outside the sidebar, closes it

Based on this, I will make changes based on this comment. Otherwise I can go with implement changes stated here

[jhildenbiddle]

I think we should refocus on the stated issue: To fix the presentation of the sidebar at <= 768px. In that spirit, I believe the steps should be the following:

1. Remove the sliding coverpage effect (i.e. the changes thus far). If this change introduces a new issues (unwanted horizontal scrolling when the menu is open, which I wasn't aware of) then we should skip it so we can address the stated issue first. That's more important, IMHO.
2. Delete the following line:
   

   docsify/src/themes/basic/_coverpage.styl

   Line 25 in da9af18

   z-index 1

The end result is a simple one-line fix.

All other issues / enhancements (sliding coverpage, button delays, hidden content rendered outside of the viewport, etc.) are all outside the scope of this PR. That's not to say they aren't important, but this PR isn't the best place to explore or implement them. If we want to explore additional enhancements, let's file separate issue(s) so we can review and triage accordingly.

[mohammedsahl]

Not exactly sure why the e2e tests are failing. Running npm run test:e2e locally results in all tests passing

[jhildenbiddle]

Fixed!

Can't comment on the e2e tests as I haven't worked with them yet.

[anikethsaha]

the e2e CI is kind of moody 😄
it needs re-run sometimes.

[anikethsaha]

Thanks for your contribution 👍

[trusktr]

@anikethsaha This wasn't ready yet. I had requested changes above. This causes the mobile site to zoom and scroll sideways, which we need to fix.

[trusktr]

Oh wait, nvm, I see @mohammedsahl made it to the cover doesn't translate. 👍