Last Comment Bug 340829 - New ATK: Implement nsHyperTextAccessible
: New ATK: Implement nsHyperTextAccessible
Status: RESOLVED FIXED
: access
Product: Core
Classification: Components
Component: Disability Access APIs (show other bugs)
: Trunk
: x86 All
: -- normal (vote)
: ---
Assigned To: Aaron Leventhal
:
:
Mentors:
: 237247 (view as bug list)
Depends on: 340822 341650 341745 351505
Blocks: 719047 312093 newatk 340661 340665 340667 340668 340671 340830 340833 342035 342045
  Show dependency treegraph
 
Reported: 2006-06-08 07:52 PDT by Aaron Leventhal
Modified: 2012-01-18 08:07 PST (History)
5 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Work in progress -- builds on Windows (63.39 KB, patch)
2006-06-14 13:48 PDT, Aaron Leventhal
no flags Details | Diff | Splinter Review
Still work in progress, with files add/removed (219.28 KB, patch)
2006-06-14 17:02 PDT, Aaron Leventhal
no flags Details | Diff | Splinter Review
Builds on Linux with some warnings (213.03 KB, patch)
2006-06-14 18:06 PDT, Aaron Leventhal
no flags Details | Diff | Splinter Review
Merged with trunk, warnings about selection methods should go away on Linux (216.49 KB, patch)
2006-06-15 11:47 PDT, Aaron Leventhal
no flags Details | Diff | Splinter Review
Builds on Linux again, warnings are gone (210.47 KB, patch)
2006-06-15 12:09 PDT, Aaron Leventhal
no flags Details | Diff | Splinter Review
Tree walking works on Windows again (214.03 KB, patch)
2006-06-15 13:59 PDT, Aaron Leventhal
no flags Details | Diff | Splinter Review
Initalizes mTextChildren lazily, although I don't know if we'll even need that array in the future (215.09 KB, patch)
2006-06-15 14:31 PDT, Aaron Leventhal
no flags Details | Diff | Splinter Review
1) Merged with trunk after ROLE_TEXT fix, 2) Removes ns*FormControlAccessibleWrap.* (82.99 KB, patch)
2006-06-16 10:42 PDT, Aaron Leventhal
no flags Details | Diff | Splinter Review
1) Use ROLE_SECTION, ROLE_FORM, ROLE_PARAGRAPH, ROLE_HEADING, 2) Removes GetIsEditable() and uses GetExtState() for that (229.08 KB, patch)
2006-06-16 12:18 PDT, Aaron Leventhal
no flags Details | Diff | Splinter Review
Impl GetText and GetCharacterCount (needs testing); Have not yet removed redundant ROLE_TEXT leaf nodes from ATK tree (229.58 KB, patch)
2006-06-16 13:51 PDT, Aaron Leventhal
no flags Details | Diff | Splinter Review
Implements nsIAccessibleHyperText, but not tested (229.58 KB, patch)
2006-06-16 18:02 PDT, Aaron Leventhal
no flags Details | Diff | Splinter Review
Implements nsIAccessibleHyperText, but not tested (221.83 KB, patch)
2006-06-16 18:05 PDT, Aaron Leventhal
no flags Details | Diff | Splinter Review
Implement nsIAccessibleHyperLink as well (221.26 KB, patch)
2006-06-16 18:23 PDT, Aaron Leventhal
no flags Details | Diff | Splinter Review
Fix QueryInterface() so HyperLink, HyperText, Text, EditableText are only supported when they should be (222.53 KB, patch)
2006-06-16 18:42 PDT, Aaron Leventhal
no flags Details | Diff | Splinter Review
Impl GetCharacterExtents() and GetOffsetAtPoint() (228.44 KB, patch)
2006-06-17 20:53 PDT, Aaron Leventhal
no flags Details | Diff | Splinter Review
Work on GetAttributeRange() and readability of code (231.06 KB, patch)
2006-06-18 19:21 PDT, Aaron Leventhal
no flags Details | Diff | Splinter Review
Attempt to fix compiler errors/warnings (233.33 KB, patch)
2006-06-19 06:34 PDT, Aaron Leventhal
no flags Details | Diff | Splinter Review
Fixes compiler errors/warnings for real (226.81 KB, patch)
2006-06-19 07:14 PDT, Aaron Leventhal
no flags Details | Diff | Splinter Review
When exposing to ATK, pretend the nsIAccessible::ROLE_TEXT leaf accessible don't exist (236.41 KB, patch)
2006-06-19 11:51 PDT, Aaron Leventhal
no flags Details | Diff | Splinter Review
Exposes text and children correctly (229.83 KB, patch)
2006-06-19 12:49 PDT, Aaron Leventhal
no flags Details | Diff | Splinter Review
1) Separate nsIAccessible::ROLE_TEXT_CONTAINER vs. ROLE_TEXT_LEAF, 2) Convenience method HasRole(), 3) Remove HTML_CONTAINER_ACCESSIBLE logic (239.53 KB, patch)
2006-06-19 19:53 PDT, Aaron Leventhal
no flags Details | Diff | Splinter Review
Builds on Linux again, contains Ginn's suggestions for text roles (232.93 KB, patch)
2006-06-19 21:01 PDT, Aaron Leventhal
no flags Details | Diff | Splinter Review
Fixes ROLE_STATICTEXT issues (243.79 KB, patch)
2006-06-20 08:35 PDT, Aaron Leventhal
no flags Details | Diff | Splinter Review
Builds on Linux, and has probable crash fix (236.93 KB, patch)
2006-06-20 09:00 PDT, Aaron Leventhal
ginn.chen: review+
Details | Diff | Splinter Review

