backdrop / backdrop-issues

Issue tracker for Backdrop core.
144 stars 38 forks source link

Dropdown menu flickering #5638

Closed olafgrabienski closed 7 months ago

olafgrabienski commented 2 years ago

Description of the bug

The default header dropdown menu flickers sometimes when you use the menu links.

Steps To Reproduce

To reproduce the behavior:

  1. Add three or more pages to a fresh Backdrop site, put them as menu items in the Primary navigation under the "About" item.
  2. Go to the home page. Click the "Home" menu item several times, e.g. 15 times.

Actual behavior

The "About" sub-menu shows up for a quite short time, and disappears again – it 'flickers'.

Note: This doesn't happen on every page load. Also, the visibility of the issue depends on factors like server performance, bandwidth and browser. To reproduce the issue more easily, you may enable bandwidth throttling using the browser dev tools.

Expected behavior

No flickering.

Additional information

Add any other information that could help, such as:

indigoxela commented 2 years ago

@olafgrabienski many thanks for opening this issue.

The problem has been confirmed by several people (in the forum and in Zulip). So what can we do?

One suggestion is to hide the submenus via CSS (until js is fired).

Pro: no more flickering Con: no more non-js fallback for browsers with js disabled (submenus are inaccessible then)

Also to consider: A11Y - the smartmenus lib also does it with "display: none", but also with several aria attributes to indicate the meaning and current status of these submenus. (Screen readers don't read stuff that's not displayed at all (display / visibility)).

And yet another thing to consider: the current non-js fallback for dropdown menus in Backdrop is to expand them all - which looks awkward (for the minority (?) using script blockers). :stuck_out_tongue_winking_eye: Maybe we can also improve that a bit.

olafgrabienski commented 2 years ago

One suggestion is to hide the submenus via CSS (until js is fired).

Pro: no more flickering Con: no more non-js fallback for browsers with js disabled (submenus are inaccessible then)

Is there a way to hide submenus via CSS only when JS is enabled? Something like:

.js .menu-dropdown ul {
display: none;
}

Hm, sounds like a contradiction to the phrase "until js is fired", but I'm not sure about the technical details.

indigoxela commented 2 years ago

Is there a way to hide submenus via CSS only when JS is enabled? ... sounds like a contradiction to the phrase "until js is fired"

There is a way, but as you discovered, it has to be the other way round.

  1. Add a css class to the body (or html tag) via php
  2. Remove that class via js

Pretty much as I described in this forum comment To make sure the css selector only catches non-js page calls.

mikesoltis commented 1 year ago

Also, the flicker duration is quite variable, so it is not always observed and sometimes quite long.

Is this something that can be fixed in core? Flicker from the primary menu looks unprofessional for side menu driven pages in particular - i.e. manuals. Thanks.

klonos commented 1 year ago

I've never noticed this behaviour 🤔 ...and I couldn't reproduce it either, but we have several people reporting it. Does it happen in specific browsers perhaps?

It is very hard to fix what cannot be reproduced reliably/consistently, so it would help if we had a screengrab of this flickering as it happens, to see what it looks like. I use the open source tool Kap for these sorts of things, which allows exporting the captured video in animated .gif format that can be added here.

olafgrabienski commented 1 year ago

@klonos IIRC it happens in various browsers. I've just seen the flickering in Firefox and Chrome on Mac. In Chrome, it seems to happen more often.

Have you tried the steps from the issue description? To see the issue more clear, in your browser dev tools, enable throttling of the internet connection via the device toolbar, e.g. to "Low-end mobile" in Chrome.

Screencast with throttling enabled:

backdrop-menu-flickering

mikesoltis commented 1 year ago

This video shows the problem. I first show the drop downs for the Primary Menu. Then I click through a different menu that is in the sidebar. Every now and then you can see flicker from the primary menu at various timescales.

The problem is exacerbated if the page is loading at an anchor position on the page. For those pages there are more flickers observed than not.

https://user-images.githubusercontent.com/78069725/208006662-72f90140-ebf3-464a-b9ba-bc339fc4be55.mp4

argiepiano commented 1 year ago

One workaround is to use the module Nice Menus to render the Primary Navigation menu (you have to edit the default layout and remove the "Primary Navigation" block and instead use a Nice Menu block, and set it for "down"). You'll need to do some CSS styling to retain the look of the core menus.

