Accessible text broken for (un)ordered list items effective 4th Aug trunk build

RESOLVED FIXED

Status

()

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

People

(Reporter: Joanmarie Diggs, Assigned: Aaron Leventhal)

Tracking

(Blocks: 1 bug, {access})

Trunk
x86
Linux
access
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 4 obsolete attachments)

(Reporter)

Description

11 years ago
Steps to reproduce:

1. Open the test case
2. Launch Accerciser, locate one of the list items from the test case, and examine its text in the interface viewer

Expected results:  The text of that list item would be present.

Actual results:  The text of that list item is not present.

This seems to have been introduced in the 2007080404 trunk build; 2007080308 and earlier are fine.
(Reporter)

Comment 2

11 years ago
Updating the summary ever so slightly.  Turns out the text is not completely missing: It's simply not showing up in Accerciser -- presumably because other things are broken.

From the iPython Console when "item 1" from the test case was selected:

In [1]: text = acc.queryText()
In [2]: text.getText(0,-1)
Out[2]: 'tem 1'
In [3]: text.characterCount
Out[3]: 0

In other words, the first two characters have gone AWOL and the characterCount is bogus.  But most of the text is there.
Summary: Accessible text missing for (un)ordered list items effective 4th Aug trunk build → Accessible text broken for (un)ordered list items effective 4th Aug trunk build

Comment 3

11 years ago
(In reply to comment #0)
> Steps to reproduce:
> 
> 1. Open the test case
> 2. Launch Accerciser, locate one of the list items from the test case, and
> examine its text in the interface viewer

sorry, from what testcase?

Comment 4

11 years ago
Surkov, simply use any site that contains an unordered list. The Minefield start page, for example. Joanmarie was referring to a specific test case that was reported by an Orca user. Sorry about the confusion!
(Assignee)

Comment 5

11 years ago
Created attachment 278876 [details]
Testcase
(Assignee)

Comment 6

11 years ago
Created attachment 278878 [details] [diff] [review]
There's an nsIContent* for a list bullet, it's just not text content. Change the fall-through test to success check for AppendTextTo().
Attachment #278878 - Flags: review?(surkov.alexander)
(Assignee)

Comment 7

11 years ago
Marco, Joanie, thanks for nailing down the day and actual patch that caused the regression. That made it easier to figure out.

Comment 8

11 years ago
Comment on attachment 278878 [details] [diff] [review]
There's an nsIContent* for a list bullet, it's just not text content. Change the fall-through test to success check for AppendTextTo().

okay, looks a bit hacky but r=me.
Attachment #278878 - Flags: review?(surkov.alexander)
Attachment #278878 - Flags: review+
Attachment #278878 - Flags: approval1.9?
(Assignee)

Comment 9

11 years ago
Yes it's hacky but remember after Firefox 3 dbaron says they're going to make list bullets less hacky in general -- it will have a real text node. So at that point our code will not need this special case. One hack deserves another :)

Comment 10

11 years ago
Unfortunately this hack causes a crash in Firefox when arrowing onto these--now again complete--list items that are not links. Just tested this on the Minefield Start Page:
- If the patch is there, and I press L twice to go to the second list on the page (the first one with 5 links is OK), Firefox crashes on me.
- If I remove this patch, the crash does not happen.
Again, the items are now complete, but something is still wrong.
(Assignee)

Comment 11

11 years ago
Comment on attachment 278878 [details] [diff] [review]
There's an nsIContent* for a list bullet, it's just not text content. Change the fall-through test to success check for AppendTextTo().

Thanks Marco, I'll figure out why that is.
Attachment #278878 - Flags: approval1.9?
(Assignee)

Comment 12

11 years ago
Marco, I go to http://www.mozilla.org/projects/minefield/ with JAWS and hit L twice with this patch, and get no crash.

Comment 13

11 years ago
Aaron, I believe I was at the Gran Paradiso Alpha 8 start page when it happened. It's the start page besides the Minefield start page that sometimes gets opened as well. It crashed in the list of changes in this alpha version.

Comment 14

