Opened 3 years ago

Closed 2 years ago

#16373 closed Bug (fixed)

AVT: Title for all icons in CK Editor dialogs not displayed for Keyboard only User

Reported by: Satya Minnekanti Owned by: Marek Lewandowski
Priority: Normal Milestone:
Component: Accessibility Version: 4.0
Keywords: IBM Cc: Irina

Description

Steps to reproduce

  1. Open nightly build.
  2. Open a dialogs that has icons (ex: Smileys)
  3. Navigate to each smiley icon using TAB key (keyboard only user).

Issue: Label for each smiley not displayed for Keybaord only user. This is violation of AVT Checkpoint Checkpoint 2.1.1 Keyboardhttp://www-03.ibm.com/able/guidelines/ci162/keyboard.html

Same issue happens with icons in Special Character dialog, also Lock Ratio & Reset size icons in Image Properties dialog.

This fix should apply to any focusable icons in any dialogs i may have missed mentioning here.

This is same issue as defect #14931

Attachments (2)

Screen Shot 2016-11-24 at 08.36.41m.png (7.0 KB) - added by kkrzton 2 years ago.
Screen Shot 2016-11-24 at 08.40.34m.png (7.7 KB) - added by kkrzton 2 years ago.

Download all attachments as: .zip

Change History (10)

comment:1 Changed 2 years ago by Jakub Ś

Status: newconfirmed
Version: 4.0

comment:2 Changed 2 years ago by Tade0

Owner: set to Tade0
Status: confirmedassigned

comment:3 Changed 2 years ago by Tade0

I've added a prototype of tooltips for the specialchar dialog and put it in branch:t/14931a.

Things to do:

  • Change the position of the tooltips appearing at the bottom-right corner. This can be done by adding appropriate td:nth-last-child(1) and tr:nth-last-child(1) selectors - there's an example of this method in the plugin.
  • Add tooltips for smileys: This is tricky, because smileys are images, so they cannot have a :before pseudoelement. The solution is to move the title attr. to their parent elements and use the shadowTitle method on that.

comment:4 Changed 2 years ago by Marek Lewandowski

Owner: changed from Tade0 to Marek Lewandowski
Status: assignedreview

Unfortunately this issue required some heavy refactoring, and we couldn't get away with a simplified plugin. With this in addition to adding tooltips for following dialogs:

  • special chars
  • smileys
  • image
  • image2

We were able to add integration for things like reposition the tooltips dialog move or window resize.

Pushed to branch:t/16373 (which is on top of branch:t/14931).

comment:5 Changed 2 years ago by kkrzton

Status: reviewreview_failed

Overall it is a good and solid solution. Few things I have noticed:

Tests:

Functionality:

  • I think we may consider two labels for image/image2 lock ratio button while it has two states - locked/unlocked, but the tooltip is the same no matter the state. Unless ofc we consider the tooltip to show more like a button name not a state/action which it may trigger.
  • Some tooltips for special chars in special chars dialog have text/value like &quote; (see screenshot) where others have regular names. Maybe we should try to make it consistent and more user friendly (it is probably a Special chars dialog issue, not a keyboardtooltip plugin itself but maybe it can be fixed?).

Code: Looks good :)

From readability point of view, there is a shadowTitle function which is basically a function adding tooltips (or kind of a proxy to CKEDITOR.plugins.keyboardtooltip.addTooltip). It is used as a main way to add tooltips, but the function itself has a not so obvious name and is hidden on the end of the code of a whole plugin which makes it a little harder to understand.

You may also consider a little refactoring in offset helper to not duplicate the code, like:

offset: {
	left: function( elem ) {
		return this.calculateOffset( elem, 'offsetLeft' );
	},

	top: function( elem ) {
		return this.calculateOffset( elem, 'offsetTop' );
	},

	bottom: function( elem ) {
		return this.calculateOffset( elem, 'offsetTop', elem.offsetHeight );
	},

	calculateOffset: function( elem, offsetType, initValue) {
		var offset = initValue || 0;

		do {
			if ( !isNaN( elem[ offsetType ] ) ) {
				offset += elem[ offsetType ];
			}
		} while ( elem = elem.offsetParent );

		return offset;
	}
},

Changed 2 years ago by kkrzton

Changed 2 years ago by kkrzton

comment:6 Changed 2 years ago by Marek Lewandowski

Status: review_failedreview

Tests:

  • fixed failing unit test - actually there was missing one fallback in shadowTitle function which might reduce the accessibility.
  • I've added tests for dialogs integration.

Functionality:

I think we may consider two labels for image/image2 lock ratio button while it has two states - locked/unlocked, but the tooltip is the same no matter the state. Unless ofc we consider the tooltip to show more like a button name not a state/action which it may trigger.

Actually the problem here is the tooltip text. Since #7569(https://dev.ckeditor.com/ticket/7569) it works as a checkbox. As a checkbox it should indicate feature name, rather than what action will it take.

So instead "Lock ratio" it should be "Ratio lock" which can be turned on and off (indicated both by lock icon, and aria checkbox state for a11y).

Some tooltips for special chars in special chars dialog have text/value like &quote; (see screenshot) where others have regular names. Maybe we should try to make it consistent and more user friendly (it is probably a Special chars dialog issue, not a keyboardtooltip plugin itself but maybe it can be fixed?).

Extracted to #16689.


Code: shadowTitle agree that the name is wrong for reasons you have given, I've renamed it.

Changes repushed to t/16373.

comment:7 Changed 2 years ago by kkrzton

Status: reviewreview_passed

All tests passing, code looks good. LGTM!

comment:8 Changed 2 years ago by Marek Lewandowski

Resolution: fixed
Status: review_passedclosed

This feature will be merged to major in issue #16373.

Note: See TracTickets for help on using tickets.
© 2003 – 2019 CKSource – Frederico Knabben. All rights reserved. | Terms of use | Privacy policy