Closed Bug 345759 Opened 16 years ago Closed 14 years ago

Spell as you type not accessible

Categories

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

x86
All
defect

Tracking

()

VERIFIED FIXED
mozilla1.9.1a1

People

(Reporter: deangelo, Assigned: surkov)

References

(Blocks 1 open bug)

Details

(Keywords: access, Whiteboard: orca:high)

Attachments

(2 files, 26 obsolete files)

5.22 KB, text/html
Details
107.45 KB, patch
Details | Diff | Splinter Review
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.
Blocks: themea11y
Flags: blocking-firefox2?
Blocks: fox2access
No longer blocks: themea11y
No longer blocks: fox2access
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.
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.
Flags: blocking-firefox2? → blocking-firefox2-
Target Milestone: --- → Firefox 3 alpha2
Blocks: keya11y
Blocks: textattra11y
No longer blocks: keya11y
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.
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));
nsHyperTextAccessible might want to use nsISelection2 to get the ranges of misspelled words for a given interval.
Assignee: nobody → 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.
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.

Attached patch draft patch (obsolete) — Splinter Review
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.
Attachment #247167 - Flags: review?(aaronleventhal)
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.
Attachment #247167 - Flags: review?(aaronleventhal) → review+
Comment on attachment 247167 [details] [diff] [review]
draft patch

