Closed Bug 1241631 Opened 5 years ago Closed 11 months ago

IA2 Accessible Name value has no spaces

Categories

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

43 Branch
defect

Tracking

()

RESOLVED FIXED
mozilla71
Tracking Status
firefox71 --- fixed

People

(Reporter: mark, Assigned: jfkthame)

References

(Blocks 1 open bug)

Details

Attachments

(7 files)

Attached file 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.
Component: Untriaged → Disability Access APIs
Version: unspecified → 43 Branch
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?
Blocks: namea11y
Status: UNCONFIRMED → NEW
Ever confirmed: true
Flags: needinfo?(roc)
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.
Flags: needinfo?(roc)
I guess we should avoid trimming leading whitespace as well, if it's after a soft line break.
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;
Attachment #8725016 - Flags: review?(mats) → review+
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?
Attachment #8725017 - Flags: review?(mats) → review+
> 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.
It was a stupid bug... TRIM_WHITESPACE_AT_LINE_BOUNDARIES in GetRenderedText should be TRIM_WHITESPACE_AT_HARD_LINE_BOUNDARIES.
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.
Flags: needinfo?(mats)
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.
Flags: needinfo?(mats) → needinfo?(mreavy)
Alex this is an old bug, what priority would you give it?
Flags: needinfo?(mreavy) → needinfo?(surkov.alexander)
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.
Flags: needinfo?(surkov.alexander) → needinfo?(mzehe)
Priority: -- → P3
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.
Flags: needinfo?(mzehe)
See Also: → 1494746

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

Attached file 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).

Pushed by jkew@mozilla.com:
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
Created web-platform-tests PR https://github.com/web-platform-tests/wpt/pull/19125 for changes under testing/web-platform/tests
Upstream web-platform-tests status checks passed, PR will merge once commit reaches central.
Assignee: nobody → jfkthame
Upstream PR merged by moz-wptsync-bot
Duplicate of this bug: 1494746
You need to log in before you can comment on or make changes to this bug.