FF/Chrome/IE browseMode quicknav includes rich text widgets (contentEditable) in edit and formField commands by michaelDCurran · Pull Request #7461 · nvaccess/nvda

michaelDCurran · 2017-07-31T20:12:56Z

Link to issue number:

Summary of the issue:

Rich text editors are becoming more prevalent on the web. Users expect that they can jump to these fields with NVDA quicknav commands they already know, such as e (edit) and f (formField).
These rich text editors are usually implemented either by a div with contentEditable="true" or sometimes an iframe with an embedded document set to design mode.

Description of how this pull request fixes the issue:

This PR makes it possible to jump to these rich text widgets with quick navigation in Firefox, Chrome and Internet Explorer.
In simple terms, quick navigation needs to locate the next element that is contentEditable, but whos parent is not contentEditable, as we do not wish to locate all descendants of the editable container.
To enable matching in this way, virtualBuffer searching attributes can now be prefixed with "parent::" which tells the buffer to check the parent node for that particular attribute, rather than the node it is actually looking at in the search. Which now means it is possible to construct a search that checks for contentEditable on a node, but a lack of contentEditable on its parent.
The quicknav search rules for Gecko and mshtml were updated to make use of this new parent prefix, so that they can locate top-most contentEditable nodes.

Testing performed:

Tested in Firefox, Chrome and IE testcases.zip:

content_editable.html: made sure that both e and f found the editable section.
designMode.html: made sure that both e and f found the editable document.
formFields.html: made sure that e and f found all the edit fields, including the read-only one.

Known issues with pull request:

This is not supported in Edge due to contentEditables not getting their own text pattern.

Change log entry:

What's new:

In Firefox, Chrome and Internet Explorer, quick navigation to edit fields and form fields now includes editable rich text content (I.e. contentEditable). (Single letter navigation to editable text fields should include rich text editors #5534)

…efixed by 'parent::' which means that the parent node should be checked for this attribute, rather than this node.

… seems to incorrectly expose this. Not only is it incorrect info for the user, but it can affect jumping to editable elements with quicknav.

…elds) now include editable content (E.g. div with contentEditable=true).

…ields) now includes editable content (E.g. div contentEditable=true). Also read-only edit fields (I.e. <input type="text" readonly />) are again included.

feerrenrut · 2017-08-01T06:54:42Z

