Make WordPress Core

Opened 8 years ago

Closed 7 years ago

#37013 closed defect (bug) (fixed)

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

Reported by: monikarao's profile monikarao Owned by: afercia's profile afercia
Milestone: 4.9 Priority: normal
Severity: normal Version:
Component: Widgets Keywords: has-patch has-screenshots
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 (14)

toggle-indicator-pages.png (14.1 KB) - added by monikarao 8 years ago.
Toggle Indicator on Pages
toggle-indicator-menu.png (16.9 KB) - added by monikarao 8 years ago.
Toggle Indicator on Menu
toggle-indicator-widget.png (19.4 KB) - added by monikarao 8 years ago.
Toggle Indicator on Widget
37013.diff (2.4 KB) - added by xavortm 8 years ago.
37013.2.diff (3.9 KB) - added by mihai2u 8 years ago.
37013.3.diff (6.7 KB) - added by mihai2u 8 years ago.
37013.4.diff (7.6 KB) - added by mihai2u 8 years ago.
menu-togglers.jpg (9.2 KB) - added by mihai2u 8 years ago.
widgets-togglers.jpg (11.1 KB) - added by mihai2u 8 years ago.
37013.5.diff (7.6 KB) - added by mihai2u 8 years ago.
37013.6.diff (379.2 KB) - added by mihai2u 8 years ago.
37013.7.diff (7.7 KB) - added by mihai2u 8 years ago.
37013.8.diff (7.8 KB) - added by mihai2u 7 years ago.
Refreshed with the latest in master. Functionality still functions as expected.
37013.9.diff (10.0 KB) - added by afercia 7 years ago.

Download all attachments as: .zip

Change History (47)

@monikarao
8 years ago

Toggle Indicator on Pages

@monikarao
8 years ago

Toggle Indicator on Menu

@monikarao
8 years ago

Toggle Indicator on Widget

#1 @ocean90
8 years ago

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

#2 @welcher
8 years ago

  • Keywords good-first-bug added

#3 @RubenBristian
8 years ago

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

Last edited 8 years ago by RubenBristian (previous) (diff)

@xavortm
8 years ago

#4 @xavortm
8 years 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 8 years ago by xavortm (previous) (diff)

#5 @mihai2u
8 years 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
8 years 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
8 years ago

@mihai2u
8 years ago

#7 @mihai2u
8 years 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
8 years 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
8 years 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 8 years ago by mihai2u (previous) (diff)

#10 @mihai2u
8 years 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
8 years ago

@mihai2u
8 years ago

#11 @mihai2u
8 years ago

  • Keywords has-screenshots added

@mihai2u
8 years ago

#12 @mihai2u
8 years ago

  • Keywords needs-testing added

#13 @lukecavanagh
8 years 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.


8 years ago

#15 @westonruter
8 years ago

@afercia how's it look for you?

#16 @afercia
8 years 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
8 years 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 8 years ago by Kopepasah (previous) (diff)

#18 follow-up: @mihai2u
8 years 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
8 years ago

  • Keywords needs-refresh added

#20 in reply to: ↑ 18 @afercia
8 years 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 🙂

#21 @mihai2u
8 years ago

  • Keywords needs-refresh removed

