The default bug view has changed. See this FAQ.

De-ns-ify nsHyperTextAccessible

RESOLVED FIXED in mozilla15

Status

()

Core
Disability Access APIs
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: capella, Assigned: capella)

Tracking

(Blocks: 1 bug)

unspecified
mozilla15
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [lang=c++])

Attachments

(2 attachments)

(Assignee)

Description

5 years ago
similar to bug 748724, bug 758870, etc.

move from /html to /generic and rename all wraps etc, like we've been doing
(Assignee)

Updated

5 years ago
Assignee: nobody → markcapella
Status: NEW → ASSIGNED
(Assignee)

Updated

5 years ago
Summary: De-ns-ify nsHyperTextAcc → De-ns-ify nsHyperTextAccessbile
(Assignee)

Updated

5 years ago
Summary: De-ns-ify nsHyperTextAccessbile → De-ns-ify nsHyperTextAccessible
(Assignee)

Comment 1

5 years ago
Created attachment 628350 [details] [diff] [review]
Patch (v1)

Asking Surkov for initial feedback... you've been stealing them lately :)
Attachment #628350 - Flags: feedback?(surkov.alexander)

Comment 2

5 years ago
what about to put it under mozilla::a11y namespace?
(Assignee)

Comment 3

5 years ago
I gave it a go like the last one and ran into build problems. I tried to hack my way through for a bit, but didn't resolve it. I can try a little longer to figure it out if you'd like.

I wonder if there's a better way to do this as a comprehensive / all in one approach rather than piecing it in?

Comment 4

5 years ago
(In reply to Mark Capella [:capella] from comment #3)
> I gave it a go like the last one and ran into build problems. I tried to
> hack my way through for a bit, but didn't resolve it. I can try a little
> longer to figure it out if you'd like.
> 
> I wonder if there's a better way to do this as a comprehensive / all in one
> approach rather than piecing it in?

ok, let's keep them separately if it's not easy.

Comment 5

5 years ago
Comment on attachment 628350 [details] [diff] [review]
Patch (v1)

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

r=me, fix nits and go land it

::: accessible/src/generic/DocAccessible.cpp
@@ +171,1 @@
>                                              (void**)&foundInterface) :

wrong indentation, please check if you can keep it on the same line

::: accessible/src/html/nsHyperTextAccessible.cpp
@@ +1021,5 @@
>    * nsIAccessibleText impl.
>    */
>  NS_IMETHODIMP
> +HyperTextAccessible::GetTextBeforeOffset(PRInt32 aOffset, AccessibleTextBoundary aBoundaryType,
> +                                         PRInt32* aStartOffset, PRInt32* aEndOffset, nsAString& aText)

don't you break 80 char per line restriction? here and below

@@ +1387,5 @@
>  }
>  
>  NS_IMETHODIMP
> +HyperTextAccessible::GetLinkIndexAtOffset(PRInt32 aOffset,
> +                                          PRInt32* aLinkIndex)

keep on the same line pls

::: accessible/src/generic/Makefile.in
@@ +26,5 @@
>    TextLeafAccessible.cpp \
>    $(NULL)
> +
> +EXPORTS = \
> +  HyperTextAccessible.h \

it seems you don't need to export it

::: accessible/src/html/nsHTMLCanvasAccessible.h
@@ +10,5 @@
>  
>  /**
>   * HTML canvas accessible (html:canvas).
>   */
> +class nsHTMLCanvasAccessible : public HyperTextAccessible

please file a bug on that, it should be inherited from HyperTextAccessibleWrap

::: accessible/src/mac/mozTextAccessible.h
@@ +9,5 @@
>  @interface mozTextAccessible : mozAccessible
>  {
>    // both of these are the same old mGeckoAccessible, but already
>    // QI'd for us, to the right type, for convenience.
> +  HyperTextAccessible       *mGeckoTextAccessible;         // strong

nit: please remove those whitespaces between

::: accessible/src/msaa/nsHyperTextAccessibleWrap.cpp
@@ +47,5 @@
>  nsresult
> +HyperTextAccessibleWrap::GetModifiedText(bool aGetInsertedText,
> +                                         nsAString& aText,
> +                                         PRUint32 *aStartOffset,
> +                                         PRUint32 *aEndOffset)

type* name

::: accessible/src/xforms/nsXFormsFormControlsAccessible.cpp
@@ +118,5 @@
>    nsXFormsEditableAccessible(aContent, aDoc)
>  {
>  }
>  
> +NS_IMPL_ISUPPORTS_INHERITED3(nsXFormsInputAccessible, Accessible, HyperTextAccessible, nsIAccessibleText, nsIAccessibleEditableText)

please put each item on own line

::: accessible/tests/mochitest/hypertext/test_update.html
@@ +156,5 @@
>       href="https://bugzilla.mozilla.org/show_bug.cgi?id=625009">
>      Mozilla Bug 625009
>    </a>
>    <a target="_blank"
> +     title="Crash in HyperTextAccessible::GetText()"

ok :) you might want to rename a bug perhaps? :)

::: accessible/tests/mochitest/role/test_general.html
@@ +75,5 @@
>       title="html:input of type "file" no longer rendered to screen readers">
>      Mozilla Bug 472326
>    </a><br>
>    <a target="_blank" href="https://bugzilla.mozilla.org/show_bug.cgi?id=474261"
> +      title="Test remaining implementations in HyperTextAccessible::GetRole">

same here :)
Attachment #628350 - Flags: feedback?(surkov.alexander) → review+
(Assignee)

Comment 6

5 years ago
I wasn't sure about renaming the two test "title"s ... this way they are accurate as a tooltip in the test, and still point to the original bug with the correct name as-of that point in time, but maybe I should just leave it as it was?

Comment 7

5 years ago
(In reply to Mark Capella [:capella] from comment #6)
> I wasn't sure about renaming the two test "title"s ... this way they are
> accurate as a tooltip in the test, and still point to the original bug with
> the correct name as-of that point in time, but maybe I should just leave it
> as it was?

yes, just leave them
(Assignee)

Comment 8

5 years ago
TRY push
https://tbpl.mozilla.org/?tree=Try&rev=64467b586550
(Assignee)

Comment 9

5 years ago
Created attachment 628549 [details] [diff] [review]
Patch for inbound

Final version, includes late fixup for warning in bug 740766.
(Assignee)

Comment 10

5 years ago
Added correction into patch for bug 759035 ... removed double /** lines in accessible.h causing clang warnings.
(Assignee)

Comment 11

5 years ago
Push to inbound:
https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=58e26b8bd5ba
https://hg.mozilla.org/mozilla-central/rev/58e26b8bd5ba
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.