Closed Bug 745428 Opened 12 years ago Closed 12 years ago

densify nsTextAccessible

Categories

(Core :: Disability Access APIs, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla15

People

(Reporter: tbsaunde, Assigned: capella)

References

(Blocks 1 open bug)

Details

(Whiteboard: [good first bug][mentor=trev.saunders@gmail.com][lang=c++])

Attachments

(1 file, 3 obsolete files)

similar to bug 742695 with base/nsTextAccessible.cpp/h this time.  Make sure to update the nsTextAccessibleWraps to in msaa/ atk/ mac/ and other/
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?
(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
(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.
Trevor, ping.
(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?
(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.
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
Assignee: nobody → markcapella
Status: NEW → ASSIGNED
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.
(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.
Attached patch Patch (v1) (obsolete) — Splinter Review
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 :)
Attachment #624997 - Flags: feedback?(trev.saunders)
(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)
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?
(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 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.
Attachment #624997 - Flags: feedback?(trev.saunders) → feedback+
(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).
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 ...
(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()).
Attached patch Patch (v2) (obsolete) — Splinter Review
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.
Attachment #624997 - Attachment is obsolete: true
(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 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.
Attachment #626325 - Flags: review?(surkov.alexander)
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
Attachment #626325 - Flags: review?(surkov.alexander)
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.
(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.
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?
(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.
Attached patch Patch (v3) (obsolete) — Splinter Review
Nits addressed ...
Attachment #626325 - Attachment is obsolete: true
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)
Attachment #626382 - Flags: review+
Attached patch Patch (v4)Splinter Review
Will be pushing to inbound TRY with bug 754857 changes included.
Attachment #626382 - Attachment is obsolete: true
Build ok, inbound back, re-push with tests https://tbpl.mozilla.org/?tree=Try&rev=37388e8b479b
https://hg.mozilla.org/mozilla-central/rev/ac38f99528e0
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Depends on: 765110
You need to log in before you can comment on or make changes to this bug.