850987 - RenderedToContentOffset/ContentToRenderedOffset should return an offset unchanged for HTMLTextFieldAccessible

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[alexander :surkov (:asurkov)]

spun off bug 770564 comment #7:

rendered text equals to content text in textareas and inputs thus RenderedToContentOffset/ContentToRenderedOffset can be trivial and return the given offset (see accessible/src/generic/HyperTextAccessbile.h/cpp).

You should add IsTextField() method to Accessible.h and add eHTMLTextFieldType from AccTypes.h to HTMLTextFieldAccessible

Comment 1

[Alexandre BM (:rednaks)]

I'm taking it !

Comment 2

[Alexandre BM (:rednaks)]

Attachment: patch

The build seems ok but I don't know if there are any tests I should run !

Comment 3

[alexander :surkov (:asurkov)]

(In reply to Alexandre BM (:rednaks) from comment #2)
> Created attachment 726577 [details] [diff] [review]
> patch
> 
> The build seems ok but I don't know if there are any tests I should run !

the patch doesn't address the first paragraph of comment #0

Comment 4

[Alexandre BM (:rednaks)]

Yes ! Sorry I missed to mention it ! I haven't very much time so it was a quick patch :)
So can you tell me more about the first paragraph ? should I override the two methods in HTMLTextFieldAccessible or what ? 
Thanks !

Comment 5

[alexander :surkov (:asurkov)]

(In reply to Alexandre BM (:rednaks) from comment #4)
> Yes ! Sorry I missed to mention it ! I haven't very much time so it was a
> quick patch :)
> So can you tell me more about the first paragraph ? should I override the
> two methods in HTMLTextFieldAccessible or what ? 

I think you can have one version at HyperTextAccessible that check if that's a IsTextField()

Comment 6

[Alexandre BM (:rednaks)]

Attachment: patch V2

Comment 7

[alexander :surkov (:asurkov)]

Comment on attachment 730895 [details] [diff] [review]
patch V2

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

you need to ask somebody for review

Comment 8

[Trevor Saunders (:tbsaunde)]

Comment on attachment 730895 [details] [diff] [review]
patch V2

>@@ -2005,16 +2005,22 @@
>                                              uint32_t* aRenderedOffset)
> {
>   if (!aFrame) {
>     // Current frame not rendered -- this can happen if text is set on
>     // something with display: none
>     *aRenderedOffset = 0;
>     return NS_OK;
>   }
>+
>+  if(IsTextField()) {
>+    *aRenderedOffset = aContentOffset;

so, what happens if I have <textfield>foo<div>bar</div></textfield> ?  I suspect even if we don't optimize that this might be fine.

> nsresult
> HyperTextAccessible::RenderedToContentOffset(nsIFrame* aFrame, uint32_t aRenderedOffset,
>                                              int32_t* aContentOffset)
> {
>+  if(IsTextField()) {
>+    *aContentOffset = aRenderedOffset;

same

A>+++ b/accessible/src/generic/HyperTextAccessible.h	Thu Mar 28 22:44:00 2013 +0100
>@@ -55,21 +55,21 @@
>-  static nsresult ContentToRenderedOffset(nsIFrame *aFrame, int32_t aContentOffset,
>+  nsresult ContentToRenderedOffset(nsIFrame *aFrame, int32_t aContentOffset,
>                                           uint32_t *aRenderedOffset);

nit, fix alignment of second line

>-  static nsresult RenderedToContentOffset(nsIFrame *aFrame, uint32_t aRenderedOffset,
>+  nsresult RenderedToContentOffset(nsIFrame *aFrame, uint32_t aRenderedOffset,
>                                           int32_t *aContentOffset);

same

Comment 9

[alexander :surkov (:asurkov)]

(In reply to Trevor Saunders (:tbsaunde) from comment #8)

> so, what happens if I have <textfield>foo<div>bar</div></textfield> ?  I
> suspect even if we don't optimize that this might be fine.

textarea treats all content as CDATA so it displays it as a text.

> A>+++ b/accessible/src/generic/HyperTextAccessible.h	Thu Mar 28 22:44:00
> 2013 +0100
> >@@ -55,21 +55,21 @@
> >-  static nsresult ContentToRenderedOffset(nsIFrame *aFrame, int32_t aContentOffset,
> >+  nsresult ContentToRenderedOffset(nsIFrame *aFrame, int32_t aContentOffset,
> >                                           uint32_t *aRenderedOffset);
> 
> nit, fix alignment of second line
> 
> >-  static nsresult RenderedToContentOffset(nsIFrame *aFrame, uint32_t aRenderedOffset,
> >+  nsresult RenderedToContentOffset(nsIFrame *aFrame, uint32_t aRenderedOffset,
> >                                           int32_t *aContentOffset);
> 
> same

Alexandre, please fix these two and upload a patch (add checkin-needed keyword)

Comment 10

[Alexandre BM (:rednaks)]

Attachment: patch V3

Comment 11

[Alexandre BM (:rednaks)]

Attachment: patch

Comment 12

[alexander :surkov (:asurkov)]

Comment on attachment 733747 [details] [diff] [review]
patch

I will land it (sent to try server)

Comment 13

[alexander :surkov (:asurkov)]

http://hg.mozilla.org/integration/mozilla-inbound/rev/052ef269b9c3

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/052ef269b9c3