Last Comment Bug 745428 - densify nsTextAccessible
: densify nsTextAccessible
Status: RESOLVED FIXED
[good first bug][mentor=trev.saunders...
:
Product: Core
Classification: Components
Component: Disability Access APIs (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla15
Assigned To: Mark Capella [:capella]
:
: alexander :surkov
Mentors:
Depends on: 765110
Blocks: densifya11y
  Show dependency treegraph
 
Reported: 2012-04-14 01:08 PDT by Trevor Saunders (:tbsaunde)
Modified: 2012-06-27 06:52 PDT (History)
3 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch (v1) (32.06 KB, patch)
2012-05-17 19:53 PDT, Mark Capella [:capella]
tbsaunde+mozbugs: feedback+
Details | Diff | Splinter Review
Patch (v2) (31.60 KB, patch)
2012-05-22 22:12 PDT, Mark Capella [:capella]
no flags Details | Diff | Splinter Review
Patch (v3) (32.37 KB, patch)
2012-05-23 03:26 PDT, Mark Capella [:capella]
surkov.alexander: review+
Details | Diff | Splinter Review
Patch (v4) (31.47 KB, patch)
2012-05-23 05:41 PDT, Mark Capella [:capella]
no flags Details | Diff | Splinter Review

Description Trevor Saunders (:tbsaunde) 2012-04-14 01:08:57 PDT
similar to bug 742695 with base/nsTextAccessible.cpp/h this time.  Make sure to update the nsTextAccessibleWraps to in msaa/ atk/ mac/ and other/
Comment 1 alexander :surkov 2012-04-15 17:43:05 PDT
Trevor, I was thinking to move nsTextAccessible into nsBaseWidgetAccessible and densify nsBaseWidgetAccessible instead.

For XUL/HTML parts we could XULBaseWidgetAccessible instead of nsXULTextAccessible, same for HTML. Those files keeps more than just accessible for text nodes.

Sounds good?
Comment 2 Trevor Saunders (:tbsaunde) 2012-04-15 19:28:07 PDT
(In reply to alexander :surkov from comment #1)
> Trevor, I was thinking to move nsTextAccessible into nsBaseWidgetAccessible
> and densify nsBaseWidgetAccessible instead.

hm, I was actually considering breaking nsBaseWidgetAccessible.cpp up, by class with some only needing a header.

> For XUL/HTML parts we could XULBaseWidgetAccessible instead of
> nsXULTextAccessible, same for HTML. Those files keeps more than just
> accessible for text nodes.

renaming them certainly makes sense, but BaseWidget seems kind of funny is there a reason to not do just XULWidgetAccessible?

> Sounds good?

I tend to prefer to keep things in seperate files when we can, so you can need to rebuild less
Comment 3 alexander :surkov 2012-04-15 20:39:09 PDT
(In reply to Trevor Saunders (:tbsaunde) from comment #2)
> (In reply to alexander :surkov from comment #1)
> > Trevor, I was thinking to move nsTextAccessible into nsBaseWidgetAccessible
> > and densify nsBaseWidgetAccessible instead.
> 
> hm, I was actually considering breaking nsBaseWidgetAccessible.cpp up, by
> class with some only needing a header.

we should get many many small files, especially if we are going to do the same for form controls. But it's not necessary bad.

Taking into account Widget doesn't seem to be a correct term (especially if we are going to merge with TextAccessible.h) and I can't think of any good term then perhaps it's worth to do the breaking now. But if we would have a proper name then I wouldn't break right now I think.
Comment 4 alexander :surkov 2012-04-25 04:47:01 PDT
Trevor, ping.
Comment 5 Trevor Saunders (:tbsaunde) 2012-04-28 22:56:00 PDT
(In reply to alexander :surkov from comment #3)
> (In reply to Trevor Saunders (:tbsaunde) from comment #2)
> > (In reply to alexander :surkov from comment #1)
> > > Trevor, I was thinking to move nsTextAccessible into nsBaseWidgetAccessible
> > > and densify nsBaseWidgetAccessible instead.
> > 
> > hm, I was actually considering breaking nsBaseWidgetAccessible.cpp up, by
> > class with some only needing a header.
> 
> we should get many many small files, especially if we are going to do the
> same for form controls. But it's not necessary bad.

I'm not sure I want to put absolutely every class in its own file, so perhaps not that many files.  I'm not sure how bad many small files would really be either, it might be nice for somethings like knowing what is in a file.

> Taking into account Widget doesn't seem to be a correct term (especially if
> we are going to merge with TextAccessible.h) and I can't think of any good
> term then perhaps it's worth to do the breaking now. But if we would have a
> proper name then I wouldn't break right now I think.

ok, I can't think of a good name either.

My first thought about breaking up xul is that nsXULLinkAccessible seems big enough that it should probably get its own file.  Maybe the list item / bullet / list itself html accessibles should have one file together seperate from the other text accessibles?
Comment 6 alexander :surkov 2012-04-30 01:52:38 PDT
(In reply to Trevor Saunders (:tbsaunde) from comment #5)

> My first thought about breaking up xul is that nsXULLinkAccessible seems big
> enough that it should probably get its own file.  Maybe the list item /
> bullet / list itself html accessibles should have one file together seperate
> from the other text accessibles?

I agree about list accessibles, less certain about XUL link accessible.

since nsTextAccessible is used as part of nsHTMLTextAccessible then I suggest to morph this bug to merge these two classes.

after that we can rename nsHTMLTextAccessible/nsXULTextAccessible files to HTMLElementAccessible/XULElementAccessible.
Comment 7 alexander :surkov 2012-04-30 01:54:14 PDT
How to fix:

1) rename nsTextAccessible to TextAccessible (put under mozilla::a11y namespace), don't forget about wrappers as per bug description (comment #0).
2) move nsHTMLTextAccessible class logic into TextAccessible
3) replace instances of nsHTMLTextAccessible to TextAccessbile
4) rename CreateHTMLTextAccessible to CreateTextAccessible
Comment 8 Mark Capella [:capella] 2012-05-17 13:55:48 PDT
Most of this is straight-forward / consistent with previous De-ns-ify bugs I've completed. I have some questions regarding item 2/3) above ... please elaborate a little and correct me in the following areas:

After merging the nsHTMLTextAccessible class and members, from the original two files nsHTMLTextAccessible.h/.cpp into the newly renamed TextAccessible.h/.cpp, the following four classes will still exist...
  class nsHTMLHRAccessible : public nsLeafAccessible
  class nsHTMLBRAccessible : public nsLeafAccessible
  class nsHTMLLabelAccessible : public nsHyperTextAccessibleWrap
  class nsHTMLOutputAccessible : public nsHyperTextAccessibleWrap

Did you mean for me to merge these classes also and remove the nsHTMLTextAccessible.h/.cpp files entirely? If not, would there be a better / different file name for this collection now?

In either case, I'll need some more explanation / help merging the classes inheritance functions and the NativeState() and NativeRole() members, as I have little understanding of how this works yet.
Comment 9 Trevor Saunders (:tbsaunde) 2012-05-17 14:32:49 PDT
(In reply to Mark Capella [:capella] from comment #8)
> Most of this is straight-forward / consistent with previous De-ns-ify bugs
> I've completed. I have some questions regarding item 2/3) above ... please
> elaborate a little and correct me in the following areas:
> 
> After merging the nsHTMLTextAccessible class and members, from the original
> two files nsHTMLTextAccessible.h/.cpp into the newly renamed
> TextAccessible.h/.cpp, the following four classes will still exist...
>   class nsHTMLHRAccessible : public nsLeafAccessible
>   class nsHTMLBRAccessible : public nsLeafAccessible
>   class nsHTMLLabelAccessible : public nsHyperTextAccessibleWrap
>   class nsHTMLOutputAccessible : public nsHyperTextAccessibleWrap

yes

> Did you mean for me to merge these classes also and remove the
> nsHTMLTextAccessible.h/.cpp files entirely? If not, would there be a better
> / different file name for this collection now?

no, and not sure

> In either case, I'll need some more explanation / help merging the classes
> inheritance functions and the NativeState() and NativeRole() members, as I
> have little understanding of how this works yet.

since we never create a nsTextAccessible you can just put the code from its impl where the call in nsHTMLTextAccessible members calls up into the super class.
Comment 10 Mark Capella [:capella] 2012-05-17 19:53:18 PDT
Created attachment 624997 [details] [diff] [review]
Patch (v1)

First pass ... asking mentor for feedback, but surkov: can steal if he wants to ... builds ok on WIN and mochitests all pass ... the tests also seems to run faster, but that's probably me being optimistic :)
Comment 11 alexander :surkov 2012-05-17 20:18:56 PDT
(In reply to Trevor Saunders (:tbsaunde) from comment #9)

> > Did you mean for me to merge these classes also and remove the
> > nsHTMLTextAccessible.h/.cpp files entirely? If not, would there be a better
> > / different file name for this collection now?
> 
> no, and not sure

yep, let's deal with it in followup. We could end up with something like
base/WidgetsAccessible (leaf, enum, linkable)
html/HTMLWidgetsAccessible (br, hr, label, output)
xul/XULWidgetsAccessible (link, tooltip)
Comment 12 alexander :surkov 2012-05-17 20:22:25 PDT
btw, it's sort of late for this stage but wouldn't it be more correct to rename TextAccessible to TextLeafAccessible because text accessible is an accessible implementing text interfaces (we also refer to it as hypertext accessible to avoid confusion). We can do that in follow up.

Trevor, what do you think on this?
Comment 13 Trevor Saunders (:tbsaunde) 2012-05-21 12:59:25 PDT
(In reply to alexander :surkov from comment #12)
> btw, it's sort of late for this stage but wouldn't it be more correct to
> rename TextAccessible to TextLeafAccessible because text accessible is an
> accessible implementing text interfaces (we also refer to it as hypertext
> accessible to avoid confusion). We can do that in follow up.

sounds nice
Comment 14 Trevor Saunders (:tbsaunde) 2012-05-21 13:37:40 PDT
Comment on attachment 624997 [details] [diff] [review]
Patch (v1)

>--- a/accessible/src/base/NotificationController.cpp
>+++ b/accessible/src/base/NotificationController.cpp
>@@ -39,17 +39,17 @@
> #include "NotificationController.h"
> 
> #include "Accessible-inl.h"
> #include "nsAccessibilityService.h"
> #include "nsAccUtils.h"
> #include "nsCoreUtils.h"
> #include "nsDocAccessible.h"
> #include "nsEventShell.h"
>-#include "nsTextAccessible.h"
>+#include "TextAccessible.h"
> #include "FocusManager.h"
> #include "Role.h"
> #include "TextUpdater.h"

nit, out of order

> #include "nsARIAMap.h"
> #include "nsCoreUtils.h"
> #include "nsDocAccessible.h"
> #include "nsHyperTextAccessible.h"
> #include "nsIAccessibleTypes.h"
>-#include "nsTextAccessible.h"
>+#include "TextAccessible.h"
> #include "Role.h"
> #include "States.h"

nit, out of order

>+TextAccessible::NativeState()
>+{
>+  PRUint64 state = nsAccessible::NativeState();

use nsLinkableAccessible::NativeState()

>+
>+  nsDocAccessible* docAccessible = Document();
>+  if (docAccessible) {
>+     PRUint64 docState = docAccessible->State();
>+     if (0 == (docState & states::EDITABLE)) {
>+       state |= states::READONLY; // Links not focusable in editor
>+     }
>+  }

please update for the patch taht removes this.

>-  nsTextAccessible(nsIContent* aContent, nsDocAccessible* aDoc);
>+  TextAccessible(nsIContent* aContent, nsDocAccessible* aDoc);

nit, add virtual destructor please

> 
>   // nsAccessible
>   virtual mozilla::a11y::role NativeRole();

mozilla::a11y:: shouldn't be needed now

> #define _nsHTMLTextAccessible_H_
> 
>-#include "nsTextAccessibleWrap.h"
>+#include "TextAccessibleWrap.h"

why is this needed?

this eems fine, but would be nice if you fixed nits before getting review.
Comment 15 alexander :surkov 2012-05-22 02:55:33 PDT
(In reply to Trevor Saunders (:tbsaunde) from comment #13)
> (In reply to alexander :surkov from comment #12)
> > btw, it's sort of late for this stage but wouldn't it be more correct to
> > rename TextAccessible to TextLeafAccessible because text accessible is an
> > accessible implementing text interfaces (we also refer to it as hypertext
> > accessible to avoid confusion). We can do that in follow up.
> 
> sounds nice

Mark, would you mind to change TextAccessible to TextLeafAccessible? (sorry for asking this late).
Comment 16 Mark Capella [:capella] 2012-05-22 03:01:47 PDT
That's not a bad change, but we will wind up with long names like TextLeafAccessibleWrap.cpp and mozilla_a11y_TextLeafAccessibleWrap_h__ if that's ok ...
Comment 17 alexander :surkov 2012-05-22 03:06:39 PDT
(In reply to Mark Capella [:capella] from comment #16)
> That's not a bad change, but we will wind up with long names like
> TextLeafAccessibleWrap.cpp and mozilla_a11y_TextLeafAccessibleWrap_h__ if
> that's ok ...

yeah, they are 4 chars longer :) but the name seems more correct. (Also we use textLeaf here and there like nsAccessible::AsTextLeaf()).
Comment 18 Mark Capella [:capella] 2012-05-22 22:12:41 PDT
Created attachment 626325 [details] [diff] [review]
Patch (v2)

Not asking for a review yet, as I had to re-do the patch from the start ... the file re-naming seemed to confuse MQ ... this version should be all we had before and now includes the TextLeafAccessible naming convention. (Please review carefully.) As usual, this builds and tests ok as-is.

re: comment 14 ...please update for the patch taht removes this.

I'm not sure what patch is being referred to here, please elaborate.
Comment 19 alexander :surkov 2012-05-22 22:21:37 PDT
(In reply to Mark Capella [:capella] from comment #18)
> Created attachment 626325 [details] [diff] [review]
> Patch (v2)
> 
> Not asking for a review yet, as I had to re-do the patch from the start ...
> the file re-naming seemed to confuse MQ ... this version should be all we
> had before and now includes the TextLeafAccessible naming convention.
> (Please review carefully.) As usual, this builds and tests ok as-is.

I didn't understand whether I should review or not :)

> re: comment 14 ...please update for the patch taht removes this.
> 
> I'm not sure what patch is being referred to here, please elaborate.

I think Trevor meant you should update your local copy to trunk.
Comment 20 Mark Capella [:capella] 2012-05-22 22:26:37 PDT
Comment on attachment 626325 [details] [diff] [review]
Patch (v2)

Ok, let's ask you/surkov for a review, but I also welcome further comments from Trev. Since I started over on the patch, I first pulled -u and thus this has been re-based.
Comment 21 alexander :surkov 2012-05-23 00:04:44 PDT
Comment on attachment 626325 [details] [diff] [review]
Patch (v2)

Review of attachment 626325 [details] [diff] [review]:
-----------------------------------------------------------------

nice, I would take another pass since many nits

::: accessible/src/atk/nsTextAccessibleWrap.h
@@ +11,5 @@
>  
> +namespace mozilla {
> +namespace a11y {
> + 
> +typedef class TextAccessible TextAccessibleWrap;

TextLeafAccessible TextLeafAccessibleWrap;

::: accessible/src/base/nsTextAccessible.cpp
@@ +31,2 @@
>  {
> +  nsIFrame *frame = GetFrame();

type* name;

@@ +31,4 @@
>  {
> +  nsIFrame *frame = GetFrame();
> +  // Don't return on null frame -- we still return a role
> +  // after accessible is shutdown/DEFUNCT

I'd say you don't need this comment

::: accessible/src/base/nsTextAccessible.h
@@ +26,3 @@
>    virtual void AppendTextTo(nsAString& aText, PRUint32 aStartOffset = 0,
>                              PRUint32 aLength = PR_UINT32_MAX);
> +  virtual mozilla::a11y::ENameValueFlag Name(nsString& aName);

nit: remove mozilla::a11y

@@ +26,4 @@
>    virtual void AppendTextTo(nsAString& aText, PRUint32 aStartOffset = 0,
>                              PRUint32 aLength = PR_UINT32_MAX);
> +  virtual mozilla::a11y::ENameValueFlag Name(nsString& aName);
> +  virtual nsresult GetAttributesInternal(nsIPersistentProperties *aAttributes);

type* name

::: accessible/src/html/nsHTMLTextAccessible.cpp
@@ -57,5 @@
> -     PRUint64 docState = docAccessible->State();
> -     if (0 == (docState & states::EDITABLE)) {
> -       state |= states::READONLY; // Links not focusable in editor
> -     }
> -  }

fyi, this code is removed by bug 754857 (I have good chances to be first :) )

@@ -70,5 @@
> -    nsAutoString oldValueUnused;
> -    aAttributes->SetStringProperty(NS_LITERAL_CSTRING("auto-generated"),
> -                                  NS_LITERAL_STRING("true"), oldValueUnused);
> -  }
> -

fyi: this code goes away in bug 445516 (you have good chances to be first).

::: accessible/src/html/nsHTMLTextAccessible.h
@@ +9,3 @@
>  #include "nsAutoPtr.h"
>  #include "nsBaseWidgetAccessible.h"
> +#include "TextLeafAccessibleWrap.h"

you don't need this include, right?

::: accessible/src/mac/nsTextAccessibleWrap.h
@@ +10,5 @@
>  
> +namespace mozilla {
> +namespace a11y {
> + 
> +typedef class TextAccessible TextAccessibleWrap;

TextLeaf as in atk

::: accessible/src/msaa/Makefile.in
@@ +19,5 @@
>    ApplicationAccessibleWrap.cpp \
>    ARIAGridAccessibleWrap.cpp \
>    nsAccessNodeWrap.cpp \
>    nsAccessibleWrap.cpp \
> +  TextLeafAccessibleWrap.cpp \

you'd need to move it to keep in alphabetical order

::: accessible/src/msaa/nsTextAccessibleWrap.cpp
@@ +18,5 @@
>  
>  using namespace mozilla::a11y;
>  
>  ////////////////////////////////////////////////////////////////////////////////
> +// TextLeafAccessibleWrap Accessible

nit: remove 'Accessible'

@@ +28,4 @@
>  {
>  }
>  
> +STDMETHODIMP_(ULONG) TextLeafAccessibleWrap::AddRef()

nit: please keep return type on own line like
STDMETHODIMP_(ULONG)
TextLeafAccessibleWrap::AddRef()

here and below

@@ +159,5 @@
>  } __except(FilterA11yExceptions(::GetExceptionCode(), GetExceptionInformation())) { }
>    return S_OK;
>  }
>  
> +nsIFrame* TextLeafAccessibleWrap::GetPointFromOffset(nsIFrame *aContainingFrame, 

nit: type* name

::: accessible/src/other/nsTextAccessibleWrap.h
@@ +10,5 @@
>  
> +namespace mozilla {
> +namespace a11y {
> + 
> +typedef class TextAccessible TextAccessibleWrap;

TextLeaf here as well
Comment 22 Mark Capella [:capella] 2012-05-23 01:02:50 PDT
Re: > +#include "TextLeafAccessibleWrap.h"
you don't need this include, right?

I think Trevor asked about this also ... it was there previously, and was re-named ... pulling it out gives me a build failure:

nsAccessibilityService.cpp(412) : error C2514: 'mozilla::a11y::TextLeafAccessible' : class has no constructor

So I added it there instead.
Comment 23 alexander :surkov 2012-05-23 01:04:20 PDT
(In reply to Mark Capella [:capella] from comment #22)
> Re: > +#include "TextLeafAccessibleWrap.h"
> you don't need this include, right?
> 
> I think Trevor asked about this also ... it was there previously, and was
> re-named ... pulling it out gives me a build failure:
> 
> nsAccessibilityService.cpp(412) : error C2514:
> 'mozilla::a11y::TextLeafAccessible' : class has no constructor
> 
> So I added it there instead.

into nsAccessibilityService.cpp you mean? If yes then that's right.
Comment 24 Mark Capella [:capella] 2012-05-23 01:15:24 PDT
Yes #include "TextLeafAccessible.h" into nsAccessibilityService.cpp.

re: The NativeState() method that you're removing from nsHTMLTextAccessible.h / nsHTMLTextAccessible.cpp in bug 754857 ... is that the entire change there?

I mean should I remove the code now from this patch so I don't have to do it later?
Comment 25 alexander :surkov 2012-05-23 01:29:24 PDT
(In reply to Mark Capella [:capella] from comment #24)
> Yes #include "TextLeafAccessible.h" into nsAccessibilityService.cpp.
> 
> re: The NativeState() method that you're removing from
> nsHTMLTextAccessible.h / nsHTMLTextAccessible.cpp in bug 754857 ... is that
> the entire change there?

yes. there's a patch there :)

> I mean should I remove the code now from this patch so I don't have to do it
> later?

you can remove it from TextLeafAccessible files but in either case you need to merge nsHTMLTextAccessible files.
Comment 26 Mark Capella [:capella] 2012-05-23 03:26:32 PDT
Created attachment 626382 [details] [diff] [review]
Patch (v3)

Nits addressed ...
Comment 27 alexander :surkov 2012-05-23 04:33:20 PDT
Comment on attachment 626382 [details] [diff] [review]
Patch (v3)

Review of attachment 626382 [details] [diff] [review]:
-----------------------------------------------------------------

thanks, r=me (note bug 754857 is on inbound)
Comment 28 Mark Capella [:capella] 2012-05-23 05:41:22 PDT
Created attachment 626402 [details] [diff] [review]
Patch (v4)

Will be pushing to inbound TRY with bug 754857 changes included.
Comment 29 Mark Capella [:capella] 2012-05-23 12:02:09 PDT
FYI Inbound Try push https://tbpl.mozilla.org/?tree=Try&rev=99f631fcb377
Comment 30 Mark Capella [:capella] 2012-05-23 16:30:14 PDT
Build ok, inbound back, re-push with tests https://tbpl.mozilla.org/?tree=Try&rev=37388e8b479b
Comment 31 Mark Capella [:capella] 2012-05-23 21:17:46 PDT
https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=ac38f99528e0
Comment 32 Ed Morley [:emorley] 2012-05-24 09:23:03 PDT
https://hg.mozilla.org/mozilla-central/rev/ac38f99528e0

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