Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(220)

Issues Search
    My Issues | Starred     Open | Closed | All

Issue 8770021: Initial implementation of IAccessible2 scrollTo and setTextSelection and (Closed)

Created:
9 years ago by dmazzoni
Modified:
9 years ago
Reviewers:
David Tseng
CC:
chromium-reviews, ctguil+watch_chromium.org, yusukes+watch_chromium.org, hashimoto+watch_chromium.org, aboxhall+watch_chromium.org, yoshiki+watch_chromium.org, davidbarr+watch_chromium.org, jam, penghuang+watch_chromium.org, yuzo+watch_chromium.org, dpranke-watch+content_chromium.org, joi+watch-content_chromium.org, darin-cc_chromium.org, dmazzoni+watch_chromium.org, dtseng+watch_chromium.org, Paweł Hajdan Jr., James Su, zork+watch_chromium.org
Visibility:
Public.

Description

Initial implementation of IAccessible2 scrollTo and setTextSelection and related methods. Scrolling currently only handles the main frame, though it's designed to be extended to support scrolling any scrollable container shortly. Setting the text selection is only implemented for <input> elements, pending WebKit enhancements. BUG=104468, 104469 TEST=Adds new test for scrolling logic, manual testing with accProbe. Committed: http://src.chromium.org/viewvc/chrome?view=rev&revision=112880

Patch Set 1 #

Patch Set 2 : '' #

Total comments: 23

Patch Set 3 : '' #

Unified diffs Side-by-side diffs Delta from patch set Stats (+358 lines, -34 lines) Patch
M chrome/browser/accessibility/browser_accessibility_win_unittest.cc View 1 2 1 chunk +41 lines, -0 lines 0 comments Download
M content/browser/accessibility/browser_accessibility.h View 1 2 1 chunk +47 lines, -0 lines 0 comments Download
M content/browser/accessibility/browser_accessibility.cc View 1 2 1 chunk +86 lines, -0 lines 0 comments Download
M content/browser/accessibility/browser_accessibility_win.h View 1 2 3 chunks +27 lines, -33 lines 0 comments Download
M content/browser/accessibility/browser_accessibility_win.cc View 1 2 4 chunks +157 lines, -1 line 0 comments Download

Messages

Total messages: 6 (0 generated)
dmazzoni
David - please take a look at browser_accessibility.cc, browser_accessibility_win.cc, and the new unittest first. I'm ...
9 years ago (2011-12-01 22:31:26 UTC) #1
David Tseng
First pass. Looking at the unit tests next. http://codereview.chromium.org/8770021/diff/3005/content/browser/accessibility/browser_accessibility.cc File content/browser/accessibility/browser_accessibility.cc (right): http://codereview.chromium.org/8770021/diff/3005/content/browser/accessibility/browser_accessibility.cc#newcode184 content/browser/accessibility/browser_accessibility.cc:184: int ...
9 years ago (2011-12-02 00:02:17 UTC) #2
David Tseng
http://codereview.chromium.org/8770021/diff/3005/chrome/browser/accessibility/browser_accessibility_win_unittest.cc File chrome/browser/accessibility/browser_accessibility_win_unittest.cc (right): http://codereview.chromium.org/8770021/diff/3005/chrome/browser/accessibility/browser_accessibility_win_unittest.cc#newcode376 chrome/browser/accessibility/browser_accessibility_win_unittest.cc:376: EXPECT_EQ(125, SCROLL(125, 125, 175, 125, 175, 0, 100)); Just ...
9 years ago (2011-12-02 00:33:03 UTC) #3
David Tseng
http://codereview.chromium.org/8770021/diff/3005/content/browser/accessibility/browser_accessibility_win.cc File content/browser/accessibility/browser_accessibility_win.cc (right): http://codereview.chromium.org/8770021/diff/3005/content/browser/accessibility/browser_accessibility_win.cc#newcode697 content/browser/accessibility/browser_accessibility_win.cc:697: gfx::Rect view(-1000000, -1000000, 1000000, 1000000); #include <limits> ... std::numeric_limits<int>::max(); ...
9 years ago (2011-12-02 00:51:51 UTC) #4
dmazzoni
http://codereview.chromium.org/8770021/diff/3005/chrome/browser/accessibility/browser_accessibility_win_unittest.cc File chrome/browser/accessibility/browser_accessibility_win_unittest.cc (right): http://codereview.chromium.org/8770021/diff/3005/chrome/browser/accessibility/browser_accessibility_win_unittest.cc#newcode376 chrome/browser/accessibility/browser_accessibility_win_unittest.cc:376: EXPECT_EQ(125, SCROLL(125, 125, 175, 125, 175, 0, 100)); On ...
9 years ago (2011-12-03 00:37:10 UTC) #5
David Tseng
9 years ago (2011-12-03 00:39:43 UTC) #6 
lgtm