In my testing I'm not seeing the flickering with Nice Menus. Additionally, you can disable "Use Javascript" in admin/config/user-interface/nice_menus to have the dropdown work just with CSS.

As explained above, this flickering is produced since the Javascript code for smartmenus (used in core) that adds the class .sm (which hides the submenus) sometimes takes a while to run. The workaround posted by @indigoxela above works fine (it hides the submenus with css while the browser gets ready to run JS), but the issue with that workaround is that, if a user has JS disabled in the browser, the submenus will remain hidden with no possibility of being opened.

laryn commented 1 year ago

this flickering is produced since the Javascript code for smartmenus (used in core) that adds the class .sm (which hides the submenus) sometimes takes a while to run.

Can we pull this bit out and make it run sooner?

mikesoltis commented 1 year ago

I am not seeing flickering using Nice Menus. This appears to be a good work around - thanks.

argiepiano commented 1 year ago

I seem to have somewhat reduced the flickering of the core dropdown menu if I disable "Use background fetch for cached pages" in admin/config/development/performance

mikesoltis commented 1 year ago

Unfortunately the disabling of background fetch does not reduce the flickering for my site.

klonos commented 1 year ago

... The workaround posted by @indigoxela above works fine (it hides the submenus with css while the browser gets ready to run JS), but the issue with that workaround is that, if a user has JS disabled in the browser, the submenus will remain hidden with no possibility of being opened.

I'm gonna throw this one out there, and you guys make what you will of it:

There's dozens of stats out there that show that the percentage of people having JS disabled in their browsers is in the very low 1-digits, regardless of the year the metrics were counted or the country etc. This is well below the 80% use case that we base our decisions on. So, just saying.

argiepiano commented 7 months ago

Further thought, after another forum post about this.

One possible solution is to add .js-hide to all submenus, and to remove that with Javascript after smart menus have run. The benefit of using .js-hide is that these will only be hidden if JS is enabled - they will be shown if not. This circumvents the problem mentioned above (basically, that hiding submenus by adding special css classes during server process and removing those classes via JS on the browser will result on those not being visible if JS is disabled). I haven't tested this to see if js-hide is processed fast and soon enough to avoid the flicker.

olafgrabienski commented 7 months ago

Good to see new ideas! I'm happy to test a PR, when available.

richarda78 commented 7 months ago

Not sure why ..

.menu-dropdown li ul {
display: none;
}

doesn't work for me, can see why it does work, perhaps it's not a ul that appears with only primary links, maybe a div.

argiepiano commented 7 months ago

@olafgrabienski, here's an attempt. The PR is farily simple. It adds js-hide to the wrapper element for submenus (basically, to the ul). This hides the submenus only when js is enabled, which bypasses the limitations mentioned in this tread about hiding elements and making them visible with javascript. As soon as smartmenus processes the links, the effect of js-hide is removed (although the class remains - but it has no effect, as the submenus are displayed correctly).

All of this is done only for dropdown menus, and is done directly to the renderable array of the menu tree, so no messing around with theming functions, css or with JS. This should make these adjustments available in other themes as well.

@klonos, you had a video you took when you throttled the browser to slow down the problem. How did you do that? In my testing with a "normal" browser speed, I don't see the flickering anymore, but it'd be interesting to try this in a slower browser.

argiepiano commented 7 months ago

I have added some sub-pages to the tugboat preview site, found here: https://pr4601-iulkpvh2jicjjeyaudu74kan2pi0n6a2.tugboatqa.com/

argiepiano commented 7 months ago

I have provided an alternative PR that fixes Basis (but could be applied to other themes) and only uses a single CSS line. It also plays well with non-JS enabled browsers, since it's namespaced with .js. The display: none; is overridden once the mouse hovers the parent and smartmenus adds its own online css.

klonos commented 7 months ago

@klonos, you had a video you took when you throttled the browser to slow down the problem. How did you do that?

I think that that might have been @olafgrabienski - not me. If I were to do it, I'd probably export it as an animated gif, and then would have adjusted the fps setting for it to slow it down. It wouldn't be a slower browser/rendering - more of a slower animation. Just need to make sure that you grab enough frames per second.