I've refreshed the patch with the latest in master. There were 2 small conflicts on common.css which I've handled easily, and created a quick video showing the widgets page now navigating through both the widget areas and the widgets themselves (as part of the improvements in #31476) and they play nice together.

The menus still work like in the previous video I uploaded, so we're also good on that front.

Now to make it easier for you to go through this, I'll go through the code changes to explain what I did.

Accessibility improvement
In these two commits targeting the Widgets page (HTML change):
https://github.com/xwp/wordpress-develop/pull/222/commits/76b1f0815083831b7ffb5b68d9d3505d9d87bd26
https://github.com/xwp/wordpress-develop/pull/222/commits/aeb2e2ec95fd04d34ee35a5f0d7a7d9aa6ee5b9d
I replaced the div which was only used as a graphic representation of the state of the box with a button, because we want to be able to interact with it.

In this commit targeting the Menu accordion (HTML change):
https://github.com/xwp/wordpress-develop/pull/222/commits/950fc0a026b720adf55471e9c77f66d433efdf42
I removed the tabindex from the H3 element, and deleted the span that was showing the arrow state.
Instead I added inside the H3 element the button, because it's a better user experience to be able to highlight the arrow instead of the entire heading when focusing with the keyboard. This way it doesn't get confused with the currently open accordion heading.
In the following related commit I added back the hndle class, which was needed for styling purposes:
https://github.com/xwp/wordpress-develop/pull/222/commits/97cbb10ce204418d7763235a39b0687e4bec4c16

In this commit targeting the Menu accordion (JS change):
https://github.com/xwp/wordpress-develop/pull/222/commits/c44adff2767e974946302705452cf26aa01e87fb
I added the .handlediv (the buttons I added used this class) to the JS selector in order to allow it to handle the expanding / closing of the accordions.
To the comment regarding buttons not needing the keydown event, I am aware this isn't necessary for the current modern browsers, but since the code was already there, I thought it might be targeting some IE relative, and didn't want to introduce a potential bug, going by the don't fix it if it ain't broken philosophy.

In this commit all of the styling changes are pulled together:
https://github.com/xwp/wordpress-develop/pull/222/commits/553bb0ffb21b151bd8743163aee337e2896b5b99
I'll go through the changes here in no particular order:

  • with the introduction of the .handlediv I styled the arrow inside it (closed / open states)
  • moved the content: "\f142" from the collapse arrow indicator in a separate set of selectors in order to allow it to be easily reused on other places where it might be needed. It's a tiny code refactoring which makes things more readable with the icon having the open state / closed state comments before it.
  • the old sidebar arrow had been replaced, some selectors/styles which used to be connected to it got removed
  • the metabox-holder styles section represents the new styles connected to the arrow button in the menu
  • the sidebar / widgets .handlediv styles section represents the styles connected to the arrow button and its focus state on the widgets page

Hope it will be easy to follow along the code with my indications and links to individual git commits.

I'll upload the refreshed patch next.

@mihai2u
8 years ago

@mihai2u
8 years ago

#22 @mihai2u
8 years ago

Updated patch with the merge conflict solved is attached (37013.7.diff) matching the
https://github.com/xwp/wordpress-develop/pull/222.diff

Looking forward to your feedback or core commit @afercia

Thank you!

This ticket was mentioned in Slack in #accessibility by afercia. View the logs.


8 years ago

This ticket was mentioned in Slack in #accessibility by afercia. View the logs.


8 years ago

#25 @afercia
8 years ago

  • Milestone changed from 4.8 to 4.8.1

Punting to 4.8.1 per today's bug scrub.

#26 @westonruter
7 years ago

  • Milestone changed from 4.8.1 to 4.9

@mihai2u
7 years ago

Refreshed with the latest in master. Functionality still functions as expected.

This ticket was mentioned in Slack in #accessibility by afercia. View the logs.


7 years ago

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


7 years ago

#29 @afercia
7 years ago

I think the best path to help this ticket move on is splitting the changes in the widgets screen from the ones in the menus screen.

Menus screen: do_accordion_sections() is something that plugins may use too, and any change there should be carefully considered; will split this part in a new ticked.

Widgets screen: there are a few more things to take into considerations. For example, when there are more than on sidebar, only the first one is open by default and the other ones are closed:

https://cldup.com/XgAM7qmVNW.png

Thus, the aria-expanded attribute on the toggle buttons must be updated on page load accordingly. When dragging a widget from one sidebar to another sidebar, the related panels open and close: also in this case, aria-expanded needs to be updated accordingly. Basically every time the sidebar containers closed CSS class gets added/removed, the aria-expanded attribute needs to be updated to reflect the container expanded/collapsed state.

Last edited 7 years ago by afercia (previous) (diff)

@afercia
7 years ago

#30 @afercia
7 years ago

  • Keywords needs-testing removed

37013.9.diff contains the changes for the Widgets screen. It also tries to standardize a bit the CSS classes used for the toggles, using .toggle-indicator and .handlediv.

For clarity, this change is about the toggle controls that expand the widget wrappers on the left and the sidebar containers on the right:

https://cldup.com/aECPsYbtHe.png

Last edited 7 years ago by afercia (previous) (diff)

#31 @afercia
7 years ago

See #42002 for the accordions.

#32 @afercia
7 years ago

Worth noting the latest patch avoids to introduce text strings like Toggle panel... because the term "toggle" is difficult or impossible to translate in many languages, see #34753. This also allows some simplification to what screen readers will announce to users.

#33 @afercia
7 years ago

  • Resolution set to fixed
  • Status changed from assigned to closed

In 41621:

Accessibility: Improve the sidebar toggles in the Widgets screen.

  • uses button elements for the toggles
  • uses aria-expanded on the toggles to communicate to assistive technologies the panels expanded/collapsed state
  • adds the "circular focus" style to the toggles to give users a clear indication of the currently focused element
  • standardizes CSS class names to .toggle-indicator and .handlediv as these names are already used across the admin for similar controls

Props monikarao, xavortm, mihai2u, Kopepasah.
Fixes #37013.

Note: See TracTickets for help on using tickets.