Closed
Bug 759305
Opened 12 years ago
Closed 12 years ago
De-ns-ify nsHyperTextAccessible
Categories
(Core :: Disability Access APIs, defect)
Core
Disability Access APIs
Tracking
()
RESOLVED
FIXED
mozilla15
People
(Reporter: capella, Assigned: capella)
References
(Blocks 1 open bug)
Details
(Whiteboard: [lang=c++])
Attachments
(2 files)
171.02 KB,
patch
|
surkov
:
review+
|
Details | Diff | Splinter Review |
170.22 KB,
patch
|
Details | Diff | Splinter Review |
similar to bug 748724, bug 758870, etc. move from /html to /generic and rename all wraps etc, like we've been doing
Assignee | ||
Updated•12 years ago
|
Assignee: nobody → markcapella
Status: NEW → ASSIGNED
Assignee | ||
Updated•12 years ago
|
Summary: De-ns-ify nsHyperTextAcc → De-ns-ify nsHyperTextAccessbile
Assignee | ||
Updated•12 years ago
|
Summary: De-ns-ify nsHyperTextAccessbile → De-ns-ify nsHyperTextAccessible
Assignee | ||
Comment 1•12 years ago
|
||
Asking Surkov for initial feedback... you've been stealing them lately :)
Attachment #628350 -
Flags: feedback?(surkov.alexander)
Comment 2•12 years ago
|
||
what about to put it under mozilla::a11y namespace?
Assignee | ||
Comment 3•12 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•12 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•12 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•12 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•12 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•12 years ago
|
||
TRY push https://tbpl.mozilla.org/?tree=Try&rev=64467b586550
Assignee | ||
Comment 9•12 years ago
|
||
Final version, includes late fixup for warning in bug 740766.
Assignee | ||
Comment 10•12 years ago
|
||
Added correction into patch for bug 759035 ... removed double /** lines in accessible.h causing clang warnings.
Assignee | ||
Comment 11•12 years ago
|
||
Push to inbound: https://tbpl.mozilla.org/?tree=Mozilla-Inbound&rev=58e26b8bd5ba
Comment 12•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/58e26b8bd5ba
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•