Page MenuHomePhabricator

Avoid using vector-menu classes in the dropdown template
Closed, ResolvedPublic2 Estimated Story Points

Description

Background

Now that the Menu template is split up, we want to ensure that vector-menu classes are only used with the Menu template. New classes should be created for the Dropdown template.

TODO

  • Dropdown templates use new dropdown classes instead of menu classes
  • CSS is updated while considering cached HTML

QA

  • no visual changes
  • ensure click tracking on menu items works the same

Event Timeline

@bwang: Can you please add a codebase related project tag? Thanks!

Jdlrobson subscribed.

@bwang considering this for an upcoming sprint but I think it needs revising in light of the WikimediaEvents dependency? Could you take a look?

bwang renamed this task from Avoid using vector-menu classes in the dropdown template, and replace Portal/Tabs with Menu to Avoid using vector-menu classes in the dropdown template.Jun 14 2023, 4:54 PM
bwang updated the task description. (Show Details)

@Jdlrobson I updated the description and removed the TODO about updating WikimediaEvent, I dont think thats actually necessary. Menus will continue to have the .vector-menu class so that click tracking should continue to work. Perhaps I can add a QA note to check that click tracking still works though.

I pulled this into sprint 6 per the discussion from SHDT, I think this task would be good to do alongside T303488

Jdlrobson raised the priority of this task from Low to Medium.Jun 14 2023, 5:41 PM
Jdlrobson set the point value for this task to 2.Jun 15 2023, 5:37 PM

Change 933141 had a related patch set uploaded (by Bernard Wang; author: Bernard Wang):

[mediawiki/skins/Vector@master] Separate vector-menu classes from Dropdowns

https://gerrit.wikimedia.org/r/933141

Change 933140 had a related patch set uploaded (by Bernard Wang; author: Bernard Wang):

[mediawiki/skins/Vector@master] Remove references to heading in dropdown PHP

https://gerrit.wikimedia.org/r/933140

Change 933183 had a related patch set uploaded (by Bernard Wang; author: Bernard Wang):

[mediawiki/skins/Vector@master] Remove cached HTML code

https://gerrit.wikimedia.org/r/933183

Change 933140 merged by jenkins-bot:

[mediawiki/skins/Vector@master] Remove references to heading in dropdown PHP

https://gerrit.wikimedia.org/r/933140

Change 933189 had a related patch set uploaded (by Jdlrobson; author: Jdlrobson):

[mediawiki/skins/Vector@master] Add new classes for dropdown component

https://gerrit.wikimedia.org/r/933189

Change 933183 abandoned by Jdlrobson:

[mediawiki/skins/Vector@master] Remove cached HTML code

Reason:

Squashed into https://gerrit.wikimedia.org/r/c/mediawiki/skins/Vector/+/933141

https://gerrit.wikimedia.org/r/933183

Change 933189 merged by jenkins-bot:

[mediawiki/skins/Vector@master] Add new classes for dropdown component

https://gerrit.wikimedia.org/r/933189

Change 933141 merged by jenkins-bot:

[mediawiki/skins/Vector@master] Separate vector-menu classes from Dropdowns

https://gerrit.wikimedia.org/r/933141

Click tracking appears to work as before. I clicked items in all the menus and saw events for each.

@Jdlrobson Could you help me figure out what to do with MoreMenu / Twinkle? This breaks it quite badly...

I tried adding making the same changes as 1e6b647971 but there are still many issues. For example, the checkbox styling for some reason has a height of 100%, making the menu contents unclickable. Additionally, the entire system of show/hiding the menus appears to no longer work. Do we need to implement the JS ourselves now?

@MusikAnimal the following modifications should be all the ones needed (for Twinkle... hopefully MoreMenu can be derived from these changes):

$('#p-twinkle .vector-menu-checkbox' ).addClass( 'vector-dropdown-checkbox' );
$('#p-twinkle .vector-menu-content').addClass( 'vector-dropdown-content' );
$('#p-twinkle .vector-menu-heading').addClass( 'vector-dropdown-label');
$('#p-twinkle .vector-menu-checkbox').attr('id', 'p-twinkle-dropdown-checkbox' );
$('#p-twinkle-label').attr('for', 'p-twinkle-dropdown-checkbox')

@MusikAnimal the following modifications should be all the ones needed (for Twinkle... hopefully MoreMenu can be derived from these changes):