Sorry, meant to minus that until those issues are dealt with.
Attachment #247167 - Flags: review+ → review-
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.
Blocks: lsr
Attached patch patch v2 (obsolete) — Splinter Review
Comments addressed.
Attachment #247167 - Attachment is obsolete: true
Attachment #269157 - Flags: review?(aaronleventhal)
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.
Attachment #269157 - Flags: review?(aaronleventhal) → review-
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.

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;
     }
   }
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.
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.
Yes, we need extra work for descendant hyper text accessible.
:(
OS: Windows XP → All
Depends on: 386026
split bug 386026 for easier code review
Assignee: ginn.chen → surkov.alexander
Attached patch wip (obsolete) — Splinter Review
Attachment #269157 - Attachment is obsolete: true
Attached patch patch (obsolete) — Splinter Review
Attachment #289233 - Attachment is obsolete: true
Attachment #289303 - Flags: review?(aaronleventhal)
Attachment #289303 - Flags: review?(ginn.chen)
Status: NEW → ASSIGNED
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

Assignee: surkov.alexander → aaronleventhal
Status: ASSIGNED → NEW
Component: Disability Access → Disability Access APIs
Flags: blocking-firefox2-
Product: Firefox → Core
QA Contact: disability.access → accessibility-apis
Target Milestone: Firefox 3 alpha2 → ---
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
Attachment #289303 - Flags: review?(ginn.chen)
Attachment #289303 - Flags: review?(aaronleventhal)
Attachment #289303 - Flags: review-
Attached patch wip2 (obsolete) — Splinter Review
Assignee: aaronleventhal → surkov.alexander
Attachment #289303 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attached patch patch2 (obsolete) — Splinter Review
ready for review
Attachment #289899 - Attachment is obsolete: true
Attachment #289911 - Flags: review?(aaronleventhal)
Attached patch wip3 (obsolete) — Splinter Review
+css text attributes
Attached patch patch3 (obsolete) — Splinter Review
Attachment #289911 - Attachment is obsolete: true
Attachment #289953 - Attachment is obsolete: true
Attachment #289960 - Flags: review?(aaronleventhal)
Attachment #289911 - Flags: review?(aaronleventhal)
Attachment #289960 - Flags: review?(ginn.chen)
Attached patch wip4 (obsolete) — Splinter Review
+misspelling text attribute change event
Attached patch patch4 (obsolete) — Splinter Review
Attachment #289960 - Attachment is obsolete: true
Attachment #289986 - Attachment is obsolete: true
Attachment #290023 - Flags: review?(aaronleventhal)
Attachment #289960 - Flags: review?(ginn.chen)
Attachment #289960 - Flags: review?(aaronleventhal)
Attachment #290023 - Flags: review?(ginn.chen)
Attachment #290023 - Flags: superreview?(roc)
Attachment #290023 - Flags: review?(roc)
+   * 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.
(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?
+    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.
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.
(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.
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.
(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.
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.
(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)?
Attached patch patch5 (obsolete) — Splinter Review
fixed roc's comments, aaron's first comment, updated to trunk
Attachment #290023 - Attachment is obsolete: true
Attachment #290685 - Flags: review?(aaronleventhal)
Attachment #290023 - Flags: superreview?(roc)
Attachment #290023 - Flags: review?(roc)
Attachment #290023 - Flags: review?(ginn.chen)
Attachment #290023 - Flags: review?(aaronleventhal)
> 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.
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 on attachment 290685 [details] [diff] [review]
patch5

Clearing request until last question answered.
Attachment #290685 - Flags: review?(aaronleventhal)
Attached patch patch6 (obsolete) — Splinter Review
Attachment #290685 - Attachment is obsolete: true
Attachment #291642 - Flags: review?(aaronleventhal)
Attachment #291642 - Flags: superreview?(roc)
Attachment #291642 - Flags: review?(ginn.chen)
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.
Attachment #291642 - Flags: superreview?(roc)
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?
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.
(In reply to comment #46)

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

addrefed can't be converted to pointer directly
Attached patch patch (obsolete) — Splinter Review
Aaron, Ginn please put your thoughts about code organization?
Attachment #291642 - Attachment is obsolete: true
Attachment #292279 - Flags: review?(aaronleventhal)
Attachment #291642 - Flags: review?(ginn.chen)
Attachment #291642 - Flags: review?(aaronleventhal)
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.


And, EVENT_TEXT_ATTRIBUTE_CHANGED is not handled by
nsAccessibleWrap::FireAccessibleEvent

Do you want to file a follow-up bug for it?
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.
Why not move all the new text attribute related methods to nsTextUtils? Why add any new methods to nsHyperTextAccessible?
Adding orca:high to this since it helps a lot with getting text attributes.
Whiteboard: orca:high
(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.
(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.
> 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
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.
  
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.
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.
Aaron, how will indentations like when replying to a message in Thunderbird, and quoting it, be reflected?
Marco, great question. File a separate bug for that one, we'll need to discuss it.
Attachment #292279 - Flags: review?(aaronleventhal) → review-
(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.
Attached patch patch8 (obsolete) — Splinter Review
Attachment #292279 - Attachment is obsolete: true
Attachment #293715 - Flags: review?(aaronleventhal)
(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.
(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
(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()
Attached file test case (obsolete) —
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.
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.
(In reply to comment #68)

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

How it's possible? How can I reproduce this?
Just run with JAWS for a while.
Can't reproduce with Jaws 8
Maybe I didnt rebuild enough directories. I'll see if I can repro. Anyway, a null check there would be good.
Attachment #293715 - Flags: review?(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));
Attachment #293715 - Flags: review?(ginn.chen) → review+
Attached patch patch9 (obsolete) — Splinter Review
updated to trunk with Ginn's comment
Attachment #293715 - Attachment is obsolete: true
Attachment #298915 - Flags: review?(aaronleventhal)
Attachment #293715 - Flags: review?(aaronleventhal)
(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.
Still looking for gCSSTextAttrsMap to get a simpler tabular format and remove redundancies where possible.
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.
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.
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 ?
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() ?
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.
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().
  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.




  	

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));

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.
I wonder if we're still getting selection notifications after the caret accessible has been shutdown or destroyed.
Attachment #299061 - Flags: review?(surkov.alexander)
Comment on attachment 299061 [details] [diff] [review]
Another null check and move ClearControlSelectionListener() up

This was supposed to be in bug 405951.
Attachment #299061 - Attachment is obsolete: true
Attachment #299061 - Flags: review?(surkov.alexander)
Attachment #298915 - Flags: review?(aaronleventhal) → review-
Please add more comments to GetRangeForTextAttr(), because I don't find the logic totally obvious.
(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 ;)
(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
(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
(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
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?

(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

(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.
> 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.
(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. 
(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 ..
(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?
> 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.
Hmm, Roc tells me that GetComputedStyle is slow and unreliable. We should get what we want from the frame.
Attached patch patch10 (obsolete) — Splinter Review
next version, fixed some Aaron's comments, updated to trunk
Attachment #298915 - Attachment is obsolete: true
Attachment #300297 - Flags: review?(aaronleventhal)
(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.
(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.
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.

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.
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.
Attachment #300297 - Flags: review?(aaronleventhal) → review-
(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?
(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?
(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?
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.
(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.
Attached patch patch (obsolete) — Splinter Review
Attachment #300297 - Attachment is obsolete: true
Attachment #301618 - Flags: review?(aaronleventhal)
Comment on attachment 301618 [details] [diff] [review]
patch

I need to fit range backward as well
Attachment #301618 - Flags: review?(aaronleventhal)
Attached file testcase2
next version of testcase, added check for ranges calculation through the children
Attachment #293719 - Attachment is obsolete: true
Attached patch patch12 (obsolete) — Splinter Review
Attachment #301618 - Attachment is obsolete: true
Attachment #301624 - Flags: review?(aaronleventhal)
Attached patch patch13 (obsolete) — Splinter Review
added some commets for range calculation
Attachment #301624 - Attachment is obsolete: true
Attachment #301842 - Flags: review?(aaronleventhal)
Attachment #301624 - Flags: review?(aaronleventhal)
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.
Priority: -- → P2
Target Milestone: --- → mozilla1.9
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.
Blocks: 191a11y
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.
Attached patch patch14 (obsolete) — Splinter Review
updated to trunk
Attachment #301842 - Attachment is obsolete: true
Attachment #329461 - Flags: review?(aaronleventhal)
Attachment #301842 - Flags: review?(aaronleventhal)
Attachment #329461 - Flags: superreview?(neil)
Attachment #329461 - Flags: review?(Olli.Pettay)
Flags: wanted1.9.1?
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 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.
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.
(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!
(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.
 
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?
Please add some comments to FitRange*OnChildren. The code is starting to look clearer but some comments will make it even better.
(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?
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.
(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.
(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. 
(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 on attachment 329461 [details] [diff] [review]
patch14

r=me for the selection type thing.
Attachment #329461 - Flags: review?(Olli.Pettay) → review+
(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.
(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
(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.
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.
(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.
(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.
(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?
(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.
(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.
(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.
Attached patch patch15 (obsolete) — Splinter Review
Attachment #329461 - Attachment is obsolete: true
Attachment #329698 - Flags: superreview?(neil)
Attachment #329698 - Flags: review?(aaronleventhal)
Attachment #329461 - Flags: superreview?(neil)
Attachment #329461 - Flags: review?(aaronleventhal)
(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 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 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.
(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.
Attached patch patch16 (obsolete) — Splinter Review
Attachment #329698 - Attachment is obsolete: true
Attachment #329777 - Flags: superreview?(neil)
Attachment #329777 - Flags: review?(aaronleventhal)
Attachment #329698 - Flags: superreview?(neil)
Attachment #329698 - Flags: review?(aaronleventhal)
Flags: in-testsuite?
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.
Attachment #329777 - Flags: superreview?(neil) → superreview+
  // 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?

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?
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.
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).
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






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?
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?
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 on attachment 329777 [details] [diff] [review]
patch16

Awaiting next patch.
Attachment #329777 - Flags: review?(aaronleventhal)
> 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).

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!
(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.
(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 [ , ]?
(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.
(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
(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?
(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.
(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?
(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
(In reply to comment #169)

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

I filed bug 445677
<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.

Attached patch patch17 (obsolete) — Splinter Review
Attachment #329777 - Attachment is obsolete: true
Attachment #329959 - Flags: review?(aaronleventhal)
(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).
>+  // 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.
Attached patch patch18 (obsolete) — Splinter Review
additional comments+MarcoZ comment
Attachment #329959 - Attachment is obsolete: true
Attachment #329972 - Flags: review?(aaronleventhal)
Attachment #329959 - Flags: review?(aaronleventhal)
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.
Attachment #329972 - Flags: review?(aaronleventhal) → review+
Attached patch patch19Splinter Review
for checkin
Attachment #329972 - Attachment is obsolete: true
Pushed on Surkov's behalf in changeset:
http://hg.mozilla.org/mozilla-central/index.cgi/rev/0aa1bf42562a

Thanks everyone for this great cooperation!
Status: ASSIGNED → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
Target Milestone: mozilla1.9 → mozilla1.9.1a1
Verified in Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9.1a1pre) Gecko/2008071803 Minefield/3.1a1pre
Status: RESOLVED → VERIFIED
Depends on: 448836
Flags: in-testsuite? → in-testsuite+
Depends on: 451376
Depends on: 454538
Depends on: 454654
Depends on: 466629
Flags: wanted1.9.1?
You need to log in before you can comment on or make changes to this bug.