Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
660633002:
Fixed IAccessibleText::TextAtOffset with IA2_TEXT_BOUNDARY_WORD to return text that spans from the … (Closed)
|
Created:
6 years, 2 months ago by nektarios Modified:
5 years, 10 months ago Reviewers:
dmazzoni CC:
chromium-reviews, plundblad+watch_chromium.org, aboxhall+watch_chromium.org, jam, yuzo+watch_chromium.org, darin-cc_chromium.org, dmazzoni+watch_chromium.org, dtseng+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionFixed IAccessibleText::TextAtOffset with IA2_TEXT_BOUNDARY_WORD to return text that spans from the start of one word to the start of the next.
BUG=347852
R=dmazzoni@chromium.org
Committed: https://crrev.com/6baff46f520e31ff92669890207be5708064d16e
Cr-Commit-Position: refs/heads/master@{#317996}
Patch Set 1 #
Total comments: 2
Patch Set 2 : Moved implementation of GetWordStartBoundary to base class and fixed one-by-off offset error when m… #Patch Set 3 : Added browser test. #
Total comments: 7
Patch Set 4 : Added semi-automated NVDA test for word navigation and removed dependency on pywinauto. #
Total comments: 11
Patch Set 5 : Implemented testing for special offsets (cursor position and text length). Fixed memory leak when r… #
Total comments: 8
Patch Set 6 : Broke up different boundary tests in their own test cases. #
Total comments: 8
Patch Set 7 : Split single from multi line tests. #Patch Set 8 : Moved manual test changes to another branch. #
Total comments: 5
Patch Set 9 : Removed flaky linebreak detection. #Patch Set 10 : Single-line text fields all tests pass. #
Total comments: 3
Patch Set 11 : Fixed setting the caret and added a corresponding test (preliminary). #Patch Set 12 : Added logging to AccessibilityWaiter. #Patch Set 13 : Fixed SetCaret tests. #Patch Set 14 : Limited effect of change to edit fields. #Patch Set 15 : Fixed forward declaring enum. #Patch Set 16 : Fixed word navigation on Windows and added browser tests. #
Total comments: 1
Patch Set 17 : Disabled mltiline unit test. #Messages
Total messages: 32 (5 generated)
Cool, looks like this is on the right track https://codereview.chromium.org/660633002/diff/1/content/browser/accessibilit... File content/browser/accessibility/browser_accessibility_win.cc (right): https://codereview.chromium.org/660633002/diff/1/content/browser/accessibilit... content/browser/accessibility/browser_accessibility_win.cc:3312: LONG BrowserAccessibilityWin::FindWordStartBoundary( Please move this to browser_accessibility.cc instead so that we can use it on other platforms if we want to. I believe we need it on Mac too. https://codereview.chromium.org/660633002/diff/1/content/browser/accessibilit... content/browser/accessibility/browser_accessibility_win.cc:3343: if (!word_starts.empty()) { When possible, reverse the direction of a test to minimize indentation, i.e.: if (word_starts.empty()) continue;
Looking really good! The test seems like a great example of what we want to test for. https://codereview.chromium.org/660633002/diff/40001/content/browser/accessib... File content/browser/accessibility/accessibility_win_browsertest.cc (right): https://codereview.chromium.org/660633002/diff/40001/content/browser/accessib... content/browser/accessibility/accessibility_win_browsertest.cc:128: LONG start_offset = 0; Consider adding a SCOPED_TRACE here to aid in debugging - if one of the expectations below fails, it can help make it clear which check failed by logging the whole context of the failure https://codereview.chromium.org/660633002/diff/40001/content/browser/accessib... content/browser/accessibility/accessibility_win_browsertest.cc:855: for (LONG offset = 0; offset < contents_length; ++offset) { Consider a SCOPED_TRACE inside each loop - that way if CheckTextAtOffset fails it's more clear which loop and which iteration of the loop failed. https://codereview.chromium.org/660633002/diff/40001/content/browser/accessib... content/browser/accessibility/accessibility_win_browsertest.cc:914: // Test sentense navigation (not currently implemented). nit: sentense -> sentence https://codereview.chromium.org/660633002/diff/40001/content/browser/accessib... File content/browser/accessibility/browser_accessibility.cc (right): https://codereview.chromium.org/660633002/diff/40001/content/browser/accessib... content/browser/accessibility/browser_accessibility.cc:358: return word_start; Perhaps the return value should vary based on |direction|? if direction is ui::FORWARDS_DIRECTION, if no word start boundaries are found don't you think it should return the total length of the recursive text? https://codereview.chromium.org/660633002/diff/40001/content/browser/accessib... content/browser/accessibility/browser_accessibility.cc:372: child_end += child_len; // End is one passed the last character. nit: one passed -> one past https://codereview.chromium.org/660633002/diff/40001/content/browser/accessib... content/browser/accessibility/browser_accessibility.cc:400: // Return the start of the last word in the previous child. One option would be to treat an inline text box boundary as the start / end of a word. There are only a few cases where a word would span an inline text box boundary. Case 1: suppose that the word wrap is set to break words with no hyphens - i believe that's one of the allowed options. So then a word might start on one line and continue to the next. If you're moving by word in a screen reader, would you really want it to jump from the middle of one line to the middle of the next? I'm not sure I would - I'd probably want it to jump to the beginning of the line, but I'm not sure Case 2: suppose someone makes half of a word bold - should moving by word break at the style transition, or should it just skip to the beginning / end of the word spanning the style transition? The other cases will be similar - the only reason a single word will span two inline text boxes is if there's a good reason, like a line break, a style chnage, etc. - and I just want to make sure we're deliberate about what we choose to do. https://codereview.chromium.org/660633002/diff/40001/content/browser/accessib... content/browser/accessibility/browser_accessibility.cc:415: return word_start; Same - if searching forwards I think the default to return should be the maximum value, not zero.
https://codereview.chromium.org/660633002/diff/40001/content/browser/accessib... > content/browser/accessibility/browser_accessibility.cc:358: return > word_start; > Perhaps the return value should vary based on |direction|? if direction > is ui::FORWARDS_DIRECTION, if no word start boundaries are found don't > you think it should return the total length of the recursive text? > > https://codereview.chromium.org/660633002/diff/40001/content/browser/accessib... > > content/browser/accessibility/browser_accessibility.cc:415: return > word_start; > Same - if searching forwards I think the default to return should be the > maximum value, not zero. I agree but isn't that what it currently does? Perhaps there is a bug? Initially it is set to 0 but it is incremented every time the boundary is not found, until it reaches the size of the text. if (start >= child_len) { word_start += child_len; ... // In case there are no more children or remaining children have no // text, e.g., an image. word_start = child_end; To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On Thu, Oct 30, 2014 at 11:56 AM, Nektarios Paisios <nektar@google.com> wrote: > https://codereview.chromium.org/660633002/diff/40001/ > content/browser/accessibility/browser_accessibility.cc#newcode358 > >> content/browser/accessibility/browser_accessibility.cc:358: return >> word_start; >> Perhaps the return value should vary based on |direction|? if direction >> is ui::FORWARDS_DIRECTION, if no word start boundaries are found don't >> you think it should return the total length of the recursive text? >> >> https://codereview.chromium.org/660633002/diff/40001/ >> content/browser/accessibility/browser_accessibility.cc#newcode415 >> content/browser/accessibility/browser_accessibility.cc:415: return >> word_start; >> Same - if searching forwards I think the default to return should be the >> maximum value, not zero. >> > > I agree but isn't that what it currently does? Perhaps there is a bug? > Initially it is set to 0 but it is incremented every time the boundary is > not found, until it reaches the size of the text. > if (start >= child_len) { > word_start += child_len; > Ah, I see now. You're right, thanks. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
https://codereview.chromium.org/660633002/diff/60001/tools/accessibility/nvda... File tools/accessibility/nvda/nvda_chrome_tests.py (right): https://codereview.chromium.org/660633002/diff/60001/tools/accessibility/nvda... tools/accessibility/nvda/nvda_chrome_tests.py:34: import winapi I can't find the winapi module. Did you mean win32api? https://codereview.chromium.org/660633002/diff/60001/tools/accessibility/nvda... tools/accessibility/nvda/nvda_chrome_tests.py:109: winapi.focusWindow(self._chrome_proc.pid) I can't find the focusWindow function in win32api, where is it defined? https://codereview.chromium.org/660633002/diff/60001/tools/accessibility/nvda... tools/accessibility/nvda/nvda_chrome_tests.py:228: 'data:text/html,<!DOCTYPE html><html><body>Word navigation</body></html>') You'll need an editable text field here for this to work.
On 11/1/2014 2:45 AM, dmazzoni@chromium.org wrote: > > https://codereview.chromium.org/660633002/diff/60001/tools/accessibility/nvda... > > File tools/accessibility/nvda/nvda_chrome_tests.py (right): > > https://codereview.chromium.org/660633002/diff/60001/tools/accessibility/nvda... > > tools/accessibility/nvda/nvda_chrome_tests.py:34: import winapi > I can't find the winapi module. Did you mean win32api? > No. I wrote this module. It's in the same directory. I added it to git with "git add". Didn't it appear? > https://codereview.chromium.org/660633002/diff/60001/tools/accessibility/nvda... > > tools/accessibility/nvda/nvda_chrome_tests.py:109: > winapi.focusWindow(self._chrome_proc.pid) > I can't find the focusWindow function in win32api, where is it defined? > It's not in win32api, it's in my own module. > https://codereview.chromium.org/660633002/diff/60001/tools/accessibility/nvda... > > tools/accessibility/nvda/nvda_chrome_tests.py:228: > 'data:text/html,<!DOCTYPE html><html><body>Word > navigation</body></html>') > You'll need an editable text field here for this to work. > Yes, but I want to have two tests. One for navigating with the virtual cursor and one with the interactive one. I have to first test out if sending insert+space would work to turn on the interactive cursor with NVDA. I tried it with Jaws on Friday sending insert+z and it didn't work. I think that Jaws installs a very low-level keyboard hook or something. I have to try it with NVDA today and see if it would work, or otherwise find another way to turn on the interactive cursor first, before writing the actual test. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
On 2014/11/03 15:16:28, chromium-reviews wrote: > > tools/accessibility/nvda/nvda_chrome_tests.py:34: import winapi > > I can't find the winapi module. Did you mean win32api? > > > No. I wrote this module. It's in the same directory. I added it to git > with "git add". Didn't it appear? Oh! Sorry I missed it, didn't even see the new file. Looks great, you know more low-level Windows internals than I do. Did you base this on anything or is it from scratch? > Yes, but I want to have two tests. One for navigating with the virtual > cursor and one with the interactive one. I have to first test out if > sending insert+space would work to turn on the interactive cursor with > NVDA. I tried it with Jaws on Friday sending insert+z and it didn't > work. I think that Jaws installs a very low-level keyboard hook or > something. I have to try it with NVDA today and see if it would work, or > otherwise find another way to turn on the interactive cursor first, > before writing the actual test. Oh cool, I see. If you can't figure it out right away, maybe write a test with an editable text field and then we can figure out how to write a test that sends NVDA or JAWS commands next.
On 11/3/2014 11:08 AM, dmazzoni@chromium.org wrote: > On 2014/11/03 15:16:28, chromium-reviews wrote: >> > tools/accessibility/nvda/nvda_chrome_tests.py:34: import winapi >> > I can't find the winapi module. Did you mean win32api? >> > >> No. I wrote this module. It's in the same directory. I added it to git >> with "git add". Didn't it appear? > > Oh! Sorry I missed it, didn't even see the new file. > > Looks great, you know more low-level Windows internals than I do. Did > you base > this on anything or is it from scratch? > From scratch, i.e., from the docs on MSDN. I also took a look at the NVDA source code to see if they were doing anything that I had forgotten, but didn't find anything else except the inclusion of the key codes for "back", "foreward" and "home" browser buttons, which I included. However, this key codes are also listed on MSDN, so NVDA code didn't actually help and BTW works in a different way than I wanted mine to. I didn't copy anything other than the key codes from it, which as I said, are also listed on MSDN here: http://msdn.microsoft.com/en-us/library/windows/desktop/dd375731%28v=vs.85%29... >> Yes, but I want to have two tests. One for navigating with the virtual >> cursor and one with the interactive one. I have to first test out if >> sending insert+space would work to turn on the interactive cursor with >> NVDA. I tried it with Jaws on Friday sending insert+z and it didn't >> work. I think that Jaws installs a very low-level keyboard hook or >> something. I have to try it with NVDA today and see if it would work, or >> otherwise find another way to turn on the interactive cursor first, >> before writing the actual test. > > Oh cool, I see. > > If you can't figure it out right away, maybe write a test with an > editable text > field and then we can figure out how to write a test that sends NVDA > or JAWS > commands next. > > I could try and send enter to turn on forms mode equivalent for NVDA > but it might be flaky. Trying now. Also, I want to automate the installation of NVDA portable. I think I can pass a commandline switch to NVDA installer to do that, but how do I automate the download of the actual installer? The URL will change for each version. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
https://codereview.chromium.org/660633002/diff/60001/content/browser/accessib... File content/browser/accessibility/accessibility_win_browsertest.cc (right): https://codereview.chromium.org/660633002/diff/60001/content/browser/accessib... content/browser/accessibility/accessibility_win_browsertest.cc:49: static HRESULT QueryIAccessible2(IAccessible* accessible, IAccessible2** accessible2); nit: wrap to 80 chars https://codereview.chromium.org/660633002/diff/60001/content/browser/accessib... content/browser/accessibility/accessibility_win_browsertest.cc:182: L" at " + expected_start_offset + L'-' + expected_end_offset; I don't think you can append integers to strings in C++ and expect it to do what you want. Try this syntax: SCOPED_TRACE(testing::Message() << "While checking for " << expected_text << " at " << expected_start_offset... https://codereview.chromium.org/660633002/diff/60001/content/browser/accessib... content/browser/accessibility/accessibility_win_browsertest.cc:883: // Test word navigation. Please add tests for passing IA2_TEXT_OFFSET_LENGTH and IA2_TEXT_OFFSET_CARET as the start offset argument. https://codereview.chromium.org/660633002/diff/60001/content/browser/accessib... File content/browser/accessibility/browser_accessibility.cc (right): https://codereview.chromium.org/660633002/diff/60001/content/browser/accessib... content/browser/accessibility/browser_accessibility.cc:378: ui::AX_ATTR_WORD_STARTS); Interesting, did WORD_ENDS end up being useless? Is the proper behavior to always move to the beginning of each word no matter what direction you're moving? https://codereview.chromium.org/660633002/diff/60001/content/browser/accessib... File content/browser/accessibility/browser_accessibility_win.cc (right): https://codereview.chromium.org/660633002/diff/60001/content/browser/accessib... content/browser/accessibility/browser_accessibility_win.cc:3300: if (ia2_boundary == IA2_TEXT_BOUNDARY_WORD && IsEditableText()) The fact that you're calling IsEditableText() here makes me think that this code isn't going to work when moving by word in non-editable text. Did you notice a difference in behavior in NVDA or JAWS when moving by word in static text? Honestly I didn't even realize they call IAccessibleText for static text - do they? https://codereview.chromium.org/660633002/diff/60001/content/browser/accessib... content/browser/accessibility/browser_accessibility_win.cc:3303: HandleSpecialTextOffset(text, &start_offset); I think you should do this call before GetWordStartBoundary, otherwise you wont't handle IA2_TEXT_OFFSET_LENGTH and IA2_TEXT_OFFSET_CARET correctly. https://codereview.chromium.org/660633002/diff/60001/tools/accessibility/nvda... File tools/accessibility/nvda/winapi.py (right): https://codereview.chromium.org/660633002/diff/60001/tools/accessibility/nvda... tools/accessibility/nvda/winapi.py:17: _ENUM_WINDOWS_CALLBACK = ctypes.WINFUNCTYPE(wintypes.BOOL, wintypes.HWND, wintypes.LPARAM) nit: wrap to 80 chars https://codereview.chromium.org/660633002/diff/60001/tools/accessibility/nvda... tools/accessibility/nvda/winapi.py:130: return hWnds[0] if len(hWnds) > 0 else None If there's more than one, could you return the one that's active or topmost?
Removed memory leak when retrieving accessible children. Implemented special offsets (cursor and text length). Nothing tested yet. Nektarios. On 11/3/2014 11:26 AM, dmazzoni@chromium.org wrote: > > https://codereview.chromium.org/660633002/diff/60001/content/browser/accessib... > > File content/browser/accessibility/accessibility_win_browsertest.cc > (right): > > https://codereview.chromium.org/660633002/diff/60001/content/browser/accessib... > > content/browser/accessibility/accessibility_win_browsertest.cc:49: > static HRESULT QueryIAccessible2(IAccessible* accessible, IAccessible2** > accessible2); > nit: wrap to 80 chars > > https://codereview.chromium.org/660633002/diff/60001/content/browser/accessib... > > content/browser/accessibility/accessibility_win_browsertest.cc:182: L" > at " + expected_start_offset + L'-' + expected_end_offset; > I don't think you can append integers to strings in C++ and expect it to > do what you want. Try this syntax: > > SCOPED_TRACE(testing::Message() << "While checking for " << > expected_text << " at " << expected_start_offset... > > https://codereview.chromium.org/660633002/diff/60001/content/browser/accessib... > > content/browser/accessibility/accessibility_win_browsertest.cc:883: // > Test word navigation. > Please add tests for passing IA2_TEXT_OFFSET_LENGTH and > IA2_TEXT_OFFSET_CARET as the start offset argument. > > https://codereview.chromium.org/660633002/diff/60001/content/browser/accessib... > > File content/browser/accessibility/browser_accessibility.cc (right): > > https://codereview.chromium.org/660633002/diff/60001/content/browser/accessib... > > content/browser/accessibility/browser_accessibility.cc:378: > ui::AX_ATTR_WORD_STARTS); > Interesting, did WORD_ENDS end up being useless? Is the proper behavior > to always move to the beginning of each word no matter what direction > you're moving? > > https://codereview.chromium.org/660633002/diff/60001/content/browser/accessib... > > File content/browser/accessibility/browser_accessibility_win.cc (right): > > https://codereview.chromium.org/660633002/diff/60001/content/browser/accessib... > > content/browser/accessibility/browser_accessibility_win.cc:3300: if > (ia2_boundary == IA2_TEXT_BOUNDARY_WORD && IsEditableText()) > The fact that you're calling IsEditableText() here makes me think that > this code isn't going to work when moving by word in non-editable text. > Did you notice a difference in behavior in NVDA or JAWS when moving by > word in static text? Honestly I didn't even realize they call > IAccessibleText for static text - do they? > > https://codereview.chromium.org/660633002/diff/60001/content/browser/accessib... > > content/browser/accessibility/browser_accessibility_win.cc:3303: > HandleSpecialTextOffset(text, &start_offset); > I think you should do this call before GetWordStartBoundary, otherwise > you wont't handle IA2_TEXT_OFFSET_LENGTH and IA2_TEXT_OFFSET_CARET > correctly. > > https://codereview.chromium.org/660633002/diff/60001/tools/accessibility/nvda... > > File tools/accessibility/nvda/winapi.py (right): > > https://codereview.chromium.org/660633002/diff/60001/tools/accessibility/nvda... > > tools/accessibility/nvda/winapi.py:17: _ENUM_WINDOWS_CALLBACK = > ctypes.WINFUNCTYPE(wintypes.BOOL, wintypes.HWND, wintypes.LPARAM) > nit: wrap to 80 chars > > https://codereview.chromium.org/660633002/diff/60001/tools/accessibility/nvda... > > tools/accessibility/nvda/winapi.py:130: return hWnds[0] if len(hWnds) > > 0 else None > If there's more than one, could you return the one that's active or > topmost? > > https://codereview.chromium.org/660633002/ To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
A few formatting nits. I'll take a final pass when the test passes. https://codereview.chromium.org/660633002/diff/80001/content/browser/accessib... File content/browser/accessibility/accessibility_win_browsertest.cc (right): https://codereview.chromium.org/660633002/diff/80001/content/browser/accessib... content/browser/accessibility/accessibility_win_browsertest.cc:53: IAccessible2** accessible2); nit: check indentation https://codereview.chromium.org/660633002/diff/80001/content/browser/accessib... content/browser/accessibility/accessibility_win_browsertest.cc:123: IAccessible* accessible, IAccessible2** accessible2) { I like naming output parameters with an "out_" prefix, so: IAccessible2** out_accessible2. https://codereview.chromium.org/660633002/diff/80001/content/browser/accessib... content/browser/accessibility/accessibility_win_browsertest.cc:428: child = obtained_children.begin(); I'd indent this 4 more spaces as a continuation of the previous piece of the for loop https://codereview.chromium.org/660633002/diff/80001/content/browser/accessib... content/browser/accessibility/accessibility_win_browsertest.cc:430: && child != obtained_children.end(); same here - indent 4 more. https://codereview.chromium.org/660633002/diff/80001/content/browser/accessib... content/browser/accessibility/accessibility_win_browsertest.cc:433: AccessibilityWinBrowserTest::GetAccessibleFromVariant(parent, It'd be more readable if you didn't need the AccessibilityWinBrowserTest:: - maybe the static helpers should just be top-level functions in an unnamed namespace at the top. I know you put them in the test class because they call ASSERT_EQ and stuff, but doesn't that defeat the purpose of that rule if you're calling them from a helper class like you are here? Personally I don't see any problem with putting an ASSERT or EXPECT anywhere in this file as long as you know it can only be called from a test. https://codereview.chromium.org/660633002/diff/80001/content/browser/accessib... content/browser/accessibility/accessibility_win_browsertest.cc:434: child->AsInput())); indent 4 more https://codereview.chromium.org/660633002/diff/80001/content/browser/accessib... content/browser/accessibility/accessibility_win_browsertest.cc:809: IN_PROC_BROWSER_TEST_F(AccessibilityWinBrowserTest, TestTextAtOffset) { Consider splitting this into several smaller tests - like maybe one each for character, word, sentence, etc. If someone breaks one test by accident and tries to debug it, it's nice if the test is short and sweet. https://codereview.chromium.org/660633002/diff/80001/content/browser/accessib... content/browser/accessibility/accessibility_win_browsertest.cc:939: // If the offset is at the punctuation, it should return Nit: no indentatino for this comment.
> https://codereview.chromium.org/660633002/diff/60001/content/browser/accessib... > > content/browser/accessibility/browser_accessibility.cc:378: > ui::AX_ATTR_WORD_STARTS); > Interesting, did WORD_ENDS end up being useless? Is the proper behavior > to always move to the beginning of each word no matter what direction > you're moving? > Only on Windows. This is how it should work according to the NVDA people and my own experience. > https://codereview.chromium.org/660633002/diff/60001/content/browser/accessib... > > File content/browser/accessibility/browser_accessibility_win.cc (right): > > https://codereview.chromium.org/660633002/diff/60001/content/browser/accessib... > > content/browser/accessibility/browser_accessibility_win.cc:3300: if > (ia2_boundary == IA2_TEXT_BOUNDARY_WORD && IsEditableText()) > The fact that you're calling IsEditableText() here makes me think that > this code isn't going to work when moving by word in non-editable text. > Did you notice a difference in behavior in NVDA or JAWS when moving by > word in static text? Honestly I didn't even realize they call > IAccessibleText for static text - do they? I think they call it for menu and toolbar items too per the original bug. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Updated by breaking up the large test into multiple ones. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
This is pretty close, please address the formatting issues and other nits below, and let's land the change! https://codereview.chromium.org/660633002/diff/100001/content/browser/accessi... File content/browser/accessibility/accessibility_win_browsertest.cc (right): https://codereview.chromium.org/660633002/diff/100001/content/browser/accessi... content/browser/accessibility/accessibility_win_browsertest.cc:8: #include "net/base/escape.h" nit: sort this https://codereview.chromium.org/660633002/diff/100001/content/browser/accessi... content/browser/accessibility/accessibility_win_browsertest.cc:106: AccessibilityTreeFormatter* atf = AccessibilityTreeFormatter::Create(web_contents); Use a scoped_ptr rather than manually deleting a pointer https://codereview.chromium.org/660633002/diff/100001/content/browser/accessi... content/browser/accessibility/accessibility_win_browsertest.cc:120: // Returns true if successful and false otherwise. nit: no longer returns a bool https://codereview.chromium.org/660633002/diff/100001/content/browser/accessi... File content/browser/accessibility/browser_accessibility.cc (right): https://codereview.chromium.org/660633002/diff/100001/content/browser/accessi... content/browser/accessibility/browser_accessibility.cc:343: int BrowserAccessibility::GetWordStartBoundary( A few more comments in this function please? https://codereview.chromium.org/660633002/diff/100001/content/browser/accessi... content/browser/accessibility/browser_accessibility.cc:357: const std::vector<int32>& lines = child->GetIntListAttribute( I think you can get rid of the line break search, right? https://codereview.chromium.org/660633002/diff/100001/content/browser/accessi... content/browser/accessibility/browser_accessibility.cc:396: int local_start = start - child_start; nit: indentation https://codereview.chromium.org/660633002/diff/100001/content/browser/accessi... File content/browser/accessibility/browser_accessibility_win.cc (right): https://codereview.chromium.org/660633002/diff/100001/content/browser/accessi... content/browser/accessibility/browser_accessibility_win.cc:3304: if (ia2_boundary == IA2_TEXT_BOUNDARY_WORD && IsEditableText()) Why IsEditableText()? Can't you implement this for static text too? https://codereview.chromium.org/660633002/diff/100001/tools/accessibility/nvd... File tools/accessibility/nvda/nvda_chrome_tests.py (right): https://codereview.chromium.org/660633002/diff/100001/tools/accessibility/nvd... tools/accessibility/nvda/nvda_chrome_tests.py:34: import winapi Please pull the changes to tools/accessibility/nvda into a separate patch.
Are you still working on this change?
This looks pretty close once you remove the last of the line break stuff for now https://codereview.chromium.org/660633002/diff/140001/content/browser/accessi... File content/browser/accessibility/browser_accessibility.cc (right): https://codereview.chromium.org/660633002/diff/140001/content/browser/accessi... content/browser/accessibility/browser_accessibility.cc:348: child->GetStringAttribute(ui::AX_ATTR_VALUE, &child_text); Minor nit: it's slightly better to write: const std::string& child_text = child->GetStringAttribute(ui::AX_ATTR_VALUE); because then it doesn't need to make a copy of the string. https://codereview.chromium.org/660633002/diff/140001/content/browser/accessi... content/browser/accessibility/browser_accessibility.cc:365: } else if (direction == ui::BACKWARDS_DIRECTION) { nit: handle the case where direction is not FORWARDS or BACKWARDS, maybe with NOTREACHED(). https://codereview.chromium.org/660633002/diff/140001/content/browser/accessi... File content/browser/accessibility/browser_accessibility_win.cc (right): https://codereview.chromium.org/660633002/diff/140001/content/browser/accessi... content/browser/accessibility/browser_accessibility_win.cc:933: //DCHECK_EQ(2U, product_components.size()); if these checks are failing, please file a bug and add a comment above this line of code with the bug number https://codereview.chromium.org/660633002/diff/140001/content/browser/accessi... File content/browser/accessibility/browser_accessibility_win_unittest.cc (right): https://codereview.chromium.org/660633002/diff/140001/content/browser/accessi... content/browser/accessibility/browser_accessibility_win_unittest.cc:377: ASSERT_EQ(0, start); Why does this return 0 for start and end? We asked for offset test1_len. https://codereview.chromium.org/660633002/diff/140001/ui/accessibility/ax_tex... File ui/accessibility/ax_text_utils.cc (right): https://codereview.chromium.org/660633002/diff/140001/ui/accessibility/ax_tex... ui/accessibility/ax_text_utils.cc:29: if (line_break >= start_offset) Maybe don't include this change in the changelist?
Keep trying to fix failing tests but comments addressed. https://codereview.chromium.org/660633002/diff/140001/content/browser/accessi... > File content/browser/accessibility/browser_accessibility.cc (right): > > https://codereview.chromium.org/660633002/diff/140001/content/browser/accessi... > > content/browser/accessibility/browser_accessibility.cc:348: > child->GetStringAttribute(ui::AX_ATTR_VALUE, &child_text); > Minor nit: it's slightly better to write: > > const std::string& child_text = > child->GetStringAttribute(ui::AX_ATTR_VALUE); because then it doesn't > need to make a copy of the string. > Done. > https://codereview.chromium.org/660633002/diff/140001/content/browser/accessi... > > content/browser/accessibility/browser_accessibility.cc:365: } else if > (direction == ui::BACKWARDS_DIRECTION) { > nit: handle the case where direction is not FORWARDS or BACKWARDS, maybe > with NOTREACHED(). > Done with NOTREACHED(); > https://codereview.chromium.org/660633002/diff/140001/content/browser/accessi... > > File content/browser/accessibility/browser_accessibility_win.cc (right): > > https://codereview.chromium.org/660633002/diff/140001/content/browser/accessi... > > content/browser/accessibility/browser_accessibility_win.cc:933: > //DCHECK_EQ(2U, product_components.size()); > if these checks are failing, please file a bug and add a comment above > this line of code with the bug number > They are only failing locally. I'll uncomment these lines out before checking in. I have it in mind to fix this. > https://codereview.chromium.org/660633002/diff/140001/content/browser/accessi... > > File content/browser/accessibility/browser_accessibility_win_unittest.cc > (right): > > https://codereview.chromium.org/660633002/diff/140001/content/browser/accessi... > > content/browser/accessibility/browser_accessibility_win_unittest.cc:377: > ASSERT_EQ(0, start); > Why does this return 0 for start and end? We asked for offset test1_len. > Because offset is zero-based and so text1_len is one past the last character. The spec says that only line boundary should return S_OK, all others should return S_FAULSE. It's not in the API definition. I found this in some other part of the spec. > https://codereview.chromium.org/660633002/diff/140001/ui/accessibility/ax_tex... > > File ui/accessibility/ax_text_utils.cc (right): > > https://codereview.chromium.org/660633002/diff/140001/ui/accessibility/ax_tex... > > ui/accessibility/ax_text_utils.cc:29: if (line_break >= start_offset) > Maybe don't include this change in the changelist? > Done. It's wrong anyway. To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
Should be fine with async issue fixed https://codereview.chromium.org/660633002/diff/180001/content/browser/accessi... File content/browser/accessibility/accessibility_win_browsertest.cc (right): https://codereview.chromium.org/660633002/diff/180001/content/browser/accessi... content/browser/accessibility/accessibility_win_browsertest.cc:109: AccessibilityTreeFormatter* atf = AccessibilityTreeFormatter::Create( Can you remove this logging when done? If you want to keep it, use a scoped_ptr and I'd use VLOG(1) rather than DLOG, that makes it possible for someone to take advantage of it in a release build. https://codereview.chromium.org/660633002/diff/180001/content/browser/accessi... content/browser/accessibility/accessibility_win_browsertest.cc:207: hr = (*textarea_text)->setCaretOffset(caret_offset); This is asynchronous https://codereview.chromium.org/660633002/diff/180001/content/browser/accessi... content/browser/accessibility/accessibility_win_browsertest.cc:1370: TestMultiLineTextAtOffsetWithBoundaryLine) { Are you going to delete this test?
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/660633002/300001
The CQ bit was unchecked by commit-bot@chromium.org
No LGTM from a valid reviewer yet. Only full committers are accepted. Even if an LGTM 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.
I think I fixed all the issues. Could you please LGTM? To unsubscribe from this group and stop receiving emails from it, send an email to chromium-reviews+unsubscribe@chromium.org.
lgtm https://codereview.chromium.org/660633002/diff/300001/content/browser/accessi... File content/browser/accessibility/accessibility_win_browsertest.cc (right): https://codereview.chromium.org/660633002/diff/300001/content/browser/accessi... content/browser/accessibility/accessibility_win_browsertest.cc:983: LONG caret_offset = 0; nit: indentation is wrong here, should be two spaces
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/660633002/300001
The CQ bit was checked by nektar@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from dmazzoni@chromium.org Link to the patchset: https://codereview.chromium.org/660633002/#ps320001 (title: "Disabled mltiline unit test.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/660633002/320001
Message was sent while issue was closed.
Committed patchset #17 (id:320001)
Message was sent while issue was closed.
Patchset 17 (id:??) landed as https://crrev.com/6baff46f520e31ff92669890207be5708064d16e Cr-Commit-Position: refs/heads/master@{#317996} |
