Closed Bug 340829 Opened 18 years ago Closed 18 years ago

New ATK: Implement nsHyperTextAccessible

Categories

(Core :: Disability Access APIs, defect)

x86
All
defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: aaronlev, Assigned: aaronlev)

References

(Blocks 1 open bug)

Details

(Keywords: access)

Attachments

(1 file, 23 obsolete files)

236.93 KB, patch
ginnchen+exoracle
: review+
Details | Diff | Splinter Review
nsHTMLContainerAccessible will implement nsIAccessibleText and nsIAccessibleHyperText

nsAccessibleWrap
     |
nsHTMLContainerAccessible
     |               \
nsBlockAccessible    nsLinkableAccessible
     |
nsDocAccessible

The QI to nsIAccessibleHyperText does not need to succeed in all cases. 

An object with children is a hypertext when it:
1) contains text, and thus the position of the children within that text must be exposed, or 
2) the child contents could be broken into several lines with multiple children, and thus the line breaks must be exposed a start and end offset which contains several children (we may simplify this)

Any HTML container of display:block will need to use this class (example, typically headings and paragraphs are styled that way). See bug 340671 for the bug on assigning the correct roles to thos objects.
Summary: Implement nsHTMLContainerAccessible → New ATK: Implement nsHTMLContainerAccessible
The text exposed with nsIAccessibleText needs to provide unicode embedded object placeholding characters where children are inserted in the text. The character is 0xfffc.
Starting working up a rather large patch for this :)
Status: NEW → ASSIGNED
Blocks: 340668
Blocks: 340833
Blocks: 340830
Blocks: 340671
Blocks: 340667
Blocks: 340665
Blocks: 340661
Blocks: 312093
Attachment #225614 - Attachment is obsolete: true
Attachment #225646 - Attachment is obsolete: true
Next steps:
- Fix windows regression wrt walking into children
- Make text interfaces work with ATK
- Address XXX comments or file bugs for them
Changing name from nsHTMLContainerAccessible to nsHyperTextAccessible
Summary: New ATK: Implement nsHTMLContainerAccessible → New ATK: Implement nsHyperTextAccessible
Attachment #225658 - Attachment is obsolete: true
Depends on: 341650
Attachment #225740 - Attachment is obsolete: true
Attachment #225744 - Attachment is obsolete: true
Ginn, I'm getting ready to start fixing the nsIAccessibleText impls in nsHyperTextAccessible. One question though: how much should I do before we try to get this in? Once we get it in everyone can work on it together. On the other hand, you may want me to prove this works and is the right approach.
Depends on: 341745
Comment on attachment 225899 [details] [diff] [review]
1) Use ROLE_SECTION, ROLE_FORM, ROLE_PARAGRAPH, ROLE_HEADING, 2) Removes GetIsEditable() and uses GetExtState() for that

This patch adds:
nsHyperTextAccessible.*

This patch removes:
nsHTMLBlockAccessible.*
nsAccessibleText.*
nsAccessibleHyperText.*
nsHTMLFormControlAccessibleWrap.*
nsXULFormCOntrolAccessibleWrap.*
Attachment #225899 - Attachment is obsolete: true
Attachment #225912 - Attachment is obsolete: true
Attachment #225945 - Attachment is obsolete: true
Attachment #225946 - Attachment is obsolete: true
Still not implemented: 
- the more complex text routines
- caret/selection support
- editable text
- stripping out extra ROLE_TEXT objects when exposing via ATK
Attachment #225950 - Attachment is obsolete: true
Attachment #226037 - Attachment is obsolete: true
For compiling,
1) we need to fix the multi-character character constant warning, it's annoying and not portable
2) src/atk/nsAccessibleWrap.cpp:553 ATK_STATE_VISITED misspelled
3) src/atk/Makefile.in still has nsHTMLFormControlAccessibleWrap.cpp and nsXULFormControlAccessibleWrap.cpp
4) nsAccessibilityService.cpp still uses nsHTMLRadioButtonAccessibleWrap, nsHTMLTextFieldAccessibleWrap, nsXULTextFieldAccessibleWrap
The code changes are reasonable to me.