11 years ago
(In reply to comment #12)
> Marco, I go to http://www.mozilla.org/projects/minefield/ with JAWS and hit L
> twice with this patch, and get no crash.

Erm, I should read these when I'm more awake. :-) This bug was filed against Linux, so I was testing the patch with Orca. And there, on the Minefield start page, hitting L twice, results in a crash in Firefox, Orca continues running.
JAWS is, indeed, fine on Windows.

Sorry for the confusion!
(Assignee)

Comment 15

11 years ago
I get a lockup -- not a crash (small but significant diference)

Comment 16

11 years ago
Nope, definitely a crash here. I get thrown back to the Terminal window I started this self-compiled Firefox from, with a "segmentation fault - Core dumped" message.
(Assignee)

Comment 17

11 years ago
You're right, actually, as usual :)
(Assignee)

Comment 18

11 years ago
Created attachment 279641 [details] [diff] [review]
Completely avoid using GetRenderedText() or gfxCharIter etc. on non-text frames

This addresses the crash, although I get lots of beeps when I try Orca with Firefox. Possibly operator error?

I believe this patch fixes this bug.
Attachment #278878 - Attachment is obsolete: true
Attachment #279641 - Flags: review?(surkov.alexander)
(Assignee)

Comment 19

11 years ago
Created attachment 279642 [details] [diff] [review]
Correct patch (last one had a small error)
Attachment #279641 - Attachment is obsolete: true
Attachment #279642 - Flags: review?(surkov.alexander)
Attachment #279641 - Flags: review?(surkov.alexander)
(Reporter)

Comment 20

11 years ago
I just tried the patch and noticed a couple of things:

1. In your test case, the list item text seems to begin with an embedded object character. (e.g. Oranges), but the list item doesn't indicate any children that correspond to that embedded object.

2. Potentially bogus extents on list items.  If I look at the first list using Accerciser's Interface viewer, I see that its size is 1664 x 132 (Maximized window 1680 pixels wide).  If I then look at the first child of that list, I see that its size is 3248 x 22.  This may be a side effect of my multi-head system, however I would think that the extents of a list item would not be permitted to exceed that of the parent list.

Comment 21

11 years ago
Can you figure rule when to use the check: frame->GetType() == nsAccessibilityAtoms::textFrame
(Assignee)

Comment 22

11 years ago
Joanie, the list item bounds are a different bug. I don't know about the embedded object issue -- that might have been an issue before this bug as well. Probably another separate issue.

Surkov, I didn't understand your last comment.

Comment 23

11 years ago
(In reply to comment #22)

> Surkov, I didn't understand your last comment.
> 

I'm just not sure why do you use that check.
(Assignee)

Comment 24

11 years ago
Because we want to make sure we only use rendered text (e.g. use gfxSkipChars etc.) on text frames.
(Assignee)

Comment 25

11 years ago
Created attachment 279690 [details] [diff] [review]
More careful when to use text frame only methods
Attachment #279690 - Flags: review?(surkov.alexander)
(Assignee)

Updated

11 years ago
Attachment #279642 - Attachment is obsolete: true
Attachment #279642 - Flags: review?(surkov.alexander)

Comment 26

11 years ago
Comment on attachment 279690 [details] [diff] [review]
More careful when to use text frame only methods

>Index: accessible/src/base/nsAccessible.cpp
>===================================================================
>       nsresult rv = nsHyperTextAccessible::ContentToRenderedOffset(frame, content->TextLength(), &length);
>-      return NS_SUCCEEDED(rv) ? length : -1;
>+      return NS_SUCCEEDED(rv) ? NS_STATIC_CAST(PRInt32, length) : -1;

use static_cast<>

what about GetBoundsForString()?
(Assignee)

Comment 27

11 years ago
Created attachment 279695 [details] [diff] [review]
Surkov's comments

We still have open bugs on bounds for list bullets etc. and I don't want to try and fix that here.
Attachment #279690 - Attachment is obsolete: true
Attachment #279695 - Flags: review?(surkov.alexander)
Attachment #279690 - Flags: review?(surkov.alexander)
(Reporter)

Comment 28

11 years ago
> Joanie, the list item bounds are a different bug. 

Okie dokie.

> I don't know about the embedded object issue -- 
> that might have been an issue before this bug as well.

On that one I would beg to differ.  ;-) Before we were getting numbers (e.g. 1. Oranges).  And with the latest version of your patch it seems that we are once again getting the numbers.  (Yea!) With your previous version of the patch, that was not the case.

Bogus bounds are not good, but Orca seems to be reading list items correctly with your latest patch.  Thanks!!!
(Reporter)

Comment 29

11 years ago
By "latest patch" I mean the one from Comment #25.

Comment 30

11 years ago
Comment on attachment 279695 [details] [diff] [review]
Surkov's comments

r=me
Attachment #279695 - Flags: review?(surkov.alexander)
Attachment #279695 - Flags: review+
Attachment #279695 - Flags: approval1.9?

Updated

11 years ago
Attachment #279695 - Flags: approval1.9? → approval1.9+
(Assignee)

Updated

11 years ago
Status: NEW → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.