Description Aaron Leventhal 2006-06-08 07:52:14 PDT
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.
Comment 1 Aaron Leventhal 2006-06-08 08:47:58 PDT
The text exposed with nsIAccessibleText needs to provide unicode embedded object placeholding characters where children are inserted in the text. The character is 0xfffc.
Comment 2 Aaron Leventhal 2006-06-13 06:52:31 PDT
Starting working up a rather large patch for this :)
Comment 3 Aaron Leventhal 2006-06-14 13:48:38 PDT
Created attachment 225614 [details] [diff] [review]
Work in progress -- builds on Windows
Comment 4 Aaron Leventhal 2006-06-14 17:02:03 PDT
Created attachment 225646 [details] [diff] [review]
Still work in progress, with files add/removed
Comment 5 Aaron Leventhal 2006-06-14 18:06:36 PDT
Created attachment 225658 [details] [diff] [review]
Builds on Linux with some warnings
Comment 6 Aaron Leventhal 2006-06-14 18:19:26 PDT
Next steps:
- Fix windows regression wrt walking into children
- Make text interfaces work with ATK
- Address XXX comments or file bugs for them
Comment 7 Aaron Leventhal 2006-06-15 07:12:04 PDT
Changing name from nsHTMLContainerAccessible to nsHyperTextAccessible
Comment 8 Aaron Leventhal 2006-06-15 11:47:04 PDT
Created attachment 225740 [details] [diff] [review]
Merged with trunk, warnings about selection methods should go away on Linux
Comment 9 Aaron Leventhal 2006-06-15 12:09:33 PDT
Created attachment 225744 [details] [diff] [review]
Builds on Linux again, warnings are gone
Comment 10 Aaron Leventhal 2006-06-15 13:59:08 PDT
Created attachment 225768 [details] [diff] [review]
Tree walking works on Windows again
Comment 11 Aaron Leventhal 2006-06-15 14:31:58 PDT
Created attachment 225773 [details] [diff] [review]
Initalizes mTextChildren lazily, although I don't know if we'll even need that array in the future
Comment 12 Aaron Leventhal 2006-06-15 16:49:30 PDT
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.
Comment 13 Aaron Leventhal 2006-06-16 10:42:03 PDT
Created attachment 225886 [details] [diff] [review]
1) Merged with trunk after ROLE_TEXT fix, 2) Removes ns*FormControlAccessibleWrap.*
Comment 14 Aaron Leventhal 2006-06-16 12:18:12 PDT
Created attachment 225899 [details] [diff] [review]
1) Use ROLE_SECTION, ROLE_FORM, ROLE_PARAGRAPH, ROLE_HEADING, 2) Removes GetIsEditable() and uses GetExtState() for that
Comment 15 Aaron Leventhal 2006-06-16 13:34:40 PDT
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.*
Comment 16 Aaron Leventhal 2006-06-16 13:51:27 PDT
Created attachment 225912 [details] [diff] [review]
Impl GetText and GetCharacterCount (needs testing); Have not yet removed redundant ROLE_TEXT leaf nodes from ATK tree
Comment 17 Aaron Leventhal 2006-06-16 18:02:35 PDT
Created attachment 225945 [details] [diff] [review]
Implements nsIAccessibleHyperText, but not tested
Comment 18 Aaron Leventhal 2006-06-16 18:05:29 PDT
Created attachment 225946 [details] [diff] [review]
Implements nsIAccessibleHyperText, but not tested
Comment 19 Aaron Leventhal 2006-06-16 18:23:09 PDT
Created attachment 225948 [details] [diff] [review]
Implement nsIAccessibleHyperLink as well
Comment 20 Aaron Leventhal 2006-06-16 18:42:00 PDT
Created attachment 225950 [details] [diff] [review]
Fix QueryInterface() so HyperLink, HyperText, Text, EditableText are only supported when they should be
Comment 21 Aaron Leventhal 2006-06-16 18:43:13 PDT
Still not implemented: 
- the more complex text routines
- caret/selection support
- editable text
- stripping out extra ROLE_TEXT objects when exposing via ATK
Comment 22 Aaron Leventhal 2006-06-17 20:53:52 PDT
Created attachment 226037 [details] [diff] [review]
Impl GetCharacterExtents() and GetOffsetAtPoint()
Comment 23 Aaron Leventhal 2006-06-18 19:21:00 PDT
Created attachment 226099 [details] [diff] [review]
Work on GetAttributeRange() and readability of code
Comment 24 Ginn Chen 2006-06-18 23:32:29 PDT
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
Comment 25 Ginn Chen 2006-06-19 04:17:52 PDT
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
Comment 26 Aaron Leventhal 2006-06-19 06:34:37 PDT
Created attachment 226143 [details] [diff] [review]
Attempt to fix compiler errors/warnings
Comment 27 Aaron Leventhal 2006-06-19 07:14:54 PDT
Created attachment 226147 [details] [diff] [review]
Fixes compiler errors/warnings for real
Comment 28 Aaron Leventhal 2006-06-19 08:17:48 PDT
- 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
Comment 29 Aaron Leventhal 2006-06-19 11:51:05 PDT
Created attachment 226184 [details] [diff] [review]
When exposing to ATK, pretend the nsIAccessible::ROLE_TEXT leaf accessible don't exist
Comment 30 Aaron Leventhal 2006-06-19 12:49:09 PDT
Created attachment 226196 [details] [diff] [review]
Exposes text and children correctly

