Bug 229713 - Braille display is blank in contenteditable elements when the field is followed by another element.
Summary: Braille display is blank in contenteditable elements when the field is follow...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Andres Gonzalez
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2021-08-31 08:12 PDT by Andres Gonzalez
Modified: 2021-09-02 15:39 PDT (History)
9 users (show)

See Also:


Attachments
Patch (8.72 KB, patch)
2021-08-31 08:41 PDT, Andres Gonzalez
no flags Details | Formatted Diff | Diff
Patch (4.20 KB, patch)
2021-08-31 14:13 PDT, Andres Gonzalez
no flags Details | Formatted Diff | Diff
Patch (10.86 KB, patch)
2021-09-01 19:08 PDT, Andres Gonzalez
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Andres Gonzalez 2021-08-31 08:12:29 PDT
Braille display is blank in contenteditable elements when the field is followed by another element.
Comment 1 Andres Gonzalez 2021-08-31 08:24:39 PDT
rdar://82095237
Comment 2 Andres Gonzalez 2021-08-31 08:28:20 PDT
To reproduce:
1. Safari with VoiceOver and Braille output.
2. open a page containing the following HTML snippet:

<div id="textfield" contenteditable="true">this is a test.</div>
<button>Submit</button>

Focus the text field and observe Braille output.
Comment 3 Andres Gonzalez 2021-08-31 08:41:05 PDT
Created attachment 436887 [details]
Patch
Comment 4 Andres Gonzalez 2021-08-31 14:13:39 PDT
Created attachment 436944 [details]
Patch
Comment 5 chris fleizach 2021-08-31 14:14:48 PDT
Comment on attachment 436944 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=436944&action=review

> Source/WebCore/ChangeLog:15
> +        ranges in text fields to match the number of charaters in the line.

if we're changing behavior, then I think we need a new test that will capture that
Comment 6 Andres Gonzalez 2021-09-01 19:08:30 PDT
Created attachment 437105 [details]
Patch
Comment 7 Andres Gonzalez 2021-09-01 19:14:25 PDT
(In reply to chris fleizach from comment #5)
> Comment on attachment 436944 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=436944&action=review
> 
> > Source/WebCore/ChangeLog:15
> > +        ranges in text fields to match the number of charaters in the line.
> 
> if we're changing behavior, then I think we need a new test that will
> capture that

Added test cases for single and multiline TextArea and content editable, and single line input text field.
Comment 8 chris fleizach 2021-09-01 23:09:56 PDT
Comment on attachment 437105 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=437105&action=review

> Source/WebCore/accessibility/AccessibilityRenderObject.cpp:2468
> +    return it.text().length() == 1 && it.text()[0] == '\n';

is it worth caching it.text(). it must cost something to gather the text for a range
Comment 9 Andres Gonzalez 2021-09-02 05:30:08 PDT
(In reply to chris fleizach from comment #8)
> Comment on attachment 437105 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=437105&action=review
> 
> > Source/WebCore/accessibility/AccessibilityRenderObject.cpp:2468
> > +    return it.text().length() == 1 && it.text()[0] == '\n';
> 
> is it worth caching it.text(). it must cost something to gather the text for
> a range

It is an inline accessor to m_text:

    StringView text() const { ASSERT(!atEnd()); return m_text; }
Comment 10 EWS 2021-09-02 05:45:54 PDT
Committed r281920 (241229@main): <https://commits.webkit.org/241229@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 437105 [details].
Comment 11 Andres Gonzalez 2021-09-02 06:01:55 PDT
rdar://82095237
Comment 12 Darin Adler 2021-09-02 15:39:39 PDT
Comment on attachment 437105 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=437105&action=review

>>> Source/WebCore/accessibility/AccessibilityRenderObject.cpp:2468
>>> +    return it.text().length() == 1 && it.text()[0] == '\n';
>> 
>> is it worth caching it.text(). it must cost something to gather the text for a range
> 
> It is an inline accessor to m_text:
> 
>     StringView text() const { ASSERT(!atEnd()); return m_text; }

Pushing in the opposite direction: I’m pretty sure you can just write:

    return it.text() == "\n";

And the code will be nearly as efficient; don’t have to write out the length/character check.