I met several issues while using at-poke.
1) I got STATE_BUSY for ROLE_HYPER_LINK, it's triggered by setting STATE_TEXT_SELECTABLE.
Maybe it's a compiling issue.
I updated my at-spi, atk, at-poke to HEAD, but didn't help.

2) With google.com.
The accessible tree is
scroll pane->pane->document frame (it was html container), this is fine.
For table, I got,
table->html container->table cell
I don't know if it is what we want.

3) links are not implemented AtkHyperlink interface yet

4) click the root accessible (the first level under role_application) will crash.
stack:
#6  0xb7db937e in nsAString_internal::Length (this=0x0) at nsTAString.cpp:66
#7  0xb64a91e8 in nsAString_internal::Truncate (this=0x0, aNewLength=0)
    at nsTAString.h:379
#8  0xb6a016cd in nsXULElement::GetAttr (this=0x86d2298, aNameSpaceID=0,
    aName=0x80acdf8, aResult=@0x0) at nsXULElement.cpp:1288
#9  0xb52a97e8 in nsXULTextAccessible::GetName (this=0x8a5b160, aName=@0x0)
    at nsXULTextAccessible.cpp:63
#10 0xb7db96e4 in nsAString_internal::SetLength (this=0x8a5b17c, size=0)
    at nsTAString.cpp:222
#11 0xb7d016ff in nsAString_internal::Truncate (this=0x8a5b17c, aNewLength=0)
    at nsTAString.h:380
#12 0xb7dad0ff in CopyUTF8toUTF16 (aSource=@0xbfc57494, aDest=@0x8a5b17c)
    at nsReadableUtils.cpp:92
#13 0xb7d0c7e1 in AtomImpl::ToString (this=0x84d1200, aBuf=@0x8a5b17c)
    at nsAtomTable.cpp:477
#14 0xb67415aa in nsNodeInfo::GetQualifiedName (this=0x86b5b18,
    aQualifiedName=@0x8a5b17c) at nsNodeInfo.cpp:130
#15 0xb672856b in nsGenericElement::GetNodeName (this=0x86d2298,
    aNodeName=@0x8a5b17c) at nsGenericElement.cpp:1063
#16 0xb6a09076 in nsXULElement::GetNodeName (this=0x86d2298,
    aNodeName=@0x8a5b17c) at nsXULElement.h:552
#17 0xb527a211 in nsHyperTextAccessible::GetStartPosAndText (this=0x8a3dea0,
    aStartOffset=@0xbfc576d4, aEndOffset=-1, aText=@0xbfc576ec)
    at nsHyperTextAccessible.cpp:241
#18 0xb527a494 in nsHyperTextAccessible::GetText (this=0x8a3dea0,
    aStartOffset=0, aEndOffset=-1, aText=@0xbfc576ec)
    at nsHyperTextAccessible.cpp:297
#19 0xb5292053 in getTextCB (aText=0x8a56c28, aStartOffset=0, aEndOffset=-1)
    at nsMaiInterfaceText.cpp:97
#20 0xb79a27e6 in atk_text_get_text (text=0x8a56c28, start_offset=0,
    end_offset=-1) at atktext.c:187
Attachment #226099 - Attachment is obsolete: true
- Not sure what to do with STATE_BUSY issue, maybe we can file a separate bug.
- Removed extra HTML_CONTAINER objects from table.
- Links are now implemented
- Did not get crash
Blocks: 342035
Blocks: 342045
Attachment #226143 - Attachment is obsolete: true
Attachment #226147 - Attachment is obsolete: true
Still to do:
1) the more complex text routines, GetTextBefore/After, etc.
2) caret/selection support
3) Textfields and textareas
4) editable text
Attachment #226184 - Attachment is obsolete: true
Attachment #226196 - Flags: review?(ginn.chen)
Attachment #226265 - Flags: review? → review?(ginn.chen)
Comment on attachment 226265 [details] [diff] [review]
1) Separate nsIAccessible::ROLE_TEXT_CONTAINER vs. ROLE_TEXT_LEAF, 2) Convenience method HasRole(), 3) Remove HTML_CONTAINER_ACCESSIBLE logic

