405248 - Space between embedded objects characters is missing

Status: VERIFIED FIXED

(Core :: Disability Access APIs, defect)

Description

[Joanmarie Diggs]

Attachment: 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!

Comment 1

[Evan Yan]

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

[alexander :surkov (:asurkov)]

(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.

Comment 3

[Evan Yan]

Problem #2 in description is a dupe of bug 384101

Comment 4

[Evan Yan]

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?

Comment 5

[Joanmarie Diggs]

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!!

Comment 6

[Evan Yan]

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

[Aaron Leventhal]

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()

Comment 8

[Evan Yan]

(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

[Aaron Leventhal]

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.

Comment 10

[Evan Yan]

Attachment: replace IsEmpty() with textLenght > 0 && text != '\n'

Marco, could you help test the patch please?

Comment 11

[Evan Yan]

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

[Aaron Leventhal]

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?

Comment 13

[Evan Yan]

(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. :(

Comment 14

[Aaron Leventhal]

Where do we get the extra accessible objects if we just remove the \n rule and have only textLength == 0 ?

Comment 15

[Evan Yan]

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

Comment 16

[Evan Yan]

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

Comment 17

[Aaron Leventhal]

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

[Aaron Leventhal]

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

[Aaron Leventhal]

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.

Comment 20

[Evan Yan]

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.

Comment 21

[Evan Yan]

Attachment: patch v2

Comment 22

[Aaron Leventhal]

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.

Comment 23

[Evan Yan]

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

Comment 24

[Aaron Leventhal]

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.

Comment 25

[Marco Zehe (:MarcoZ)]

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.

Comment 26

[Evan Yan]

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

[Marco Zehe (:MarcoZ)]

(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?

Comment 28

[Evan Yan]

Attachment: 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

[Marco Zehe (:MarcoZ)]

(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.

Comment 30

[Evan Yan]

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

[Marco Zehe (:MarcoZ)]

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.

Comment 32

[Evan Yan]

Thanks Marco, I'll ask review for the later patch.

Comment 33

[Evan Yan]

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

Aaron, this patch fix invisible state based on the former patch.

Comment 34

[Aaron Leventhal]

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

[Aaron Leventhal]

Attachment: 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.

Comment 36

[Aaron Leventhal]

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

This was good but I want to streamline it. See new patch.

Comment 37

[Evan Yan]

patch checked in.

Comment 38

[Aaron Leventhal]

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

Comment 39

[Marco Zehe (:MarcoZ)]

Verified using Mozilla/5.0 (Windows; U; Windows NT 6.0; en-US; rv:1.9b3) Gecko/2008020514 Firefox/3.0b3