olafgrabienski commented 7 months ago

@argiepiano When you enable the Device toolbar in the Chrome Developer tools, you see a litte throttling related menu in the toolbar. See screenshot:

image

olafgrabienski commented 7 months ago

Thanks for the two PRs, @argiepiano! They both work for me (yay!), and I don't see any difference in the behavior.

Regarding the approaches, did you have a scenario in mind where the alternative approach could be preferable?

argiepiano commented 7 months ago

Thanks for testing, @olafgrabienski. The css approach is the fastest as there is no additional server side processing. But the PR only applies to Basis., which may actually be desirable. If we wanted to go the css route, we could add the new display setting in a core css file somewhere. Using CSS also makes it easier to override the hidden. Open to suggestions.

I'm also going to test both using other libraries like Nice Menus, to make sure it doesn't interfere with those.

argiepiano commented 7 months ago

Tested Nice Menus: neither PR interferes with Nice Menus, as NM doesn't use function system_menu_block_build() and it doesn't use the same css classes as core's dropdown menus.

richarda78 commented 7 months ago

Thanks argiepiano, still seeing the flicker on your site. It happens when clicking between the level 1 links, page in top level and about, can see the height increases momentarily. I've looked at what appears and it's a copy of the level 1 menu links with a bullet, like it's reverting to some other css!

argiepiano commented 7 months ago

Thanks @richrda78 - I'm not seeing that. What browser? Any chance you can record a screen capture?

richarda78 commented 7 months ago

Firefox, I'll try doing a screen capture.

richarda78 commented 7 months ago

Screencast from 10-12-23 16:49:33.webm

argiepiano commented 7 months ago

OK, I'm seeing a slight "trembling" of the lower black edge, but I think this is not being produced by the original problem reported here. The OP was about showing a full, visible rendition of all the submenu items - it was really visible.

As for what you are showing, my guess is that the browser takes a split second to apply css rules that override the base css ones. I'm not sure if there is a way to fix that, but I'll welcome any thoughts you may have.

argiepiano commented 7 months ago

One more test, @richrda78: could you compare both:

https://pr4601-iulkpvh2jicjjeyaudu74kan2pi0n6a2.tugboatqa.com/

and https://pr4602-5cbkny4si5p6srpwda5lx1hzbll6kghj.tugboatqa.com/

The second one uses only css, the first one adds js-hide in the server.

richarda78 commented 7 months ago

They both exhibit the flickering behaviour. I think it may be related to this issue as it's a flicker. I've tried my site without css

Screencast from 10-12-23 20:22:10.webm

switching between the primary links jumps quite badly, doesn't look very professional.

argiepiano commented 7 months ago

@richrda78 let's try to summarize this. So, are you saying that none of the PRs makes ANY difference? Or is there a difference between the previous state without any of the two PRs, and the current state, with one PR or the other.

@olafgrabienski also tested, and reported that both PRs got rid of the problem - he was the one who reported it in the first place. Please clarify.

richarda78 commented 7 months ago

Not to the flicker I'm talking about, but maybe I should start another issue - could be related that's all I'm saying.

olafgrabienski commented 7 months ago

I still think, the menu flickering with visible sub menu items, as shown in https://github.com/backdrop/backdrop-issues/issues/5638#issuecomment-1353200926, is fixed by both PRs.

That said, I also noticed some sort of 'header flickering' in the PR sandboxes. I guess we didn't notice the header flickering before because the menu flickering is much more perceivable. But when I look now at a fresh Backdrop demo site, I see both the menu and the header flickering. I'll try to provide screencasts later or tomorrow to illustrate my observations.

olafgrabienski commented 7 months ago

Below are my screen captures to compare two different flickering issues. For the time being, I call them "menu flickering" and "header flickering".


(1) Backdrop demo site with menu (and header) flickering

Note: the sub menu items under "About" are visible.

https://github.com/backdrop/backdrop-issues/assets/14188307/b15fede0-f0ce-447f-820d-2ad35b7221bf


(2) Backdrop sandbox of PR 4601, only header flickering

No visible sub items, only a sort of header jumping.

https://github.com/backdrop/backdrop-issues/assets/14188307/d32e4f09-f345-474f-a59f-62bc84f361f0

