Speed up text leaf accessible state calculation (aka don't expose readonly state on text leafs)

RESOLVED FIXED in mozilla15

Status

()

Core
Disability Access APIs
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: surkov, Assigned: surkov)

Tracking

(Blocks: 2 bugs, {access})

unspecified
mozilla15
access
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment)

(Assignee)

Description

5 years ago
similar to bug 754830, we need to get rid document->State() call. It's used to expose or not to expose readonly state on text leafs (statictext role). So if the text leaf is inside the editable document then we don't expose readonly state, otherwise we do. The behavior is not consistent since it doesn't take into account that text leaf can be contained by editable area (like p@contentEditable="true" or text entry). This behavior was introduced at the beginning of a11y times (readonly state on text leafs is bug 204186, but before that readonly state on linkable accessible (text leafs are inherited from it) was introduced prior bug 102777 - I didn't dig up to initial bug where it was introduced). It makes me think we probably don't need that readonly state on text leafs at all nowdays. So let's try to remove it and do through out testing.
(Assignee)

Comment 1

5 years ago
Marco, can you do testing of different screen readers of different versions please?

http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/surkov.alexander@gmail.com-0506889f332f/

Comment 2

5 years ago
I tested NVDA, JAWS 11 and 13, and Window-Eyes 7.5 (older versions no longer available) on various pages, and nothing seems to be broken. At least I didn't notice anything bad in text, headings, links and other stuff that also has text leaf accessibles. Nothing that shouldn't be there or the like.
(Assignee)

Comment 3

5 years ago
thank you, Marco, just in case did you try midas documents?
(Assignee)

Comment 4

5 years ago
Created attachment 624322 [details] [diff] [review]
patch
Attachment #624322 - Flags: review?(trev.saunders)

Comment 5

5 years ago
Comment on attachment 624322 [details] [diff] [review]
patch

>-       state |= states::READONLY; // Links not focusable in editor

What was the original test case for this line?
(Assignee)

Comment 6

5 years ago
(In reply to Marco Zehe (:MarcoZ) from comment #5)

> >-       state |= states::READONLY; // Links not focusable in editor
> 
> What was the original test case for this line?

it's in the dark of cvs history (I guess from beginning of times :) ).
(In reply to alexander :surkov from comment #0)
> similar to bug 754830, we need to get rid document->State() call. It's used
> to expose or not to expose readonly state on text leafs (statictext role).
> So if the text leaf is inside the editable document then we don't expose
> readonly state, otherwise we do. The behavior is not consistent since it
> doesn't take into account that text leaf can be contained by editable area
> (like p@contentEditable="true" or text entry). This behavior was introduced
> at the beginning of a11y times (readonly state on text leafs is bug 204186,
> but before that readonly state on linkable accessible (text leafs are
> inherited from it) was introduced prior bug 102777 - I didn't dig up to
> initial bug where it was introduced). It makes me think we probably don't
> need that readonly state on text leafs at all nowdays. So let's try to
> remove it and do through out testing.

its not quiet as fast as not checking at all, but I think it would be mroe correct to get the hypertext accessible containing the text accessible I think it should just be the parent? then get its editor and get that editors flags?
(Assignee)

Comment 8

5 years ago
(In reply to Trevor Saunders (:tbsaunde) from comment #7)

> its not quiet as fast as not checking at all, but I think it would be mroe
> correct to get the hypertext accessible containing the text accessible I
> think it should just be the parent? then get its editor and get that editors
> flags?

yes, correct vs fast. If nobody relies on correct behavior (note, text nodes accessibles are MSAA rudiment) so I'd go with fast approach (note, correct approach is not really fast because figuring whether the parent is editable or not assumes tree traversal).
(Assignee)

Comment 9

5 years ago
Trevor, bumping it in your queue
(In reply to alexander :surkov from comment #8)
> (In reply to Trevor Saunders (:tbsaunde) from comment #7)
> 
> > its not quiet as fast as not checking at all, but I think it would be mroe
> > correct to get the hypertext accessible containing the text accessible I
> > think it should just be the parent? then get its editor and get that editors
> > flags?
> 
> yes, correct vs fast. If nobody relies on correct behavior (note, text nodes
> accessibles are MSAA rudiment) so I'd go with fast approach (note, correct
> approach is not really fast because figuring whether the parent is editable
> or not assumes tree traversal).
true, but just with accessible tree that shouldn't be too bad, but not lightning fast either.  anyway lets take it and see who breaks.
Attachment #624322 - Flags: review?(trev.saunders) → review+
(Assignee)

Comment 11

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/6f2475c2968e
Target Milestone: --- → mozilla15
https://hg.mozilla.org/mozilla-central/rev/6f2475c2968e
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED

Comment 13

5 years ago
Damn. I unfortunately missed this, as the summary made me think it was just a perf improvement with no functional change. :(

This breaks NVDA's dialog text calculation for alertdialogs and alerts, which relies on text leaf accessibles having the read-only state. I can fix this in NVDA, but I'm wondering whether this breaks other less obvious functionality in other ATs.
(Assignee)

Comment 14

5 years ago
(In reply to James Teh [:Jamie] from comment #13)
> Damn. I unfortunately missed this, as the summary made me think it was just
> a perf improvement with no functional change. :(

I should be better on bug summaries.

> This breaks NVDA's dialog text calculation for alertdialogs and alerts,
> which relies on text leaf accessibles having the read-only state. 

Can you give rationale or history on this please?

> I can fix
> this in NVDA, but I'm wondering whether this breaks other less obvious
> functionality in other ATs.

Marco, your call?

Comment 15

5 years ago
(In reply to alexander :surkov from comment #14)
> > This breaks NVDA's dialog text calculation for alertdialogs and alerts,
> > which relies on text leaf accessibles having the read-only state. 
> Can you give rationale or history on this please?
To get text for a dialog, we walk through its descendants looking for objects that might contain dialog text and concatenate the resulting text. This is general code, not Mozilla specific, so we don't walk embedded objects. Read-only text objects are considered for dialog text.

Btw, as to why the read-only state was present in the first place, it is probably because IE exposes its text leaves as read -only text. I have no idea why they used read-only ROLE_SYSTEM_TEXT instead of just ROLE_SYSTEM_STATICTEXT, but I suspect the reason for that is buried in the depths of ancient history/legacy.
(Assignee)

Updated

5 years ago
Summary: Speed up text leaf accessible state calculation → Speed up text leaf accessible state calculation (aka don't expose readonly state on text leafs)
(Assignee)

Updated

5 years ago
Depends on: 781740
(Assignee)

Comment 16

5 years ago
Jamie, let's move discussion into bug 781740
You need to log in before you can comment on or make changes to this bug.