Closed Bug 1052866 Opened 10 years ago Closed 6 years ago

Expose IAccessibleText on tables, rows, etc.

Categories

(Core :: Disability Access APIs, defect)

x86_64
Windows 8.1
defect
Not set
normal

Tracking

()

VERIFIED FIXED
mozilla65
Tracking Status
firefox64 --- verified
firefox65 --- verified

People

(Reporter: Jamie, Assigned: surkov)

References

(Blocks 1 open bug)

Details

Attachments

(2 files, 2 obsolete files)

This is more a proposal for consideration than a "please definitely do this" bug. :)

At present, IAccessibleText is exposed on almost everything except tables. The problem is that this makes it rather tricky to deal with any range of text that includes a table. The most practical example is where part or all of a table is selected, as well as other stuff outside it. To get the selection, a client primarily deals with IAccessibleText::selection, but has to handle tables specially; e.g. check the selected state or perhaps use the table cell interface to query for selection. Ideally, you could just ask for the selection on a table or row, which would return offsets covering the embedded objects for the selected rows/cells.
See also bug 634126.
Blocks: tablea11y
I agree that having no text interface on tables makes them special to deal with. If we decide to not go with text ranges then we can fix this bug especially since many AT find it useful.
Dealing with selection is actually worse than I thought (even aside from bug 1169238). It seems Gecko doesn't expose a table cell as selected (i.e. selected state and selection methods in table interfaces) unless the whole cell it selected. So, if you just select part of the text in a cell, it doesn't get exposed as selected. This means you have to walk through every row and cell in the table getting the text interface and asking whether it has a selection.

Getting the caret is just as bad. Again, you have to potentially walk through every row and cell. This could be improved by bug 873438, but that doesn't address selection.

Just as a matter of interest, Chroe does seem to implement IAccessibleText on tables.
(In reply to James Teh [:Jamie] from comment #3)
> Dealing with selection is actually worse than I thought (even aside from bug
> 1169238). It seems Gecko doesn't expose a table cell as selected (i.e.
> selected state and selection methods in table interfaces) unless the whole
> cell it selected. So, if you just select part of the text in a cell, it
> doesn't get exposed as selected. This means you have to walk through every
> row and cell in the table getting the text interface and asking whether it
> has a selection.

I think these two are different selections at least on API level. Table interface is supposed to deal with control's selection, while selected text in the cell is text selection, which I assume you can retrieve it from table rows.

> Getting the caret is just as bad. Again, you have to potentially walk
> through every row and cell.

