If you think a bug might affect users in the 57 release, please set the correct tracking and status flags for Release Management.

provide robust text attribute offset testing

RESOLVED FIXED

Status

()

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

People

(Reporter: surkov, Assigned: MarcoZ)

Tracking

(Blocks: 1 bug)

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

(Reporter)

Description

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

Comment 2

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

Comment 3

9 years ago
Created attachment 359052 [details] [diff] [review]
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.
Assignee: nobody → marco.zehe
Status: NEW → ASSIGNED
(Assignee)

Comment 4

9 years ago
Created attachment 359066 [details] [diff] [review]
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.
Attachment #359052 - Attachment is obsolete: true
Attachment #359066 - Flags: review?(surkov.alexander)
(Reporter)

Comment 5

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

Comment 6

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

Comment 7

9 years ago
Marco, it sounds font-family, text-decoration=line-through, underline aren't addressed in mochitests.
(Reporter)

Updated

9 years ago
Attachment #359066 - Flags: review?(surkov.alexander)
(Reporter)

Updated

9 years ago
Blocks: 475522
(Reporter)

Comment 8

9 years ago
Marco, please note we do not expose 'language' text attribute as default text
attribute which is a bug.
(Assignee)

Comment 9

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

Comment 10

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

Comment 11

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

Comment 12

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

Comment 13

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

Comment 14

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

Comment 15

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

Comment 16

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

Comment 17

9 years ago
Created attachment 359492 [details] [diff] [review]
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.
Attachment #359066 - Attachment is obsolete: true
Attachment #359492 - Flags: review?(surkov.alexander)
(Reporter)

Updated

9 years ago
Attachment #359492 - Flags: review?(surkov.alexander)
(Reporter)

Comment 18

9 years ago
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?
(Assignee)

Comment 19

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

Comment 20

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

Comment 21

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

Comment 22

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

Comment 23

9 years ago
Created attachment 360054 [details] [diff] [review]
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.
Attachment #359492 - Attachment is obsolete: true
(Reporter)

Comment 24

9 years ago
checked in with the patch of bug 475522.
Status: ASSIGNED → RESOLVED
Last Resolved: 9 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.