Space between embedded objects characters is missing

VERIFIED FIXED

Status

()

Core
Disability Access APIs
VERIFIED FIXED
11 years ago
10 years ago

People

(Reporter: Joanmarie Diggs, Assigned: Evan Yan)

Tracking

(Blocks: 1 bug, {access})

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: orca:high)

Attachments

(2 attachments, 3 obsolete attachments)

(Reporter)

Description

11 years ago
Created attachment 290049 [details]
test case

Problem 1, Step to reproduce:

1. Use Accerciser to examine the text of the paragraph in the test case.

Expected results:  There would be a space in between the two embedded object characters because there is a space in between them in the HTML.

Actual results: There is no space in between the two embedded object characters.

Problem 2, Steps to reproduce:

1. Resize your browser window so that "first link" is at the end of the line.
2. Select the paragraph in Accerciser's list of accessibles.
3. In Accerciser's iPython console, type the following:

   acc.queryText().getTextAtOffset(0, TEXT_BOUNDARY_LINE_START)

Expected results:  The second embedded object character would not be included in the returned text because it's not on the same line as the character at offset 0.

Actual results: The second embedded object character is included in the returned text.

Blocking bug 396346 to request consideration of this issue as an FF3 commitment: It impacts using getTextAtOffset() rather than character extents to determine what's on a given line.  Thanks!
(Assignee)

Comment 1

11 years ago
We don't create accessible for white space.

How can we tell the difference between formating spaces and a meaningful space like in the test case of this bug?
Aaron, any idea?

Comment 2