Still to do:
1) the more complex text routines, GetTextBefore/After, etc.
2) caret/selection support
3) Textfields and textareas
4) editable text
Comment 31 Aaron Leventhal 2006-06-19 19:53:00 PDT
Created 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
Comment 32 Aaron Leventhal 2006-06-19 20:46:44 PDT
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.
Comment 33 Aaron Leventhal 2006-06-19 21:01:21 PDT
Created attachment 226272 [details] [diff] [review]
Builds on Linux again, contains Ginn's suggestions for text roles
Comment 34 Ginn Chen 2006-06-20 04:41:51 PDT
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?
Comment 35 Aaron Leventhal 2006-06-20 08:21:27 PDT
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.
Comment 36 Aaron Leventhal 2006-06-20 08:35:44 PDT
Created attachment 226336 [details] [diff] [review]
Fixes ROLE_STATICTEXT issues
Comment 37 Aaron Leventhal 2006-06-20 09:00:09 PDT
Created attachment 226339 [details] [diff] [review]
 Builds on Linux, and has probable crash fix

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.
Comment 38 Håkan Waara 2006-06-20 09:24:17 PDT
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 39 Ginn Chen 2006-06-21 00:24:02 PDT
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;
Comment 40 Aaron Leventhal 2006-06-29 21:02:23 PDT
*** Bug 237247 has been marked as a duplicate of this bug. ***
Comment 41 Boris Zbarsky [:bz] (still a bit busy) 2006-09-05 20:36:02 PDT
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.

Note You need to log in before you can comment on or make changes to this bug.