735645 - expose sub and sup elements in text attributes

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[alexander :surkov (:asurkov)]

follow up from bug 473569

Comment 1

[alexander :surkov (:asurkov)]

fix to check sub and sup tagnames in:
TextAttrsMgr::TextPosTextAttr::
  GetTextPosValue(nsIFrame* aFrame) const
(http://mxr.mozilla.org/mozilla-central/source/accessible/src/base/TextAttrs.cpp?force=1#719)

also please add mochitests attributes/test_text.html (after area6)

Comment 2

[Max Li [:maxli]]

Attachment: Patch v1

Comment 3

[Hubert Figuiere [:hub]]

Comment on attachment 611311 [details] [diff] [review]
Patch v1

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

::: accessible/src/base/TextAttrs.cpp
@@ +764,5 @@
> +  if (tagName == nsGkAtoms::sup) {
> +    return eTextPosSuper;
> +  } else if (tagName == nsGkAtoms::sub) {
> +    return eTextPosSub;
> +  }

lose the { and } as there is only one statement (this is the coding style of the module)

remove the else as we do a return in the if.

Comment 4

[Trevor Saunders (:tbsaunde)]

Comment on attachment 611311 [details] [diff] [review]
Patch v1

>+  const nsIAtom* tagName = aFrame->GetContent()->Tag();

I think GetContent can return null here.

also please fix comment 3

Comment 5

[Max Li [:maxli]]

Attachment: Patch v2

Comment 6

[alexander :surkov (:asurkov)]

Comment on attachment 611335 [details] [diff] [review]
Patch v2

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

::: accessible/src/base/TextAttrs.cpp
@@ +760,5 @@
>      }
>    }
>  
> +  const nsIContent* content = aFrame->GetContent();
> +  if (content) {

please add content->IsHTML check

::: accessible/tests/mochitest/attributes/test_text.html
@@ +205,5 @@
>        attrs = { "text-position": "sub" };
>        testTextAttrs(ID, 128, attrs, defAttrs, 128, 142);
>  
>        //////////////////////////////////////////////////////////////////////////
> +      // area7 (sup and sub elements, bug 735645)

refer to bug # for details
otherwise it makes me feel we have a bug

@@ +603,5 @@
>    </p>
>  
> +  <p id="area7">
> +    <sup>superscript</sup><sub>subscript</sub>
> +  </p>

please put it under area6 test, note it has some incorrect tests for <sup> element

Comment 7

[Trevor Saunders (:tbsaunde)]

Comment on attachment 611335 [details] [diff] [review]
Patch v2

canceling review per last comment.

Comment 8

[Max Li [:maxli]]

Attachment: Patch v3

Comment 9

[Trevor Saunders (:tbsaunde)]

Comment on attachment 611613 [details] [diff] [review]
Patch v3

The C++ looks right although I'm not sure what we should have override in cases like <sup style="text-pos:su">

I'm sort of short on time and I think Alex had some nits about tests, so I'll let him do review.

Comment 10

[alexander :surkov (:asurkov)]

Comment on attachment 611613 [details] [diff] [review]
Patch v3

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

r=me with comment addressed

::: accessible/tests/mochitest/attributes/test_text.html
@@ +156,5 @@
>  
>        attrs = {};
>        testTextAttrs(ID, 13, attrs, defAttrs, 13, 27);
>  
> +      tempElem = getNode(ID).firstChild.nextSibling;

this should point to sup element (This <sup>sentence</sup> has the word) but you test second text node (has the word).

maybe you could have like:
var attrs = {};
var supAttrs = { "text-position": "super" }
var subAttrs = { "text-position": "sub" }

testTextAttrs(ID, 5, supAttrs, defAttrs, 5, 13);
testTextAttrs(ID, 13, attrs, defAttrs, 13, 27);

no font-size is needed since it's part of other tests

Comment 11

[Max Li [:maxli]]

(In reply to alexander :surkov from comment #10)

> >        attrs = {};
> >        testTextAttrs(ID, 13, attrs, defAttrs, 13, 27);
> >  
> > +      tempElem = getNode(ID).firstChild.nextSibling;
> 
> this should point to sup element (This <sup>sentence</sup> has the word) but
> you test second text node (has the word).

Um, the sup element gets tested the line above the one you quoted (unless I'm misunderstanding something...)


> no font-size is needed since it's part of other tests

If I remove that, the mochitest fails since it would otherwise expect 12pt font for the 10pt sup/sub element.

Comment 12

[alexander :surkov (:asurkov)]

(In reply to Max Li [:maxli] from comment #11)
> (In reply to alexander :surkov from comment #10)
> 
> > >        attrs = {};
> > >        testTextAttrs(ID, 13, attrs, defAttrs, 13, 27);
> > >  
> > > +      tempElem = getNode(ID).firstChild.nextSibling;
> > 
> > this should point to sup element (This <sup>sentence</sup> has the word) but
> > you test second text node (has the word).
> 
> Um, the sup element gets tested the line above the one you quoted (unless
> I'm misunderstanding something...)

well, these firstChild.nextSibling.oldestGrandMa are hard readable things. All they are used to get CSS vertical align computed value, I think we can hardcode it instead.

> > no font-size is needed since it's part of other tests
> 
> If I remove that, the mochitest fails since it would otherwise expect 12pt
> font for the 10pt sup/sub element.

I see.

Comment 13

[Max Li [:maxli]]

Attachment: Patch v4

Tests updated.

Comment 14

[alexander :surkov (:asurkov)]

Comment on attachment 612409 [details] [diff] [review]
Patch v4

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

r=me, thank you

Comment 15

[alexander :surkov (:asurkov)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/f3acabd0b48c

Comment 16

[(no longer active)]

http://hg.mozilla.org/mozilla-central/rev/f3acabd0b48c