345759 - Spell as you type not accessible

Status: VERIFIED FIXED

(Core :: Disability Access APIs, defect, P2)

Description

[Wayne DeAngelo]

Test environment:  Bon Echo nightly build 20070723, JAWS or WE.
Test done on IBM T42 ThinkPad, with Screen Resolution 1024x768 pixels using High Contrast #1 (extra large).  

Description:  
The spell-as-you-type function is not accessible to blind users.  Neither Window-Eye or JAWS gives any indication when a misspelled word is underlined by Firefox.  Nor does Inspect Objects show any MSAA information concerning the misspelling.

Steps to Reproduce:
1. Open any page with an input form textarea, such as Questions/Comments area on this page: http://store.mozilla.org/help.php
2. Type a misspelled word into the text area.  You will see a dotted red line under the misspelled word.  (This is very cool, by the way.)
3. Using Inspect Objects (or your favorite MSAA object inspection tool), point at the misspelled word. Note that there is no indication of a misspelling.  (Granted, this is not very well architected, and there may be no way of doing this.)
4. Use a screen reader (Window-Eyes or JAWS) to read the text area.  Note that the screen reader does not give any indication that the word is underlined.

Expected Result:
To be accessible, any information displayed visually by an application must have a corresponding method of presenting the same information to a blind user.  As nice as this feature is, if there is no way for a blind user to access this information, this particular feature might not be accessible.

Comment 1

[Aaron Leventhal]

Unless we do something such as add a UI to the spell checker, this is going to have to be a negative point on our VPAT. We need some way to navigate to the next error.

There is no way to fix this just for screen readeers in Firefox 2, because we'll need to support new accessibility APIs in order to handle it that way.

Comment 2

[Mike Connor [:mconnor]]

Per Aaron/Mark's comments, can't block on something we can't fix.  Targeting for Fx3 a2, hopefully that's far enough out to get a solution in place.

Comment 3

[Aaron Leventhal]

Does the red underlined bit have have it's own DOM Node? If so, expose via nsHTMLTextAccessible::GetAttributes() for Midas text areas. In that case, for text boxes where spell checking is on (it can be on for single line textfields if the spellcheck attribute is used), it can also be done via nsHTMLTextAccessible.

However for Xforms inputs, if they implement nsIAccessibleText separately, they'll need to expose these attribute runs.

In any case, let's call it misspelled=true for those ranges which are incorrect.

Comment 4

[Aaron Leventhal]

The red squiggly line is implemented in Gecko as a type of selection.

frame->GetSelectionController(presContext, getter_AddRefs(selCon));
selCon->GetSelection(nsISelectionController::SELECTION_SPELLCHECK, 
                     getter_AddRefs(domSel));

Comment 5

[Aaron Leventhal]

nsHyperTextAccessible might want to use nsISelection2 to get the ranges of misspelled words for a given interval.

Comment 6

[Ginn Chen]

I think we need to change the "GetAttributeRange" API.
It doesn't return the Attributes set like getRunAttributesCB does.

GetAttributeSet() doesn't have offset parameters.

We want to get attributes set of a11y object, around a certain offset.

Comment 7

[Aaron Leventhal]

I agree with all of Ginn's statement here:

(In reply to comment #6)
> I think we need to change the "GetAttributeRange" API.
> It doesn't return the Attributes set like getRunAttributesCB does.
> 
> GetAttributeSet() doesn't have offset parameters.
> 
> We want to get attributes set of a11y object, around a certain offset.

Comment 8

[Ginn Chen]

Attachment: draft patch

A draft patch

I don't know what attributes should be used to misspelled words.
As Bill Haneman suggested, we should include both typographic attr and semantic attr.
(Maybe we also want underline-style:dotted.)

I've tested this patch with multiline input and MIDAS.

Comment 9

[Aaron Leventhal]

Comment on attachment 247167 [details] [diff] [review]
draft patch

+   if (aOffset < hypertextStartOffset)
            break;
First, are we guaranteed the misspellings in the selection range always be in order? That would be great, but we better make sure, and comment on that.
Second, let's say I have some text like |A gooqbye"| with an attribute range from 2 to the end with "misspelled=true" 
Doesn't that mean if I ask for the attribute range for offset 0 that i should get a range from 0 to 1 for it? 
I don't think your code deals with that.

Please check the if NS_FAILED(DOMPointToOffset())

+  if (hypertextStartOffset <= aOffset && aOffset <=hypertextEndOffset) {
There is a space missing in the 2nd <=

In general the new code could use more comments. Similarly, perhaps you should rename |hypertextStartOffset| to |misspellingStartOffset| to make it clearer what that does.

+NS_IMETHODIMP nsHyperTextAccessible::GetAttributeRange(PRInt32 aOffset,
+                                         PRInt32 *aRangeStartOffset, 
+                                         PRInt32 *aRangeEndOffset,
+                                         nsIPersistentProperties **aAttributes,
+                                         nsIAccessible **aAccessibleWithAttrs)
Whitespace is off a bit.

+            (*aAttributes)->SetStringProperty(NS_LITERAL_CSTRING("underline"), NS_LITERAL_STRING("true"), oldValueUnused);
We're already going to be using text-decoration: underline, and it would be confusing to use this as well.
Because there could be conflicting underlines (there could be a CSS underline plus this underline), I'm going to respectfully disagree with Bill this time and say we only need to expose this as misspelled=true.
Otherwise you will have to write a lot of code, which isn't really needed, to deal with when there are 2 types of underline with different styles and colors.
The semantic attribute "misspelled=true" should be enough.

Otherwise looks very good.

Comment 10

[Aaron Leventhal]

Comment on attachment 247167 [details] [diff] [review]
draft patch

Sorry, meant to minus that until those issues are dealt with.

Comment 11

[Aaron Leventhal]

Some more things:

   /**
-   * Get the accessible and start/end offsets around the given offset.
+   * Get the accessible, start/end offsets and the attributes
+   * around the given offset.
    * This accessible get return the DOM node and layout frame
-   * with the uniform attributes for this range of text
+   * for this range of text.
    */
   nsIAccessible getAttributeRange (in long offset,
                                    out long rangeStartOffset,
-                                   out long rangeEndOffset);
+                                   out long rangeEndOffset,
+                                   out nsIPersistentProperties attributes);
 
You can return the nsIPersistantProperties and no longer return nsIAccessible anymore, it's no longer needed. The properties are being reported directly now.

Also, can you fix the case BOUNDARY_ATTRIBUTE_RANGE in ::GetTextHelper()? I think you can just call into GetAttributeRange to make that easier, correct? You can also file a separate bug on that if you wish.

Comment 12

[Ginn Chen]

Attachment: patch v2

Comments addressed.

Comment 13

[alexander :surkov (:asurkov)]

Comment on attachment 269157 [details] [diff] [review]
patch v2

getAttributeRange() is used in IA2 (CAccessibleText.cpp). It looks you have not changes there in your patch.

Comment 14

[Aaron Leventhal]

Ginn, Can you remove this comment in nsIAccessibleText? It looks like you're addressing the comment through this patch so we don't need the comment anymore.

182 
183  getRangeAttributes defined to return an nsISupports
184  interface instead of a pango specific data structure.
185  It may be that some other return type is more appropriate
186  for mozilla text attributes.

Comment 15

[Aaron Leventhal]

Nit: this can be rewritten so length is only used once:

     if (*aRangeStartOffset + length > aOffset) {
       *aRangeEndOffset = *aRangeStartOffset + length;
-      NS_ADDREF(*aAccessibleWithAttrs = accessible);
-      return NS_OK;
+      break;
     }
     *aRangeStartOffset += length;
   }
 
How about:

     *aRangeStartOffset += length;
     if (*aRangeStartOffset > aOffset) {
       break;
     }
   }

Comment 16

[Aaron Leventhal]

We are doing a lot of extra work when we call GetState() to find out if something is editable. 
This is a problem in both nsHyperTextAccessible::QueryInterfaces() and in this new GetAttributeRange().

Also, what happens when we're editing a paragraph inside of a document that is editable? It looks like STATE_EDITABLE/nsIAccessibleEditableeText is only on the document itself, but it also be on all of the descendant hyper text accessible objects. I'm not sure if GetEditor() should work on the editable descendants of a document, but currently it will not.