I accidentally didn't keep the previous patches changes, so I need to merge the changes from those 2.
Attachment #226265 - Attachment is obsolete: true
Attachment #226265 - Flags: review?(ginn.chen) → review?
Attachment #226272 - Flags: review?(ginn.chen)
Comment on attachment 226272 [details] [diff] [review]
Builds on Linux again, contains Ginn's suggestions for text roles

great work, links are better exposed now, and the table issue is solved.

Here is my comments:
typo,
+    PRInt32 count - 9;
and
+      // We dont' create ATK objects for nsIAccessible plain text leave

STATE_BUSY issue is a bug in atk's code
I filed a bug in gnome bugzilla.
http://bugzilla.gnome.org/show_bug.cgi?id=345407

The crash issue is still there.
The AtkObject with ROLE_FRAME implemented AtkText interface.
at-poke tried to get_text and caused crash.
I also met a crash with the show_bug page.
There are
....
<table> (first line under the banner)
<Search page>
<Enter new bug>
<separator>
<form>
<separator>
<Search page>
<Enter new bug>
<form>
click the <form> between 2 separators will cause a crash. (still from getTextCB)

GOK won't dig into an accessible which implements Text, Hypertext or Editabletext.
(it's not only the problem of DOCUMENT_FRAME or HTML_CONTAINER, everything implements nsIAccessibleHyperText now)
So GOK will not work with HTML content.
I'm not sure we should solve it in Firefox or GOK.
We can file another bug to follow up.


A question: why do we change ROLE_STATICTEXT in last patch?
Ginn,

Bill Haneman says we can fix the Hypertext issue in GOK. I filed a bug on GOK for that: http://bugzilla.gnome.org/show_bug.cgi?id=345429

> A question: why do we change ROLE_STATICTEXT in last patch?
ROLE_STATICTEXT used to sometimes mean <label>, because that's something IE uses for <label>. However, STATICTEXT also indicates text which was inserted by the layout engine (usually via :before, :after or <ol> numbers). I've updated the patch to separate these 2 things from each other. From now on, ROLE_LABEL will be for <label>, which is a container. ROLE_STATICTEXT will be for layout engine text. The layout engine text is always leaf text and should be stripped from the tree just like ROLE_TEXT_LEAF is.
Attached patch Fixes ROLE_STATICTEXT issues (obsolete) — Splinter Review
Attachment #226272 - Attachment is obsolete: true
Attachment #226272 - Flags: review?(ginn.chen)
Attachment #226265 - Flags: review?
The crash does not happen for me, but I think the ROLE_STATICTEXT changes actually fixed it. The crash could have occured because for ROLE_STATICTEXT objects, GetText() was trying to get nsITextContent, and things like <label> and the combo box test field were role static text, but the DOM Node is not nsITextContent. It probably fired the assertion right before the crash.
Attachment #226339 - Flags: review?(ginn.chen)
Attachment #226336 - Attachment is obsolete: true
Comment on attachment 226339 [details] [diff] [review]
 Builds on Linux, and has probable crash fix

Just patched my tree with this, and it compiles fine (OS X).
Comment on attachment 226339 [details] [diff] [review]
 Builds on Linux, and has probable crash fix

Great job!
And it doesn't crash.

Nit:
first hunk of src/atk/nsAccessibleWrap.cp
// We don't

nsHyperTextAccessible::SetCaretOffset
move this return into #if 0
+  }
+#endif
+  return NS_ERROR_INVALID_ARG;
Attachment #226339 - Flags: review?(ginn.chen) → review+
Status: ASSIGNED → RESOLVED
Closed: 18 years ago
Resolution: --- → FIXED
*** Bug 237247 has been marked as a duplicate of this bug. ***
Why didn't this patch change the nsIAccesibleDocument IID?  And the same for all other interfaces it changed?

If you guys want to operate without sr, please do not make the sort of mistakes that sr would prevent.  See the "Newbie request for guidance" thread in m.d.t.xpcom for the sort of havoc that this mistake can cause.
Depends on: 351505
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: