Last Comment Bug 735645 - expose sub and sup elements in text attributes
: expose sub and sup elements in text attributes
Status: RESOLVED FIXED
[good first bug][mentor=trev.saunders...
: access
Product: Core
Classification: Components
Component: Disability Access APIs (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla14
Assigned To: Max Li [:maxli]
:
Mentors:
http://dev.w3.org/html5/spec/the-sub-...
Depends on:
Blocks: textattra11y
  Show dependency treegraph
 
Reported: 2012-03-14 06:16 PDT by alexander :surkov
Modified: 2012-04-05 11:39 PDT (History)
3 users (show)
surkov.alexander: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch v1 (3.78 KB, patch)
2012-04-01 13:22 PDT, Max Li [:maxli]
no flags Details | Diff | Splinter Review
Patch v2 (3.84 KB, patch)
2012-04-01 18:20 PDT, Max Li [:maxli]
no flags Details | Diff | Splinter Review
Patch v3 (4.30 KB, patch)
2012-04-02 15:17 PDT, Max Li [:maxli]
surkov.alexander: review+
Details | Diff | Splinter Review
Patch v4 (4.32 KB, patch)
2012-04-04 17:12 PDT, Max Li [:maxli]
surkov.alexander: review+
Details | Diff | Splinter Review

Description alexander :surkov 2012-03-14 06:16:20 PDT
follow up from bug 473569
Comment 1 alexander :surkov 2012-03-14 06:42:55 PDT
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] 2012-04-01 13:22:19 PDT
Created attachment 611311 [details] [diff] [review]
Patch v1
Comment 3 Hubert Figuiere [:hub] 2012-04-01 15:17:15 PDT
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) 2012-04-01 16:46:05 PDT
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] 2012-04-01 18:20:18 PDT
Created attachment 611335 [details] [diff] [review]
Patch v2
Comment 6 alexander :surkov 2012-04-01 19:25:12 PDT
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) 2012-04-01 21:53:49 PDT
Comment on attachment 611335 [details] [diff] [review]
Patch v2

canceling review per last comment.
Comment 8 Max Li [:maxli] 2012-04-02 15:17:01 PDT
Created attachment 611613 [details] [diff] [review]
Patch v3
Comment 9 Trevor Saunders (:tbsaunde) 2012-04-02 17:29:19 PDT
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 2012-04-02 20:48:20 PDT
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] 2012-04-03 17:55:25 PDT
(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 2012-04-03 22:19:00 PDT
(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] 2012-04-04 17:12:41 PDT
Created attachment 612409 [details] [diff] [review]
Patch v4

Tests updated.
Comment 14 alexander :surkov 2012-04-04 22:23:39 PDT
Comment on attachment 612409 [details] [diff] [review]
Patch v4

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

r=me, thank you

Note You need to log in before you can comment on or make changes to this bug.