Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2781613003:
Added a class acting as a fake caret for accessibility. (Closed)
|
Created:
3 years, 9 months ago by nektarios Modified:
3 years, 6 months ago CC:
aboxhall+watch_chromium.org, chromium-reviews, danakj+watch_chromium.org, darin-cc_chromium.org, dmazzoni+watch_chromium.org, dougt+watch_chromium.org, dtseng+watch_chromium.org, jam, jbauman+watch_chromium.org, je_julie, kalyank, nektar+watch_chromium.org, nona+watch_chromium.org, piman+watch_chromium.org, shuchen+watch_chromium.org, James Su, tfarina, yusukes+watch_chromium.org, yuzo+watch_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdded a class acting as a fake caret for accessibility.
Required by some screen magnifiers and Windows built-in Magnifier.
R=aleventhal@chromium.org, dmazzoni@chromium.org, ellyjones@chromium.org, ananta@chromium.org, sadrul@chromium.org
TESTED=Manually
Review-Url: https://codereview.chromium.org/2781613003
Cr-Commit-Position: refs/heads/master@{#471624}
Committed: https://chromium.googlesource.com/chromium/src/+/3eda6b165d26b7cc5e4549575780ddfe10502d62
Patch Set 1 #
Total comments: 7
Patch Set 2 : Fixed formatting. #Patch Set 3 : Fixed some bugs. #Patch Set 4 : Activate the fake caret only when Windows is focused. #
Total comments: 6
Patch Set 5 : Addressed comments. #Patch Set 6 : y #Patch Set 7 : Switched to Leaky lazy instance. #Patch Set 8 : Switched to Leaky lazy instance. #Patch Set 9 : Added lib to build file. #
Total comments: 3
Patch Set 10 : Changed ownership of AXFakeCaretWin from a lazy instance to one owned by HWND. #Patch Set 11 : Added code that handles views. #
Total comments: 3
Patch Set 12 : Addressed comments on Views implementation. #Patch Set 13 : Removed Views implementation. #Patch Set 14 : Removed Views implementation. #
Total comments: 1
Patch Set 15 : Addressed final comment. #Patch Set 16 : Addressed final comment. #Patch Set 17 : Fixed rebase conflict. #
Total comments: 2
Patch Set 18 : Removed copyright symbol. #Patch Set 19 : Fixed formatting. #Patch Set 20 : Added a class acting as a fake caret for accessibility. #Patch Set 21 : Fixed compilation error due to rebase. #Messages
Total messages: 96 (57 generated)
Description was changed from ========== Added a class acting as a fake caret for accessibility. I don't like the way I did lifetime management. I will be turning AXFakeCaretWin into a singleton. Otherwise, you may have a first look. ========== to ========== Added a class acting as a fake caret for accessibility. I don't like the way I did lifetime management. I will be turning AXFakeCaretWin into a singleton. Otherwise, you may have a first look. R=dmazzoni@chromium.org, aleventhal@chromium.org TESTED=not quite ready yet ==========
nektar@chromium.org changed reviewers: + aleventhal@chromium.org, dmazzoni@chromium.org
drive by review. https://codereview.chromium.org/2781613003/diff/1/content/browser/accessibili... File content/browser/accessibility/browser_accessibility_manager_win.cc (right): https://codereview.chromium.org/2781613003/diff/1/content/browser/accessibili... content/browser/accessibility/browser_accessibility_manager_win.cc:208: // We also need to fire an older MSAA event used by some assistive software. This change seems unrelated to the fake caret. Nit: The comment here says "also", but it happens before the event EVENT_OBJECT_LOCATIONCHANGE. https://codereview.chromium.org/2781613003/diff/1/content/browser/renderer_ho... File content/browser/renderer_host/legacy_render_widget_host_win.cc (right): https://codereview.chromium.org/2781613003/diff/1/content/browser/renderer_ho... content/browser/renderer_host/legacy_render_widget_host_win.cc:192: } else if (static_cast<DWORD>(OBJID_CARET) == obj_id) { nit: the above if-stmt returns making the following else stmt a non-sequitur. It might be clearer to not use the 'else'. https://codereview.chromium.org/2781613003/diff/1/content/browser/renderer_ho... content/browser/renderer_host/legacy_render_widget_host_win.cc:195: DCHECK_EQ(fake_caret.m_hResFinalConstruct, S_OK); I am confused by this DCHECK. Are you concerned with the result of the construction of the stack of AXFakeCaretWin's? Or are you just not sure if you should check it or not at this point? https://codereview.chromium.org/2781613003/diff/1/ui/accessibility/platform/a... File ui/accessibility/platform/ax_platform_node_win.cc (right): https://codereview.chromium.org/2781613003/diff/1/ui/accessibility/platform/a... ui/accessibility/platform/ax_platform_node_win.cc:185: // We also need to fire an older MSAA event used by some assistive software. Nit: comment is odd here since we're not dispatching any events after this
We may need to not return it when the LegacyWin doesn't have focus, not sure. https://codereview.chromium.org/2781613003/diff/1/content/browser/accessibili... File content/browser/accessibility/browser_accessibility_manager_win.cc (right): https://codereview.chromium.org/2781613003/diff/1/content/browser/accessibili... content/browser/accessibility/browser_accessibility_manager_win.cc:208: // We also need to fire an older MSAA event used by some assistive software. On 2017/03/28 15:23:44, dougt wrote: > This change seems unrelated to the fake caret. > > > Nit: The comment here says "also", but it happens before the event > EVENT_OBJECT_LOCATIONCHANGE. This is definitely related to the fake caret, since the fake caret moves anytime we get a selection changed event https://codereview.chromium.org/2781613003/diff/1/content/browser/renderer_ho... File content/browser/renderer_host/render_widget_host_view_aura.cc (right): https://codereview.chromium.org/2781613003/diff/1/content/browser/renderer_ho... content/browser/renderer_host/render_widget_host_view_aura.cc:66: #include "ui/accessibility/platform/ax_fake_caret_win.h" I think this needs to go in the #if defined(OS_WIN) block below https://codereview.chromium.org/2781613003/diff/1/content/browser/renderer_ho... content/browser/renderer_host/render_widget_host_view_aura.cc:2371: !GetRenderWidgetHost()->GetRootBrowserAccessibilityManager()) Would it be possible to make this code work without a BrowserAccessibilityManager? The Windows Magnifier can follow the caret without accessibility being enabled otherwise, for example.
Another thought: rather than another class that inherits from IAccessible, would it be possible to just use an AXPlatformNodeWin directly? Let the fake caret class inherit from AXPlatformNodeDelegate and own an AXPlatformNodeWin. You may need to add one more delegate method if the existing ones aren't sufficient, but that seems more robust to me.
On 3/28/2017 11:23 AM, dougt@chromium.org wrote: > drive by review. > > Thanks for the review. > https://codereview.chromium.org/2781613003/diff/1/content/browser/accessibili... > File content/browser/accessibility/browser_accessibility_manager_win.cc > (right): > > https://codereview.chromium.org/2781613003/diff/1/content/browser/accessibili... > content/browser/accessibility/browser_accessibility_manager_win.cc:208: > // We also need to fire an older MSAA event used by some assistive > software. > This change seems unrelated to the fake caret. > > It appears unrelated but the screen magnifier vendor wanted me to add > this event together with the fake caret. That's their trigger that > tells them that they should retrieve the fake caret. > Nit: The comment here says "also", but it happens before the event > EVENT_OBJECT_LOCATIONCHANGE. > I take it to mean, we also need to fire another event before the one we wanted to fire. > https://codereview.chromium.org/2781613003/diff/1/content/browser/renderer_ho... > File content/browser/renderer_host/legacy_render_widget_host_win.cc > (right): > > https://codereview.chromium.org/2781613003/diff/1/content/browser/renderer_ho... > content/browser/renderer_host/legacy_render_widget_host_win.cc:192: } > else if (static_cast<DWORD>(OBJID_CARET) == obj_id) { > nit: > > the above if-stmt returns making the following else stmt a non-sequitur. > It might be clearer to not use the 'else'. > Done. > https://codereview.chromium.org/2781613003/diff/1/content/browser/renderer_ho... > content/browser/renderer_host/legacy_render_widget_host_win.cc:195: > DCHECK_EQ(fake_caret.m_hResFinalConstruct, S_OK); > I am confused by this DCHECK. Are you concerned with the result of the > construction of the stack of AXFakeCaretWin's? Or are you just not sure > if you should check it or not at this point? > Removed. > https://codereview.chromium.org/2781613003/diff/1/ui/accessibility/platform/a... > File ui/accessibility/platform/ax_platform_node_win.cc (right): > > https://codereview.chromium.org/2781613003/diff/1/ui/accessibility/platform/a... > ui/accessibility/platform/ax_platform_node_win.cc:185: // We also need > to fire an older MSAA event used by some assistive software. > Nit: comment is odd here since we're not dispatching any events after > this Doesn't the next line do that? -- 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.
Converted to using AXPlatformNodeDelegate. Still looking into how to ensure that this works only when the LegacyWin has focus and to also work in Views. > https://codereview.chromium.org/2781613003/diff/1/content/browser/renderer_ho... > File content/browser/renderer_host/render_widget_host_view_aura.cc > (right): > > https://codereview.chromium.org/2781613003/diff/1/content/browser/renderer_ho... > content/browser/renderer_host/render_widget_host_view_aura.cc:66: > #include "ui/accessibility/platform/ax_fake_caret_win.h" > I think this needs to go in the #if defined(OS_WIN) block below > Done. > https://codereview.chromium.org/2781613003/diff/1/content/browser/renderer_ho... > content/browser/renderer_host/render_widget_host_view_aura.cc:2371: > !GetRenderWidgetHost()->GetRootBrowserAccessibilityManager()) > Would it be possible to make this code work without a > BrowserAccessibilityManager? > > The Windows Magnifier can follow the caret without accessibility > being enabled otherwise, for example. Done. -- 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.
nektar@chromium.org changed reviewers: + ellyjones@chromium.org
https://codereview.chromium.org/2781613003/diff/60001/content/browser/accessi... File content/browser/accessibility/browser_accessibility_manager_win.cc (right): https://codereview.chromium.org/2781613003/diff/60001/content/browser/accessi... content/browser/accessibility/browser_accessibility_manager_win.cc:209: ::NotifyWinEvent(EVENT_OBJECT_LOCATIONCHANGE, hwnd, OBJID_CARET, child_id); I don't think we want this here, because this means Windows Magnifier won't work because it doesn't enable accessibility. I suggest calling this in RenderWidgetHostViewBase::OnSelectionBoundsChanged instead. https://codereview.chromium.org/2781613003/diff/60001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_view_aura.cc (right): https://codereview.chromium.org/2781613003/diff/60001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_aura.cc:2377: ui::AXFakeCaretWin::g_fake_caret->SetBounds(caret_rect); I'd call something like ui::AXFakeCaretWin::Get() here. I'd also fire the notification here, or else fire it when calling SetBounds() - and if so I'd rename it something like MoveCaretTo(). https://codereview.chromium.org/2781613003/diff/60001/ui/accessibility/platfo... File ui/accessibility/platform/ax_fake_caret_win.cc (right): https://codereview.chromium.org/2781613003/diff/60001/ui/accessibility/platfo... ui/accessibility/platform/ax_fake_caret_win.cc:17: data_.role = AX_ROLE_DIV; We should create AX_ROLE_CARET and map it to ROLE_SYSTEM_CARET (yes, that is an MSAA role) https://codereview.chromium.org/2781613003/diff/60001/ui/accessibility/platfo... ui/accessibility/platform/ax_fake_caret_win.cc:29: base::win::ScopedComPtr<IAccessible> caret_accessible; I don't think you need this. Just call caret_->GetNativeViewAccessible() https://codereview.chromium.org/2781613003/diff/60001/ui/accessibility/platfo... ui/accessibility/platform/ax_fake_caret_win.cc:37: caret_->Lock(); Why are you locking / unlocking? It's not clear where this is every used anywhere other than the UI thread. If it is, the right solution is to post a callback on the UI thread, not to lock (which may block and cause jank). https://codereview.chromium.org/2781613003/diff/60001/ui/accessibility/platfo... File ui/accessibility/platform/ax_fake_caret_win.h (right): https://codereview.chromium.org/2781613003/diff/60001/ui/accessibility/platfo... ui/accessibility/platform/ax_fake_caret_win.h:54: g_ax_fake_caret_win = LAZY_INSTANCE_INITIALIZER; Put this in the C++ file, and provide a method to access it from the header file, like AXFakeCaretWin::Get()
On 4/17/2017 1:07 AM, dmazzoni@chromium.org wrote: > > https://codereview.chromium.org/2781613003/diff/60001/content/browser/accessi... > File content/browser/accessibility/browser_accessibility_manager_win.cc > (right): > > https://codereview.chromium.org/2781613003/diff/60001/content/browser/accessi... > content/browser/accessibility/browser_accessibility_manager_win.cc:209: > ::NotifyWinEvent(EVENT_OBJECT_LOCATIONCHANGE, hwnd, OBJID_CARET, > child_id); > I don't think we want this here, because this means Windows Magnifier > won't work because it doesn't enable accessibility. I suggest calling > this in > RenderWidgetHostViewBase::OnSelectionBoundsChanged instead. > Moved to AXFakeCaret itself. > https://codereview.chromium.org/2781613003/diff/60001/content/browser/rendere... > File content/browser/renderer_host/render_widget_host_view_aura.cc > (right): > > https://codereview.chromium.org/2781613003/diff/60001/content/browser/rendere... > content/browser/renderer_host/render_widget_host_view_aura.cc:2377: > ui::AXFakeCaretWin::g_fake_caret->SetBounds(caret_rect); > I'd call something like ui::AXFakeCaretWin::Get() here. > Done. > I'd also fire the notification here, or else fire it when calling > SetBounds() - and if so I'd rename it something like MoveCaretTo(). > Done. > https://codereview.chromium.org/2781613003/diff/60001/ui/accessibility/platfo... > File ui/accessibility/platform/ax_fake_caret_win.cc (right): > > https://codereview.chromium.org/2781613003/diff/60001/ui/accessibility/platfo... > ui/accessibility/platform/ax_fake_caret_win.cc:17: data_.role = > AX_ROLE_DIV; > We should create AX_ROLE_CARET and map it to ROLE_SYSTEM_CARET (yes, > that is an MSAA role) > Done. > https://codereview.chromium.org/2781613003/diff/60001/ui/accessibility/platfo... > ui/accessibility/platform/ax_fake_caret_win.cc:29: > base::win::ScopedComPtr<IAccessible> caret_accessible; > I don't think you need this. Just call caret_->GetNativeViewAccessible() > I wanted to show a ownership more clearly. If I return a raw pointer, who owns the caret? This is because we need to do AddRef before returning an IAccessible. QueryInterface does it for us. > https://codereview.chromium.org/2781613003/diff/60001/ui/accessibility/platfo... > ui/accessibility/platform/ax_fake_caret_win.cc:37: caret_->Lock(); > Why are you locking / unlocking? It's not clear where this is every used > anywhere other than the UI thread. If it is, the right solution is to > post a > callback on the UI thread, not to lock (which may block and cause jank). > Because the COM apartment model is multi threaded. Anyway, I don't think we honor the multithreaded apartment so removed. > https://codereview.chromium.org/2781613003/diff/60001/ui/accessibility/platfo... > File ui/accessibility/platform/ax_fake_caret_win.h (right): > > https://codereview.chromium.org/2781613003/diff/60001/ui/accessibility/platfo... > ui/accessibility/platform/ax_fake_caret_win.h:54: g_ax_fake_caret_win = > LAZY_INSTANCE_INITIALIZER; > Put this in the C++ file, and provide a method to access it from the > header file, > like AXFakeCaretWin::Get() > Done. -- 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 to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...) linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by nektar@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by nektar@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by nektar@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_compile_dbg_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_comp...)
The CQ bit was checked by nektar@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
I just patched this in and confirmed that it works correctly with Windows Magnifier! Nice job! Note that we also need to support this for Views, but let's do that in a separate patch. I'll take a final review pass now.
Getting really close, and I don't want to hold this up a lot, but a couple more concerns about the code. Really excellent work on this, though. https://codereview.chromium.org/2781613003/diff/160001/content/browser/access... File content/browser/accessibility/browser_accessibility_manager_win.cc (right): https://codereview.chromium.org/2781613003/diff/160001/content/browser/access... content/browser/accessibility/browser_accessibility_manager_win.cc:209: ::NotifyWinEvent(EVENT_OBJECT_LOCATIONCHANGE, hwnd, OBJID_CARET, child_id); I think I commented before, I think we should delete this, it doesn't belong in browser_accessibility_win.cc because it ought to work even when accessibility is off https://codereview.chromium.org/2781613003/diff/160001/content/browser/render... File content/browser/renderer_host/render_widget_host_view_aura.cc (right): https://codereview.chromium.org/2781613003/diff/160001/content/browser/render... content/browser/renderer_host/render_widget_host_view_aura.cc:889: ui::AXFakeCaretWin::Get()->SetTargetForNativeAccessibilityEvent( Hmmm, I wonder if it'd make more sense for each HWND to just own its own instance of AXFakeCaretWin, then you wouldn't need to do any of this. What do you think? https://codereview.chromium.org/2781613003/diff/160001/ui/accessibility/platf... File ui/accessibility/platform/ax_fake_caret_win.cc (right): https://codereview.chromium.org/2781613003/diff/160001/ui/accessibility/platf... ui/accessibility/platform/ax_fake_caret_win.cc:62: event_target_ = event_target; So yeah, my suggestion would be that rather than making it a singleton, each HWND would own one and it'd pass the gfx::AcceleratedWidget in the constructor. I think that would make lifetime management easier. I think that would also make the Views implementation easy, I think the caret could be owned by native_widget_win.
https://codereview.chromium.org/2781613003/diff/160001/content/browser/access... > File content/browser/accessibility/browser_accessibility_manager_win.cc > (right): > > https://codereview.chromium.org/2781613003/diff/160001/content/browser/access... > content/browser/accessibility/browser_accessibility_manager_win.cc:209: > ::NotifyWinEvent(EVENT_OBJECT_LOCATIONCHANGE, hwnd, OBJID_CARET, > child_id); > I think I commented before, I think we should delete this, > it doesn't belong in browser_accessibility_win.cc because > it ought to work even when accessibility is off > I apologize, I read your comment but forgot to remove this. Done. > https://codereview.chromium.org/2781613003/diff/160001/content/browser/render... > File content/browser/renderer_host/render_widget_host_view_aura.cc > (right): > > https://codereview.chromium.org/2781613003/diff/160001/content/browser/render... > content/browser/renderer_host/render_widget_host_view_aura.cc:889: > ui::AXFakeCaretWin::Get()->SetTargetForNativeAccessibilityEvent( > Hmmm, I wonder if it'd make more sense for each > HWND to just own its own instance of AXFakeCaretWin, > then you wouldn't need to do any of this. What do > you think? > Done. > https://codereview.chromium.org/2781613003/diff/160001/ui/accessibility/platf... > File ui/accessibility/platform/ax_fake_caret_win.cc (right): > > https://codereview.chromium.org/2781613003/diff/160001/ui/accessibility/platf... > ui/accessibility/platform/ax_fake_caret_win.cc:62: event_target_ = > event_target; > So yeah, my suggestion would be that rather than making it a > singleton, each HWND would own one and it'd pass the > gfx::AcceleratedWidget in the constructor. I think that > would make lifetime management easier. > Done. > I think that would also make the Views implementation easy, > I think the caret could be owned by native_widget_win. > I believe HWNDMessageHandler because it inherits from WindowImpl and because it handles all Win specific logic and owns the HWND. I am still looking into how to wire caret movement events from views TextField to HWNDMessageHandler using the right abstractions. -- 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.
Tracking the caret in views looks on the right track, but how about just landing this change first, without that last patch set, then split off the views changes into a follow-up patch? https://codereview.chromium.org/2781613003/diff/200001/ui/views/win/hwnd_mess... File ui/views/win/hwnd_message_handler.cc (right): https://codereview.chromium.org/2781613003/diff/200001/ui/views/win/hwnd_mess... ui/views/win/hwnd_message_handler.cc:1541: } else if (static_cast<DWORD>(OBJID_CARET) == obj_id) { Only if focused??? https://codereview.chromium.org/2781613003/diff/200001/ui/views/win/hwnd_mess... File ui/views/win/hwnd_message_handler.h (right): https://codereview.chromium.org/2781613003/diff/200001/ui/views/win/hwnd_mess... ui/views/win/hwnd_message_handler.h:231: void OnTextInputTypeChanged(const TextInputClient* client) override {} You should handle this in order to handle the case where the caret disappears, i.e. focus moves to something that's not text. https://codereview.chromium.org/2781613003/diff/200001/ui/views/win/hwnd_mess... ui/views/win/hwnd_message_handler.h:236: void OnInputMethodDestroyed(const InputMethod* input_method) override; Possibly this too. See chrome/browser/chromeos/accessibility/accessibility_highlight_manager.cc for another class where I extended InputMethodObserver to do focus highlighting, you should follow similar logic
Comments addressed. I prefer not to spend time splitting this, especially since there will be reviews from owners which might modify the design even further. -- 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.
Description was changed from ========== Added a class acting as a fake caret for accessibility. I don't like the way I did lifetime management. I will be turning AXFakeCaretWin into a singleton. Otherwise, you may have a first look. R=dmazzoni@chromium.org, aleventhal@chromium.org TESTED=not quite ready yet ========== to ========== Added a class acting as a fake caret for accessibility. Required by some screen magnifiers and Windows built-in Magnifier. R=dmazzoni@chromium.org, aleventhal@chromium.org TESTED=Manually ==========
Description was changed from ========== Added a class acting as a fake caret for accessibility. Required by some screen magnifiers and Windows built-in Magnifier. R=dmazzoni@chromium.org, aleventhal@chromium.org TESTED=Manually ========== to ========== Added a class acting as a fake caret for accessibility. Required by some screen magnifiers and Windows built-in Magnifier. R=aleventhal@chromium.org, dmazzoni@chromium.org, ellyjones@chromium.org, ananta@chromium.org, sadrul@chromium.org TESTED=Manually ==========
nektar@chromium.org changed reviewers: + ananta@chromium.org, sadrul@chromium.org
@ananta @sadrul Please review changes to content/browser/render_host/.... This is a fake caret that is tracking the real caret so that screen magnifiers would be able to show an enlarge caret if needed.
nektar@chromium.org changed reviewers: - ananta@chromium.org, sadrul@chromium.org
I changed my mind after looking at the list of owners. Followup patch at: https://codereview.chromium.org/2852763002 -- 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.
dmazzoni@chromium.org changed reviewers: + ananta@chromium.org
lgtm +ananta for legacy_render_widget_host_win https://codereview.chromium.org/2781613003/diff/250001/content/browser/render... File content/browser/renderer_host/render_widget_host_view_aura.cc (right): https://codereview.chromium.org/2781613003/diff/250001/content/browser/render... content/browser/renderer_host/render_widget_host_view_aura.cc:2361: // #endif defined(USE_X11) && !defined(OS_CHROMEOS) Get rid of this commented-out line
The CQ bit was checked by nektar@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
@ananta Please review changes to content/browser/renderer_host/...
legacy_render_widget_host_view_win.cc/.h lgtm
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/2781613003/#ps270001 (title: "Addressed final comment.")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: android_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...) android_cronet on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...) android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...) cast_shell_android on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...) chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...) linux_chromium_chromeos_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by nektar@chromium.org to run a CQ dry run
The CQ bit was checked by nektar@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from ananta@chromium.org, dmazzoni@chromium.org Link to the patchset: https://codereview.chromium.org/2781613003/#ps290001 (title: "Addressed final comment.")
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was checked by nektar@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from ananta@chromium.org, dmazzoni@chromium.org Link to the patchset: https://codereview.chromium.org/2781613003/#ps310001 (title: "Fixed rebase conflict.")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
@sadrul Please review changes to content/browser/renderer_host/render_widget_host_view_aura.cc
nektar@chromium.org changed reviewers: + sadrul@chromium.org
@sadrul Please review changes to content/browser/renderer_host/render_widget_host_view_aura.cc
lgtm Is it possible to write tests for this change? https://codereview.chromium.org/2781613003/diff/310001/content/browser/render... File content/browser/renderer_host/render_widget_host_view_aura.cc (right): https://codereview.chromium.org/2781613003/diff/310001/content/browser/render... content/browser/renderer_host/render_widget_host_view_aura.cc:2370: #if defined(USE_X11) && !defined(OS_CHROMEOS) You wouldn't normally need OS_CHROMEOS check here (USE_X11 is no longer turned on for chromeos). But unfortunately I think some cros bots still do use USE_X11 for chromeos :/ https://codereview.chromium.org/2781613003/diff/310001/ui/accessibility/platf... File ui/accessibility/platform/ax_fake_caret_win.cc (right): https://codereview.chromium.org/2781613003/diff/310001/ui/accessibility/platf... ui/accessibility/platform/ax_fake_caret_win.cc:1: // Copyright (c) 2017 The Chromium Authors. All rights reserved. No (c) in new files.
Is it possible to write tests for this change? I am not sure, but can I do it on a followup patch. I have another patch out for implementing fake caret on the address bar, so there is certainly more work to be done. > > https://codereview.chromium.org/2781613003/diff/310001/content/browser/render... > File content/browser/renderer_host/render_widget_host_view_aura.cc > (right): > > https://codereview.chromium.org/2781613003/diff/310001/content/browser/render... > content/browser/renderer_host/render_widget_host_view_aura.cc:2370: #if > defined(USE_X11) && !defined(OS_CHROMEOS) > You wouldn't normally need OS_CHROMEOS check here (USE_X11 is no longer > turned on for chromeos). But unfortunately I think some cros bots still > do use USE_X11 for chromeos :/ > I'll leave it in then. > https://codereview.chromium.org/2781613003/diff/310001/ui/accessibility/platf... > File ui/accessibility/platform/ax_fake_caret_win.cc (right): > > https://codereview.chromium.org/2781613003/diff/310001/ui/accessibility/platf... > ui/accessibility/platform/ax_fake_caret_win.cc:1: // Copyright (c) 2017 > The Chromium Authors. All rights reserved. > No (c) in new files. Done. -- 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
The patchset sent to the CQ was uploaded after l-g-t-m from ananta@chromium.org, sadrul@chromium.org, dmazzoni@chromium.org Link to the patchset: https://codereview.chromium.org/2781613003/#ps330001 (title: "Removed copyright symbol.")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: cast_shell_android on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...) linux_chromium_asan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by nektar@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from ananta@chromium.org, sadrul@chromium.org, dmazzoni@chromium.org Link to the patchset: https://codereview.chromium.org/2781613003/#ps350001 (title: "Fixed formatting.")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...) mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
The CQ bit was checked by nektar@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from ananta@chromium.org, sadrul@chromium.org, dmazzoni@chromium.org Link to the patchset: https://codereview.chromium.org/2781613003/#ps370001 (title: "Added a class acting as a fake caret for accessibility.")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...) win_clang on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_clang/builds/...)
The CQ bit was checked by nektar@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from ananta@chromium.org, sadrul@chromium.org, dmazzoni@chromium.org Link to the patchset: https://codereview.chromium.org/2781613003/#ps390001 (title: "Fixed compilation error due to rebase.")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
The CQ bit was checked by nektar@chromium.org
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 390001, "attempt_start_ts": 1494728082448390,
"parent_rev": "7bda7a4b1a6b2866538f903e06824500894d7d61", "commit_rev":
"3eda6b165d26b7cc5e4549575780ddfe10502d62"}
Message was sent while issue was closed.
Description was changed from ========== Added a class acting as a fake caret for accessibility. Required by some screen magnifiers and Windows built-in Magnifier. R=aleventhal@chromium.org, dmazzoni@chromium.org, ellyjones@chromium.org, ananta@chromium.org, sadrul@chromium.org TESTED=Manually ========== to ========== Added a class acting as a fake caret for accessibility. Required by some screen magnifiers and Windows built-in Magnifier. R=aleventhal@chromium.org, dmazzoni@chromium.org, ellyjones@chromium.org, ananta@chromium.org, sadrul@chromium.org TESTED=Manually Review-Url: https://codereview.chromium.org/2781613003 Cr-Commit-Position: refs/heads/master@{#471624} Committed: https://chromium.googlesource.com/chromium/src/+/3eda6b165d26b7cc5e4549575780... ==========
Message was sent while issue was closed.
Committed patchset #21 (id:390001) as https://chromium.googlesource.com/chromium/src/+/3eda6b165d26b7cc5e4549575780... |