+  PRUint32 state = 0, extState = 0;
+  GetState(&state, &extState);
+  if (extState & nsIAccessibleStates::EXT_STATE_EDITABLE) {

This will not do anything with spelling errors in a descendant hyper text accessible inside of a document.

Comment 17

[Aaron Leventhal]

If you are going to do GetEditor() anyway then you probably don't need to check GetState(). You could just make sure it's not a readonly editor.

Comment 18

[Ginn Chen]

Yes, we need extra work for descendant hyper text accessible.
:(

Comment 19

[Ginn Chen]

split bug 386026 for easier code review

Comment 20

[alexander :surkov (:asurkov)]

Attachment: wip

Comment 21

[alexander :surkov (:asurkov)]

Attachment: patch

Comment 22

[Ginn Chen]

Looks good.

But I think these comments are not addressed.

comment #9
> Second, let's say I have some text like |A gooqbye"| with an attribute range
> from 2 to the end with "misspelled=true" 
 > Doesn't that mean if I ask for the attribute range for offset 0 that i should
> get a range from 0 to 1 for it? 

Current patch returns -1 for both start and end offset in this case.

and comment #14

Comment 23

[Aaron Leventhal]

Comment on attachment 289303 [details] [diff] [review]
patch

ConvertToAtkAttributeSet():
rv is set but not used several times (I realize you're just copying that error, but please clean up any code you copy to a new place).

WHen getting the SELECTION_SPELLCHECK I suggest you either use GetRangesForIntervalCOMArray() directly or reuuse nsHyperTextAccessible::GetSelections() and add a new parameter for the selection type (which could default to SELECTION_NORMAL but allow you to pass SELECTION_SPELLCHECK in).
There is a reasonable perf advantage in doing this -- you will only need to go through the spell check errors within the range of the current hypertext).
                                              I suggest moving the spell check text run calculation into a helper method, because you will later need to mix those ranges in with other ranges from text formatting etc.

Also, please address the other comments from Ginn and before.

On the wiki we have now specified a bit better how the text attribute runs will work -- not nested, and every character is in exactly 1 attribute run.

http://wiki.mozilla.org/Accessibility/TextAttributes

Comment 24

[alexander :surkov (:asurkov)]

Attachment: wip2

Comment 25

[alexander :surkov (:asurkov)]

Attachment: patch2

ready for review

Comment 26

[alexander :surkov (:asurkov)]

Attachment: wip3

+css text attributes

Comment 27

[alexander :surkov (:asurkov)]

Attachment: patch3

Comment 28

[alexander :surkov (:asurkov)]

Attachment: wip4

+misspelling text attribute change event

Comment 29

[alexander :surkov (:asurkov)]

Attachment: patch4

Comment 30

[Robert O'Callahan (:roc) (email my personal email if necessary)]

+   * Returns a type

Returns *the* type

+   * Returns a DOM node

Returns *the* DOM node

And please document exactly what this DOM node means. Is it supposed to be a DOM node that contains every range in the selection? Because that doesn't seem to be what the code is computing.

Comment 31

[alexander :surkov (:asurkov)]

(In reply to comment #30)

> And please document exactly what this DOM node means. Is it supposed to be a
> DOM node that contains every range in the selection?

Yes, it should return a node containing every range.

> Because that doesn't seem
> to be what the code is computing.
> 

Can you show me a problem?

Comment 32

[Robert O'Callahan (:roc) (email my personal email if necessary)]

+    if (!currDOMNode) {
+      currDOMNode = nextDOMNode;
+    } else {
+      nsCOMPtr<nsINode> currNode(do_QueryInterface(currDOMNode));
+      nsCOMPtr<nsINode> nextNode(do_QueryInterface(nextDOMNode));
+      PRBool isDescendant =
+        nsContentUtils::ContentIsDescendantOf(currNode, nextNode);
+      if (isDescendant)
+        currDOMNode = nextDOMNode;
+    }

In the else case, you need to set currDOMNode to some common ancestor of currDOMNode and nextDOMNode. For example, currently if currDOMNode is a sibling of nextDOMNode, you just don't do anything so you're just forgetting about nextDOMNode.

Comment 33

[Aaron Leventhal]

We also need to listen for spell check selection changes in SetControlSelectionListener, otherwise we won't get spell check selection change events for textarea's.

I don't think we want GetContainerNode() for the entire selection. The container for the entire selection is not relevant.
Ultimately want the accessible text container for the focus node for the range that changed.
For example, if we have the following:
<body>
  <h1>The Stateq of Politics</h1>
          ~~~~~~
  <p>Our politicians are evil! Helpq! What shall we do?</p>
                               ~~~~~
</body>

As the user types the "!" after Helpq, the spell check selection changes, but we only care about the addition of the range (we would also care about the remove of a range).
We care that, now this range is misspelled, or now it's no longer misspelled. We need to fire an event for the paragraph text accessible, and the spell check selection on the heading
should have nothing to do with it.

Comment 34

[alexander :surkov (:asurkov)]

(In reply to comment #33)
> We also need to listen for spell check selection changes in
> SetControlSelectionListener, otherwise we won't get spell check selection
> change events for textarea's.
> 
> I don't think we want GetContainerNode() for the entire selection. The
> container for the entire selection is not relevant.
> Ultimately want the accessible text container for the focus node for the range
> that changed.

That won't work if I enable option 'Spellcheck as you type' I guess.

Comment 35

[Aaron Leventhal]

A major use case for the attributes changed event is 'Spellcheck as you type'. That way, a screen reader user will hear when they have mistyped a word.

Comment 36

[alexander :surkov (:asurkov)]

(In reply to comment #35)
> A major use case for the attributes changed event is 'Spellcheck as you type'.
> That way, a screen reader user will hear when they have mistyped a word.
> 

they will hear in any way (does it make a difference which accessible for?). Just if I will use focused range to notify about selection change then they will hear nothing if they will switch that option on.

Comment 37

[Aaron Leventhal]

It will be very hard for the AT to figure out what now is misspelled or not. The AT would need to cache all of the ranges for an entire rich text editing range and then do a diff on those. They'd like to get the event on the deepest accessible that covers the entire change.

Comment 38

[alexander :surkov (:asurkov)]

(In reply to comment #37)
> It will be very hard for the AT to figure out what now is misspelled or not.
> The AT would need to cache all of the ranges for an entire rich text editing
> range and then do a diff on those. They'd like to get the event on the deepest
> accessible that covers the entire change.
> 

Is it ok for meantime implementation to skip events when 'enable spell as you type' option is turned on/off (because editor window may not be focused at all)?

Comment 39

[alexander :surkov (:asurkov)]

Attachment: patch5

fixed roc's comments, aaron's first comment, updated to trunk

Comment 40

[Aaron Leventhal]

> Is it ok for meantime implementation to skip events when 'enable spell as you
> type' option is turned on/off 

That's reasonable -- the use case where care about is that screen reader users need to be notified when something changes in what they are editing.

Comment 41

[Aaron Leventhal]

What about the second comment? Ultimately we need events to be fired on the closest accessible that contains the spell check change.

If I'm editing a rich text document, and as I type something becomes misspelled, the event should be fired for the accessible text containing what became misspelled.

Comment 42

[Aaron Leventhal]

Comment on attachment 290685 [details] [diff] [review]
patch5

Clearing request until last question answered.

Comment 43

[alexander :surkov (:asurkov)]

Attachment: patch6

Comment 44

[Aaron Leventhal]

Comment on attachment 291642 [details] [diff] [review]
patch6

This is quite large and Roc is very busy with layout stuff for the release. Let's wait to request Roc's sr+ until after Ginn & I have already reviewed and think it's ready.

Comment 45

[alexander :surkov (:asurkov)]

Should I remove 'line-number' code from the patch before you will start the review or will I do it with fixes for another your comments?

Comment 46

[Aaron Leventhal]

It would be great if you remove line-number. I'm looking at the patch today, but my first comment is that we need to factor some code out of nsHyperTextAccessible.cpp. It's getting too big.

If I see you on IRC over the next days we can chat about this patch. It's so large that I think it's better if I can just ask you questions directly.

One nit:

+  nsCOMPtr<nsIDOMElement> elm(nsAccUtils::GetDOMElementFor(node, nodeOffset));
+  NS_IF_ADDREF(*aElm = elm);

This could just be

*aElm = nsAccUtils::GetDOMElementFor(node, nodeOffset) since that will addref for you.

Comment 47

[alexander :surkov (:asurkov)]

(In reply to comment #46)

> *aElm = nsAccUtils::GetDOMElementFor(node, nodeOffset) since that will addref
> for you.
> 

addrefed can't be converted to pointer directly

Comment 48

[alexander :surkov (:asurkov)]

Attachment: patch

Aaron, Ginn please put your thoughts about code organization?

Comment 49

[Ginn Chen]

For mozilla/accessible/src/atk/nsMaiInterfaceText.cpp

+    nsresult rv = accText->GetTextAttributes(PR_FALSE, aOffset,
+                                             &startOffset, &endOffset,
+                                             getter_AddRefs(attributes));
+    NS_ENSURE_SUCCESS(rv, nsnull);
+
     *aStartOffset = startOffset;
     *aEndOffset = endOffset;
-    if (NS_FAILED(rv))
-        return nsnull;

I don't think this change is good.
If accText->GetTextAttributes, we'd better set *aStartOffset, *aEndOffset to 0 or -1 rather than keep it as random number.

For nsHyperTextAccessible::HypertextOffsetToDOMElement
You can change
+  nsCOMPtr<nsIDOMElement> elm = nsAccUtils::GetDOMElementFor(node, nodeOffset);
+  NS_IF_ADDREF(*aElm = elm);
+}

to 
*aElm = nsAccUtils::GetDOMElementFor(node, nodeOffset).get();

The most IMPORTANT, you forgot return NS_OK; at end of this method.

Comment 50

[Ginn Chen]

And, EVENT_TEXT_ATTRIBUTE_CHANGED is not handled by
nsAccessibleWrap::FireAccessibleEvent

Do you want to file a follow-up bug for it?

Comment 51

[Aaron Leventhal]

Surkov, we need a very good test case page. Can you build one and put the URL in this patch? It can go under www.mozilla.org/access/qa somewhere.

That will help with the review (and approval), as well as give us a place to check for regressions.

Comment 52

[Aaron Leventhal]

Why not move all the new text attribute related methods to nsTextUtils? Why add any new methods to nsHyperTextAccessible?

Comment 53

[Willie Walker]

Adding orca:high to this since it helps a lot with getting text attributes.

Comment 54

[alexander :surkov (:asurkov)]

(In reply to comment #51)
> Surkov, we need a very good test case page. Can you build one and put the URL
> in this patch? It can go under www.mozilla.org/access/qa somewhere.
> 
> That will help with the review (and approval), as well as give us a place to
> check for regressions.
> 

I have testcase based on midas editor demo. Possibly I can reuse some files from original demo.

Comment 55

[alexander :surkov (:asurkov)]

(In reply to comment #52)
> Why not move all the new text attribute related methods to nsTextUtils? Why add
> any new methods to nsHyperTextAccessible?
> 

Because they use nsHyperTextAccessible methods. Either I should pass there nsHyperTextAccessible or change nsHyperTextAccessible methods. But since they use its methods I thought it's right place to keep them there.

I can see four methods (that aren't used mDOMNode and another methods) to move from nsHyperTextAccessible to nsTextUtils. It will reduce codesize a bit too.

Comment 56

[Aaron Leventhal]

> I have testcase based on midas editor demo. 
That makes sense for some of the things, but I'd like to see some static text cases for things like:
Normal text [Bold text [[ bold + italics ]] back to bold ] back to normal
And other weird combinations that include language changes, embedded objects, etc. It would be good if we had that posted, because AT vendors can use it to test their support. It should have at least one item for each item at http://wiki.mozilla.org/Accessibility/TextAttributes

Comment 57

[Aaron Leventhal]

nsAccessNode::GetLanguage() and nsAccUtils::GetLanguageFor():
- Why hasn't the code to check doc->headerData been moved to nsAccUtils::GetLanguageFor(). The header can affect the language of any node?
- It's possible to put xml:lang instead of just lang. This prefix is hardcoded to a namespace. I'm seeing we don't check for it. That's wrong. Authors are dicouraged from using xml:lang in HTML but user agents must process it. See http://www.whatwg.org/specs/web-apps/current-work/#the-lang

nsCaretAccessible::SpellcheckSelectionChange()
- Will GetAccessibleInParentChain() ever be returned for something that is not an nsIAccessibleText? I think we should at least assert that it's a text accessible.
  

Comment 58

[Aaron Leventhal]

Also, the language for the doc accessible could be set using either the lang or xml:lange attribute on either <body> or <html>. Including the possibility of it being in the <meta> or HTTP headers, that's a lot of places we need to check, and it doesn't appear that we check them all.

Comment 59

[Aaron Leventhal]

Remove support for line-height, we're not supporting it. See the list of supported & unsupported text attributes for Firefox here:
http://wiki.mozilla.org/Accessibility/TextAttributes
Remove margin-left, margin-top, margin-right, margin-bottom: they are not supported because the CSS attributes are not related to page margins. This is more appropriate in a word processor. For Firefox the bounds of an object can be used to find where it is located.
Also please make sure you get notifcations of changes to the TextAttributes wiki page. We made a lot of changes before you wrote a lot of this code, but I don't see it reflected.

nsCSSTextAttributeMapItem:
- If we really end up with a map that covers are cases, I'd like to it formatted as a table so I can read it more easily, something like:

const char* kAnyValue = "";
const char* kCopyName = "";
const char* kCopyValue = "";
static nsCSSTextAttrMapItem gCSSTextAttrsMap[] = {
  {
  // CSS name             // CSS value      // Text attr name        //  Text attr value
  {"background-color",     kAnyValue,       kCopyName,               kCopyValue},
  {"font-family",          kAnyValue,       kCopyName,               kCopyValue}, 
  {"font-size",            kAnyValue,       kCopyName,               kCopyValue}, 
  {"font-style",           "italic",        kCopyName,               kCopyValue}, 
  {"font-weight",          kAnyValue,       kCopyName,               kCopyValue}, 
  {"text-decoration",      kAnyValue,       kCopyName,               kCopyValue}, 
  {"text-align",           kAnyValue,       kCopyName,               kCopyValue}, 
  {"text-indent",          kAnyValue,       kCopyName,               kCopyValue}, 
  ...
  }
  
However, it doesn't look like you've taken into account the translation of values that we describe in the wiki page. For example, the boldface is the computed value 0-900. We don't want "bolder" to be in there. 
This is so the AT has a limited set of normalized values it has to deal with. Same with colors, etc.

Therefore, I think you're probably going to have to do most of that in code instead of a table.

And, because there are these differences, it might be a good idea to support just a few attributes in the first patch to get the core support in, and add support for each new group of attributes in separate patches.

Comment 60

[Marco Zehe (:MarcoZ)]

Aaron, how will indentations like when replying to a message in Thunderbird, and quoting it, be reflected?

Comment 61

[Aaron Leventhal]

Marco, great question. File a separate bug for that one, we'll need to discuss it.

Comment 62

[Marco Zehe (:MarcoZ)]

(In reply to comment #61)
> Marco, great question. File a separate bug for that one, we'll need to discuss
> it.

Done, bug 408793. I am just not sure what metabug(s) that one would block. I'll leave that to you.

Comment 63

[alexander :surkov (:asurkov)]

Attachment: patch8

Comment 64

[alexander :surkov (:asurkov)]

(In reply to comment #57)
> nsAccessNode::GetLanguage() and nsAccUtils::GetLanguageFor():
> - Why hasn't the code to check doc->headerData been moved to
> nsAccUtils::GetLanguageFor(). The header can affect the language of any node?

There I want to get explicit language to trac whether it's different from default language.

> - It's possible to put xml:lang instead of just lang. This prefix is hardcoded
> to a namespace. I'm seeing we don't check for it. That's wrong. Authors are
> dicouraged from using xml:lang in HTML but user agents must process it. See
> http://www.whatwg.org/specs/web-apps/current-work/#the-lang

I didn't address this in patch8. If you would like to keep it in the bug then I'll fix.

> nsCaretAccessible::SpellcheckSelectionChange()
> - Will GetAccessibleInParentChain() ever be returned for something that is not
> an nsIAccessibleText? I think we should at least assert that it's a text
> accessible.
> 

if it's not text accessible, right? I'll put an assertion into following up patch.

Comment 65

[alexander :surkov (:asurkov)]

(In reply to comment #58)
> Also, the language for the doc accessible could be set using either the lang or
> xml:lange attribute on either <body> or <html>. Including the possibility of it
> being in the <meta> or HTTP headers, that's a lot of places we need to check,
> and it doesn't appear that we check them all.
> 

didn't address in patch8 too, the same like prev my comment

Comment 66

[alexander :surkov (:asurkov)]

(In reply to comment #59)
> Remove support for line-height, we're not supporting it. See the list of
> supported & unsupported text attributes for Firefox here:
> http://wiki.mozilla.org/Accessibility/TextAttributes
> Remove margin-left, margin-top, margin-right, margin-bottom: they are not
> supported because the CSS attributes are not related to page margins. This is
> more appropriate in a word processor. For Firefox the bounds of an object can
> be used to find where it is located.

done

> Also please make sure you get notifcations of changes to the TextAttributes
> wiki page. We made a lot of changes before you wrote a lot of this code, but I
> don't see it reflected.

done ;)

> 
> nsCSSTextAttributeMapItem:
> - If we really end up with a map that covers are cases, I'd like to it
> formatted as a table so I can read it more easily, something like:

forgot to fix, will fix it

> However, it doesn't look like you've taken into account the translation of
> values that we describe in the wiki page. For example, the boldface is the
> computed value 0-900. We don't want "bolder" to be in there. 
> This is so the AT has a limited set of normalized values it has to deal with.
> Same with colors, etc.

computed styles returns all we need, for bold is number, for color is rgb()

Comment 67

[alexander :surkov (:asurkov)]

Attachment: test case

Please look at testcase. Possibly we can put it on mozilla.org. Though I'm not sure how it would useful due to security issue.

Comment 68

[Aaron Leventhal]

When running with JAWS I get a crash at 
nsCaretAccessible::NotifySelectionChanged(nsIDOMDocument *aDoc,
                                          nsISelection *aSel,
                                          PRInt16 aReason)
{
  nsCOMPtr<nsISelection2> sel2(do_QueryInterface(aSel));

  PRInt16 type = 0;
  sel2->GetType(&type);
...

aSel points to an nsTypedSelection but sel2 is null.

Comment 69

[alexander :surkov (:asurkov)]

(In reply to comment #68)

> aSel points to an nsTypedSelection but sel2 is null.
> 

How it's possible? How can I reproduce this?

Comment 70

[Aaron Leventhal]

Just run with JAWS for a while.

Comment 71

[alexander :surkov (:asurkov)]

Can't reproduce with Jaws 8

Comment 72

[Aaron Leventhal]

Maybe I didnt rebuild enough directories. I'll see if I can repro. Anyway, a null check there would be good.

Comment 73

[Ginn Chen]

Comment on attachment 293715 [details] [diff] [review]
patch8

nsHyperTextAccessible.cpp
The first rv assignment is not used.

-      nsresult rv = GetSelections(nsnull, getter_AddRefs(domSel));
+      nsresult rv = GetSelections(nsISelectionController::SELECTION_NORMAL,
+                                  nsnull, getter_AddRefs(domSel));
       nsCOMPtr<nsISelectionPrivate> privateSelection(do_QueryInterface(domSel));
       nsCOMPtr<nsFrameSelection> frameSelection;
       rv = privateSelection->GetFrameSelection(getter_AddRefs(frameSelection));

Comment 74

[alexander :surkov (:asurkov)]

Attachment: patch9

updated to trunk with Ginn's comment

Comment 75

[Aaron Leventhal]

(In reply to comment #64)
> (In reply to comment #57)
> > nsAccessNode::GetLanguage() and nsAccUtils::GetLanguageFor():
> > - Why hasn't the code to check doc->headerData been moved to
> > nsAccUtils::GetLanguageFor(). The header can affect the language of any node?
> 
> There I want to get explicit language to trac whether it's different from
> default language.

The method name and comments should probably reflect that, because I expected the opposite when I saw the method.

Comment 76

[Aaron Leventhal]

Still looking for gCSSTextAttrsMap to get a simpler tabular format and remove redundancies where possible.

Comment 77

[Aaron Leventhal]

class nsTextAttr
{
public:
  /**
   * Return true if the text attribute for the given element equals with
   * predefined attribute.
   */
  virtual PRBool compare(nsIDOMElement *aElm) = 0;
};

I suggest changing the name to |equal| because often times compare functions return 0 for equal, -1 for less and 1 for greater than.

Or you could overload the == operator.

Comment 78

[Aaron Leventhal]

Forget the == operator I wasn't thinking.

nsHypertextAccessible::GetTextHelper():
We need support for
  case BOUNDARY_ATTRIBUTE_RANGE:
but that should be a followup bug since this is so large (although perhaps it is relatively simple).

nsCSSTextAttr:
Is it worth caching the current value in a member variable so it doesn't need to be looked up multiple times?
  nsAutoString value;
  rv = mStyleDecl->GetPropertyValue(cssName, value);
  return NS_SUCCEEDED(rv) && value == currValue;

nsHyperTextAccessible::GetDefaultTextAttributes():
I think the first argument here should be PR_FALSE, because the default attribute values are something we document and the screen reader or other AT will use when we don't pass a text attribute. That way there is less data passing back and forth over IPC:
 nsCSSTextAttr textAttr(PR_TRUE, element, nsnull);
Basically whenever some attribute is equal to the default attribute value, we don't need to expose that text attribute unless it's a change from an ancestor that set it.

Comment 79

[Aaron Leventhal]

nsHyperTextAccessible::GetSpellTextAttribute():
Why do we care about ranges before or after the point? I think we only care about the range at the point (if there is one, and there can only be one).

                          The point
                               |
I like bananaz, coconuts and gwapes.
       xxxxxxx               xxxxxx

We don't care about the bananaz range, so in our range loop why are we looking at result == 1 or result == -1 ?

Comment 80

[Aaron Leventhal]

In nsCaretAccessible, please make sure all methods that should, guard against mRootAccessible == nsnull (I already checked in some checks but it looks like we need some more, especially after this patch). So you need to update to trunk again.

nsHyperTextAccessible::GetTextAttributes():
...
Why does GetSpellTextAttribute() take a hypertext offset and comput its own DOM point, yet then we compute the DOM point again and pass that to GetLangTextAttributes() ?

Comment 81

[Aaron Leventhal]

Another perf win in GetSpellTextAttribute():
We're using GetSelections() which gets all the ranges for the entire hypertext accessible.
     rv = selection2->GetRangesForIntervalCOMArray(mDOMNode, 0,
                                                   mDOMNode, numChildren,
                                                   PR_TRUE, aRanges);
     NS_ENSURE_SUCCESS(rv, rv);

But we should allow ourselves to pass in just the node+offset for the hypertext offset in question.
1. If the current hypertext offset is on an embedded object char, forget it, there is no spell attribute
2. Otherwise get the node+offset and either pass that into GetSelections() or deal with GetRangesForIntervalCOMArray directly.

If we do that we should always get 0 or 1 spell check range, and we can eliminate that range loop entirely.

Comment 82

[Aaron Leventhal]

It needs to be clear when aStartHTOffset/aEndHTOffset params are just out params, and when they are in/out params. I think in the GetSpellAttribute() method they are only out, and everywhere else they are in/out, right?

Perhaps change the name when they are inout to aStartHTOffsetInOut, etc. 
You might even want to make them inout everywhere just for consistency, which means setting the default start/end before you call GetSpellAttribute().

Comment 83

[Aaron Leventhal]

  rv = GetRangeForTextAttr(aSourceNode, &textAttr,
                           aStartHTOffset, aEndHTOffset);
  return rv;

Can be shortened to just |return GetRangeForTextAttr(...);|

In GetLangTextAttributes():
  if (!resultLang.IsEmpty() && (aIncludeDefAttrs || lang != rootLang))

What if you have:
English
  \
   French
    \
     English

On the bottom English node it's the same as the root language, so you would not expose the change. I think you need to compare the language with the parent element's language and decide whether to expose it from that.

I'm not sure but perhaps the default attributes should expose the lang if it is set on mDOMNode for the hypertext and it is different from the parent.




  	

Comment 84

[Aaron Leventhal]

Actually, I see now it looks like you are almost doing that, but if aSourceNode is a text node child of mDOMNode, won't content == rootContent ?
  nsCOMPtr<nsIDOMElement> sourceElm(nsAccUtils::GetDOMElementFor(aSourceNode));
  nsCOMPtr<nsIContent> content(do_QueryInterface(sourceElm));
  nsCOMPtr<nsIContent> rootContent(do_QueryInterface(mDOMNode));

Comment 85

[Aaron Leventhal]

That's it for now.

Note to myself, I still need to look at GetRangeForTextAttr() and of course we need to test this very well.

Comment 86

[Aaron Leventhal]

Attachment: Another null check and move ClearControlSelectionListener() up

I wonder if we're still getting selection notifications after the caret accessible has been shutdown or destroyed.

Comment 87

[Aaron Leventhal]

Comment on attachment 299061 [details] [diff] [review]
Another null check and move ClearControlSelectionListener() up

This was supposed to be in bug 405951.

Comment 88

[Aaron Leventhal]

Please add more comments to GetRangeForTextAttr(), because I don't find the logic totally obvious.

Comment 89

[alexander :surkov (:asurkov)]

(In reply to comment #76)
> Still looking for gCSSTextAttrsMap to get a simpler tabular format and remove
> redundancies where possible.
> 

would be nice to get some ideas of format ;)

Comment 90

[alexander :surkov (:asurkov)]

(In reply to comment #89)
> (In reply to comment #76)
> > Still looking for gCSSTextAttrsMap to get a simpler tabular format and remove
> > redundancies where possible.
> > 
> 
> would be nice to get some ideas of format ;)
> 

though it sounds you gave it in comment #59

Comment 91

[alexander :surkov (:asurkov)]

(In reply to comment #78)
> Forget the == operator I wasn't thinking.
> 
> nsHypertextAccessible::GetTextHelper():
> We need support for
>   case BOUNDARY_ATTRIBUTE_RANGE:
> but that should be a followup bug since this is so large (although perhaps it
> is relatively simple).

I'd rather to file new bug for this.

> nsCSSTextAttr:
> Is it worth caching the current value in a member variable so it doesn't need
> to be looked up multiple times?
>   nsAutoString value;
>   rv = mStyleDecl->GetPropertyValue(cssName, value);
>   return NS_SUCCEEDED(rv) && value == currValue;

sorry didn't get you. I use current value once only while I iterate through the names. Why to cache?

> nsHyperTextAccessible::GetDefaultTextAttributes():
> I think the first argument here should be PR_FALSE, because the default
> attribute values are something we document and the screen reader or other AT
> will use when we don't pass a text attribute. That way there is less data
> passing back and forth over IPC:
>  nsCSSTextAttr textAttr(PR_TRUE, element, nsnull);
> Basically whenever some attribute is equal to the default attribute value, we
> don't need to expose that text attribute unless it's a change from an ancestor
> that set it.
> 

not sure I got you here too. GetDefaultTextAttributes() doens't take arguments, nsCSSTextAttr is for private usage and haven't default arguments values

Comment 92

[alexander :surkov (:asurkov)]

(In reply to comment #79)

> We don't care about the bananaz range, so in our range loop why are we looking
> at result == 1 or result == -1 ?
> 

because of
>                           The point
>                                  |
> I like bananaz, coconuts and gwapes.
>           xxxxxxx                          xxxxxx

Comment 93

[Aaron Leventhal]

I mean in nsHyperTextAccessible::GetDefaultTextAttributes(), where we use 
nsCSSTextAttr textAttr(PR_TRUE, element, nsnull);
I think it should maybe be
nsCSSTextAttr textAttr(PR_FALSE, element, nsnull);
I'm not sure.

Anyway the point is this. On the wiki we have a column "Default value if attribute not exposed".
For example, font-style. If we don't expose it the AT knows that it's "normal". 

Does your code do that?

Comment 94

[alexander :surkov (:asurkov)]

(In reply to comment #81)
> Another perf win in GetSpellTextAttribute():
> We're using GetSelections() which gets all the ranges for the entire hypertext
> accessible.
>      rv = selection2->GetRangesForIntervalCOMArray(mDOMNode, 0,
>                                                    mDOMNode, numChildren,
>                                                    PR_TRUE, aRanges);
>      NS_ENSURE_SUCCESS(rv, rv);
> 
> But we should allow ourselves to pass in just the node+offset for the hypertext
> offset in question.

but we should know where is no selection in the accessible to compute correct
offsets

Comment 95

[alexander :surkov (:asurkov)]

(In reply to comment #93)

> Anyway the point is this. On the wiki we have a column "Default value if
> attribute not exposed".
> For example, font-style. If we don't expose it the AT knows that it's "normal". 
> 
> Does your code do that?
> 

No. Default attributes is all attributes, it doesn't matter which value it has. But sure it makes a difference for the attributes.

Comment 96

[Aaron Leventhal]

> but we should know where is no selection in the accessible to compute correct
> offsets
I don't understand. Can you restate that?


> No. Default attributes is all attributes, it doesn't matter which value it has.
> But sure it makes a difference for the attributes.

No that's not the plan. Here's why -- it will cost a lot of cross-process communication if we pass everything.

And in IA2, the default attributes get mixed into everything.

So if something equals the "default value if not exposed", you don't have to expose it. The AT will see it is missing and use the default value. I checked with the AT vendors on this and they liked it.

Comment 97

[alexander :surkov (:asurkov)]

(In reply to comment #83)

> What if you have:
> English
>   \
>    French
>     \
>      English
> 
> On the bottom English node it's the same as the root language, so you would not
> expose the change. I think you need to compare the language with the parent
> element's language and decide whether to expose it from that.

It seems to correct not to expose this attribute because it is matched with
default attribute but sure I should fix ranges but it seems I do this. 

Comment 98

[alexander :surkov (:asurkov)]

(In reply to comment #96)

> No that's not the plan. Here's why -- it will cost a lot of cross-process
> communication if we pass everything.
> 
> And in IA2, the default attributes get mixed into everything.
> 
> So if something equals the "default value if not exposed", you don't have to
> expose it. The AT will see it is missing and use the default value. I checked
> with the AT vendors on this and they liked it.
> 

it sounds we do not have a lot attributes to expose now. But how I will know whether this value of attribute is default. What if my application has style
* {
  font-size: 24pt;
}
which is not usual but obviously default for my application? ... if I'm thinking of the right direction ..

Comment 99

[alexander :surkov (:asurkov)]

(In reply to comment #84)
> Actually, I see now it looks like you are almost doing that, but if aSourceNode
> is a text node child of mDOMNode, won't content == rootContent ?
>   nsCOMPtr<nsIDOMElement> sourceElm(nsAccUtils::GetDOMElementFor(aSourceNode));
>   nsCOMPtr<nsIContent> content(do_QueryInterface(sourceElm));
>   nsCOMPtr<nsIContent> rootContent(do_QueryInterface(mDOMNode));
> 

yes, it should. Do you mean it's worth to have additional check for this?

Comment 100

[Aaron Leventhal]

> which is not usual but obviously default for my application? ... if I'm
> thinking of the right direction 
Right, for font size the wiki says:
no default, always specified
Same for font-family.

It might not seem like a lot of info, but some ATs walk entire subtrees and it could get expensive for a lot of nodes.

> yes, it should. Do you mean it's worth to have additional check for this?
I think we need to, for it to work correctly.

Comment 101

[Aaron Leventhal]

Hmm, Roc tells me that GetComputedStyle is slow and unreliable. We should get what we want from the frame.

Comment 102

[alexander :surkov (:asurkov)]

Attachment: patch10

next version, fixed some Aaron's comments, updated to trunk

Comment 103

[alexander :surkov (:asurkov)]

(In reply to comment #100)

> > yes, it should. Do you mean it's worth to have additional check for this?
> I think we need to, for it to work correctly.
> 

Ok, I will add in the following version of the patch.

Comment 104

[alexander :surkov (:asurkov)]

(In reply to comment #101)
> Hmm, Roc tells me that GetComputedStyle is slow and unreliable. We should get
> what we want from the frame.
> 

good news and it time :) Possibly we can land (or to do test build) to test if this is really performance problem.

Comment 105

[Aaron Leventhal]

For spell check, I was expecting GetSpellTextAttribute() to change so that it only gets the single range for the current node, offset (by pasing just that into GetRangesForIntervalCOMArray()). So it shouldn't need the range loop, just use the first range and assert that there is only 1.

In GetRangeForTextAttr() use backwards and forwards instead of left and right, because text can go from right to left for Hebrew or Arabic.

Comment 106

[Aaron Leventhal]

In GetRangeForTextAttr() I'm not sure if your tree walking algorithm visits enough nodes. It never goes into children, only to siblings or parents. What if you have this:

    HyperText 
  /   |      \
Text   Span   Text
     /  |  \
   Text Span Text
          |
          Text 

Do we need to delve into span subtrees to find out when a text attribute stops?

If the given subtree is represented by a single embedded object char I would say we don't need to dive into it.

You might be better off using one of the tree walking classes like nsIDOMTreeWalker.

Comment 107

[Aaron Leventhal]

And if we decide to get style info directly from the frames we should consider use a frame tree walker in GetRangeForTextAttr() -- it would be very fast.

Comment 108

[alexander :surkov (:asurkov)]

(In reply to comment #105)

> In GetRangeForTextAttr() use backwards and forwards instead of left and right,
> because text can go from right to left for Hebrew or Arabic.
> 

It's good point. But do we currently support this at all, for example, when we get text from offset to line end?

Comment 109

[alexander :surkov (:asurkov)]

(In reply to comment #107)
> And if we decide to get style info directly from the frames we should consider
> use a frame tree walker in GetRangeForTextAttr() -- it would be very fast.
> 

Yes but in this case we should probably should have special version of GetRangeForTextAttr() because we can't use it for lang text attributes, right?

Comment 110

[alexander :surkov (:asurkov)]

(In reply to comment #105)
> For spell check, I was expecting GetSpellTextAttribute() to change so that it
> only gets the single range for the current node, offset (by pasing just that
> into GetRangesForIntervalCOMArray()). So it shouldn't need the range loop, just
> use the first range and assert that there is only 1.

But if there is no spellcheck selection for this node/offset then how will I compute range in this case?

Comment 111

[Aaron Leventhal]

We don't currently support right to left as far as I know, because we've never even test it. I was just suggesting changing the comment (left/right).

I don't think we necessarily need to walk the frames in GetRangeForTextAttr() but don't we need to use a depth first search? Thus DOMTreeWalker is a good bet there.

> But if there is no spellcheck selection for this node/offset then how will I
> compute range in this case?

Ah, okay I see.

Comment 112

[alexander :surkov (:asurkov)]

(In reply to comment #111)
> We don't currently support right to left as far as I know, because we've never
> even test it. I was just suggesting changing the comment (left/right).

Ok, I misread you.

> I don't think we necessarily need to walk the frames in GetRangeForTextAttr()
> but don't we need to use a depth first search? Thus DOMTreeWalker is a good bet
> there.

I agree we should look into children though I don't see how treeWalker helps here. IIRC treeWalker allows to navigate down through the tree, so I should start from mDOMNode but it seems nagivating from node offset to mDOMNode should be faster because there is chance we won't ever rich mDOMNode.

Comment 113

[alexander :surkov (:asurkov)]

Attachment: patch

Comment 114

[alexander :surkov (:asurkov)]

Comment on attachment 301618 [details] [diff] [review]
patch

I need to fit range backward as well

Comment 115

[alexander :surkov (:asurkov)]

Attachment: testcase2

next version of testcase, added check for ranges calculation through the children

Comment 116

[alexander :surkov (:asurkov)]

Attachment: patch12

Comment 117

[alexander :surkov (:asurkov)]

Attachment: patch13

added some commets for range calculation

Comment 118

[Marco Zehe (:MarcoZ)]

This one would really be good to have, since lots of web sites convey important information only through attributes on certain text or the like, and currently, with Firefox, there's no way to get to that information. But I realize that this is a huge change at a very late stage, so am not requesting blocking.

Comment 119

[Marco Zehe (:MarcoZ)]

Now that we're getting a beta 5, do you think that we can get this one reviewed/corrected and tested? Would be awesome if this would make it into the final release.

Comment 120

[Marco Zehe (:MarcoZ)]

Alexander, can you get back to this one now and update it to trunk? I'd like to get this into Gecko 1.9.1 if possible, and I'd like to have it early on so we have time to bake it.

Comment 121

[alexander :surkov (:asurkov)]

Attachment: patch14

updated to trunk

Comment 122

[Olli Pettay [:smaug][[e-mail]]]

What part the patch should I review? I'm not familiar with accessible/ (so reviewing it takes some time), and the change to layout/ is very simple.

Comment 123

[[e-mail]]

Comment on attachment 329461 [details] [diff] [review]
patch14

>+already_AddRefed<nsIDOMElement>
>+nsAccUtils::GetDOMElementFor(nsIDOMNode *aNode)
>+{
>+  nsCOMPtr<nsINode> node(do_QueryInterface(aNode));
>+
>+  nsIDOMElement *element = nsnull;
>+  if (node->IsNodeOfType(nsINode::eELEMENT))
Possibly call aNode->GetNodeType instead? No, I've changed my mind.

>+  nsresult rv = selPrivate->RemoveSelectionListener(this);
>+  NS_ENSURE_SUCCESS(rv, rv);
>+
>+  nsCOMPtr<nsISelection> spellcheckSel;
>+  controller->GetSelection(nsISelectionController::SELECTION_SPELLCHECK,
>+                           getter_AddRefs(spellcheckSel));
>+  selPrivate = do_QueryInterface(spellcheckSel);
> 
>   return selPrivate->RemoveSelectionListener(this);
Duplication.

>+  nsresult rv = selPrivate->AddSelectionListener(this);
>+  NS_ENSURE_SUCCESS(rv, rv);
>+  
>+  nsCOMPtr<nsISelection> spellcheckSel;
>+  controller->GetSelection(nsISelectionController::SELECTION_SPELLCHECK,
>+                           getter_AddRefs(spellcheckSel));
>+  selPrivate = do_QueryInterface(spellcheckSel);
>+  NS_ENSURE_TRUE(selPrivate, NS_ERROR_FAILURE);
>+  
>   return selPrivate->AddSelectionListener(this);
And again. And again! And then the removal was duplicated again.

>+  nsCOMPtr<nsIContent> content(do_QueryInterface(aNode));
>+  // Get selection controller only for form controls, not for the document.
>+  if (!content)
>+    return nsnull;
Use aNode->IsNodeOfType perhaps?

>+const char* kAnyValue = "";
>+const char* kCopyName = "";
>+const char* kCopyValue = "";
You should use nsnull as marker values. (See also below.)

>+  if (cssValue != kAnyValue) {
>+    NS_ConvertASCIItoUTF16 cssValueUTF16(cssValue);
>+    if (!aValue.Equals(cssValueUTF16))
>+      return PR_FALSE;
>+  }
The "any" value should be nsnull, so you can write this as
if (cssValue && !aValue.EqualsASCII(cssValue))
  return PR_FALSE;

>+      if (endHTOffset < *aHTEndOffset)
>+        *aHTEndOffset = endHTOffset;
>+
>+      nsAccUtils::SetAccAttr(aAttributes, nsAccessibilityAtoms::misspelled,
>+                             NS_LITERAL_STRING("true"));
>+
>+      return NS_OK;
What about the end offset for ranges after the misspelled word?

>+  readonly attribute short type;
Personally I'd prefer SelectionType here - you would have to include nsISelectionController.idl to pick up the typedef.

Comment 124

[Marco Zehe (:MarcoZ)]

I'm running with this patch now, and so far the results look very promising.

1. On our page for the spec of this bug, http://wiki.mozilla.org/Accessibility/TextAttributes, on the first paragraph after the "Introduction" heading, I get these IA2 attributes:
getAttributes(1) = org.eclipse.actf.accservice.core.win32.ia2.IA2TextSegment[text=font-style:normal;text-align:start;font-size:13px;background-color:transparent;font-weight:400;lang:en;text-indent:0px;color:rgb(0\, 0\, 0);font-family:verdana\,sans-serif;,start=0,end=186]

1.a Question: Why is the background color always exposed as "transparent"? I've observed this everywhere I've tried it. Checking the colors on the screen, the background color should be "white", meaning:
background-color:rgb(0\, 0\, 0);

1.b I don't seem to see the misspelling in IA2 when getting the attributes on some text I purposely misspelled in a textarea. Is this currently not exposed yet?

2. Note to Aaron and myself: The spec must be updated to reflect a few changes:
2.a the "lang" attribute is exposed by this patch (and should be), yet is currently in the "Gecko-specific" section.
2.b Same goes for the "line-through" attribute, which is exposed, but is currently in the "not supported" section.

On a side note: JAWS already gets the font-family without any additional work on the FS side. It does not yet get the size, or the colors or any of the text attributes such as underlined etc.

Comment 125

[Marco Zehe (:MarcoZ)]

(In reply to comment #122)
> What part the patch should I review? I'm not familiar with accessible/ (so
> reviewing it takes some time), and the change to layout/ is very simple.

Hi Smaug, we'd need review on the Layout part from you. Thanks!

Comment 126

[Aaron Leventhal]

(In reply to comment #124)
> 1.a Question: Why is the background color always exposed as "transparent"? I've
> observed this everywhere I've tried it. Checking the colors on the screen, the
> background color should be "white", meaning:
> background-color:rgb(0\, 0\, 0);
I suspect that it uses "transparent" when it simply inherits whatever the default background color is from current settings. However, we could make "transparent" the default for background-color so that we simply don't expose the background-color attribute in that (normal) case.

> 1.b I don't seem to see the misspelling in IA2 when getting the attributes on
> some text I purposely misspelled in a textarea. Is this currently not exposed
> yet?
It's definitely turned on by default in a textarea. For example, as I type in this comment, the word "textarea" is underlined as a misspelling.

> 2. Note to Aaron and myself: The spec must be updated to reflect a few changes:
> 2.a the "lang" attribute is exposed by this patch (and should be), yet is
> currently in the "Gecko-specific" section.
Ah, we should change the patch to use "language", which is what it's called here: http://www.linuxfoundation.org/en/Accessibility/IAccessible2/TextAttributes

> 2.b Same goes for the "line-through" attribute, which is exposed, but is
> currently in the "not supported" section.
This appears correct in the spec. It says "text-line-through-style" can be "solid" under supported. However,  text-line-through-mode and  text-line-through-type are not supported, which is correct for the web today.
 

Comment 127

[Aaron Leventhal]

So, there are 2 changes I see that we need:

1. Based on looking at the current version of http://www.linuxfoundation.org/en/Accessibility/IAccessible2/TextAttributes the lang text attribute should now be called language
2. To line up with how ARIA does things, please change misspelled=true to invalid=spelling. This allows future types of invalid text. In addition, if aria-invalid is used on a span of text, the value of the invalid attribute should be exposed as the "invalid" text attribute value. (This is planned to be added to ARIA spec shortly).

Is it correct that the patch does not support text direction? (The spec now calls this writing-mode). If so, which of the spec features are still unsupported?

Comment 128

[Aaron Leventhal]

Please add some comments to FitRange*OnChildren. The code is starting to look clearer but some comments will make it even better.

Comment 129

[Marco Zehe (:MarcoZ)]

(In reply to comment #127)
> Is it correct that the patch does not support text direction? (The spec now
> calls this writing-mode). If so, which of the spec features are still
> unsupported?

The only one left would be "static". What about that one?

Comment 130

[Aaron Leventhal]

There's only one place to support "static" -- for list bullets. Once we fix bug 346833 then we can do it for :before and :after as well.

We actually try to expose static now. For list bullet text leaves, we use ROLE_STATICTEXT -- see nsHTMLListBulletAccessible::GetRole(). Then in http://mxr.mozilla.org/seamonkey/source/accessible/src/html/nsHTMLTextAccessible.cpp#105 we map that to the object attribute static=true. The only reason we did that is that at the time I thought object attributes on text leaves would get mapped to text attributes. However, I don't think that makes sense anymore. So we probably need to remove that but have Surkov's new patch look for ROLE_STATICTEXT and turn that into static=true directly.

Comment 131

[alexander :surkov (:asurkov)]

(In reply to comment #125)
> (In reply to comment #122)
> > What part the patch should I review? I'm not familiar with accessible/ (so
> > reviewing it takes some time), and the change to layout/ is very simple.
> 
> Hi Smaug, we'd need review on the Layout part from you. Thanks!
> 

Yes, Olli. Please make content/layout review. In any way we need review for these changes even if they are simple.

Comment 132

[alexander :surkov (:asurkov)]

(In reply to comment #123)

> >+  nsresult rv = selPrivate->RemoveSelectionListener(this);
> >+  NS_ENSURE_SUCCESS(rv, rv);
> >+
> >+  nsCOMPtr<nsISelection> spellcheckSel;
> >+  controller->GetSelection(nsISelectionController::SELECTION_SPELLCHECK,
> >+                           getter_AddRefs(spellcheckSel));
> >+  selPrivate = do_QueryInterface(spellcheckSel);
> > 
> >   return selPrivate->RemoveSelectionListener(this);
> Duplication.
> 
> >+  nsresult rv = selPrivate->AddSelectionListener(this);
> >+  NS_ENSURE_SUCCESS(rv, rv);
> >+  
> >+  nsCOMPtr<nsISelection> spellcheckSel;
> >+  controller->GetSelection(nsISelectionController::SELECTION_SPELLCHECK,
> >+                           getter_AddRefs(spellcheckSel));
> >+  selPrivate = do_QueryInterface(spellcheckSel);
> >+  NS_ENSURE_TRUE(selPrivate, NS_ERROR_FAILURE);
> >+  
> >   return selPrivate->AddSelectionListener(this);
> And again. And again! And then the removal was duplicated again.
> 

Neil, it's not dublication. We use 'this' as selection listener for two selections: normal and spellcheck. 

Comment 133

[alexander :surkov (:asurkov)]

(In reply to comment #126)

> > 1.b I don't seem to see the misspelling in IA2 when getting the attributes on
> > some text I purposely misspelled in a textarea. Is this currently not exposed
> > yet?
> It's definitely turned on by default in a textarea. For example, as I type in
> this comment, the word "textarea" is underlined as a misspelling.

It's not for me. Try to choose 'Check Spelling' menuitem of context menu on the entry field (I tried html:input). Next version of the patch will hold mochitest for spelling.

Comment 134

[Olli Pettay [:smaug][[e-mail]]]

Comment on attachment 329461 [details] [diff] [review]
patch14

r=me for the selection type thing.

Comment 135

[alexander :surkov (:asurkov)]

(In reply to comment #127)

> Is it correct that the patch does not support text direction? (The spec now
> calls this writing-mode). If so, which of the spec features are still
> unsupported?
> 

I guess yes because our existed code isn't ready to take text directions. All
our text functions assumes text is going from left to right.

Comment 136

[alexander :surkov (:asurkov)]

(In reply to comment #123)

> >   return selPrivate->RemoveSelectionListener(this);
> Duplication.

comment #132

> >+  nsCOMPtr<nsIContent> content(do_QueryInterface(aNode));
> >+  // Get selection controller only for form controls, not for the document.
> >+  if (!content)
> >+    return nsnull;
> Use aNode->IsNodeOfType perhaps?

changed
 
> >+const char* kAnyValue = "";
> >+const char* kCopyName = "";
> >+const char* kCopyValue = "";
> You should use nsnull as marker values. (See also below.)

ok

> The "any" value should be nsnull, so you can write this as
> if (cssValue && !aValue.EqualsASCII(cssValue))
>   return PR_FALSE;

though I would prefer to use kAnyValue constant, that should look more readable I guess.
if (cssValue != kAnyValue && !aValue.EqualsASCII(cssValue))

> 
> >+      if (endHTOffset < *aHTEndOffset)
> >+        *aHTEndOffset = endHTOffset;
> >+
> >+      nsAccUtils::SetAccAttr(aAttributes, nsAccessibilityAtoms::misspelled,
> >+                             NS_LITERAL_STRING("true"));
> >+
> >+      return NS_OK;
> What about the end offset for ranges after the misspelled word?

Sorry I didn't catch the question. Could you explain more?

> >+  readonly attribute short type;
> Personally I'd prefer SelectionType here - you would have to include
> nsISelectionController.idl to pick up the typedef.
> 

ok

Comment 137

[alexander :surkov (:asurkov)]

(In reply to comment #130)
> There's only one place to support "static" -- for list bullets. Once we fix bug
> 346833 then we can do it for :before and :after as well.
> 
> We actually try to expose static now. For list bullet text leaves, we use
> ROLE_STATICTEXT -- see nsHTMLListBulletAccessible::GetRole(). Then in
> http://mxr.mozilla.org/seamonkey/source/accessible/src/html/nsHTMLTextAccessible.cpp#105
> we map that to the object attribute static=true. The only reason we did that is
> that at the time I thought object attributes on text leaves would get mapped to
> text attributes. However, I don't think that makes sense anymore. So we
> probably need to remove that but have Surkov's new patch look for
> ROLE_STATICTEXT and turn that into static=true directly.
> 

Actually I would prefer to save this for another bug if you'll allow me :) This one started to be big one.

Comment 138

[Aaron Leventhal]

Yes, please save static=true for another bug. It's also not in the official IA2 text attributes, so who knows maybe we'll do it differently after talking it over with Pete Brunet and the IA2 list.

Comment 139

[[e-mail]]

(In reply to comment #136)
> (In reply to comment #123)
> > >   return selPrivate->RemoveSelectionListener(this);
> > Duplication.
> comment #132
Well can you at least comment it, so that you don't confuse anyone else? ;-)

> > >+      if (endHTOffset < *aHTEndOffset)
> > >+        *aHTEndOffset = endHTOffset;
> > >+
> > >+      nsAccUtils::SetAccAttr(aAttributes, nsAccessibilityAtoms::misspelled,
> > >+                             NS_LITERAL_STRING("true"));
> > >+
> > >+      return NS_OK;
> > What about the end offset for ranges after the misspelled word?
> Sorry I didn't catch the question. Could you explain more?
As you loop through the ranges you update the start and end hypertext offset. But as soon as you hit a misspelled word you return early. This means that the resulting values differ depending on whether the current word is misspelled.

Comment 140

[alexander :surkov (:asurkov)]

(In reply to comment #126)
> (In reply to comment #124)
> > 1.a Question: Why is the background color always exposed as "transparent"? I've
> > observed this everywhere I've tried it. Checking the colors on the screen, the
> > background color should be "white", meaning:
> > background-color:rgb(0\, 0\, 0);
> I suspect that it uses "transparent" when it simply inherits whatever the
> default background color is from current settings. However, we could make
> "transparent" the default for background-color so that we simply don't expose
> the background-color attribute in that (normal) case.

I still really don't get about default values and how to recognize them. I ran through the bug and it seems we didn't finish the discussion.

Comment 141

[alexander :surkov (:asurkov)]

(In reply to comment #139)
> (In reply to comment #136)
> > (In reply to comment #123)
> > > >   return selPrivate->RemoveSelectionListener(this);
> > > Duplication.
> > comment #132
> Well can you at least comment it, so that you don't confuse anyone else? ;-)

Sure :) Thanks you pointed out this.

> 
> > > >+      if (endHTOffset < *aHTEndOffset)
> > > >+        *aHTEndOffset = endHTOffset;
> > > >+
> > > >+      nsAccUtils::SetAccAttr(aAttributes, nsAccessibilityAtoms::misspelled,
> > > >+                             NS_LITERAL_STRING("true"));
> > > >+
> > > >+      return NS_OK;
> > > What about the end offset for ranges after the misspelled word?
> > Sorry I didn't catch the question. Could you explain more?
> As you loop through the ranges you update the start and end hypertext offset.
> But as soon as you hit a misspelled word you return early. This means that the
> resulting values differ depending on whether the current word is misspelled.
> 

Yes. If the offset is inside of misspelled word then we want to return start and end offsets for the misspelled word. If the offset is before or after of misspelled word then we should adjust end or start of range (where misspelled word is started or ended). Did I understand you correctly?

Comment 142

[alexander :surkov (:asurkov)]

(In reply to comment #136)

> > >+  readonly attribute short type;
> > Personally I'd prefer SelectionType here - you would have to include
> > nsISelectionController.idl to pick up the typedef.
> > 
> 
> ok
> 

SelectionType is defined for C++ in nsISelectionController.idl so it's invisible for public mehtods in nsISelection2. Also selection type constains declared in nsISelectionController.idl are declared as short. Not sure I would like here to interfere in the code of nsISelectionController.idl. Though if you like then I'll file new bug for this and put the patch there.

Comment 143

[[e-mail]]

(In reply to comment #141)
> (In reply to comment #139)
> > (In reply to comment #136)
> > > (In reply to comment #123)
> > > > >+      if (endHTOffset < *aHTEndOffset)
> > > > >+        *aHTEndOffset = endHTOffset;
> > > > >+
> > > > >+      nsAccUtils::SetAccAttr(aAttributes, nsAccessibilityAtoms::misspelled,
> > > > >+                             NS_LITERAL_STRING("true"));
> > > > >+
> > > > >+      return NS_OK;
> > > > What about the end offset for ranges after the misspelled word?
> > > Sorry I didn't catch the question. Could you explain more?
> > As you loop through the ranges you update the start and end hypertext offset.
> > But as soon as you hit a misspelled word you return early. This means that the
> > resulting values differ depending on whether the current word is misspelled.
> Yes. If the offset is inside of misspelled word then we want to return start
> and end offsets for the misspelled word. If the offset is before or after of
> misspelled word then we should adjust end or start of range (where misspelled
> word is started or ended). Did I understand you correctly?
Yes, but the code doesn't do that unconditionally.

(In reply to comment #142)
> (In reply to comment #136)
> 
> > > >+  readonly attribute short type;
> > > Personally I'd prefer SelectionType here - you would have to include
> > > nsISelectionController.idl to pick up the typedef.
> > ok
> SelectionType is defined for C++ in nsISelectionController.idl so it's
> invisible for public mehtods in nsISelection2. Also selection type constains
> declared in nsISelectionController.idl are declared as short. Not sure I would
> like here to interfere in the code of nsISelectionController.idl. Though if you
> like then I'll file new bug for this and put the patch there.
Oh, that's silly. Do please file a new bug.

Comment 144

[Marco Zehe (:MarcoZ)]

(In reply to comment #143)
> (In reply to comment #141)
> > (In reply to comment #139)
> > > (In reply to comment #136)
> > > > (In reply to comment #123)
> > > > > >+      if (endHTOffset < *aHTEndOffset)
> > > > > >+        *aHTEndOffset = endHTOffset;
> > > > > >+
> > > > > >+      nsAccUtils::SetAccAttr(aAttributes, nsAccessibilityAtoms::misspelled,
> > > > > >+                             NS_LITERAL_STRING("true"));
> > > > > >+
> > > > > >+      return NS_OK;
> > > > > What about the end offset for ranges after the misspelled word?
> > > > Sorry I didn't catch the question. Could you explain more?
> > > As you loop through the ranges you update the start and end hypertext offset.
> > > But as soon as you hit a misspelled word you return early. This means that the
> > > resulting values differ depending on whether the current word is misspelled.
> > Yes. If the offset is inside of misspelled word then we want to return start
> > and end offsets for the misspelled word. If the offset is before or after of
> > misspelled word then we should adjust end or start of range (where misspelled
> > word is started or ended). Did I understand you correctly?
> Yes, but the code doesn't do that unconditionally.

I just tested this. If I have an empty textarea with the words:

Is Fireffox the

Querying the attributes of the text interface (HyperText interface is not exposed on a textarea accessible), I get the following results:
- Offsets 0 trough 2: no misspelling, offsets are 0 and 3
- 3 through 10: misspelled:true is exposed, startoffset is 3, endoffset is 11
- 11 through 14: no misspelling is exposed, startoffset is 11, endoffset is 16 (characters plus crlf I assume).

So, it would appear that the code does iterate over all offsets even the ones after the misspelling.

Comment 145

[alexander :surkov (:asurkov)]

Attachment: patch15

Comment 146

[alexander :surkov (:asurkov)]

(In reply to comment #143)
> (In reply to comment #141)
> > (In reply to comment #139)
> > > (In reply to comment #136)
> > > > (In reply to comment #123)
> > > > > >+      if (endHTOffset < *aHTEndOffset)
> > > > > >+        *aHTEndOffset = endHTOffset;
> > > > > >+
> > > > > >+      nsAccUtils::SetAccAttr(aAttributes, nsAccessibilityAtoms::misspelled,
> > > > > >+                             NS_LITERAL_STRING("true"));
> > > > > >+
> > > > > >+      return NS_OK;
> > > > > What about the end offset for ranges after the misspelled word?
> > > > Sorry I didn't catch the question. Could you explain more?
> > > As you loop through the ranges you update the start and end hypertext offset.
> > > But as soon as you hit a misspelled word you return early. This means that the
> > > resulting values differ depending on whether the current word is misspelled.
> > Yes. If the offset is inside of misspelled word then we want to return start
> > and end offsets for the misspelled word. If the offset is before or after of
> > misspelled word then we should adjust end or start of range (where misspelled
> > word is started or ended). Did I understand you correctly?
> Yes, but the code doesn't do that unconditionally.

Sorry still don't catch you. Could you show an example?

Comment 147

[alexander :surkov (:asurkov)]

Comment on attachment 329698 [details] [diff] [review]
patch15


> 
>+already_AddRefed<nsISelectionController>
>+nsCaretAccessible::GetSelectionControllerForNode(nsIDOMNode *aNode)
>+{
>+  if (!aNode)
>+    return nsnull;
>+
>+  nsCOMPtr<nsIPresShell> presShell = mRootAccessible->GetPresShellFor(aNode);
>+  if (!presShell)
>+    return nsnull;
>+
>+  nsCOMPtr<nsIDocument> doc = presShell->GetDocument();
>+  if (!doc)
>+    return nsnull;
>+
>+  // Get selection controller only for form controls, not for the document.
>+  nsCOMPtr<nsINode> node(do_QueryInterface(aNode));
>+  if (!node->IsNodeOfType(nsINode::eDOCUMENT))
>+    return nsnull;
>+

sorry, here is misspelling , should be if (node->IsNodeOfType(nsINode::eDOCUMENT))

Comment 148

[[e-mail]]

Comment on attachment 329698 [details] [diff] [review]
patch15

>+      if (startHTOffset > *aHTStartOffset)
>+        *aHTStartOffset = startHTOffset;
>+      if (endHTOffset < *aHTEndOffset)
>+        *aHTEndOffset = endHTOffset;
OK, so I see what's going on now: you're finding the latest end offset of all the ranges before the point and converting that to a start hypertext offset, and the earliest start offset of all the ranges after the point and converting that to an end hypertext offset. This effectively gives you the range of correctly spelled text that contains the point.

However, if the point is in a misspelled range, then you can bypass that and simply convert the range to a pair of hypertext offsets. Not only don't you need to check against any of the offsets that you calculated so far, but you can simply compute the results directly into the output parameters.

By the way, I couldn't see where the start and end offsets are initialised - is the caller supposed to set them to the start and end of the original text?

>+  if (!node->IsNodeOfType(nsINode::eDOCUMENT))
>+    return nsnull;
I'm sorry, I misread the code. Your original if (!content) test was correct.

>+const char* kAnyValue = nsnull;
>+const char* kCopyName = nsnull;
>+const char* kCopyValue = nsnull;
This doesn't do what you think - it declares three variables that point to constant strings. It's unlikely that your compiler is clever enough to notice that the value never changes. Either use #define or const char* const here.

Comment 149

[alexander :surkov (:asurkov)]

(In reply to comment #148)
> (From update of attachment 329698 [details] [diff] [review])
> >+      if (startHTOffset > *aHTStartOffset)
> >+        *aHTStartOffset = startHTOffset;
> >+      if (endHTOffset < *aHTEndOffset)
> >+        *aHTEndOffset = endHTOffset;
> OK, so I see what's going on now: you're finding the latest end offset of all
> the ranges before the point and converting that to a start hypertext offset,
> and the earliest start offset of all the ranges after the point and converting
> that to an end hypertext offset. This effectively gives you the range of
> correctly spelled text that contains the point.
> 
> However, if the point is in a misspelled range, then you can bypass that and
> simply convert the range to a pair of hypertext offsets. Not only don't you
> need to check against any of the offsets that you calculated so far, but you
> can simply compute the results directly into the output parameters.

It seems that's I do. If I found misspelled range then I fit start and end offsets and then return from the method. What do you keep in mind?

> By the way, I couldn't see where the start and end offsets are initialised - is
> the caller supposed to set them to the start and end of the original text?

start and end offsets are in and out parameters. They are initialized by caller.

> >+  if (!node->IsNodeOfType(nsINode::eDOCUMENT))
> >+    return nsnull;
> I'm sorry, I misread the code. Your original if (!content) test was correct.

Ok, I'll get back this.

> >+const char* kAnyValue = nsnull;
> >+const char* kCopyName = nsnull;
> >+const char* kCopyValue = nsnull;
> This doesn't do what you think - it declares three variables that point to
> constant strings. It's unlikely that your compiler is clever enough to notice
> that the value never changes. Either use #define or const char* const here.
> 

Ok, thank you.

Comment 150

[alexander :surkov (:asurkov)]

Attachment: patch16

Comment 151

[[e-mail]]

Comment on attachment 329777 [details] [diff] [review]
patch16

(In reply to comment #149)
> (In reply to comment #148)
> > By the way, I couldn't see where the start and end offsets are initialised - is
> > the caller supposed to set them to the start and end of the original text?
> start and end offsets are in and out parameters. They are initialized by
> caller.
Ah, in that case the code makes more sense.

Comment 152

[Aaron Leventhal]

  // XXX: fire an event for accessible of focus node of the selection. If
  // spellchecking is enabled then we will fire the number of events for
  // the same accessible for newly appended range of the selection (for every
  // misspelled word). If spellchecking is disabled (for example,
  // @spellcheck="false" on html:body) then we won't fire any event.

Usually XXX means "still to do", but we are firing an event. Is part of this unfinished? Perhaps the part where we check the spellcheck attribute?

Comment 153

[Marco Zehe (:MarcoZ)]

One question about the "end" out parameter for the getAttributes function: The following (also previously mentioned) example gives me the following results (entered in a blank textarea):

Is Fireffox the 

"Fireffox" is obviously misspelled.
Querying the attributes from before, at, and after the misspelling, I get:
- Offsets 0 to 2: Misspelling is not reported, startoffset is 0, end offset is 3, which corresponds to the first character that is misspelled.
- Offsets 3 through 11: Misspelling is reported, start offset is 3, end offset is 11. This corresponds to the actual misspelling, including the concluding space character.
- Offsets 12 through 17: No misspelling, and the start offset is 11, end offset is 17. Note that the start offset points to the space that is actually also included in the misspelling (see previous).

So, the end offset of the first non-misspelled segment points at the offset the misspelling starts, and the start offset of the later non-misspelled segment points at the last misspelled character. Is that correct?

Comment 154

[Aaron Leventhal]

1. Nit: add a little comment here to make this more readable, like this:
static nsCSSTextAttrMapItem gCSSTextAttrsMap[] = {
  //  CSS Name           CSS Value        Attr Name                   Attr Value
  //  --------           ---------        ---------                   ----------
  { "background-color",  kAnyValue,       kCopyName,                  kCopyValue },
  { "color",             kAnyValue,       kCopyName,                  kCopyValue },
  { "font-family",       kAnyValue,       kCopyName,                  kCopyValue },

2. I hate to bring it up, but roc said this would be really slow, and we 
   should ask the frame tree for info. How do we want to address that? I guess 
   let's just get this in so people can test the interface, but file a follow 
   up bug to check perf with and without text attribute usage. If there's much 
   of a difference we should use the frame tree. We can just ask the NVDA folks 
   to do the perf test, since they are well set up for that already.
>   nsAccessNode::GetComputedStyleDeclaration(EmptyString(), aElm,
>                                            getter_AddRefs(mStyleDecl));

3. Nit: please add some comments before the various code blocks in nsCSSTextAttr::get()

4. Do we deal with default attribute values yet? 
   For example, our spec says if background-color = transparent or if 
   font-style = normal we don't need to expose that attribute. We can address 
   that in a separate bug if necessary though, because normal IA2 spec actually 
   doesn't say this so it may still be worth discussion.
   However, AT vendors liked the idea of default attribute values, when I
   asked them.

5. We'll need a follow up bug to deal with aria-invalid as a text attribute. It's more like the lang attribute than anything.

I'm up to nsHyperTextAccessible.cpp now.

Comment 155

[Aaron Leventhal]

Looking at the current implementation of 
nsHyperTextAccessible::GetDefaultTextAttributes(nsIPersistentProperties **aAttributes)
If a parent node has the same attribute, I'm not sure we need to reexpose it here.
For example: <p style="text-decoration: underline">Hello<table>blah</table> bye</p>
I think exposing the default attribute on the <p> is enough, and we don't need to do it throughout the subtree for the table.
But, it would be good to get clarification from the spec or the Orca team (since default attributes are an ATK/AT-SPI thing).

Comment 156

[Aaron Leventhal]

In GetTextAttributes are start and end bounds in/our params that can only shrink? 
Could use a comment about how range calculation works overall.

I still dream of more comments. Who knows whether we will all be around when someone needs to understand this code?
For example this line:
 if (!resultLang.IsEmpty() && (aIncludeDefAttrs || lang != rootLang))
A quick comment on lines like that as to what the logic is really doing would help. Somethine like:
// aIncludeDefAttrs == PR_FALSE in ATK, because default attributes are exposed separately
//                  == PR_TRUE in IA2, so that they are included in text ranges
// In IA2 case, we will expose language attribute even if it is the same as the root language for this hypertext
//              When it is the same, either the start and end offsets will cover the entire hypertext, or
//              there will have been some other language changes in this hypertext so we need to re-expose the language
//              to show there is a change.
// In ATK we will expose the language only if it is different from the root language for this hypertext

Comment 157

[Aaron Leventhal]

How about a high level overview in GetTextAttributes(). I think I understand now that this is correct:
// 1. First we get spell check, then language, then the set of CSS-based attributes
// 2. As we get each new attribute, we pass the current start and end range as in/out parameters. In other words, as attributes are collected, the attribute range itself can only stay the same or get smaller
// Example:
// Current: range 5-10
// Adding:  range 7-12
// Result:  range 7-10

Right?

Comment 158

[Aaron Leventhal]

I'm nearly done -- now reviewing GetRangeForTextAttr() and the FitRange*() methods. I don't understand what the word "fit" means in the context and what the two FitRange*() methods return or actually do. Is there maybe a better name in English for what they do?

Comment 159

[Aaron Leventhal]

Looking at the Fit* methods. How about this to make it clearer?
        
// Return a new end offset if the given text attribute stops
// inside of this subtree.
// Do not return a new offset that comes after the original end
// offset passed in -- we do not want the range to grow
// Return PR_TRUE if a new offset was found
PRBool
nsHyperTextAccessible::FindEndOffsetInSubtree(nsIDOMNode *aCurrNode,
                                              nsTextAttr *aComparer,
                                              PRInt32 *aEndOffsetInOut)



And, instead of
    PRBool res = FitRangeForwardsOnChildren(nextNode, aComparer, aHTOffset);
    if (res)
      return res;
how about just:
    if (FindEndOffsetInSubtree(nextNode, aComparer, aEndOffset))
      return PR_TRUE;

Why is this not in a loop?
aCurrNode->GetNextSibling(getter_AddRefs(nextNode));
We only check the first 2 children?

Should it stop early even when things are equal, but we get past the original end's node?

Do we need to walk the whole subtree each time? Or if something has an embedded object character can we not walk into that?


        

  

Comment 160

[Aaron Leventhal]

Comment on attachment 329777 [details] [diff] [review]
patch16

Awaiting next patch.

Comment 161

[Aaron Leventhal]

> Do we need to walk the whole subtree each time? Or if something has an embedded
> object character can we not walk into that?
I'm going to answer my own question. No. If you have
abc*def     (where * is an embedded object char)
then you should never need to walk into the subtree.

Also replying to my question in comment #155:
> Looking at the current implementation of 
> nsHyperTextAccessible::GetDefaultTextAttributes(nsIPersistentProperties
> **aAttributes)
> If a parent node has the same attribute, I'm not sure we need to reexpose it
> here.
> For example: <p style="text-decoration: underline">Hello<table>blah</table>
> bye</p>
> I think exposing the default attribute on the <p> is enough, and we don't need
> to do it throughout the subtree for the table.
> But, it would be good to get clarification from the spec or the Orca team
> (since default attributes are an ATK/AT-SPI thing).
Actually, I do think we probably want to repeat in descendants, now that I think about it. While we repeat stuff in descendants we were going to avoid extra text attributes via the default attribute values (bug 445511).

Comment 162

[Marco Zehe (:MarcoZ)]

Just found out that IA"_TEXT_ATTRIBUTE_CHANGED events are not firing when correcting a spelling mistake or causing one to appear. The appropriate text removed and text inserted events are fired, but not the text attribute changed one.

I also don't see this in DomI in that Accessible Event viewer. There, i also see the text insertions and removals, caret moves etc., but no text attribute changes. So this part of the patch may not be working yet!

Comment 163

[alexander :surkov (:asurkov)]

(In reply to comment #152)
>   // XXX: fire an event for accessible of focus node of the selection. If
>   // spellchecking is enabled then we will fire the number of events for
>   // the same accessible for newly appended range of the selection (for every
>   // misspelled word). If spellchecking is disabled (for example,
>   // @spellcheck="false" on html:body) then we won't fire any event.
> 
> Usually XXX means "still to do", but we are firing an event. Is part of this
> unfinished? Perhaps the part where we check the spellcheck attribute?
> 

XXX because it's undesired behaviour. Will file following up bug for this.

Comment 164

[alexander :surkov (:asurkov)]

(In reply to comment #153)
> One question about the "end" out parameter for the getAttributes function: The
> following (also previously mentioned) example gives me the following results
> (entered in a blank textarea):
> 
> Is Fireffox the 
> 
> "Fireffox" is obviously misspelled.
> Querying the attributes from before, at, and after the misspelling, I get:
> - Offsets 0 to 2: Misspelling is not reported, startoffset is 0, end offset is
> 3, which corresponds to the first character that is misspelled.
> - Offsets 3 through 11: Misspelling is reported, start offset is 3, end offset
> is 11. This corresponds to the actual misspelling, including the concluding
> space character.
> - Offsets 12 through 17: No misspelling, and the start offset is 11, end offset
> is 17. Note that the start offset points to the space that is actually also
> included in the misspelling (see previous).
> 
> So, the end offset of the first non-misspelled segment points at the offset the
> misspelling starts, and the start offset of the later non-misspelled segment
> points at the last misspelled character. Is that correct?
> 

MarcoZ, do you mean we get intervals [ , ) but we want [ , ]?

Comment 165

[alexander :surkov (:asurkov)]

(In reply to comment #156)

> For example this line:
>  if (!resultLang.IsEmpty() && (aIncludeDefAttrs || lang != rootLang))
> A quick comment on lines like that as to what the logic is really doing would
> help. Somethine like:
> // aIncludeDefAttrs == PR_FALSE in ATK, because default attributes are exposed
> separately
> //                  == PR_TRUE in IA2, so that they are included in text ranges
> // In IA2 case, we will expose language attribute even if it is the same as the
> root language for this hypertext
> //              When it is the same, either the start and end offsets will
> cover the entire hypertext, or
> //              there will have been some other language changes in this
> hypertext so we need to re-expose the language
> //              to show there is a change.
> // In ATK we will expose the language only if it is different from the root
> language for this hypertext
> 

I'm not sure I would like to add ATK/IA2 comments into crossplatform code. And especially into language calculation code. IDL file holds comments about aIncludeDefAttrs. I put comments for that line that adds 'language' attribute.

Comment 166

[alexander :surkov (:asurkov)]

(In reply to comment #157)
> How about a high level overview in GetTextAttributes(). I think I understand
> now that this is correct:
> // 1. First we get spell check, then language, then the set of CSS-based
> attributes
> // 2. As we get each new attribute, we pass the current start and end range as
> in/out parameters. In other words, as attributes are collected, the attribute
> range itself can only stay the same or get smaller
> // Example:
> // Current: range 5-10
> // Adding:  range 7-12
> // Result:  range 7-10
> 
> Right?
> 

Right

Comment 167

[Marco Zehe (:MarcoZ)]

(In reply to comment #164)
> (In reply to comment #153)
> > One question about the "end" out parameter for the getAttributes function: The
> > following (also previously mentioned) example gives me the following results
> > (entered in a blank textarea):
> > 
> > Is Fireffox the 
> > 
> > "Fireffox" is obviously misspelled.
> > Querying the attributes from before, at, and after the misspelling, I get:
> > - Offsets 0 to 2: Misspelling is not reported, startoffset is 0, end offset is
> > 3, which corresponds to the first character that is misspelled.
> > - Offsets 3 through 11: Misspelling is reported, start offset is 3, end offset
> > is 11. This corresponds to the actual misspelling, including the concluding
> > space character.
> > - Offsets 12 through 17: No misspelling, and the start offset is 11, end offset
> > is 17. Note that the start offset points to the space that is actually also
> > included in the misspelling (see previous).
> > 
> > So, the end offset of the first non-misspelled segment points at the offset the
> > misspelling starts, and the start offset of the later non-misspelled segment
> > points at the last misspelled character. Is that correct?
> MarcoZ, do you mean we get intervals [ , ) but we want [ , ]?

That was my question: Do we want the intervals that are not misspelled to poit at the last or first characters that are misspelled, whereas the misspelling itself only points at the first and last characters of the misspelling? For example, I would have expected the first range in my example to go from 0 to 2 instead of 3, and the last to go from 12 to 17 instead of 11 to 17.

So I'm asking: Is the current behaviour what you wanted, or not?

Comment 168

[alexander :surkov (:asurkov)]

(In reply to comment #167)

> That was my question: Do we want the intervals that are not misspelled to poit
> at the last or first characters that are misspelled, whereas the misspelling
> itself only points at the first and last characters of the misspelling? For
> example, I would have expected the first range in my example to go from 0 to 2
> instead of 3, and the last to go from 12 to 17 instead of 11 to 17.
> 
> So I'm asking: Is the current behaviour what you wanted, or not?
> 

I would assume intervals should be closed but we may want to save this for following up bug.

Comment 169

[alexander :surkov (:asurkov)]

(In reply to comment #159)
> Looking at the Fit* methods. How about this to make it clearer?
> 
> // Return a new end offset if the given text attribute stops
> // inside of this subtree.
> // Do not return a new offset that comes after the original end
> // offset passed in -- we do not want the range to grow
> // Return PR_TRUE if a new offset was found
> PRBool
> nsHyperTextAccessible::FindEndOffsetInSubtree(nsIDOMNode *aCurrNode,
>                                               nsTextAttr *aComparer,
>                                               PRInt32 *aEndOffsetInOut)

I fixed the java-style comment in header.

> 
> Why is this not in a loop?
> aCurrNode->GetNextSibling(getter_AddRefs(nextNode));
> We only check the first 2 children?

I assume because FindEndOffsetInSubtree is called for the nextNode. So we run all children.

> Should it stop early even when things are equal, but we get past the original
> end's node?
> 
> Do we need to walk the whole subtree each time? Or if something has an embedded
> object character can we not walk into that?

Good idea. File one more bug for optimization?

Comment 170

[alexander :surkov (:asurkov)]

(In reply to comment #143)

> (In reply to comment #142)
> > (In reply to comment #136)
> > 
> > > > >+  readonly attribute short type;
> > > > Personally I'd prefer SelectionType here - you would have to include
> > > > nsISelectionController.idl to pick up the typedef.
> > > ok
> > SelectionType is defined for C++ in nsISelectionController.idl so it's
> > invisible for public mehtods in nsISelection2. Also selection type constains
> > declared in nsISelectionController.idl are declared as short. Not sure I would
> > like here to interfere in the code of nsISelectionController.idl. Though if you
> > like then I'll file new bug for this and put the patch there.
> Oh, that's silly. Do please file a new bug.
> 

I filed bug 445676

Comment 171

[alexander :surkov (:asurkov)]

(In reply to comment #169)

> 
> Good idea. File one more bug for optimization?
> 

I filed bug 445677

Comment 172

[Aaron Leventhal]

<snip>
> I'm not sure I would like to add ATK/IA2 comments into crossplatform code. And
> especially into language calculation code. IDL file holds comments about
> aIncludeDefAttrs. I put comments for that line that adds 'language' attribute.

I think, do whatever it takes so Marco, myself and any future mystery person can understand it without any struggle. For me, it's okay to describe what the code is "really" doing.

Comment 173

[alexander :surkov (:asurkov)]

Attachment: patch17

Comment 174

[alexander :surkov (:asurkov)]

(In reply to comment #162)
> Just found out that IA"_TEXT_ATTRIBUTE_CHANGED events are not firing when
> correcting a spelling mistake or causing one to appear. The appropriate text
> removed and text inserted events are fired, but not the text attribute changed
> one.
> 
> I also don't see this in DomI in that Accessible Event viewer. There, i also
> see the text insertions and removals, caret moves etc., but no text attribute
> changes. So this part of the patch may not be working yet!
> 

They are fired. I added mochitest. Possibly you need full rebuild. Because when I tried to build things separately it doesn't work with me (spellcheckin stopped to work at all).

Comment 175

[Marco Zehe (:MarcoZ)]

>+  // Remove 'this' registered as selection listener for the spellcheck
>+  // selection.
>+  nsCOMPtr<nsISelection> spellcheckSel;
>+  controller->GetSelection(nsISelectionController::SELECTION_SPELLCHECK,
>+                           getter_AddRefs(spellcheckSel));
>+  selPrivate = do_QueryInterface(spellcheckSel);
> 
>   return selPrivate->RemoveSelectionListener(this);

No null checks here? You do them below when you add the listeners.

Comment 176

[alexander :surkov (:asurkov)]

Attachment: patch18

additional comments+MarcoZ comment

Comment 177

[Aaron Leventhal]

Comment on attachment 329972 [details] [diff] [review]
patch18

Surkov and I chatted about this, and we need to get this in, because it's just such a large patch.

There is one area that I see needs to change, which is the algorithm that gets the size of the current range. 
It does not need to recursively walk the DOM tree. The accessible children contain all the text accessibles which can possible affect the size of the range. So the range size calculator becomes a simple loop, iterating through child accessibles. For language it will have to get the DOM node and make sure it looks at parents up until mDOMNode.

This refactoring needs to be done ASAP after this patch goes in, before we do any major testing. It could be done in bug 445677.

Comment 178

[alexander :surkov (:asurkov)]

Attachment: patch19

for checkin

Comment 179

[Marco Zehe (:MarcoZ)]

Pushed on Surkov's behalf in changeset:
http://hg.mozilla.org/mozilla-central/index.cgi/rev/0aa1bf42562a

Thanks everyone for this great cooperation!

Comment 180

[Marco Zehe (:MarcoZ)]

Verified in Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1a1pre) Gecko/2008071803 Minefield/3.1a1pre

Comment 181

[Firefox Bug Husbandry Bot]

https://bugzilla.mozilla.org/show_bug.cgi?id=1527381