Last Comment Bug 754857 - Speed up text leaf accessible state calculation (aka don't expose readonly state on text leafs)
: Speed up text leaf accessible state calculation (aka don't expose readonly st...
Status: RESOLVED FIXED
: access
Product: Core
Classification: Components
Component: Disability Access APIs (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla15
Assigned To: alexander :surkov
:
Mentors:
Depends on: 781740
Blocks: a11yperf 732872
  Show dependency treegraph
 
Reported: 2012-05-14 08:15 PDT by alexander :surkov
Modified: 2012-08-09 23:01 PDT (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch (1.80 KB, patch)
2012-05-16 01:36 PDT, alexander :surkov
tbsaunde+mozbugs: review+
Details | Diff | Review

Description alexander :surkov 2012-05-14 08:15:03 PDT
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.
Comment 1 alexander :surkov 2012-05-15 19:35:30 PDT
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 Marco Zehe (:MarcoZ) 2012-05-16 01:21:10 PDT
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.
Comment 3 alexander :surkov 2012-05-16 01:30:21 PDT
thank you, Marco, just in case did you try midas documents?
Comment 4 alexander :surkov 2012-05-16 01:36:03 PDT
Created attachment 624322 [details] [diff] [review]
patch
Comment 5 Marco Zehe (:MarcoZ) 2012-05-16 03:59:33 PDT
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?
Comment 6 alexander :surkov 2012-05-16 06:53:02 PDT
(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 :) ).
Comment 7 Trevor Saunders (:tbsaunde) 2012-05-16 16:44:48 PDT
(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?
Comment 8 alexander :surkov 2012-05-16 23:02:28 PDT
(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).
Comment 9 alexander :surkov 2012-05-22 02:48:01 PDT
Trevor, bumping it in your queue
Comment 10 Trevor Saunders (:tbsaunde) 2012-05-22 11:22:46 PDT
(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.
Comment 12 Ed Morley [:emorley] 2012-05-24 10:58:50 PDT
https://hg.mozilla.org/mozilla-central/rev/6f2475c2968e
Comment 13 James Teh [:Jamie] 2012-08-07 03:15:03 PDT
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.
Comment 14 alexander :surkov 2012-08-09 18:42:29 PDT
(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 James Teh [:Jamie] 2012-08-09 19:35:30 PDT
(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.
Comment 16 alexander :surkov 2012-08-09 23:01:31 PDT
Jamie, let's move discussion into bug 781740

Note You need to log in before you can comment on or make changes to this bug.