Closed Bug 405248 Opened 13 years ago Closed 13 years ago

Space between embedded objects characters is missing

Categories

(Core :: Disability Access APIs, defect)

x86
Linux
defect
Not set
normal

Tracking

()

VERIFIED FIXED

People

(Reporter: jdiggs, Assigned: evan.yan)

References

(Blocks 1 open bug)

Details

(Keywords: access, Whiteboard: orca:high)

Attachments

(2 files, 3 obsolete files)

Attached file 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!
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?
(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.
Problem #2 in description is a dupe of bug 384101
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?
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
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.
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()
(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.
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.
Marco, could you help test the patch please?
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)
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?
(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. :(
Attachment #292910 - Attachment is obsolete: true
Where do we get the extra accessible objects if we just remove the \n rule and have only textLength == 0 ?
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
Basically in html script, if the author put any space or newline between two tags, there will be an extra accessible object.
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?

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
  }
}
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.
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.
Attached patch patch v2 (obsolete) — Splinter Review
Assignee: aaronleventhal → Evan.Yan
Status: NEW → ASSIGNED
Attachment #292921 - Flags: review?(aaronleventhal)
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.
GetNextInFlow() returns null for the space between two links. So I guess it's not reliable for testing useful whitespace.
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+
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.
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?
(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?
Attached patch fix invisible state (obsolete) — Splinter Review
the text node was invisible because it just some newline, and has zero area rect.

Marco, could you try this patch please?
(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.
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?
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.
Thanks Marco, I'll ask review for the later patch.
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)
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;
+      }
+    }
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 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+
Attachment #292921 - Attachment is obsolete: true
Attachment #293523 - Flags: review?(Evan.Yan) → review+
Attachment #293523 - Flags: approval1.9?
Attachment #293523 - Flags: approval1.9? → approval1.9+
patch checked in.
Status: ASSIGNED → RESOLVED
Closed: 13 years ago
Resolution: --- → FIXED
Depends on: 409473
Evan, unfortunately we're missing a check of some kind. This is causing bug 409473.
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.