11 years ago
(In reply to comment #1)
> We don't create accessible for white space.
> 
> How can we tell the difference between formating spaces and a meaningful space
> like in the test case of this bug? 

We should use rendered text I guess and expose those spaces that are rendered.
(Assignee)

Comment 3

11 years ago
Problem #2 in description is a dupe of bug 384101
(Assignee)

Comment 4

11 years ago
For the space, we ignored it in nsAccessibilityService::GetAccessible()

1353     if (frame->IsEmpty()) {
1354       *aIsHidden = PR_TRUE;
1355       return NS_OK;
1356     }

and in nsTextFrame::GetAccessible()

3056   if (!IsEmpty() || GetNextInFlow()) {

The frame for the space is visible but empty.

It seems we mean to ignore spaces between two objects.
If we expose those spaces now, would it cause any regression?
(Reporter)

Comment 5

11 years ago
Changing the whiteboard status to orca:high.

I'm working on moving Orca away from using extents to determine and present line contents.  Having the whitespace where it belongs would be helpful.  Thanks!!
Whiteboard: orca:high
(Assignee)

Comment 6

11 years ago
Aaron, do you think it's OK to create accessible for whitespace?

We have checked frame's visibility, so we won't create accessible for invisible ones. Though we will still create a lot of extra accessibles. And Joanie thinks it's helpful.

Comment 7

11 years ago
Evan, interesting question. So IsEmpty() is true when there's a single space of rendered text for the frame? Or is it that the engine decided it doesn't need to have any rendered text for that frame?

I suggest trying that approach and seeing if it even works before we go further. We might be able to have a better test for ignoring text frames than just IsEmpty()
(Assignee)

Comment 8

11 years ago
(In reply to comment #7)
> Evan, interesting question. So IsEmpty() is true when there's a single space of
> rendered text for the frame? Or is it that the engine decided it doesn't need
> to have any rendered text for that frame?
> 
frame->IsEmpty() is true, frame->GetNextInFlow() is null, frame->GetStyleVisibility()->IsVisible() is true.

> I suggest trying that approach and seeing if it even works before we go
> further.

If I delete the two blocks mentioned in comment #4, all visible whitespaces will have accessibles.
And both of the two problems in the bug description are gone.

Comment 9

11 years ago
Okay. I just don't want to create accessibles for text nodes that really are empty -- they just have no text at all. Why? Because the DOM often inserts blank text nodes all over the place and we'd have lots of extra objects.

Take a look at CVS blame and see if you can find why we made these changes in the first place.

Finally, when you put up a patch, Marco can test with it. But don't just remove the IsEmpty() test. Replace it with a test perhaps if textContent->GetLength() == 0 so that really empty text nodes don't get accessibles.
(Assignee)

Comment 10

11 years ago
Created attachment 292910 [details] [diff] [review]
replace IsEmpty() with textLenght > 0 && text != '\n'

Marco, could you help test the patch please?
(Assignee)

Comment 11

11 years ago
The test of text != '\n' prevents most of extra objects as far as my testing. It won't break <br>, we still have accessible for <br>.

We added IsEmpty() in bug 287738. This patch doesn't break that one. That patch is mainly to fix a condition of 
if (frameSize.height == 0 || frameSize.width == 0)

Comment 12

11 years ago
Evan, what does a single \n mean? If the source in the testcase for this bug was arranged with a newline between the links, does your patch still work?
(Assignee)

Comment 13

11 years ago
(In reply to comment #12)
> Evan, what does a single \n mean? If the source in the testcase for this bug
> was arranged with a newline between the links, does your patch still work?
> 
Aaron you're right. A newline will break the fix, although the newline is rendered as space.

I test '\n' because we did it before. See the patch of bug 287738
https://bugzilla.mozilla.org/attachment.cgi?id=183538
But now it turns out testing for '\n' still prevents useful accessibles.
It's just not easy to create useful accessibles for spaces without creating too much extra objects. :(
(Assignee)

Updated

11 years ago
Attachment #292910 - Attachment is obsolete: true

Comment 14

11 years ago
Where do we get the extra accessible objects if we just remove the \n rule and have only textLength == 0 ?
(Assignee)

Comment 15

11 years ago
For example, with a simple html

<head>
</head>
<body>
<a href="1">1</a> <a href="1">2</a>
</body>

We have accessible tree in DOM Inspector as

document
  - text  // extra
  - A     // the first link
  - text  // the space between the two links
  - A     // the second link
  - text  // extra
(Assignee)

Comment 16

11 years ago
Basically in html script, if the author put any space or newline between two tags, there will be an extra accessible object.

Comment 17

11 years ago
Here's an idea, but I don't know if it will work. What if you check for emptiness by doing:

if (frame->IsEmpty()) {
  nsAutoString renderedWhitespace;
  frame->GetRenderedText(renderedWhitespace, nsnull, nsnull, 0, 1);
  if (renderedWhitespace.IsEmpty()) {
    // Really empty -- nothing is rendered
  }
}

Or does that just put us back where we already are?

Comment 18

11 years ago
Another thing that might work is that we really only want to keep whitespace frames in between 2 inline frames
if (frame->IsEmpty() && frame->TextLength() > 0) {
  nsIFrame *nextSibling = frame->GetNextSibling();
  if (!nextSibling || nextSibling->GetType() != nsAccessibilityAtoms::inlineFrame) {
    return; // Is hidden and not before an inline frame
  }
  nsIFrame *prevSibling = GetPreviousSiblingFrame(); // not sure how to do this fast
  if (!prevSibling || prevSibling->GetType() != nsAccessibilityAtoms::inlineFrame) {
    return; // Is hidden and not before an inline frame
  }
}

Comment 19

11 years ago
Or, for comment 18, test for type == blockFrame, since whitespace adjacent to frames other than inline frames should still be kept. However, whitespace next to a block frame should be thrown out.
(Assignee)

Comment 20

11 years ago
Aaron, your idea in comment #17 works!
Do we still need check for inlineFrame? I haven't test them, but exposing rendered whitespace looks right and simple to me.
(Assignee)

Comment 21

11 years ago
Created attachment 292921 [details] [diff] [review]
patch v2
Assignee: aaronleventhal → Evan.Yan
Status: NEW → ASSIGNED
Attachment #292921 - Flags: review?(aaronleventhal)

Comment 22

11 years ago
Comment on attachment 292921 [details] [diff] [review]
patch v2

Looks good. Frame type check would not be necesssary. What about GetNextInFlow() check -- are you sure we don't need that? I'm nervous to remove it.
(Assignee)

Comment 23

11 years ago
GetNextInFlow() returns null for the space between two links. So I guess it's not reliable for testing useful whitespace.

Comment 24

11 years ago
Comment on attachment 292921 [details] [diff] [review]
patch v2

So GetNextInFlow() was there to make sure we got the space at the end of a line, right?

Please wait for Marco to test whether this affects the JAWS virtual buffer before seeking approval 1.9.
Attachment #292921 - Flags: review?(aaronleventhal) → review+

Comment 25

11 years ago
This patch changes the accessible hierarchy of some markup, resulting in JAWS Screen Display mode running paragraphs together when it shouldn't.

Without the patch, visit http://www.heise.de/newsticker/meldung/100540.
Following the Heading Level 2 is aDIV element that separates the header from the paragraph. In AccExplorer32, the heading is made up of two nodes: The text and the link that follows it, which itself contains a graphic.
Following the Heading node is another node that contains all the paragraphs as children.

Visiting this page with the patch:
1. adds a third child to the heading level 2 node that is an empty editable text which, in addition, is invisible.
Following the Heading 2 node are, on the same level, the paragraph nodes.

In summary: The patch adds a third child to the Heading 2 node and moves the paragraphs onto the same logical level as the Heading 2, out of a sub tree.

This causes JAWS to run the heading and first paragraph together, and that would be a regression.
(Assignee)

Comment 26

11 years ago
Marco, thanks for your testing.

From DOMInspector, I see a third child is added to the Heading 2 node. The patch means to do that.

But I didn't see any accessible is moved to a different level. The paragraph nodes are children of a sibling of Heading 2 node, no matter with or without the patch.

Could you check that again? Theoretically, the patch shouldn't change accessible hierarchy.

Aaron, is accessible tree exposed on Windows always the same as nsIAccessible tree, which we see in DOMInspector?

Comment 27

11 years ago
(In reply to comment #26)
> From DOMInspector, I see a third child is added to the Heading 2 node. The
> patch means to do that.

Does it also intend for this third node to be invisible and have big negative location numbers?

> But I didn't see any accessible is moved to a different level. The paragraph
> nodes are children of a sibling of Heading 2 node, no matter with or without
> the patch.

Yes, I see that today, too. I don't know if it was me -- it was late in the evening --, or if somehow AccExplorer misbehaved. But today, I see the hierarchy is the same in both cases.

So the question is why JAWS sees a NewLine without the patch, but runs the heading and paragraph together on one line with the patch. Perhaps if you exposed that additional node as being visible, that would change things. Could you make me a patch with that extra node being visible?
(Assignee)

Comment 28

11 years ago
Created attachment 293281 [details] [diff] [review]
fix invisible state

the text node was invisible because it just some newline, and has zero area rect.

Marco, could you try this patch please?

Comment 29

11 years ago
(In reply to comment #28)
> the text node was invisible because it just some newline, and has zero area
> rect.

Evan, the node is now visible. However, the fact that JAWS should go to a new line still isn't fixed. A second example of that page http://www.heise.de/newsticker/meldung/100540 is down at the bottom with the "Hilfe" link and the following Copyright notice. Without the patch, the CopyRight notice starts on a new line with screen layout mode, with this patch, that NewLine information is lost, and JAWS runs them together, separated by only a space.
(Assignee)

Comment 30

11 years ago
Marco, I really have no idea why JAWS read that two accessibles together. The accessible tree seems good to me.

Could that be a bug of JAWS? Or do you have any clue that what JAWS depends on to read multiple accessibles together?

Comment 31

11 years ago
Evan, could be a bug of JAWS that is now exposed. I do not know exactly what prompts JAWS to run the accessibles together like this. It appears to see a space following the link and thus ignoring the start of the paragraph.

Since Screen layout mode is not the default mode for browsing in JAWS, I would suggest you get review and land the fix nevertheless. It helps Orca and possibly others and thus is important enough.
(Assignee)

Comment 32

11 years ago
Thanks Marco, I'll ask review for the later patch.
(Assignee)

Comment 33

11 years ago
Comment on attachment 293281 [details] [diff] [review]
fix invisible state

Aaron, this patch fix invisible state based on the former patch.
Attachment #293281 - Flags: review?(aaronleventhal)

Comment 34

11 years ago
I'm looking at the code in nsAccessible::IsVisible(). It appears that the |if (frame->GetNextContinuation())| check at the top of that if/else/else sequence should possibly be removed if the new GetRenderedText() check is a better test for the same thing.

     if (frame->GetNextContinuation()) {
       // Zero area rects can occur in the first frame of a multi-frame text flow,
       // in which case the next frame exists because the text flow is visible
       rectVisibility = nsRectVisibility_kVisible;
     }
     else if (IsCorrectFrameType(frame, nsAccessibilityAtoms::inlineFrame)) {
       // Yuck. Unfortunately inline frames can contain larger frames inside of them,
       // so we can't really believe this is a zero area rect without checking more deeply.
       // GetBounds() will do that for us.
       PRInt32 x, y, width, height;
       GetBounds(&x, &y, &width, &height);
       if (width > 0 && height > 0) {
         rectVisibility = nsRectVisibility_kVisible;    
       }
     }
+    else if (IsCorrectFrameType(frame, nsAccessibilityAtoms::textFrame)) {
+      // textFrame with zero area rect still could be visible, for example it only renders some newline
+      nsAutoString renderedText;
+      frame->GetRenderedText (&renderedText, nsnull, nsnull, 0, 1);
+      if (!renderedText.IsEmpty()) {
+        rectVisibility = nsRectVisibility_kVisible;
+      }
+    }

Comment 35

11 years ago
Created attachment 293523 [details] [diff] [review]
Tweak Evan's patch - he has a better check for relevant zero area text frames than what we currently have. This is his patch our old check removed

Evan, r+ for your patch but I'd like to make this tweak to it.
Attachment #293281 - Attachment is obsolete: true
Attachment #293523 - Flags: review?(Evan.Yan)
Attachment #293281 - Flags: review?(aaronleventhal)

Comment 36

11 years ago
Comment on attachment 293281 [details] [diff] [review]
fix invisible state

This was good but I want to streamline it. See new patch.
Attachment #293281 - Flags: review+

Updated

11 years ago
Attachment #292921 - Attachment is obsolete: true
(Assignee)

Updated

11 years ago
Attachment #293523 - Flags: review?(Evan.Yan) → review+

Updated

11 years ago
Attachment #293523 - Flags: approval1.9?

Updated

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

Comment 37

11 years ago
patch checked in.
Status: ASSIGNED → RESOLVED
Last Resolved: 11 years ago
Resolution: --- → FIXED

Updated

11 years ago
Depends on: 409473

Comment 38

11 years ago
Evan, unfortunately we're missing a check of some kind. This is causing bug 409473.

Comment 39

10 years ago
Verified using Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9b3) Gecko/2008020514 Firefox/3.0b3
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.