+		// A given attribute can contain a parent prefix, which means the parent node will be checked for that attribute instead of this one. 
+		if(this->parent&&attribName->find(parentPrefix)==0) {
+			VBufStorage_attributeMap_t::const_iterator foundAttrib = this->parent->attributes.find(attribName->substr(parentPrefix.length()));
+			if (foundAttrib != this->parent->attributes.end())


Please add braces for this line

feerrenrut · 2017-08-01T06:55:23Z

+			test << L";";
+		} else { // not a parent attribute
+			VBufStorage_attributeMap_t::const_iterator foundAttrib = attributes.find(*attribName);
+			if (foundAttrib != attributes.end())


also on this line

feerrenrut · 2017-08-01T06:55:41Z

@@ -149,13 +149,22 @@ inline void outputEscapedAttribute(wostringstream& out, const wstring& text) {

 bool VBufStorage_fieldNode_t::matchAttributes(const std::vector<std::wstring>& attribs, const std::wregex& regexp) {
 	wostringstream test;


Is this still necessary?

feerrenrut · 2017-08-01T06:58:28Z

-			outputEscapedAttribute(test, foundAttrib->second);
-		test << L";";
+		// A given attribute can contain a parent prefix, which means the parent node will be checked for that attribute instead of this one. 
+		if(this->parent&&attribName->find(parentPrefix)==0) {


So we cover parent:: at index 0, and npos, but not if it is later in the string. As we discussed could you add a comment to explain that its valid for the string to contain parent:: at other points in the string without it meaning that parent node needs to be checked for the attribute. An example of blah blah grandparent::

feerrenrut · 2017-08-01T07:19:08Z

-				{"IAccessible::role":[oleacc.ROLE_SYSTEM_COMBOBOX,oleacc.ROLE_SYSTEM_TEXT]},
+				{"IAccessible::role":[oleacc.ROLE_SYSTEM_COMBOBOX]},
+				# Focusable edit fields (input type=text, including readonly ones)
+				{"IAccessible::role":[oleacc.ROLE_SYSTEM_TEXT],"IAccessible::state_%s"%oleacc.STATE_SYSTEM_FOCUSABLE:[1]},


As discussed I found this a little confusing to read. Specifically that STATE_SYTEM_FOCUSABLE belongs to the format string, and is the key to the value [1]. You could consider doing "IAccessible::state_{}".format(oleacc.STATE_SYSTEM_FOCUSABLE) : [1] instead.

feerrenrut · 2017-08-01T07:20:41Z

-			attrs={"IAccessible::role":[oleacc.ROLE_SYSTEM_TEXT,oleacc.ROLE_SYSTEM_COMBOBOX],"IAccessible::state_%s"%oleacc.STATE_SYSTEM_FOCUSABLE:[1],"IHTMLElement::isContentEditable":[1]}
+			attrs=[
+				# Focusable edit fields (input type=text, including readonly ones)
+				{"IAccessible::role":[oleacc.ROLE_SYSTEM_TEXT],"IAccessible::state_%s"%oleacc.STATE_SYSTEM_FOCUSABLE:[1]},


Since "IAccessible::state_%s"%oleacc.STATE_SYSTEM_FOCUSABLE is evaluated twice, could it instead be stored in a named variable?

michaelDCurran · 2017-08-01T11:42:40Z

I have done all the requested changes for storage.cpp. However, I am reluctant to make the changes to the Python attribute searching dictionaries, although I do agree in principle with them. We have written those rules in that specific coding style for many years, and I wouldn't want to change it only in one or two places. In the long term I would be happy to consider a better coding style which we could use for all of the rules.

derekriemer · 2017-08-11T15:19:45Z

Might we give an example editor, I.E. in wordpress in the whats new?

michaelDCurran added 4 commits August 1, 2017 05:33

VBufStorage_fieldNode_t::matchAttributes: allow an attribute to be pr…

043d843

…efixed by 'parent::' which means that the parent node should be checked for this attribute, rather than this node.

Gecko VBufBackend: remove the editable state from ARIA grids as Gecko…

5db7204

… seems to incorrectly expose this. Not only is it incorrect info for the user, but it can affect jumping to editable elements with quicknav.

Firefox / Chrome browseMode: quicknav commands e (edit) and f (formFi…

c5e1933

…elds) now include editable content (E.g. div with contentEditable=true).

Internet Explorer browseMode: quicknav commands e (edit) and f (formF…

e269a52

…ields) now includes editable content (E.g. div contentEditable=true). Also read-only edit fields (I.e. <input type="text" readonly />) are again included.

michaelDCurran requested a review from feerrenrut July 31, 2017 20:12

feerrenrut suggested changes Aug 1, 2017

View reviewed changes

Provide more detail in comment.

0973433

feerrenrut approved these changes Aug 2, 2017

View reviewed changes

michaelDCurran added a commit that referenced this pull request Aug 2, 2017

Incubates #7461

47ddec6

nvaccessAuto added the incubating label Aug 2, 2017

nvaccessAuto assigned michaelDCurran Aug 2, 2017

michaelDCurran merged commit 0115a3c into master Aug 29, 2017

nvaccessAuto removed the incubating label Aug 29, 2017

nvaccessAuto added this to the 2017.4 milestone Aug 29, 2017

ZaphodAndo mentioned this pull request Dec 1, 2022

Quick nav to content editable elements in an iframe #14414

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

FF/Chrome/IE browseMode quicknav includes rich text widgets (contentEditable) in edit and formField commands#7461

FF/Chrome/IE browseMode quicknav includes rich text widgets (contentEditable) in edit and formField commands#7461
michaelDCurran merged 5 commits into
masterfrom
i5534

michaelDCurran commented Jul 31, 2017

Uh oh!

feerrenrut Aug 1, 2017

Uh oh!

feerrenrut Aug 1, 2017

Uh oh!

feerrenrut Aug 1, 2017

Uh oh!

feerrenrut Aug 1, 2017

Uh oh!

feerrenrut Aug 1, 2017

Uh oh!

feerrenrut Aug 1, 2017

Uh oh!

michaelDCurran commented Aug 1, 2017 via email

Uh oh!

derekriemer commented Aug 11, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

		@@ -149,13 +149,22 @@ inline void outputEscapedAttribute(wostringstream& out, const wstring& text) {

		bool VBufStorage_fieldNode_t::matchAttributes(const std::vector<std::wstring>& attribs, const std::wregex& regexp) {
		wostringstream test;

Uh oh!

Conversation

michaelDCurran commented Jul 31, 2017

Link to issue number:

Summary of the issue:

Description of how this pull request fixes the issue:

Testing performed:

Known issues with pull request:

Change log entry:

Uh oh!

feerrenrut Aug 1, 2017

Choose a reason for hiding this comment

Uh oh!

feerrenrut Aug 1, 2017

Choose a reason for hiding this comment

Uh oh!

feerrenrut Aug 1, 2017

Choose a reason for hiding this comment

Uh oh!

feerrenrut Aug 1, 2017

Choose a reason for hiding this comment

Uh oh!

feerrenrut Aug 1, 2017

Choose a reason for hiding this comment

Uh oh!

feerrenrut Aug 1, 2017

Choose a reason for hiding this comment

Uh oh!

michaelDCurran commented Aug 1, 2017 via email

Uh oh!

derekriemer commented Aug 11, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants