WordPress.org

Make WordPress Core

Opened 11 months ago

Last modified 3 days ago

#37013 assigned defect (bug)

Minor Fixes: "Toggle indicator" in pages have focus color while the same in widgets/menus have none.

Reported by: monikarao Owned by: afercia
Milestone: 4.8 Priority: normal
Severity: normal Version:
Component: Widgets Keywords: has-patch has-screenshots needs-testing needs-refresh
Focuses: ui, accessibility Cc:

Description

Toggle indicator in pages/post has visible focus color(blue border radius) on indicator but this is not present on Widget and Menus. It should display same focus effect on "Toggle Indicator" in Widgets and Menus.

Testing Environment:-
OS: Windows 10
Browser: Chrome 50.0.2661.102 , IE-11 , Firefox 46.0.1
WP : 4.5.2
Server-Environment : VVV

Attachments (10)

toggle-indicator-pages.png (14.1 KB) - added by monikarao 11 months ago.
Toggle Indicator on Pages
toggle-indicator-menu.png (16.9 KB) - added by monikarao 11 months ago.
Toggle Indicator on Menu
toggle-indicator-widget.png (19.4 KB) - added by monikarao 11 months ago.
Toggle Indicator on Widget
37013.diff (2.4 KB) - added by xavortm 10 months ago.
37013.2.diff (3.9 KB) - added by mihai2u 10 months ago.
37013.3.diff (6.7 KB) - added by mihai2u 10 months ago.
37013.4.diff (7.6 KB) - added by mihai2u 3 weeks ago.
menu-togglers.jpg (9.2 KB) - added by mihai2u 3 weeks ago.
widgets-togglers.jpg (11.1 KB) - added by mihai2u 3 weeks ago.
37013.5.diff (7.6 KB) - added by mihai2u 3 weeks ago.

Download all attachments as: .zip

Change History (30)

@monikarao
11 months ago

Toggle Indicator on Pages

@monikarao
11 months ago

Toggle Indicator on Menu

@monikarao
11 months ago

Toggle Indicator on Widget

#1 @ocean90
11 months ago

  • Component changed from General to Widgets
  • Focuses accessibility added
  • Keywords needs-patch added
  • Milestone changed from Awaiting Review to Future Release

#2 @welcher
10 months ago

  • Keywords good-first-bug added

#3 @RubenBristian
10 months ago

I confirm this, will try a patch as my first ticket at contributor day.

Last edited 10 months ago by RubenBristian (previous) (diff)

@xavortm
10 months ago

#4 @xavortm
10 months ago

  • Keywords has-patch added; needs-patch removed

Attached a patch where the markup and styling from the "Edit post" .postbox was used to replicate the mentioned effect on :focus. Some duplicate of stylings is happening between common.js and widgets.css file with different selectors, but in order to keep the different type of elements in their own designated files seemed like the proper way to go.

Also the fix aligns the arrow to the arrows below it as pre-patch there was about 2-3px offset to the right compared to all arrows below it.

I tested for the Appearance->Widget page elements only for now.

That aside some opinion - I think the markup is needlessly different between visually similar elements. If we can reuse classes on most of the components this type of duplication won't be needed. Feedback?

Last edited 10 months ago by xavortm (previous) (diff)

#5 @mihai2u
10 months ago

Thank you @xavortm for your work.
I'll check out your diff locally and confirm the fix with a screenshot.

I agree that it's better to re-use classes on similar looking elements.

#6 @mihai2u
10 months ago

I'm making some improvement by dropping some prefixes which are only needed for less than 0.1% of the users:
http://caniuse.com/#search=box-shadow
http://caniuse.com/#search=border-radius

Also I confirmed this to be working perfectly on the Widgets view, matching the toggle button focus states available on the page meta boxes. The thing I'm not seeing though is the same thing implemented for the Menus.

@xavortm I will work on adding the same on the Menus. All of this is an accessibility enhancement allowing better and easier to follow keyboard navigation.

The main toggler focus view:
http://files.urldocs.com/share/widget-area-main-focus/widget-area-main-focus.png

The sub togglers focus view:
http://files.urldocs.com/share/widget-area-children-focus/widget-area-children-focus.png

@mihai2u
10 months ago

@mihai2u
10 months ago

#7 @mihai2u
10 months ago

I've worked on top of xavortm's work and added the focus states on the arrows next to the accordions, like the ones available on the Menus.

The previous diff I uploaded handles _just_ the focus state, but from an accessibility point of view, this wasn't yet perfect. With this, more elaborate, update I've adjusted a bit more of the menu accordion to work in a much similar fashion when using keyboard navigation. In the previous implementation with an open accordion, if you focused the next with the keyboard you wouldn't know which of the two was the actual one with the focus, which might cause confusion.

This is how the menu looks now when navigating with the keyboard from one to the next in most browsers, or when clicking specifically the arrow in Chrome:
http://files.urldocs.com/share/menu-accordion-focus-improved/menu-accordion-focus-improved.png

