475525 - provide robust text attribute offset testing

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[alexander :surkov (:asurkov)]

bug 467146 introduces regression, text attribute offsets don't take into account font-size text attribute any more (see http://mxr.mozilla.org/mozilla-central/source/accessible/src/html/nsHyperTextAccessible.cpp#2389, there is no GetRangeForTextAttr() call there). I think I will fix this as a part of bug 475522 but we must provide robust testing of text attribute offset.

Marco, do you like to take this one?

Comment 1

[David Bolter [:davidb] (NeedInfo me for attention)]

I should understand how bug 467146 caused a regression in this respect. Is it the removal of "font-size" from gCSSTextAttrsMap that eventually leads to some missing range check? Either way I agree it would be great to get this covered in a mochitest.

Comment 2

[alexander :surkov (:asurkov)]

(In reply to comment #1)
> I should understand how bug 467146 caused a regression in this respect. Is it
> the removal of "font-size" from gCSSTextAttrsMap that eventually leads to some
> missing range check?

Right. David, see http://mxr.mozilla.org/mozilla-central/source/accessible/src/html/nsHyperTextAccessible.cpp#2377 - GetRangeForTextAttr was used for fontSize attribute as a part of CSSTextAttr class implementation. Since we moved out fontsize from css text attr class then we should add GetRangeForTextAttr() call for fontsize text attr explicetly.

Comment 3

[Marco Zehe (:MarcoZ)]

Attachment: WIP, contains tests for font-size at various offsets

This contains mochitests for the font-size at various offsets. The offsets were taken from what Shiretoko current nightly delivers. The following is the log of the whole run on area9:

1252 INFO TEST-PASS | chrome://mochikit/content/a11y/accessible/test_textattrs.html | Attribute 'font-style 'has wrong value. Getting default text attributes for area9
1253 INFO TEST-PASS | chrome://mochikit/content/a11y/accessible/test_textattrs.html | Attribute 'font-size 'has wrong value. Getting default text attributes for area9
1254 INFO TEST-PASS | chrome://mochikit/content/a11y/accessible/test_textattrs.html | Attribute 'background-color 'has wrong value. Getting default text attributes for area9
1255 INFO TEST-PASS | chrome://mochikit/content/a11y/accessible/test_textattrs.html | Attribute 'font-weight 'has wrong value. Getting default text attributes for area9
1256 INFO TEST-PASS | chrome://mochikit/content/a11y/accessible/test_textattrs.html | Attribute 'color 'has wrong value. Getting default text attributes for area9
1257 INFO TEST-PASS | chrome://mochikit/content/a11y/accessible/test_textattrs.html | Attribute 'font-family 'has wrong value. Getting default text attributes for area9
1258 INFO TEST-PASS | chrome://mochikit/content/a11y/accessible/test_textattrs.html | Attribute 'text-position 'has wrong value. Getting default text attributes for area9
1259 INFO TEST-PASS | chrome://mochikit/content/a11y/accessible/test_textattrs.html | Wrong start offset for area9 at offset 0
1260 ERROR TEST-UNEXPECTED-FAIL | chrome://mochikit/content/a11y/accessible/test_textattrs.html | Wrong end offset for area9 at offset 0 - got 38, expected 6
1261 ERROR TEST-UNEXPECTED-FAIL | chrome://mochikit/content/a11y/accessible/test_textattrs.html | Wrong start offset for area9 at offset 7 - got 0, expected 6
1262 ERROR TEST-UNEXPECTED-FAIL | chrome://mochikit/content/a11y/accessible/test_textattrs.html | Wrong end offset for area9 at offset 7 - got 38, expected 12
1263 INFO TEST-PASS | chrome://mochikit/content/a11y/accessible/test_textattrs.html | Attribute 'font-size 'has wrong value for area9 at offset 7
1264 ERROR TEST-UNEXPECTED-FAIL | chrome://mochikit/content/a11y/accessible/test_textattrs.html | Wrong start offset for area9 at offset 13 - got 0, expected 12
1265 ERROR TEST-UNEXPECTED-FAIL | chrome://mochikit/content/a11y/accessible/test_textattrs.html | Wrong end offset for area9 at offset 13 - got 38, expected 21
1266 ERROR TEST-UNEXPECTED-FAIL | chrome://mochikit/content/a11y/accessible/test_textattrs.html | Wrong start offset for area9 at offset 22 - got 0, expected 21
1267 ERROR TEST-UNEXPECTED-FAIL | chrome://mochikit/content/a11y/accessible/test_textattrs.html | Wrong end offset for area9 at offset 22 - got 38, expected 31
1268 INFO TEST-PASS | chrome://mochikit/content/a11y/accessible/test_textattrs.html | Attribute 'font-size 'has wrong value for area9 at offset 22
1269 ERROR TEST-UNEXPECTED-FAIL | chrome://mochikit/content/a11y/accessible/test_textattrs.html | Wrong start offset for area9 at offset 32 - got 0, expected 31
1270 INFO TEST-PASS | chrome://mochikit/content/a11y/accessible/test_textattrs.html | Wrong end offset for area9 at offset 32

As you can see, each offset within the whole paragraph returns a start offset of 0 and an end offset of 38, which is obviously wrong.

Comment 4

[Marco Zehe (:MarcoZ)]

Attachment: Patch

Testing for all those attributes we don't have single-instance tests for yet.

This one has 4 failures currently on 3.2a1pre, passes completely on 3.1b3pre:
1260 ERROR TEST-UNEXPECTED-FAIL | chrome://mochikit/content/a11y/accessible/test_textattrs.html | Wrong end offset for area9 at offset 0 - got 21, expected 6
1261 ERROR TEST-UNEXPECTED-FAIL | chrome://mochikit/content/a11y/accessible/test_textattrs.html | Wrong start offset for area9 at offset 7 - got 0, expected 6
1262 ERROR TEST-UNEXPECTED-FAIL | chrome://mochikit/content/a11y/accessible/test_textattrs.html | Wrong end offset for area9 at offset 7 - got 21, expected 12
1264 ERROR TEST-UNEXPECTED-FAIL | chrome://mochikit/content/a11y/accessible/test_textattrs.html | Wrong start offset for area9 at offset 13 - got 0, expected 12

As we suspected on IRC, the background-color style sets the correct start and end offsets for the remaining tests, only the one for font-size is failing before, in, and after the element containing the font-size attribute alone.

Comment 5

[alexander :surkov (:asurkov)]

Comment on attachment 359066 [details] [diff] [review]
Patch


>+      ID = "area9";
>+      tempElem = document.getElementById(ID);

you could use getNode() function if you like.

>+      gComputedStyle = document.defaultView.getComputedStyle(tempElem, "");
>+      defAttrs = {
>+        "font-style": gComputedStyle.fontStyle,
>+        "font-size": "10pt",

does it work on all platforms?

>+
>+      tempElem = tempElem.firstChild.nextSibling.nextSibling.nextSibling;

would be nice small comments for long structures like this

let me play with our code to ensure if we need to check additional stuffs.

Comment 6

[Marco Zehe (:MarcoZ)]

(In reply to comment #5)
> >+      ID = "area9";
> >+      tempElem = document.getElementById(ID);
> 
> you could use getNode() function if you like.

Yeah I thought I'd stay consistent with the other tests for the time being, or I would have wanted to refactor them all. ;-)

> >+      gComputedStyle = document.defaultView.getComputedStyle(tempElem, "");
> >+      defAttrs = {
> >+        "font-style": gComputedStyle.fontStyle,
> >+        "font-size": "10pt",
> 
> does it work on all platforms?

I took this one straight from one of the other tests where it passes on all platforms. AFAIK, the only place we have different font-sizes is in the input where we test for spell checking.

> >+      tempElem = tempElem.firstChild.nextSibling.nextSibling.nextSibling;
> 
> would be nice small comments for long structures like this

Will add them.

> let me play with our code to ensure if we need to check additional stuffs.

OK, let me know if you find anything, I'll provide a new patch then.

Comment 7

[alexander :surkov (:asurkov)]

Marco, it sounds font-family, text-decoration=line-through, underline aren't addressed in mochitests.

Comment 8

[alexander :surkov (:asurkov)]

Marco, please note we do not expose 'language' text attribute as default text
attribute which is a bug.

Comment 9

[Marco Zehe (:MarcoZ)]

(In reply to comment #8)
> Marco, please note we do not expose 'language' text attribute as default text
> attribute which is a bug.

We may not know which language is the default if the author doesn't specify it.

Comment 10

[Marco Zehe (:MarcoZ)]

(In reply to comment #7)
> Marco, it sounds font-family, text-decoration=line-through, underline aren't
> addressed in mochitests.

font-family is:

>+      attrs = {"font-family": gComputedStyle.fontFamily};
>+      testTextAttrs(ID, 70, attrs, 69, 83);

I'll add tests for strikethrough and underline.

Comment 11

[alexander :surkov (:asurkov)]

(In reply to comment #9)

> We may not know which language is the default if the author doesn't specify it.

Yes, but we don't expose it even if author specified it.

Comment 12

[alexander :surkov (:asurkov)]

(In reply to comment #10)
> (In reply to comment #7)
> > Marco, it sounds font-family, text-decoration=line-through, underline aren't
> > addressed in mochitests.
> 
> font-family is:
> 
> >+      attrs = {"font-family": gComputedStyle.fontFamily};
> >+      testTextAttrs(ID, 70, attrs, 69, 83);

It's not enough I think because it's bounded by another attributes from each side (from one side it is bounded by font-style: italic, from another side it is bounded by style="font-size: smaller").

Comment 13

[Marco Zehe (:MarcoZ)]

(In reply to comment #12)
> It's not enough I think because it's bounded by another attributes from each
> side (from one side it is bounded by font-style: italic, from another side it
> is bounded by style="font-size: smaller").

I don't understand. font-family is the last test in my current patch. The offsets match the end offset of the previous node and the start offset of the next node.

Comment 14

[alexander :surkov (:asurkov)]

(In reply to comment #13)
> I don't understand. font-family is the last test in my current patch. The
> offsets match the end offset of the previous node and the start offset of the
> next node.

Let's imagine our code is broken and we do not calculate offsets for font-family attribute but we do this for any other text attribute. Let's offset point is inside of the node with font-family style (let call it current node) in your example. Previous node has font-style attribute so since font-style attribute is changed (from current node to previous node) then we will return correct start offset in your example. The same algorithm works for end offset.

Comment 15

[alexander :surkov (:asurkov)]

I mean for correct tests we should consider cases like

<p>
  <p style="font-family: bla1">bla</p>
  <p style="font-family: bla2">bla2</p>
</p>

so that any another attribute doesn't impact on offsets calculation for the currently tested text attribute.

Comment 16

[Marco Zehe (:MarcoZ)]

OK! I'll move all the tests into a paragraph that doesn't have a style by itself, but keep the one with the different font sizes.

Comment 17

[Marco Zehe (:MarcoZ)]

Attachment: Patch2

Added tests for text-decoration "underline" and "line-through". Note that I hardcoded the values because I didn't get the correct ones back from getComputedStyle. getComputedStyle gave me "underline", not "underlinesolid".
2. Added another section with the same tests in an unstyled paragraph.

This currentl has 8 failures, all having to do with the start and end offsets for font-size in both paras.

Comment 18

[alexander :surkov (:asurkov)]

Comment on attachment 359492 [details] [diff] [review]
Patch2


>+      // Walk to the span with the different background color
>+      tempElem = tempElem.firstChild.nextSibling.nextSibling.nextSibling;

I like this comment, I would like to see similar comments for similar stuffs :)

>+      tempElem = tempElem.nextSibling.nextSibling;

like this.

>+      attrs = {"text-underline-style": "underlinesolid"};

you caught a good bag actually :) it should be "solid" not "underlinesolid".

>+      attrs = {"text-line-through-style": "line-throughsolid"};

the same.

>+    <span style="text-decoration: line-through;">strikethrough</span> normal</p>

nit: put html:p on new line please.

>+
>+  <p id="area10">Normal
>+    <span style="font-size: 120%">bigger</span> smaller
>+    <span style="background-color: blue;">background blue</span> normal
>+    <span style="font-style: italic;">Different styling</span> normal
>+    <span style="font-family: tahoma;">Different font</span> normal
>+    <span style="text-decoration: underline;">underlined</span> normal
>+    <span style="text-decoration: line-through;">strikethrough</span> normal</p>

I like "area10" test but I don't see a difference between it and "area9" test from the point of view code logic. Why do you want to have two tests?

We don't have tests for language default attribute text still.

We haven't tests for getTextAttributes(true, ...) - this method should fail for background-color and font-size text attributes.

Also, organization question. Do you think we'll put this test with patch that fixes all of this, right?

Comment 19

[Marco Zehe (:MarcoZ)]

1. Let's put the "language as default attribute" test with the patch that actually fixes that bug. This set of mochitests is to test the offsets, language as default attribute is something separate which we should create a new bug for and include a mochitest for that in that bug.

2. The difference between area9 and area10 is that area9's html:p element has a style on it, area10 does not. As you can see, in the default attributes for area10, the font size is different, also the font size for "120%" is bigger there. I did this to be able to test styled elements within both a styled, and an unstyled paragraph.

3. Yes, I believe you should use this locally to test the bug fixes you make, and once your patch goes in, we'll put this one along with yours, fixing both bugs at the same time.

Comment 20

[alexander :surkov (:asurkov)]

Ok. Now I'm working on bug 475522. There I'm doing text attribute code reorganization. New code logic shouldn't allow to introduce all problems I described here, including 'language' default text attribute. That's why I'm asking to add it here. As well, I would like to see tests for getTextAttributes(true, ...) (now all our tests are use getTextAttributes(false, ...)).

I still didn't get you about area9 and area10. I don't see a difference between them from point of view our code logic, i.e. I think tests for aria9 fails if and only if tests for aria10 fails. If I'm wrong then I believe we should add similar tests for background-color and etc.

Comment 21

[Marco Zehe (:MarcoZ)]

I did what you asked me to do: Create a paragraph that has no styling, and kept the one that has styling. I added area10 because of your comment#15.

I'll add tests that include the default attributes.

Comment 22

[alexander :surkov (:asurkov)]

(In reply to comment #21)
> I did what you asked me to do: Create a paragraph that has no styling, and kept
> the one that has styling. I added area10 because of your comment#15.

I asked to kept it? Sorry. I don't think we really need it. But any way if you want to keep it then it's fine with me. Additional testing won't hurt anybody.

Comment 23

[Marco Zehe (:MarcoZ)]

Attachment: Latest work

As agreed on IRC, here's my latest patch for Alex to continue working on since he's going to change interfaces etc.

Comment 24

[alexander :surkov (:asurkov)]

checked in with the patch of bug 475522.