1241631 - IA2 Accessible Name value has no spaces

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect, P3)

Description

[mark]

Attachment: offscreen-bug.html

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.10; rv:43.0) Gecko/20100101 Firefox/43.0
Build ID: 20160105164030

Steps to reproduce:

Created an element with height and width of 1px, positioned absolutely, and with word-break set to break-word.

Test file attached, but also available:
http://sadecki.com/sandbox/offscreen-bug.html


Actual results:

Observed that the IA2 Accessible Name value contained the Accessible Name with spaces removed.


Expected results:

The IA2 Accessible Name should contain the original spaces.

Comment 1

[Marco Zehe (:MarcoZ)]

This is confirmed in Nightly (46), too. Since we are getting the text from layout, there should be a way to get the text without the spaces removed. Roc, is there a call we can make into layout, or some flag we can pass in, that gives us the names with spaces?

Comment 2

[Robert O'Callahan (:roc) (email my personal email if necessary)]

This happens because the spaces get moved to the start of each frame. I.e. "a b c" gets turned into lines containing "a", " b", and " c". nsTextFrame::GetRenderedText always trims whitespace at the start of the line (for white-space:normal text), whatever you pass for DONT_TRIM_TRAILING_WHITESPACE.

Comment 3

[Robert O'Callahan (:roc) (email my personal email if necessary)]

I guess we should avoid trimming leading whitespace as well, if it's after a soft line break.

Comment 4

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: MozReview Request: Bug 1241631. Make nsTextFrame::GetTrimmedOffsets take a flags word parameter. r=mats

Review commit: https://reviewboard.mozilla.org/r/37279/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/37279/

Comment 5

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Attachment: MozReview Request: Bug 1241631. Don't trim whitespace immediately after a soft line break when computing the rendered text for accessibility. r=mats

Review commit: https://reviewboard.mozilla.org/r/37281/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/37281/

Comment 6

[Mats Palmgren (inactive)]

Comment on attachment 8725016 [details]
MozReview Request: Bug 1241631. Make nsTextFrame::GetTrimmedOffsets take a flags word parameter. r=mats

https://reviewboard.mozilla.org/r/37279/#review33875

::: layout/generic/nsTextFrame.h:543
(Diff revision 1)
> +  enum {

Please document each of these constants.
We might want to take the opportunity of improving the name of
the last one to TRIM_IS_POST_REFLOW ?

It looks like many call sites use BEFORE+AFTER unconditionally, so it
might be worth adding an TRIM_BEFORE_AFTER alias for that.

::: layout/generic/nsTextFrame.cpp:2790
(Diff revision 1)
> -  if (aPostReflow) {
> +  if (aFlags & TRIM_POST_REFLOW) {

Using a temp bool here might improve code readability:
const bool isPostReflow = aFlags & TRIM_IS_POST_REFLOW;

Comment 7

[Mats Palmgren (inactive)]

Comment on attachment 8725017 [details]
MozReview Request: Bug 1241631. Don't trim whitespace immediately after a soft line break when computing the rendered text for accessibility. r=mats

https://reviewboard.mozilla.org/r/37281/#review33881

::: layout/generic/nsTextFrame.cpp:9287
(Diff revision 1)
> +    if (textFrame->IsAtBeginningOfLine() &&

I think this would be more readable and slightly more performant like so:

if (MOZ_LIKELY(aTrimTrailingWhitespace == 
                 RenderWhitespace::TRIM_WHITESPACE_AT_LINE_BOUNDARIES))
  if (IsAtBeginningOfLine &&
      LineStartsAtHardLineBreak)
    ...
  if (IsAtEndOfLine &&
      LineEndsInHardLineBreak)
    ...

BTW, is there a reason to not make the IsAtBeginning/EndOfLine tests
early returns in the respective Line*Break functions instead?

Comment 8

[Mats Palmgren (inactive)]

> BTW, is there a reason to not make the IsAtBeginning/EndOfLine tests
> early returns in the respective Line*Break functions instead?

To clarify: I think a call to Line*Break without checking the corresponding
frame bit first is likely to be a mistake.

Comment 9

[Robert O'Callahan (:roc) (email my personal email if necessary)]

This causes a lot of accessibility tests to fail on try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=323ee74dc5ac&selectedJob=17474493

Comment 10

[Robert O'Callahan (:roc) (email my personal email if necessary)]

It was a stupid bug... TRIM_WHITESPACE_AT_LINE_BOUNDARIES in GetRenderedText should be TRIM_WHITESPACE_AT_HARD_LINE_BOUNDARIES.

Comment 11

[Robert O'Callahan (:roc) (email my personal email if necessary)]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=6800f7e8039e

Comment 12

[Robert O'Callahan (:roc) (email my personal email if necessary)]

Still got test failures.

Comment 13

[Marco Zehe (:MarcoZ)]

Mats, any idea who could finish this? This is still a problem in latest Nightly. I know Roc is no longer with us, so...Asking you as the reviewer if you have any idea who might be able to pick this up.

Comment 14

[Mats Palmgren (inactive)]

Me or jfkthame probably, but we're both kinda busy elsewhere ATM.

FTR, what remains to be done here is:
1. unbitrot the patches
2. investigate comment 8 (which I thought was a bug IIRC), and comment 12.

Comment 15

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

Alex this is an old bug, what priority would you give it?

Comment 16

[alexander :surkov (:asurkov)]

Given the time frame the bug stays unfixed and unrevisited, I would say p3 would be fair, at least until ARIA-Core spec compatibility is in the nearest plans. So I would defer to Marco to make a final call, since he look into name computation issues lately.

Comment 17

[Marco Zehe (:MarcoZ)]

It's definitely a bug on our side, since these should include spaces, only stuff from :before or :after should be prepended/appended without spaces. But revisiting it together with other spec work is fine. Unless someone wants to pick it up before and finish Roc's patch.

Comment 18

[Jonathan Kew [:jfkthame]]

Since roc's original patches here, there have been changes to the GetTrimmedOffsets flags, so I had a go at updating this. A much reduced version of the patch is now sufficient to fix the example here (and similar cases where we return innerText with missing spaces); tryserver seems happy with this (including basic tests): https://treeherder.mozilla.org/#/jobs?repo=try&revision=b12da8209ca0532bcc7eddd8dbc8153473f46479&duplicate_jobs=visible

Comment 19

[Jonathan Kew [:jfkthame]]

Attachment: Bug 1241631 - Don't trim leading whitespace from soft-wrapped lines when getting rendered text for a11y or .innerText. r=mats

Comment 20

[Jonathan Kew [:jfkthame]]

Attachment: Bug 1241631 - Add reftest for bug in innerText with soft-wrapped text and word-break:break-all. r=mats

Depends on D45157

Comment 21

[Mats Palmgren (inactive)]

Attachment: Testcase #2

STR: load the testcase and see results in Console.
It appears this patch doesn't treat spaces between child elements the same as Chrome (case 2 and 3).
Ditto for dynamically inserted text nodes (cases 15 and 16).

Comment 22

[Jonathan Kew [:jfkthame]]

Attachment: Bug 1241631 - Add more whitespace-at-soft-break examples (based on Testcase #2) to the WPT innerText getter test. r?mats

Depends on D45159

Comment 23

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/fa06e1da9426
Don't trim leading whitespace from soft-wrapped lines when getting rendered text for a11y or .innerText. r=mats
https://hg.mozilla.org/integration/autoland/rev/040bfa810af6
Add reftest for bug in innerText with soft-wrapped text and word-break:break-all. r=mats
https://hg.mozilla.org/integration/autoland/rev/4dc6ff8d58b3
Add more whitespace-at-soft-break examples (based on Testcase #2) to the WPT innerText getter test. r=mats

Comment 24

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/19125 for changes under testing/web-platform/tests

Comment 25

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Upstream web-platform-tests status checks passed, PR will merge once commit reaches central.

Comment 26

[Alexandru Michis [:malexandru]]

https://hg.mozilla.org/mozilla-central/rev/fa06e1da9426
https://hg.mozilla.org/mozilla-central/rev/040bfa810af6
https://hg.mozilla.org/mozilla-central/rev/4dc6ff8d58b3

Comment 27

[Web Platform Test Sync Bot [:wpt-sync] (Matrix: #interop:mozilla.org)]

Upstream PR merged by moz-wptsync-bot