Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
25987002:
Accessible boundsForRange common impl (Closed)
|
Created:
7 years, 2 months ago by dmazzoni Modified:
7 years, 1 month ago CC:
chromium-reviews, hashimoto+watch_chromium.org, aboxhall+watch_chromium.org, jam, yoshiki+watch_chromium.org, yuzo+watch_chromium.org, davidbarr+watch_chromium.org, joi+watch-content_chromium.org, darin-cc_chromium.org, dmazzoni+watch_chromium.org, dtseng+watch_chromium.org, ctguil+watch_chromium.org Base URL:
svn://svn.chromium.org/chrome/trunk/src Visibility:
Public. |
DescriptionThis adds an implementation of boundsForRange to BrowserAccessibility,
based on the new InlineTextBox role. This can be leveraged to implement
various native APIs on each platform.
Depends on:
1. https://codereview.chromium.org/23983002/
2. https://codereview.chromium.org/25943003/
BUG=98977
Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=232806
Patch Set 1 #
Total comments: 27
Patch Set 2 : Added bidi test #Patch Set 3 : Address feedback, rebase #Patch Set 4 : Fix compile error #Patch Set 5 : Rebase #Patch Set 6 : Rebase #
Messages
Total messages: 12 (0 generated)
https://codereview.chromium.org/25987002/diff/1/content/browser/accessibility... File content/browser/accessibility/browser_accessibility.cc (right): https://codereview.chromium.org/25987002/diff/1/content/browser/accessibility... content/browser/accessibility/browser_accessibility.cc:209: int overlap_start = std::max(start, child_start); Could we just do if (start > child_end || end < child_start) continue; before calculating the overlap? Having something called 'overlap' which may potentially not be an overlap is breaking my brain a bit :) https://codereview.chromium.org/25987002/diff/1/content/browser/accessibility... content/browser/accessibility/browser_accessibility.cc:261: if (bounds.width() == 0 && bounds.height() == 0) Would IsEmpty() do here? Or is it conceivable that we could have a bounding box with zero width or height but not both? https://codereview.chromium.org/25987002/diff/1/content/browser/accessibility... File content/browser/accessibility/browser_accessibility.h (right): https://codereview.chromium.org/25987002/diff/1/content/browser/accessibility... content/browser/accessibility/browser_accessibility.h:117: // role is WebAXRoleStaticText. There's no chance this could be called on an inline text box? https://codereview.chromium.org/25987002/diff/1/content/browser/accessibility... File content/browser/accessibility/browser_accessibility_manager_unittest.cc (right): https://codereview.chromium.org/25987002/diff/1/content/browser/accessibility... content/browser/accessibility/browser_accessibility_manager_unittest.cc:607: root.child_ids.push_back(2); I know this will work fine, but could we do it after static_text is created anyway, just to avoid time travel? (Similarly with the child_ids of static_text.) https://codereview.chromium.org/25987002/diff/1/content/browser/accessibility... content/browser/accessibility/browser_accessibility_manager_unittest.cc:625: character_offsets1.push_back(6); It might be helpful to comment each of these with the character offset, e.g. character_offsets1.push_back(6); // 0 character_offsets1.push_back(11); // 1 Also, reading this makes me feel that the name "character_offsets" is a little misleading; that makes me expect that each value will be the offset of the start of the character from the start of the line, rather than the end of the character. I can't think of a better name, though... character_end_offsets? https://codereview.chromium.org/25987002/diff/1/content/browser/accessibility... content/browser/accessibility/browser_accessibility_manager_unittest.cc:631: character_offsets1.push_back(29); Is this meant to be the same as the previous one?
https://codereview.chromium.org/25987002/diff/1/content/browser/accessibility... File content/browser/accessibility/browser_accessibility.cc (right): https://codereview.chromium.org/25987002/diff/1/content/browser/accessibility... content/browser/accessibility/browser_accessibility.cc:200: for (size_t i = 0; i < children_.size(); i++) { You only need to iterate up to text length "end", right? https://codereview.chromium.org/25987002/diff/1/content/browser/accessibility... File content/browser/accessibility/browser_accessibility.h (right): https://codereview.chromium.org/25987002/diff/1/content/browser/accessibility... content/browser/accessibility/browser_accessibility.h:116: // top-left corner of the overall web area. Only valid when the Are you thinking about supporting this for non-static (AXStaticText) nodes (as we discussed, indexing could follow the same model that Selection/Range does today in the DOM). https://codereview.chromium.org/25987002/diff/1/content/browser/accessibility... content/browser/accessibility/browser_accessibility.h:118: gfx::Rect GetLocalBoundsForRange(int start, int len) const; If the above were true, then perhaps this should be startIndex/endIndex (to generalize indexing into children and text).. https://codereview.chromium.org/25987002/diff/1/content/browser/accessibility... content/browser/accessibility/browser_accessibility.h:121: gfx::Rect GetGlobalBoundsForRange(int start, int len) const; Is this also only available for static text? https://codereview.chromium.org/25987002/diff/1/content/browser/accessibility... File content/browser/accessibility/browser_accessibility_manager_unittest.cc (right): https://codereview.chromium.org/25987002/diff/1/content/browser/accessibility... content/browser/accessibility/browser_accessibility_manager_unittest.cc:675: static_text_accessible->GetLocalBoundsForRange(5, 3).ToString()); How about passing in lengths beyond the actual text length? Also, should we ensure nonnegative values are passed? Did you want to also test the other possible text directions? (i.e. rtl, mixed directions under a single static text).
https://codereview.chromium.org/25987002/diff/1/content/browser/accessibility... File content/browser/accessibility/browser_accessibility.cc (right): https://codereview.chromium.org/25987002/diff/1/content/browser/accessibility... content/browser/accessibility/browser_accessibility.cc:200: for (size_t i = 0; i < children_.size(); i++) { On 2013/10/17 21:32:02, David Tseng wrote: > You only need to iterate up to text length "end", right? Good point. I added a condition to the for loop so that it only continues as long as child_end < start + len. https://codereview.chromium.org/25987002/diff/1/content/browser/accessibility... content/browser/accessibility/browser_accessibility.cc:209: int overlap_start = std::max(start, child_start); On 2013/10/04 18:53:07, aboxhall wrote: > Could we just do > if (start > child_end || end < child_start) > continue; > before calculating the overlap? > > Having something called 'overlap' which may potentially not be an overlap is > breaking my brain a bit :) Done. https://codereview.chromium.org/25987002/diff/1/content/browser/accessibility... content/browser/accessibility/browser_accessibility.cc:261: if (bounds.width() == 0 && bounds.height() == 0) On 2013/10/04 18:53:07, aboxhall wrote: > Would IsEmpty() do here? Or is it conceivable that we could have a bounding box > with zero width or height but not both? It will happen - a common example is a space character that's not actually printed, like the space character right at the point where a line is wrapped. The space doesn't actually have any width. https://codereview.chromium.org/25987002/diff/1/content/browser/accessibility... File content/browser/accessibility/browser_accessibility.h (right): https://codereview.chromium.org/25987002/diff/1/content/browser/accessibility... content/browser/accessibility/browser_accessibility.h:116: // top-left corner of the overall web area. Only valid when the On 2013/10/17 21:32:02, David Tseng wrote: > Are you thinking about supporting this for non-static (AXStaticText) nodes (as > we discussed, indexing could follow the same model that Selection/Range does > today in the DOM). Sounds good - but for now, this is strictly only supposed to work for static text. I'm pretty sure that's sufficient for it to support the native boundsForRange apis on both Mac and Windows. https://codereview.chromium.org/25987002/diff/1/content/browser/accessibility... content/browser/accessibility/browser_accessibility.h:117: // role is WebAXRoleStaticText. On 2013/10/04 18:53:07, aboxhall wrote: > There's no chance this could be called on an inline text box? No. The inline text boxes are not exposed to the platform at all, so there's no way that call could hever happen. There's a DCHECK at the top also. https://codereview.chromium.org/25987002/diff/1/content/browser/accessibility... content/browser/accessibility/browser_accessibility.h:118: gfx::Rect GetLocalBoundsForRange(int start, int len) const; On 2013/10/17 21:32:02, David Tseng wrote: > If the above were true, then perhaps this should be startIndex/endIndex (to > generalize indexing into children and text).. I think we'd want a higher-level function that takes a start node and offset, and an end node and offset, and computes the bounds from that. A single start and end within the same node like this wouldn't really handle that case anyway. Let's think of this as a special case (that's useful now); later when we add the general case, this one just becomes a private helper function. https://codereview.chromium.org/25987002/diff/1/content/browser/accessibility... content/browser/accessibility/browser_accessibility.h:121: gfx::Rect GetGlobalBoundsForRange(int start, int len) const; On 2013/10/17 21:32:02, David Tseng wrote: > Is this also only available for static text? Yes. Updated the comment. https://codereview.chromium.org/25987002/diff/1/content/browser/accessibility... File content/browser/accessibility/browser_accessibility_manager_unittest.cc (right): https://codereview.chromium.org/25987002/diff/1/content/browser/accessibility... content/browser/accessibility/browser_accessibility_manager_unittest.cc:607: root.child_ids.push_back(2); On 2013/10/04 18:53:07, aboxhall wrote: > I know this will work fine, but could we do it after static_text is created > anyway, just to avoid time travel? (Similarly with the child_ids of > static_text.) Done. https://codereview.chromium.org/25987002/diff/1/content/browser/accessibility... content/browser/accessibility/browser_accessibility_manager_unittest.cc:625: character_offsets1.push_back(6); On 2013/10/04 18:53:07, aboxhall wrote: > It might be helpful to comment each of these with the character offset, e.g. > character_offsets1.push_back(6); // 0 > character_offsets1.push_back(11); // 1 Done. > Also, reading this makes me feel that the name "character_offsets" is a little > misleading; that makes me expect that each value will be the offset of the start > of the character from the start of the line, rather than the end of the > character. I can't think of a better name, though... character_end_offsets? I can't think of anything that's abundantly clear. This is one of those low-level things that you have to dig into pretty deeply to understand, it's not going to be obvious at a glance no matter what we name it. I added some more comments to the attribute definition in the other change (https://codereview.chromium.org/25943003/). https://codereview.chromium.org/25987002/diff/1/content/browser/accessibility... content/browser/accessibility/browser_accessibility_manager_unittest.cc:631: character_offsets1.push_back(29); On 2013/10/04 18:53:07, aboxhall wrote: > Is this meant to be the same as the previous one? Yes. This is an example of a space character having no width. https://codereview.chromium.org/25987002/diff/1/content/browser/accessibility... content/browser/accessibility/browser_accessibility_manager_unittest.cc:675: static_text_accessible->GetLocalBoundsForRange(5, 3).ToString()); On 2013/10/17 21:32:02, David Tseng wrote: > How about passing in lengths beyond the actual text length? Also, should we > ensure nonnegative values are passed? Sure, I'll add a test for an extreme range. > Did you want to also test the other possible text directions? (i.e. rtl, mixed > directions under a single static text). Good idea. I added a bidirectional test. Note that the Blink part of the code has more tests that cover different directions and bidi from the original html. This test is just meant to make sure that GetLocalBoundsFromRange works correctly assuming the underlying data is correct.
PTAL?
lgtm https://codereview.chromium.org/25987002/diff/1/content/browser/accessibility... File content/browser/accessibility/browser_accessibility_manager_unittest.cc (right): https://codereview.chromium.org/25987002/diff/1/content/browser/accessibility... content/browser/accessibility/browser_accessibility_manager_unittest.cc:625: character_offsets1.push_back(6); On 2013/10/21 17:25:38, Dominic Mazzoni wrote: > On 2013/10/04 18:53:07, aboxhall wrote: > Also, reading this makes me feel that the name "character_offsets" is a little > > misleading; that makes me expect that each value will be the offset of the > start > > of the character from the start of the line, rather than the end of the > > character. I can't think of a better name, though... character_end_offsets? > > I can't think of anything that's abundantly clear. This is one of those > low-level things that you have to dig into pretty deeply to understand, it's not > going to be obvious at a glance no matter what we name it. > > I added some more comments to the attribute definition in the other change > (https://codereview.chromium.org/25943003/). Looks good, thanks.
LGTM https://codereview.chromium.org/25987002/diff/1/content/browser/accessibility... File content/browser/accessibility/browser_accessibility.cc (right): https://codereview.chromium.org/25987002/diff/1/content/browser/accessibility... content/browser/accessibility/browser_accessibility.cc:200: for (size_t i = 0; i < children_.size(); i++) { nit: ++i https://codereview.chromium.org/25987002/diff/1/content/browser/accessibility... content/browser/accessibility/browser_accessibility.cc:230: default: Do you expect to get anything other than the four values below?
https://codereview.chromium.org/25987002/diff/1/content/browser/accessibility... File content/browser/accessibility/browser_accessibility.cc (right): https://codereview.chromium.org/25987002/diff/1/content/browser/accessibility... content/browser/accessibility/browser_accessibility.cc:200: for (size_t i = 0; i < children_.size(); i++) { On 2013/10/23 17:00:50, David Tseng wrote: > nit: ++i Done. https://codereview.chromium.org/25987002/diff/1/content/browser/accessibility... content/browser/accessibility/browser_accessibility.cc:230: default: On 2013/10/23 17:00:50, David Tseng wrote: > Do you expect to get anything other than the four values below? No. Changed to NOTREACHED.
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/dmazzoni@chromium.org/25987002/189001
Step "update" is always a major failure. Look at the try server FAQ for more details. http://build.chromium.org/p/tryserver.chromium/buildstatus?builder=win_rel&nu...
CQ is trying da patch. Follow status at https://chromium-status.appspot.com/cq/dmazzoni@chromium.org/25987002/469001
Message was sent while issue was closed.
Change committed as 232806 |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
