IA2 Accessible Name value has no spaces
Categories
(Core :: Disability Access APIs, defect, P3)
Tracking
()
Tracking | Status | |
---|---|---|
firefox71 | --- | fixed |
People
(Reporter: mark, Assigned: jfkthame)
References
(Blocks 1 open bug)
Details
Attachments
(7 files)
373.38 KB,
text/html
|
Details | |
58 bytes,
text/x-review-board-request
|
MatsPalmgren_bugz
:
review+
|
Details |
58 bytes,
text/x-review-board-request
|
MatsPalmgren_bugz
:
review+
|
Details |
47 bytes,
text/x-phabricator-request
|
Details | Review | |
47 bytes,
text/x-phabricator-request
|
Details | Review | |
1.65 KB,
text/html
|
Details | |
47 bytes,
text/x-phabricator-request
|
Details | Review |
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•8 years ago
|
||
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?
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.
I guess we should avoid trimming leading whitespace as well, if it's after a soft line break.
Review commit: https://reviewboard.mozilla.org/r/37279/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/37279/
Review commit: https://reviewboard.mozilla.org/r/37281/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/37281/
Comment 6•8 years ago
|
||
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•8 years ago
|
||
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•8 years ago
|
||
> 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.
This causes a lot of accessibility tests to fail on try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=323ee74dc5ac&selectedJob=17474493
It was a stupid bug... TRIM_WHITESPACE_AT_LINE_BOUNDARIES in GetRenderedText should be TRIM_WHITESPACE_AT_HARD_LINE_BOUNDARIES.
Still got test failures.
Comment 13•6 years ago
|
||
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•6 years ago
|
||
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•6 years ago
|
||
Alex this is an old bug, what priority would you give it?
Comment 16•6 years ago
|
||
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•6 years ago
|
||
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.
Assignee | ||
Comment 18•5 years ago
|
||
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
Assignee | ||
Comment 19•5 years ago
|
||
Assignee | ||
Comment 20•5 years ago
|
||
Depends on D45157
Comment 21•5 years ago
|
||
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).
Assignee | ||
Comment 22•5 years ago
|
||
Depends on D45159
Comment 23•5 years ago
|
||
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.
Comment 26•5 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/fa06e1da9426
https://hg.mozilla.org/mozilla-central/rev/040bfa810af6
https://hg.mozilla.org/mozilla-central/rev/4dc6ff8d58b3
Updated•5 years ago
|
Upstream PR merged by moz-wptsync-bot
Description
•