Closed
Bug 405248
Opened 17 years ago
Closed 17 years ago
Space between embedded objects characters is missing
Categories
(Core :: Disability Access APIs, defect)
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)
342 bytes,
text/html
|
Details | |
3.12 KB,
patch
|
evan.yan
:
review+
mtschrep
:
approval1.9+
|
Details | Diff | Splinter Review |
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?
Comment 2•17 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.
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?
Reporter | ||
Comment 5•17 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
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•17 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()
(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•17 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•17 years ago
|
||
Marco, could you help test the patch please?
Assignee | ||
Comment 11•17 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•17 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•17 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. :(
Attachment #292910 -
Attachment is obsolete: true
Comment 14•17 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•17 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•17 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•17 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•17 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•17 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•17 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•17 years ago
|
||
Assignee: aaronleventhal → Evan.Yan
Status: NEW → ASSIGNED
Attachment #292921 -
Flags: review?(aaronleventhal)
Comment 22•17 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•17 years ago
|
||
GetNextInFlow() returns null for the space between two links. So I guess it's not reliable for testing useful whitespace.
Comment 24•17 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•17 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•17 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•17 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•17 years ago
|
||
the text node was invisible because it just some newline, and has zero area rect.
Marco, could you try this patch please?
Comment 29•17 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•17 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•17 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•17 years ago
|
||
Thanks Marco, I'll ask review for the later patch.
Assignee | ||
Comment 33•17 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•17 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•17 years ago
|
||
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•17 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•17 years ago
|
Attachment #292921 -
Attachment is obsolete: true
Attachment #293523 -
Flags: review?(Evan.Yan) → review+
Updated•17 years ago
|
Attachment #293523 -
Flags: approval1.9?
Updated•17 years ago
|
Attachment #293523 -
Flags: approval1.9? → approval1.9+
Assignee | ||
Comment 37•17 years ago
|
||
patch checked in.
Status: ASSIGNED → RESOLVED
Closed: 17 years ago
Resolution: --- → FIXED
Comment 38•17 years ago
|
||
Evan, unfortunately we're missing a check of some kind. This is causing bug 409473.
Comment 39•17 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.
Description
•