Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1951353002:
Fixed a bug that assumed that static text object span only one line (Closed)
|
Created:
4 years, 7 months ago by nektarios Modified:
4 years, 7 months ago Reviewers:
dmazzoni CC:
aboxhall+watch_chromium.org, chromium-reviews, darin-cc_chromium.org, dmazzoni+watch_chromium.org, dtseng+watch_chromium.org, jam, je_julie, nektar+watch_chromium.org, yuzo+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionThe bug fixed involves not recursing into inline text boxes in the case that a static text object covered more than one line.
Also fixed a bug on Windows that didn't return the correct line boundaries if the start offset was equal to the text's length, something that is allowed by the IA2 Spec.
BUG=605670
R=dmazzoni@chromium.org
TESTED=manual testing with Gmail > Compose and Docs, browser tests
Committed: https://crrev.com/74bdbcb2a0b6cd2810e8b666cd5fefbd600039fd
Cr-Commit-Position: refs/heads/master@{#392424}
Patch Set 1 #
Total comments: 1
Patch Set 2 : Fixed a corner case when the offset is equal to the text length, e.g. caret is at the end of the te… #Patch Set 3 : Fixed as much as possible with textareas. #Patch Set 4 : Added back legasy logic for handling input and textarea elements. #
Messages
Total messages: 24 (8 generated)
Just one question https://codereview.chromium.org/1951353002/diff/1/content/browser/accessibili... File content/browser/accessibility/browser_accessibility.cc (right): https://codereview.chromium.org/1951353002/diff/1/content/browser/accessibili... content/browser/accessibility/browser_accessibility.cc:526: if (IsSimpleTextControl() && InternalChildCount() == 1) Why doesn't it work if you just delete this if statement entirely? It seems like this logic below should work for any container element by recursing until it finds the first inline text box descendant.
https://codereview.chromium.org/1951353002/diff/1/content/browser/accessibili... > File content/browser/accessibility/browser_accessibility.cc (right): > > https://codereview.chromium.org/1951353002/diff/1/content/browser/accessibili... > content/browser/accessibility/browser_accessibility.cc:526: if > (IsSimpleTextControl() && InternalChildCount() == 1) > Why doesn't it work if you just delete this if statement entirely? It > seems like this logic below should work for any container element by > recursing until it finds the first inline text box descendant. Correct. But in the case of an <input> element, there is a single div child object in the shadow DOM which holds all the text. So, if NVDA calls input->get_textAtOffset(...), they actually want div->get_textAtOffset(...). Otherwise, the input object will only expose a single embedded object character for the div, which is unexpected in a simple text box because simple text boxes don't have embedded objects in them. I could try and remove this and see if NVDA / Jaws stop working, but should we risk it? -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
That doesn't make sense to me. The input element has a single div as its child. But the div has a single child, which is a static text element. So it's going to be recursive either way. The reason I want to try to get rid of it is that it's very fragile to assume that an input has a single div as the child today. The shadow dom of an input element could be rewritten in the future and I don't want this to break. Please try getting rid of it. I think it should still work because it just keeps going recursively. On 2016/05/05 18:51:02, chromium-reviews wrote: > Correct. But in the case of an <input> element, there is a single div > child object in the shadow DOM which holds all the text. > So, if NVDA calls input->get_textAtOffset(...), they actually want > div->get_textAtOffset(...). Otherwise, the input object will only expose > a single embedded object character for the div, which is unexpected in a > simple text box because simple text boxes don't have embedded objects in > them. > I could try and remove this and see if NVDA / Jaws stop working, but > should we risk it? > > -- > You received this message because you are subscribed to the Google Groups > "Chromium-reviews" group. > To unsubscribe from this group and stop receiving emails from it, send an email > to mailto:chromium-reviews+unsubscribe@chromium.org.
As discussed offline, just add a TODO in the places where you assume that a simple text control has a single div that contains only static text. I'm worried that isn't true for textareas with multiple paragraphs now, might be worth checking that.
I checked and the textarea elements don't have multiple paragraphs inside them. Simply static text nodes for each line and a static text node with a "\n" as its name in between paragraphs. Strange that they don't use a real line break object . TODO added. Manual and automatic tests pass, except the following: If the text in a textarea wraps, a space character is inserted in the form of an extra inline text box, at the point where the break happens. This means that Jaws reads one more character from the next line. I don't know how to fix this other than adding another hack. I'd prefer to do it in another patch. -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
I decided to revert back to the old logic because the extra static text objects at the end of each line are difficult to deal with, without adding a specific hack for textareas. -- You received this message because you are subscribed to the Google Groups "Chromium-reviews" group. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
The CQ bit was checked by nektar@chromium.org
Please update the change description to reflect what this change really does and I'll take another look. Should be fine, but previously I was more concerned about the changes to standard text fields.
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1951353002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1951353002/60001
The CQ bit was unchecked by commit-bot@chromium.org
No L-G-T-M from a valid reviewer yet. CQ run can only be started by full committers or once the patch has received an L-G-T-M from a full committer. Even if an L-G-T-M may have been provided, it was from a non-committer, _not_ a full super star committer. See http://www.chromium.org/getting-involved/become-a-committer Note that this has nothing to do with OWNERS files.
Description was changed from ========== Switched to using the new line boundary code for standard text fields and fixed a bug. The bug fixed involves not recursing into inline text boxes in the case that a static text object covered more than one line. BUG=605670 R=dmazzoni@chromium.org ========== to ========== The bug fixed involves not recursing into inline text boxes in the case that a static text object covered more than one line. Also fixed a bug on Windows that didn't return the correct line boundaries if the start offset was equal to the text's length, something that is allowed by the IA2 Spec. BUG=605670 R=dmazzoni@chromium.org ==========
Description was changed from ========== The bug fixed involves not recursing into inline text boxes in the case that a static text object covered more than one line. Also fixed a bug on Windows that didn't return the correct line boundaries if the start offset was equal to the text's length, something that is allowed by the IA2 Spec. BUG=605670 R=dmazzoni@chromium.org ========== to ========== The bug fixed involves not recursing into inline text boxes in the case that a static text object covered more than one line. Also fixed a bug on Windows that didn't return the correct line boundaries if the start offset was equal to the text's length, something that is allowed by the IA2 Spec. BUG=605670 R=dmazzoni@chromium.org TESTED=manual testing with Gmail > Compose and Docs, browser tests ==========
The CQ bit was checked by nektar@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1951353002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1951353002/60001
The CQ bit was unchecked by commit-bot@chromium.org
No L-G-T-M from a valid reviewer yet. CQ run can only be started by full committers or once the patch has received an L-G-T-M from a full committer. Even if an L-G-T-M may have been provided, it was from a non-committer, _not_ a full super star committer. See http://www.chromium.org/getting-involved/become-a-committer Note that this has nothing to do with OWNERS files.
lgtm
The CQ bit was checked by nektar@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1951353002/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1951353002/60001
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== The bug fixed involves not recursing into inline text boxes in the case that a static text object covered more than one line. Also fixed a bug on Windows that didn't return the correct line boundaries if the start offset was equal to the text's length, something that is allowed by the IA2 Spec. BUG=605670 R=dmazzoni@chromium.org TESTED=manual testing with Gmail > Compose and Docs, browser tests ========== to ========== The bug fixed involves not recursing into inline text boxes in the case that a static text object covered more than one line. Also fixed a bug on Windows that didn't return the correct line boundaries if the start offset was equal to the text's length, something that is allowed by the IA2 Spec. BUG=605670 R=dmazzoni@chromium.org TESTED=manual testing with Gmail > Compose and Docs, browser tests Committed: https://crrev.com/74bdbcb2a0b6cd2810e8b666cd5fefbd600039fd Cr-Commit-Position: refs/heads/master@{#392424} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/74bdbcb2a0b6cd2810e8b666cd5fefbd600039fd Cr-Commit-Position: refs/heads/master@{#392424} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