On 12/2/11, dmazzoni@chromium.org <dmazzoni@chromium.org> wrote:
>
>
http://codereview.chromium.org/8770021/diff/3005/chrome/browser/accessibility...
> File chrome/browser/accessibility/browser_accessibility_win_unittest.cc
> (right):
>
>
http://codereview.chromium.org/8770021/diff/3005/chrome/browser/accessibility...
> chrome/browser/accessibility/browser_accessibility_win_unittest.cc:376:
> EXPECT_EQ(125, SCROLL(125, 125, 175, 125, 175, 0, 100));
> On 2011/12/02 00:33:04, David Tseng wrote:
>> Just for kicks, throw in a subfocus not equal to focus?
>
> Done.
>
>
http://codereview.chromium.org/8770021/diff/3005/chrome/browser/accessibility...
> chrome/browser/accessibility/browser_accessibility_win_unittest.cc:377:
> EXPECT_EQ(75, SCROLL(75, 125, 175, 125, 175, 0, 100));
> On 2011/12/02 00:33:04, David Tseng wrote:
>> Curious, do we handle negative scrolling? (i.e. if the viewport
> min/max is 100,
>> 200, and the focus min/max is 0, 50.
>
> This function will return a negative value, sure. The calling function
> will clip it to the allowable values for the scroll offset. I added a
> comment.
>
>
http://codereview.chromium.org/8770021/diff/3005/chrome/browser/accessibility...
> chrome/browser/accessibility/browser_accessibility_win_unittest.cc:390:
> EXPECT_EQ(150, SCROLL(0, 170, 175, 125, 175, 0, 25));
> On 2011/12/02 00:33:04, David Tseng wrote:
>> How about adding a case for nonzero initial offset?
>
> Done.
>
>
http://codereview.chromium.org/8770021/diff/3005/content/browser/accessibilit...
> File content/browser/accessibility/browser_accessibility.cc (right):
>
>
http://codereview.chromium.org/8770021/diff/3005/content/browser/accessibilit...
> content/browser/accessibility/browser_accessibility.cc:184: int
> viewport_left = std::max(viewport.x(), 0);
> On 2011/12/02 00:02:17, David Tseng wrote:
>> Why isn't this std::max(viewport.x(), location_.x())?
>
> location_ is in global coordinates. We want things in local coordinates
> relative to this container view.
>
> However, you're right that this was inconsistent - where I had
> location_.right() and location_.bottom() below, those should have been
> width and height instead.
>
> I updated it to use Intersect instead, that's even more concise.
>
>
http://codereview.chromium.org/8770021/diff/3005/content/browser/accessibilit...
> content/browser/accessibility/browser_accessibility.cc:189: int
> new_scroll_x = ComputeBestScrollOffset(
> On 2011/12/02 00:02:17, David Tseng wrote:
>> Did you try using Rect's as primitives to perform offset calculations?
> Perhaps
>> that might make this easier (i.e. getting the union of the focused
> rect and the
>> viewport rect).
>
> Used Intersect, above. Good idea.
>
> I don't think there's a way to use it anymore and make it more concise.
>
>
http://codereview.chromium.org/8770021/diff/3005/content/browser/accessibilit...
> File content/browser/accessibility/browser_accessibility.h (right):
>
>
http://codereview.chromium.org/8770021/diff/3005/content/browser/accessibilit...
> content/browser/accessibility/browser_accessibility.h:117: void
> ScrollToMakeVisible(const gfx::Rect& subfocus,
> On 2011/12/02 00:02:17, David Tseng wrote:
>> Curious why this isn't taking a more general parameter of
> BrowserAccessibility
>> but special cased only for the focused element? It might be useful to
> check that
>> |this| is an ancestor of the non-visible element.
>
>> Do we need to ever scroll |this| to make visible non-focusable items?
>
> Yes - the item to be made visible isn't necessarily focused. Do you have
> an idea for a better name for the item rectangle and the sub item
> rectangle? I used focus and subfocus in a general sense, not meant to
> imply that the item must actually be focused.
>
> I added a comment to clarify, but if you think of another name that's
> also concise, we can switch.
>
>
http://codereview.chromium.org/8770021/diff/3005/content/browser/accessibilit...
> content/browser/accessibility/browser_accessibility.h:143: // When
> constraints cannot be fully satistied, the min
> On 2011/12/02 00:02:17, David Tseng wrote:
>> satistied -> satisfied
>
> Done.
>
>
http://codereview.chromium.org/8770021/diff/3005/content/browser/accessibilit...
> File content/browser/accessibility/browser_accessibility_win.cc (right):
>
>
http://codereview.chromium.org/8770021/diff/3005/content/browser/accessibilit...
> content/browser/accessibility/browser_accessibility_win.cc:697:
> gfx::Rect view(-1000000, -1000000, 1000000, 1000000);
> On 2011/12/02 00:51:51, David Tseng wrote:
>> #include <limits>
>> ...
>
>> std::numeric_limits<int>::max();
>
> Done.
>
>
http://codereview.chromium.org/8770021/diff/3005/content/browser/accessibilit...
> content/browser/accessibility/browser_accessibility_win.cc:699: case
> IA2_SCROLL_TYPE_TOP_LEFT:
> On 2011/12/02 00:51:51, David Tseng wrote:
>> indent
>
> Done.
>
>
http://codereview.chromium.org/8770021/diff/3005/content/browser/accessibilit...
> content/browser/accessibility/browser_accessibility_win.cc:741: if
> (coordinate_type == IA2_COORDTYPE_SCREEN_RELATIVE) {
> On 2011/12/02 00:51:51, David Tseng wrote:
>> switch? Are we not handling all the cord types?
>
> Actually there are only two.
>
>
http://codereview.chromium.org/8770021/diff/3005/content/browser/accessibilit...
> content/browser/accessibility/browser_accessibility_win.cc:1950:
> STDMETHODIMP BrowserAccessibilityWin::addSelection(
> On 2011/12/02 00:51:51, David Tseng wrote:
>> Part of another patch?
>
> Yep, sorry. Gone now.
>
> http://codereview.chromium.org/8770021/
>

Powered by Google App Engine
This is Rietveld 408576698