$('#p-twinkle .vector-menu-checkbox' ).addClass( 'vector-dropdown-checkbox' );
$('#p-twinkle .vector-menu-content').addClass( 'vector-dropdown-content' );
$('#p-twinkle .vector-menu-heading').addClass( 'vector-dropdown-label');
$('#p-twinkle .vector-menu-checkbox').attr('id', 'p-twinkle-dropdown-checkbox' );
$('#p-twinkle-label').attr('for', 'p-twinkle-dropdown-checkbox')

Thanks. Using this, I managed to get a fix in for MoreMenu.

@Novem_Linguae We've got about a day before this lands on enwiki. Did you want to add this in the Twinkle repo and I can deploy? There are markup changes so it's not as simple as the last hotfix.

I note @Novem_Linguae's comment about being a day before "this" (I can't figure out what that would be) lands on enwiki, but I thought I'd report this complex menu bug as this report was just redirected from T337893. Will the fix to this bug address the following? (1) the TW menu can be dropped down by hovering, but the MoreMenus require a click (2) when TW or MoreMenu are clicked, they won't dismiss, either by click-away (if they were activated accidentally) or by right-click/new tab. Only a page refresh or of course a single click will remove them. The result can be as shown, with the Page menu barely visible.

three menus.JPG (619×511 px, 55 KB)

Hey @DavidBrooks. The Twinkle hotfix is deployed. Try things now and see if you still have the issue. If you do, may need to create a new ticket. I can help you create it if you want.

We should also try to figure out if the fix should be in vector-2022 (phabricator) or in Twinkle (github)

Still exactly as reported, unless I have something stuck in the cache (I purged the test page).

The non-disappearing More menus bug happens even if I don't touch the Twinkle dropdown, but of course that doesn't completely rule out a subtle interaction. I can disable Twinkle, and also can enter a new ticket but no time RN for either.

@DavidBrooks. I was unable to reproduce with the following steps. Please let me know if I am missing a step.

  • Log in
  • Special:Preferences -> enable Twinkle
  • Special:Preferences -> enable MoreMenu
  • Visit https://en.wikipedia.org/wiki/Main_Page?useskin=vector-2022
  • Click on TW
  • Click on Page
  • Click on TW again (menu closes)
  • Click on Page again (menu closes)
  • Make sure mouse is not hovering over either menu
  • Status: both menus are closed

Of all things, I hadn't thought of clicking on the menu again. That does dismiss the More menus; but IMO "click somewhere else" and "click another menu in the same menu bar" are normal ways of dismissing an accidentally deployed menu. Is that intended behavior in this context?

Also, the second click doesn't dismiss the TW menu for me.

Is that intended behavior in this context?

I think so. The "click to open/close the menu" behavior is implemented with a hidden check box, that becomes checked with you click it, and unchecked when you unclick it.

Also, the second click doesn't dismiss the TW menu for me.

Did you make sure your mouse isn't' still hovering over the TW menu? If the mouse is hovering over the TW menu, it will stay open. The Twinkle menu, but not the More menu, appears to stay open on hover.

WMF is working on an addPortlet function (T303488) which may help standardize these different behaviors and also prevent most breakages in the future.

Well, I still have problems with this:

  1. As shown in my screenshot, click User, then click Page (without dismissing User): the Page menu underlays the User menu, which is confusing. At least let the most recently clicked menu show itself (verified using my alternate user without TW active).
  2. For dropdown menus, there are two standard behaviors that I observe almost universally. The classic Win32 menu bar, where a dropdown is dismissed by click-away or hover over another menu in the bar. Or the more modern dropdown, where - and I just realized this was one contributor to my problem - the "V" hint at the availability of a dropdown is replaced by a "^" hint, which suggests another click to close, i.e. pull up, the dropdown. In these cases, that visual cue is not provided. It's also not provided in the shortcut menu at the top right, but that at least uses the "click away" dismissal rule.
  3. To your question on the TW click-to-close: yes, good point, because the mouse is necessarily hovering over the menu header right after the click, it sort-of closes and temporary-opens at the same time. But that is disconcerting also: did I click hard enough? Again, changing the hint icon between V and ^ will help answer that question, although having it remain open until I move away is still disconcerting.

Sorry about being so finicky; I'm chagrined I only just noticed this. Apologies if I'm catching a partial fix and T303488 is supposed to address all of this; if so give me a call when it's finished.

Thanks @DavidBrooks. I've posted T303488#8996897, which should get some discussion going about some of your concerns.