if they had a text interface then would it be much easier to work with caret and selection?
(In reply to alexander :surkov from comment #4)
> I think these two are different selections at least on API level. Table
> interface is supposed to deal with control's selection,
Right; I thought as much. This makes sense.

> while selected text
> in the cell is text selection, which I assume you can retrieve it from table
> rows.
Nope. Table rows don't get IAccessibleText either.

> > Getting the caret is just as bad. Again, you have to potentially walk
> > through every row and cell.
> if they had a text interface then would it be much easier to work with caret
> and selection?
Yes. That's what I'm requesting here: that both tables and table rows get IAccessibleText.
Could you demo the difference please for easier understanding of the benefits? some meta code snippet would be nice.
Say you have this document:
<body contentEditable="true">
<p>Paragraph</p>
<table>
<tr><td>1</td><td>2</td></tr>
</table>
</body>

1. Move the caret to "2".
2. To get the caret, an AT has to:
2.1. Get the caret offset in the document's text interface. That will give us the table.
2.2. The table has no text interface, so the AT has to walk through each cell in each row and check whether it has a caret offset. In pseudo-code:
for row in table:
  for cell in row:
    text = cell.QueryInterface(IAccessibleText)
    caret = text.caretOffset
    if caret != -1:
      break
2.3. This means we have to deal with two code paths for getting the caret (the case where there is IAccessibleText and the case where there isn't).
2.4. More importantly, this would result in hideous perf for a large table.

3. Move the caret to the "1" and select "1".
4. To get the selection, an AT has to:
4.1. Get the selection offsets in the document. That will give us the table.
4.2. When we descend into the table, we'll see there's no text interface.
4.3. To work out where the selection starts, we have to search each cell in each row from the start of the table, asking for its selection. In pseudo-code:
for row in table:
  for cell in row:
    text = row.QueryInterface(IAccessibleText)
    if text.nSelections == 0:
      continue
    selStartAcc = row
    selStartStart, selStartEnd = row.selection(0)
4.4. To work out where the selection ends, we have to search each cell in each row from the end of the table, asking for its selection. In pseudo-code:
for row in reversed(table):
  for cell in reversed(row):
    text = row.QueryInterface(IAccessibleText)
    if text.nSelections == 0:
      continue
    selEndAcc = row
    selEndStart, selEndEnd = row.selection(0)
4.5. See 2.3 and 2.4 above.
Attached patch patch (obsolete) — Splinter Review
The road to hell is paved with good intentions
Attachment #8668586 - Flags: review?(tbsaunde+mozbugs)
Jamie, how about to do UIA text ranges instead? that should be a fastest way to grab selection in the document (https://msdn.microsoft.com/en-us/library/windows/desktop/ee671372%28v=vs.85%29.aspx)
Flags: needinfo?(jamie)
As discussed yesterday, we can't use UIA text ranges for this without completely switching to UIA for *everything*. This is because there is no way to associate between IA2 and UIA for text. This is possibly even the case for objects in general, though there is some very limited support for this.

We did discuss adding a call to IAccessible2 to get the selection range; i.e. startAcc, startOffset, endAcc, endOffset. I'll discuss this with Mick and get back to you on that soon.
Flags: needinfo?(jamie)
Comment on attachment 8668586 [details] [diff] [review]
patch

doesn't really seem that bad?

Its kind of weird there are the tables that don't inherit from HyperTextAccessible, but it seems to make sense, and they are for xul, so whatever :p
Attachment #8668586 - Flags: review?(tbsaunde+mozbugs) → review+
Let me try to summarise each approach.

Approach 1: Implementing the text interface on tables
Pros:
* We don't need new API.
* Pretty much everything else already implements the text interface, including divs and ARIA listboxes. Why should tables be any different? Implementing text on tables makes traversing text (not just selection) consistent. (It's always struck me as odd that tables should have their own special events as well.)
* We already have a patch for it now and it's not overly complicated.
Cons:
* We waste a bit of memory, since every table and row now has an additional interface and there is also additional memory used by the text code. It's certainly true that there could be a lot of tables and rows. However, I don't really buy this argument, since we're already doing it for divs and spans and there are a huge number of divs and spans out there (arguably more than tables/rows).
* Getting caret and selection is still pretty ugly and has to be done in-process for decent performance. Note, however, that NVDA (and probably others) already has code for this.
Notes:
* Chrome already implements the text interface on tables. I'm not really suggesting this as a reason Mozilla should do it, since Chrome normally emulates Firefox rather than the other way around, but I felt it worth noting nevertheless.

Approach 2: New API to get selection range as descendants and offsets
Pros:
* It's more elegant and performs well even out-of-process.
* It is a logical complement to accessibleWithCaret.
Cons:
* We need to get this into IA2, which means a new interface. Also, we should really solicit feedback from the rest of the IA2 stakeholders. It seems silly (and sets a bad precedent) to add a new interface for just one method, so we would ideally wait for a new release of IA2 with a few more features, but see more on this below.
* We then need to get it into both Mozilla and NVDA/other ATs.
Notes:
* We can actually add new methods to an interface without breaking the ABI. We shouldn't do this for an existing interface. However, what this does mean is that we could create a new "unrozen" interface which we keep extending until we feel a new release of IA2 is justified. Mozilla would need ot be willing to ship a non-final release of IA2, though. As far as NVDA is concerned, we can live with this. :)
* If we're adding this and justifying it using performance, we should consider adding another method to get the range for a unit (word, line, etc.), since that's also ugly and unperformant.

Personally, I'm happy with either approach. I perhaps slightly lean towards approach 1 because it's consistent and we can have it working ASAP without jumping through other hoops. However, approach 2 is probably more elegant. It's worth noting that these approaches aren't mutually exclusive--if we choose 1, we can still do 2 later--but I accept that if we do 1 now, we'll probably be stuck with whatever baggage it brings for good.
(In reply to James Teh [:Jamie] from comment #12)
> Cons:
> * We waste a bit of memory, since every table and row now has an additional
> interface and there is also additional memory used by the text code. It's
> certainly true that there could be a lot of tables and rows. However, I
> don't really buy this argument, since we're already doing it for divs and
> spans and there are a huge number of divs and spans out there (arguably more
> than tables/rows).

let me change it a bit, we should keep trying to cut off the memory usage rather than finding an excuse in existing memory wasting to spend even more memory, i.e in an ideal world those divs should go away and text tables should not appear.

> Approach 2: New API to get selection range as descendants and offsets
> Cons:
> * We need to get this into IA2, which means a new interface. Also, we should
> really solicit feedback from the rest of the IA2 stakeholders. It seems
> silly (and sets a bad precedent) to add a new interface for just one method,

btw, don't we need to pair it with methods to change selection over different accessibles?

> so we would ideally wait for a new release of IA2 with a few more features,
> but see more on this below.

agree, but taking into account how fast ia2 evaluates it may take years :)

> * We then need to get it into both Mozilla and NVDA/other ATs.
> Notes:
> * We can actually add new methods to an interface without breaking the ABI.
> We shouldn't do this for an existing interface. However, what this does mean
> is that we could create a new "unrozen" interface which we keep extending
> until we feel a new release of IA2 is justified. Mozilla would need ot be
> willing to ship a non-final release of IA2, though. As far as NVDA is
> concerned, we can live with this. :)

are there downsides of it like some binary issues?

> * If we're adding this and justifying it using performance, we should
> consider adding another method to get the range for a unit (word, line,
> etc.), since that's also ugly and unperformant.

yep, I think we'll likely end up with something close to text ranges, a two points in the document.

> Personally, I'm happy with either approach. I perhaps slightly lean towards
> approach 1 because it's consistent and we can have it working ASAP without
> jumping through other hoops.
> However, approach 2 is probably more elegant.
> It's worth noting that these approaches aren't mutually exclusive--if we
> choose 1, we can still do 2 later--but I accept that if we do 1 now, we'll
> probably be stuck with whatever baggage it brings for good.

so am I, however I lean towards to 2nd one. Btw, I'm pretty sure that some ATs workarounded the Firefox table issue already, which may be considered as an extra argument for 2nd (elegant and performant) approach. I'd be good to start a proposal at IA2 group to see how it goes, and if we stuck then we can always take that table patch.
(In reply to alexander :surkov from comment #13)
> let me change it a bit, we should keep trying to cut off the memory usage
> rather than finding an excuse in existing memory wasting to spend even more
> memory, i.e in an ideal world those divs should go away and text tables
> should not appear.
That doesn't really hold, though, since there's probably no way to get rid of the text interface for divs and spans.

> > Approach 2: New API to get selection range as descendants and offsets
> btw, don't we need to pair it with methods to change selection over
> different accessibles?
I think you were saying we could use IAccessibleText::addSelection for that purpose. You'd have to call it on each intervening object, though, which is rather horrible. So yeah, we probably do need to add a setter. That means we also need add and remove for non-contiguous selections.

> > so we would ideally wait for a new release of IA2 with a few more features,
> > but see more on this below.
> agree, but taking into account how fast ia2 evaluates it may take years :)
Which is one of the big reasons I'm reluctant to go down this path.

> > * We can actually add new methods to an interface without breaking the ABI.
> are there downsides of it like some binary issues?
As I understand it, adding new methods to an interface doesn't break binary compatibility. Only changing or removing methods does. We certainly shouldn't do this once an official release is made, but it can be done in the interim.

> > * If we're adding this and justifying it using performance, we should
> > consider adding another method to get the range for a unit (word, line,
> > etc.), since that's also ugly and unperformant.
> yep, I think we'll likely end up with something close to text ranges, a two
> points in the document.
Coming up with an API that covers all of the use cases is quite a bit of work. This is part of the problem: if we want to do this properly, it's going to take a lot longer and consume a lot more resources, both for Mozilla and NV Access.

> Btw, I'm pretty sure that some
> ATs workarounded the Firefox table issue already, which may be considered as
> an extra argument for 2nd (elegant and performant) approach.
It might be worth checking that. I actually don't see how they could get selection fully working in tables given bug 1169238, but that's out of scope here.

> I'd be good to
> start a proposal at IA2 group to see how it goes, and if we stuck then we
> can always take that table patch.
Fair enough.
Can we say that IA2_2 interfaces are not frozen and then start to add methods as we need? In other words, we have no official releases, at least until we are certain that no new methods is needed anymore. If that doesn't work then I guess we can continue with IA2_3 interfaces. We should probably go to IA2 group for discussion.

Would it be ok approach: propose a method, prototype it, get agreement on it, ship it? If so then would you be willing to start an IA2 thread for first most wanted candidate?
> * We waste a bit of memory, since every table and row now has an additional
> interface and there is also additional memory used by the text code. It's
> certainly true that there could be a lot of tables and rows. However, I
> don't really buy this argument, since we're already doing it for divs and
> spans and there are a huge number of divs and spans out there (arguably more
> than tables/rows).

yeah, this doesn't really bother me too much, even for thousands of rows I think we're taling about less than 100k of memory.

> Cons:
> * We need to get this into IA2, which means a new interface. Also, we should
> really solicit feedback from the rest of the IA2 stakeholders. It seems
> silly (and sets a bad precedent) to add a new interface for just one method,
> so we would ideally wait for a new release of IA2 with a few more features,
> but see more on this below.

does atk need something like this too?

> Personally, I'm happy with either approach. I perhaps slightly lean towards
> approach 1 because it's consistent and we can have it working ASAP without
> jumping through other hoops. However, approach 2 is probably more elegant.
> It's worth noting that these approaches aren't mutually exclusive--if we
> choose 1, we can still do 2 later--but I accept that if we do 1 now, we'll
> probably be stuck with whatever baggage it brings for good.

I agree, I probably should worry more about what  this will mean in the future, but I'm pretty ok with tables implementing text interfaces.
(In reply to alexander :surkov from comment #15)
> Can we say that IA2_2 interfaces are not frozen and then start to add
> methods as we need? In other words, we have no official releases, at least
> until we are certain that no new methods is needed anymore.
We probably shouldn't do this. IA2_2 was already agreed on and is already in an official release, so people will assume it's frozen. In contrast, it's clear that IA2_3 isn't frozen, since it isn't in an official release.

> Would it be ok approach: propose a method, prototype it, get agreement on
> it, ship it?
I can't see a problem with that.

> If so then would you be willing to start an IA2 thread for
> first most wanted candidate?
I find it difficult to push this proposal right now, since honestly, it'd make my life easier if tables just implemented IAccessibleText like they do in Chrome, and it doesn't even seem that ugly to me given that practically everything else in Gecko implements text. That would also mean I can get this working sooner, since we already have the code to support it. If you're really pushing this approach, I won't block it and I'll do my part to get it done, but I don't really want to champion it.
(In reply to James Teh [:Jamie] from comment #17)

> > If so then would you be willing to start an IA2 thread for
> > first most wanted candidate?
> I find it difficult to push this proposal right now, since honestly, it'd
> make my life easier if tables just implemented IAccessibleText like they do
> in Chrome, and it doesn't even seem that ugly to me given that practically
> everything else in Gecko implements text. That would also mean I can get
> this working sooner, since we already have the code to support it.

I worry that that may mean we never work out the "right" and faster solution.

> If you're
> really pushing this approach, I won't block it and I'll do my part to get it
> done, but I don't really want to champion it.

I think I can drive it if that's the issue. We can manage to get it done before this year ends to keep the approach competitive to the proposed one.
(In reply to alexander :surkov from comment #18)> > I find it difficult to push this proposal right now, since honestly, it'd
> > make my life easier if tables just implemented IAccessibleText
> I worry that that may mean we never work out the "right" and faster solution.
That'd be a solid argument if this bug were about the general problem of out-proc performance of IAText, but it isn't. It's about tables being different to everything else. Yes, it'd be nice if IAText were nicer out-proc and we should perhaps try for that in future, but it's been like this since the beginning and there's never been any mention of improving that until this table discussion.
Attached patch Rebased patch (obsolete) — Splinter Review
Carrying forward Trevor's r+. Alex Surkov is the original author of this patch, I just rebased it.
Assignee: nobody → mzehe
Attachment #8668586 - Attachment is obsolete: true
Attachment #9020274 - Flags: review+
Assignee: mzehe → surkov.alexander
Pushed by mzehe@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/a69a426bdb14
Expose IAccessibleText on tables, rows, etc., r=tbsaunde
I have a fix for the test failure and will push with that applied as soon as the tree reopens.
Flags: needinfo?(surkov.alexander)
Pushed by mzehe@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/946c2698ea05
Expose IAccessibleText on tables, rows, etc., r=tbsaunde. Bustage fix by MarcoZ.
https://hg.mozilla.org/mozilla-central/rev/946c2698ea05
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla65
This isn't quite fixed. Everything works as expected for rows. However, tables still aren't getting the IAccessibleText interface. They do get text events (so this still fixes bug 1502260), but QI to IAccessibleText fails.

I think this is because HTMLTableAccessibleWrap and ARIAGridAccessibleWrap don't call hyperTextAccessibleWrap::QueryInterface.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
The first patch inherits from the right classes, but because tables have additional interfaces, QueryInterface is overridden.
It had to be updated to also include HyperTextAccessibleWrap.
Rows worked correctly because they don't have any specific interfaces and thus don't override QI.
They just inherit their QI implementation from their base class.
Here's a simple way to verify this.

Test cases:
Normal table:
data:text/html,<button>Before</button><div contentEditable="true">foo<table><tr><th>1</th><td>2</td></tr></table></div>
ARIA table:
data:text/html,<button>Before</button><div contenteditable="true">foo<div role="table"><div role="row"><div role="cell">1</div><div role="cell">2</div></div></div></div>
The two test cases should have identical behaviour, as follows.

STR (with NVDA):
1. Load the test case.
2. Tab into the content editable.
3. Press control+home.
4. Press control+a to select all.
Expected: NVDA should say "foo  12  selected"
Actual (without the second patch): NVDA says "12  selected"
This improves things for JAWS as well with these test cases. Interestingly, JAWS reports the "foo" instead of the "12" without the patches. With the patches, it says "foo  12" as well.

(The fact that both screen readers say "12" instead of "1  2" is something I understand, even if it's not quite ideal. The key point is that without the patches, part of the selection goes missing when the screen reader is asked to report it.)
Pushed by jteh@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/7ceb485a00cc
part 2: Handle QI to IAccessibleText for tables. r=MarcoZ
https://hg.mozilla.org/mozilla-central/rev/7ceb485a00cc
Status: REOPENED → RESOLVED
Closed: 6 years ago6 years ago
Resolution: --- → FIXED
Verified fixed in Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:65.0) Gecko/20100101 Firefox/65.0 ID:20181029100347. Tested as per comment 28 and bug 1502260 comment 0.
Blocks: 1502260
Blocks: 1499519
Attachment #9020274 - Attachment is obsolete: true
Comment on attachment 9020758 [details] [diff] [review]
Part 1 as landed with bustage fix

Carrying forward r+.
Attachment #9020758 - Flags: review+
Comment on attachment 9020758 [details] [diff] [review]
Part 1 as landed with bustage fix

[Beta/Release Uplift Approval Request]

Feature/Bug causing the regression: None.

User impact if declined: The NVDA screen reader has implemented a major performance improvement for Firefox users. However, without this correctness fix, they are unable to ship it to users in their next release. Thus, users will have to wait another few months before benefiting from this major performance improvement.

Is this code covered by automated tests?: Yes

Has the fix been verified in Nightly?: Yes

Needs manual test from QE?: No

If yes, steps to reproduce: 

List of other uplifts needed: Part 2 from this bug.

Risk to taking this patch: Low

Why is the change risky/not risky? (and alternatives if risky): Exposes an additional, well tested interface/event on tables which was already being exposed on most other elements. In addition, other applications already expose this interface on tables, so clients are already expecting this.

String changes made/needed: None.
Attachment #9020758 - Flags: approval-mozilla-beta?
Comment on attachment 9020663 [details]
Bug 1052866 part 2: Handle QI to IAccessibleText for tables.

[Beta/Release Uplift Approval Request]

Feature/Bug causing the regression: None.

User impact if declined: The NVDA screen reader has implemented a major performance improvement for Firefox users. However, without this correctness fix, they are unable to ship it to users in their next release. Thus, users will have to wait another few months before benefiting from this major performance improvement.

Is this code covered by automated tests?: Yes

Has the fix been verified in Nightly?: Yes

Needs manual test from QE?: No

If yes, steps to reproduce: 

List of other uplifts needed: Part 1 from this bug.

Risk to taking this patch: Low

Why is the change risky/not risky? (and alternatives if risky): Exposes an additional, well tested interface/event on tables which was already being exposed on most other elements. In addition, other applications already expose this interface on tables, so clients are already expecting this.

String changes made/needed: None.
Attachment #9020663 - Flags: approval-mozilla-beta?
Flags: qe-verify+
Comment on attachment 9020663 [details]
Bug 1052866 part 2: Handle QI to IAccessibleText for tables.

[Triage Comment]
Big speedup for NVDA users with low risk. Approved for 64.0b6.
Attachment #9020663 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Attachment #9020758 - Flags: approval-mozilla-beta? → approval-mozilla-beta+
Hi, I managed to reproduce this issue with Firefox Beta 0b5 as well as older versions of Firefox Nightly, but I can no longer reproduce this issue in Firefox Nightly 65.0a1 (2018-10-31) or in Beta version 0b6 (https://tools.taskcluster.net/index/gecko.v2.mozilla-beta.latest.firefox/win64-opt), I will mark this issue accordingly.
Status: RESOLVED → VERIFIED
Flags: qe-verify+
Duplicate of this bug: 634126
You need to log in before you can comment on or make changes to this bug.