#8 @afercia
9 months ago

  • Version 4.5.2 deleted

Thanks everyone for the ticket and the patches :) Couple things about the patch so far:

Widgets: hovering "Available Widgets", an arrow appears misplaced:
https://cldup.com/KUAHKGduo1.png

Menus: the accordion titles are too big and sidebar_name is undefined:
https://cldup.com/OjD_JPnjUW.png

About vendor prefixes and other things (e.g. use tabs and not spaces to indent code) please see the CSS coding standards. For some background about the "circular focus" see #33808 and previously #32395.

(side note: the version number is meant to indicate the version the bug/feature was first introduced, when it is possible to track that to a specific version)

#9 @mihai2u
3 weeks ago

I'm refreshing this ticket.

Based on the latest WordPress version:

  • Toggle indicator in pages/post still has visible focus color (blue border radius)
  • Toggle menu accordion now has the same background-color switch only visual treatment and no blue ring on the arrow
  • Toggle widgets is not accessible via the keyboard but has the ring focus treatment if someone clicks on the arrow

Therefore I'm updating the diff to work with the latest WP and investigate towards working on a fix for afercia's feedback with the hovering of the available widgets.

Last edited 3 weeks ago by mihai2u (previous) (diff)

#10 @mihai2u
3 weeks ago

The ticket was refreshed to the latest WP and the feedback received from @afercia has been handled.
The reason of the undefined variable has been found and fixed. The title menus font sizes has been brought back to normal. The mispositioned arrow on hover from available widgets has been fixed and I also worked on it benefiting from the same accessibility improvements like the rest of them.

PR on github:
https://github.com/xwp/wordpress-develop/pull/222

And I'm uploading the https://patch-diff.githubusercontent.com/raw/xwp/wordpress-develop/pull/222.diff as 37013.4.diff in here.

I created a 2 minutes video demo showing the accessibility improvements done under both the menus and the widgets pages:
http://files.urldocs.com/share/wordpress-contribution/demo-menu-widgets-accessibility-improvement.mp4
(I recorded the video before fixing the font sizing of the menu accordion headers, which I've done with adding back the hndle class to them)

@mihai2u
3 weeks ago

@mihai2u
3 weeks ago

#11 @mihai2u
3 weeks ago

  • Keywords has-screenshots added

@mihai2u
3 weeks ago

#12 @mihai2u
3 weeks ago

  • Keywords needs-testing added

#13 @lukecavanagh
3 weeks ago

@mihai2u

37013.5.diff Applies cleanly and seems to work well.

This ticket was mentioned in Slack in #core by jeffpaul. View the logs.


2 weeks ago

#15 @westonruter
11 days ago

@afercia how's it look for you?

#16 @afercia
11 days ago

  • Keywords good-first-bug removed
  • Milestone changed from Future Release to 4.8
  • Owner set to afercia
  • Status changed from new to assigned

@westonruter there's a previous pending patch on #31476 that's ready for commit and the latest patch here is going to conflict with that.
Apart from this, it's a bit hard to follow all the changes here. Changing color alone was an easy good first bug :) Changing element types, JS, a11y, CSS position and display, and many other details it's something that goes a bit beyond the original scope of the ticket. That's not to say it shouldn't be done, just to say it will require me a bit of time to check everything. Overall, looks like a nice improvement, thanks to everyone! One minor detail so far: buttons don't need a keydown event.

I'd rather commit #31476 first and then review the changes here, if no objections.

#17 @Kopepasah
11 days ago

Ran a few checks and all changes by @mihai2u are working as expected. Just to further expound on on @afercia's comment about the keydown event, modern browsers recognize the action of space and return keys as a click event (from my understanding).

@afercia if you see potential conflicts, I would say commit #31476 first, other than what you mentioned, it looks good to me.

Last edited 11 days ago by Kopepasah (previous) (diff)

#18 follow-up: @mihai2u
3 days ago

@afercia

Thank you for the reference. I see that the #31476 ticket is now merged in, therefore I will refresh my work to avoid any conflicts and update this thread with a new patch.
Also refering to this comment on that ticket:
"There's room for further enhancements, they should go in separate tickets. For example, other elements in the widgets screen are still not fully operable with a keyboard."
I'm covering exactly that in this update.
The work there complements nicely with what I did here, as it covers a different set of toggles and enables proper keyboard navigation.

Beyond the video walkthrough uploaded above, I can post a compact roadmap of the code updates, such that it's easy to follow from a development perspective as well.

#19 @mihai2u
3 days ago

  • Keywords needs-refresh added

#20 in reply to: ↑ 18 @afercia
3 days ago

Replying to mihai2u:

Also refering to this comment on that ticket:
"There's room for further enhancements, they should go in separate tickets. For example, other elements in the widgets screen are still not fully operable with a keyboard."
I'm covering exactly that in this update.
The work there complements nicely with what I did here

Yep, and it's a very welcomed improvement 🙂

Note: See TracTickets for help on using tickets.