argiepiano commented 7 months ago

Thank you, @olafgrabienski. I think we should open a separate issue for the header trembling, since these two PR her efix a different issue (the brief display of the submenus), and they don't really make a difference on the header trembling problem. Would you add "works for me"?

I still would appreciate people's thoughts about which route is preferable: whether to do add the css class js-hide to the submenu ul during the server processing, or to modify the css file for basis to add display: none for the submenu ul. Both solutions are JS dependent, and do not interfere with the correct display if the browser has JS disabled.

indigoxela commented 7 months ago

I still would appreciate people's thoughts about which route is preferable:

That's in fact difficult to decide.

The change in system.menu.inc will apply to all themes and may eventually collide with existing workarounds in custom themes or different CSS approaches. The change in core themes has less impact. Although changes will apply to subthemes of Basis. Personally, I'm leaning towards the (smaller) CSS change in Basis.

richarda78 commented 7 months ago

Set the header to a fixed height fixes the header flickering, so it's caused by the menu extending the header. I don't think it's a css issue as in my video css is disabled.

olafgrabienski commented 7 months ago

Added the works for me label.

Regarding the different approaches, I like the js-hide class approach which will work for all themes, so contrib themes don't have to provide the same fix. I guess, most contrib themes don't have already a workaround. And people with custom themes have the skills to adapt or remove their workaround. That said, I'm also fine with the CSS approach for Basis.

klonos commented 7 months ago

I am with @olafgrabienski on this one. This is an annoying issue which may be difficult to figure out and fix, so if we can prevent it from happening to any custom or contrib theme, then I think that it would be best.

indigoxela commented 7 months ago

Dangerous consensus for me. :grimacing: I took a quick look - the system.menu.inc approach breaks Lateral.

olafgrabienski commented 7 months ago

the system.menu.inc approach breaks Lateral

Interesting! Can you provide some details? And would you consider Lateral rather opinionated with regards to the menu, or would you expect many themes to break for similar reasons?

indigoxela commented 7 months ago

Can you provide some details? And would you consider Lateral rather opinionated with regards to the menu

Yes, this theme is pretty opinionated when it comes to menus. :wink: With the system.menu.inc patch applied, submenu items simple don't show anymore. I'd call it a major break. But it's not the first time that I have to update my contrib themes to not break by core changes. :grimacing:

laryn commented 7 months ago

I haven't looked in depth, but is it possible we could add a single line of CSS in core/modules/system/css/menu-dropdown.theme.css? If that file is overridden or unset by a theme then this change would have no effect.

indigoxela commented 7 months ago

is it possible we could add a single line of CSS in core/modules/system/css/menu-dropdown.theme.css? If that file is overridden or unset by a theme then this change would have no effect.

I think, that would work. :+1: Of course, it still has to happen in Basis, as that's one of the themes, that do override that CSS file (just like Lateral and possibly custom themes, too).

argiepiano commented 7 months ago

I haven't looked in depth, but is it possible we could add a single line of CSS in core/modules/system/css/menu-dropdown.theme.css

@laryn, I've added the display: none to core/modules/system/css/menu-dropdown.theme.css. Basis actually removes this css file (see BAsis' info file), so we need to also keep it in Basis' css file.

I tested this with Bartik and Boostrap Lite, and this (the alternate PR) gets rid of the submenu flickering (but there are lots of other, unrelated, redrawings that cause other flickering - this was there before these PRs).

Additional testing in other themes will be welcomed.

argiepiano commented 7 months ago

PR available for testing and code review. Since the "alt" approach seems to be the preferred one, I'll close the other one.

olafgrabienski commented 7 months ago

Thanks for the update, @argiepiano! I've tested in the sandbox with Basis (works fine) and on a test site with a few contributed themes:

indigoxela commented 7 months ago

Great! Works for me, too (doesn't break themes that override or disable menu-dropdown.theme.css).

@argiepiano one more file to consider: core/system/css/menu-dropdown.theme.breakpoint-queries.css

That one takes over, if one changes the default breakpoint on /admin/structure/menu/settings. See the relevant PHP code here

The relevant CSS code is here

argiepiano commented 7 months ago

one more file to consider: core/system/css/menu-dropdown.theme.breakpoint-queries.css

Done!