Each line of the toolbar tray has up to three parts: in the screen shot, I identify them as link, white space, and icon.
Currently, if you click on the link or the white space, the same thing happens: you load a new page. If you click on the icon, you toggle a sub-menu.
I vote for making the white space do the same as the icon. For one thing, the icon is small and easy to miss with my fat fingers. For another thing, anything that reduces the chance of an accidental page load (slow) compared to changing the state of the menu (fast) is a good idea IMHO.
I would also vote for a more radical change. I think it is inconsistent that if I click on "Menu" in the toolbar, then the tray opens up, but if I click on a word in the tray, I get a page load. I say, open the sub-menu when you click on the link and load the page when you click on the icon. (Maybe a different icon.) If we make this change, then my two arguments about what to do with white space are at odds with each other. If there is no sub-menu to expand, I would make the word into plain text, not a link.
Beta phase evaluation
Issue category | Task because nothing is broken yet the UX is currently lacking. |
---|---|
Unfrozen changes | Unfrozen because it only changes CSS and adds a bit of JS. |
Prioritized changes | The main goal of this issue is usability. |
Disruption | Not disruptive. |
Comments
Comment #1
Wim LeersComment #2
Wim LeersClosed #1850238: Should the menu item itself trigger expanding to the next level as a duplicate, but note that there's extra information there. Please read it. It's a *very* short issue (only 2 comments).
Comment #3
Wim LeersComment #4
Wim LeersComment #5
Tom Verhaeghe CreditAttribution: Tom Verhaeghe commentedI gave it a try:
.toolbar-box
element...Comment #6
Wim Leersto
, then
toggleClickHandler
gets called for every click event that happens within.toolbar-box
, so also for the white space in the screenshot in the issue summary. This almost gets us there; there is but one problem: it also means that any time you click a link (i.e.linkClickHandler
is called), we toggle also (i.e.toggleClickhandler
is also called!). That's not our goal.For this, JS allows you to stop propagation in an event handler, so that the event doesn't bubble up further. Due to event bubbling, the event handler of the elements closer to the event are called first, hence
.toolbar-box a
's event handler (i.e.linkClickHandler
) is called before.toolbar-box
's event handler (i.e.toggleClickHandler
). So if we stop propagation inlinkClickHandler
,toggleClickHandler
won't be called anymore, and voila: problem solved. You stop event propagation by callingevent.stopPropagation()
.Comment #7
Tom Verhaeghe CreditAttribution: Tom Verhaeghe commentedApplied the patch from issue #2103247 and changed the triggering element (.toolbar-handle) to the parent element (.toolbar-box) for toggleClickHandle.
Also, changed some CSS, making .toolbar-box's display attribute inline-block and removing paddings.
Comment #8
Tom Verhaeghe CreditAttribution: Tom Verhaeghe commentedComment #9
Wim Leers#2103247: Clicking menu links in the administration menu tray should close the admin menu tray, while in a narrow viewport where the toolbar is positioned on top of the content has now landed (congrats!), so now you can pull out that part of the patch :)
Comment #10
Tom Verhaeghe CreditAttribution: Tom Verhaeghe commentedI'll reroll that tonight :-)
Comment #11
Wim LeersWonderful, thanks :)
Comment #12
Tom Verhaeghe CreditAttribution: Tom Verhaeghe commentedRerolled previous patch
Comment #13
Tom Verhaeghe CreditAttribution: Tom Verhaeghe commentedI messed up in the previous patch; accidentaly defining 2 linkClickHandlers.
Comment #14
Wim LeersWorks perfectly in manual testing. Looks visually identical.
Beta evaluation added to the IS.
Comment #15
alexpottCommitted cba3bf6 and pushed to 8.0.x. Thanks!
Comment #18
LewisNymanThis introduced a regression in the focus styling of the tray links, which look kind of bad now:
Also I've found this change really annoying since it was introduced. Maybe we can tweak it but just increasing the width of the dropdown trigger a bit more instead of reducing the link so much?
Comment #19
Wim LeersComment #20
LewisNymanSorry! I had a busy week and forgot about this issue.
From beta3:
I think the intention of this issue is good, it just needs some fine tuning, take the image in #18
The width of the handle hit area is disproportional, there is definitely a middle area between both where it's much closer to the menu item than the handle, I expect the menu item to activate there.
Ideally we could have something like this, where the handle hit area is big enough to prevent accidental taps on the menu item but not so bit that it throws off intentional taps on the menu item.
Comment #21
Wim LeersOk so it was the entire row that got the gray background.
I agree with the visual problem (the entire row should still get that grayness), but I disagree with the ideal hit areas you propose. "text == menu item hit area" is simple. "text plus some white space === menu item hit area" is impossible to convey, even more so because the width of the text varies, which would make that nearly impossible to implement.
The visual problem is clear. But do you really have problems with the hit area?
Comment #22
LewisNymanI have had problems yes, but let's try and make this more objective.
This is simple to implement, I already played around with it in the web inspector, but let's not go into implementation details until we are sure we should change it.
It's not about what is possible to convey, because the current behaviour is also not communicated, it's just falling back to the less destructive behaviour. I think it's more what is the expected behaviour based on common implementations of this UI component. If we agree that this is the point then hopefully some research into current implementations could resolve this?
Comment #23
LewisNymanI noticed that this also affects items that don't have handles. I'll write a proposal patch soon.
Comment #24
joelpittetI've posted a patch not knowing this discussion was happening: #2409427: Toolbar vertical orientation link not expanding to fill the space.
I agree it should be clickable as the link.
Comment #25
LewisNymanLet's move the follow up discussion there.