expose sub and sup elements in text attributes

RESOLVED FIXED in mozilla14

Status

()

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

People

(Reporter: surkov, Assigned: maxli)

Tracking

(Blocks: 1 bug, {access})

unspecified
mozilla14
access
Points:
---
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [good first bug][mentor=trev.saunders@gmail.com][lang=c++], URL)

Attachments

(1 attachment, 3 obsolete attachments)

(Reporter)

Description

5 years ago
follow up from bug 473569
(Reporter)

Updated

5 years ago
(Reporter)

Comment 1

5 years ago
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)
Whiteboard: [good first bug][mentor=trev.saunders@gmail.com][lang=c++]
(Assignee)

Comment 2

5 years ago
Created attachment 611311 [details] [diff] [review]
Patch v1
Attachment #611311 - Flags: review?(trev.saunders)
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 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
Attachment #611311 - Flags: review?(trev.saunders)
(Assignee)

Comment 5

5 years ago
Created attachment 611335 [details] [diff] [review]
Patch v2
Attachment #611311 - Attachment is obsolete: true
Attachment #611335 - Flags: review?(trev.saunders)
(Reporter)

Comment 6

5 years ago
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 on attachment 611335 [details] [diff] [review]
Patch v2

canceling review per last comment.
Attachment #611335 - Flags: review?(trev.saunders)
(Assignee)

Comment 8

5 years ago
Created attachment 611613 [details] [diff] [review]
Patch v3
Assignee: nobody → maxli
Attachment #611335 - Attachment is obsolete: true
Attachment #611613 - Flags: review?(trev.saunders)
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.
Attachment #611613 - Flags: review?(trev.saunders) → review?(surkov.alexander)
(Reporter)

Comment 10

5 years ago
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
Attachment #611613 - Flags: review?(surkov.alexander) → review+
(Assignee)

Comment 11

5 years ago
(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.
(Reporter)

Comment 12

5 years ago
(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.
(Assignee)

Comment 13

5 years ago
Created attachment 612409 [details] [diff] [review]
Patch v4

Tests updated.
Attachment #612409 - Flags: review?(surkov.alexander)
(Reporter)

Comment 14

5 years ago
Comment on attachment 612409 [details] [diff] [review]
Patch v4

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

r=me, thank you
Attachment #612409 - Flags: review?(surkov.alexander) → review+
(Reporter)

Updated

5 years ago
Attachment #611613 - Attachment is obsolete: true
(Reporter)

Comment 15

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/f3acabd0b48c
Flags: in-testsuite+
Target Milestone: --- → mozilla14
http://hg.mozilla.org/mozilla-central/rev/f3acabd0b48c
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.