Opened 9 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 | Owned by: | 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)
Change History (47)
#1
@
8 years ago
- Component changed from General to Widgets
- Focuses accessibility added
- Keywords needs-patch added
- Milestone changed from Awaiting Review to Future Release
#4
@
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.
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?
#5
@
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
@
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.
#7
@
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:
#8
@
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:
Menus: the accordion titles are too big and sidebar_name
is undefined:
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
@
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.
#10
@
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)
#13
@
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
#16
@
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
@
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.
#18
follow-up:
↓ 20
@
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.
#20
in reply to:
↑ 18
@
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
@
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.
#22
@
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
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
@
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:
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.
#30
@
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:
Toggle Indicator on Pages