Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1706353002:
Added the selected text marker range to the dictionary for the selection change notification (Closed)
|
Created:
4 years, 10 months ago by nektarios Modified:
4 years, 10 months ago CC:
aboxhall+watch_chromium.org, chromium-reviews, darin-cc_chromium.org, dmazzoni+watch_chromium.org, dtseng+watch_chromium.org, Elly Fong-Jones, jam, je_julie, nektar+watch_chromium.org, plundblad+watch_chromium.org, yuzo+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdded the selected text marker range to the dictionary that accompanies the text selection change notification.
This makes cursor navigation with VoiceOver work in content editables. <textarea> and <input> will need further investigation.
BUG=530826
TESTED=Manually with VoiceOver
Committed: https://crrev.com/4909d921b4be8870e3de62d9eb24467452232900
Cr-Commit-Position: refs/heads/master@{#376515}
Patch Set 1 #Patch Set 2 : Small changes. #Patch Set 3 : Still does not work. #Patch Set 4 : Now works the first time you start the browser. #
Total comments: 4
Patch Set 5 : Removed all logging. #Patch Set 6 : Merged with dmazzoni's change to GetFocus. #
Total comments: 2
Patch Set 7 : Fixed merge issues. #Patch Set 8 : fixed standard text fields. #
Messages
Total messages: 29 (11 generated)
Description was changed from ========== Added the selected text marker range to the dictionary that accompanies the text selection change notification. BUG= ========== to ========== Added the selected text marker range to the dictionary that accompanies the text selection change notification. This makes cursor navigation with VoiceOver work in simple text field, <input> and <textarea>. BUG=530826 TESTED=Manually with VoiceOver ==========
nektar@chromium.org changed reviewers: + dmazzoni@chromium.org, mark@chromium.org
nektar@chromium.org changed reviewers: + avi@chromium.org
You may have a first look. It works the first time you alt+tab to the browser window and into a text field. Then, sadly it stops working. I know what's going on though.
Great, glad you figured it out! https://codereview.chromium.org/1706353002/diff/60001/content/browser/accessi... File content/browser/accessibility/browser_accessibility_cocoa.mm (right): https://codereview.chromium.org/1706353002/diff/60001/content/browser/accessi... content/browser/accessibility/browser_accessibility_cocoa.mm:194: LOG(ERROR) << "GetTextRange at " << start_offset << ' ' << end_offset << ' ' << [[start_object->ToBrowserAccessibilityCocoa() role] UTF8String]; nit: remove this before landing https://codereview.chromium.org/1706353002/diff/60001/content/browser/accessi... File content/browser/accessibility/browser_accessibility_manager.h (right): https://codereview.chromium.org/1706353002/diff/60001/content/browser/accessi... content/browser/accessibility/browser_accessibility_manager.h:254: virtual BrowserAccessibility* GetFocus(); fwiw, this will conflict with a change i'm hoping to land today that removes the need for the "root" argument. please just use GetFocus(GetRoot()) for now, and after my change lands it will just be GetFocus(). https://codereview.chromium.org/1706353002/diff/60001/content/browser/accessi... File content/browser/accessibility/browser_accessibility_manager_mac.h (right): https://codereview.chromium.org/1706353002/diff/60001/content/browser/accessi... content/browser/accessibility/browser_accessibility_manager_mac.h:27: using BrowserAccessibilityManager::GetFocus; Is this needed?
All the ObjC stuff LGTM. Follow up on dmazzoni's issues, and remove all the logging before commit.
https://codereview.chromium.org/1706353002/diff/60001/content/browser/accessi... content/browser/accessibility/browser_accessibility_manager_mac.h:27: using BrowserAccessibilityManager::GetFocus; Is this needed? Yes, it seems that C++ auto hides overloaded methods from superclasses unless you override them or redeclare them. -- 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.
https://codereview.chromium.org/1706353002/diff/60001/content/browser/accessi... File content/browser/accessibility/browser_accessibility_manager_mac.mm (right): https://codereview.chromium.org/1706353002/diff/60001/content/browser/accessi... content/browser/accessibility/browser_accessibility_manager_mac.mm:322: LOG(ERROR) << "Marker range " << ([native_focus_object selectedTextMarkerRange] == nil) << " " << [[native_focus_object role] UTF8String]; Remove this LOG.
Description was changed from ========== Added the selected text marker range to the dictionary that accompanies the text selection change notification. This makes cursor navigation with VoiceOver work in simple text field, <input> and <textarea>. BUG=530826 TESTED=Manually with VoiceOver ========== to ========== Added the selected text marker range to the dictionary that accompanies the text selection change notification. This makes cursor navigation with VoiceOver work in content editables. <textarea> and <input> will need further investigation. BUG=530826 TESTED=Manually with VoiceOver ==========
The CQ bit was checked by nektar@chromium.org to run a CQ dry run
On 2016/02/19 at 00:33:29, mark wrote: > https://codereview.chromium.org/1706353002/diff/60001/content/browser/accessi... > File content/browser/accessibility/browser_accessibility_manager_mac.mm (right): > > https://codereview.chromium.org/1706353002/diff/60001/content/browser/accessi... > content/browser/accessibility/browser_accessibility_manager_mac.mm:322: LOG(ERROR) << "Marker range " << ([native_focus_object selectedTextMarkerRange] == nil) << " " << [[native_focus_object role] UTF8String]; > Remove this LOG.ditable got fixed. Merged with Dominic's change. Removed logging. Ready to submit. Note that only content e
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1706353002/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1706353002/100001
Not sure if this was supposed to go atop another (Dominic’s) change or combined with it, so I’ll let him answer that. https://codereview.chromium.org/1706353002/diff/100001/content/browser/access... File content/browser/accessibility/browser_accessibility_manager.h (right): https://codereview.chromium.org/1706353002/diff/100001/content/browser/access... content/browser/accessibility/browser_accessibility_manager.h:178: void SetFocus(ui::AXNode* node, bool notify); Dominic should comment on this.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: cast_shell_android on tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/cast_shell_a...) linux_chromium_compile_dbg_32_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) linux_chromium_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...) mac_chromium_gn_rel on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_gn_r...) mac_chromium_rel_ng on tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
not lgtm. this accidentally adds back a few things i deliberately deleted in a recent patch. BrowserAccessibilityManager doesn't keep track of focus anymore, AXTree does. https://codereview.chromium.org/1706353002/diff/100001/content/browser/access... File content/browser/accessibility/browser_accessibility_manager.h (right): https://codereview.chromium.org/1706353002/diff/100001/content/browser/access... content/browser/accessibility/browser_accessibility_manager.h:369: ui::AXNode* focus_; This is definitely wrong. I got rid of this in my change yesterday, please carefully merge your change on top of it and don't add this back.
The CQ bit was checked by nektar@chromium.org to run a CQ dry run
Oh well, it's not the first time that I have been bitten by git. Merge issues fixed and verified by reading the whole diff.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1706353002/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1706353002/120001
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm
LGTM
The CQ bit was checked by nektar@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from avi@chromium.org Link to the patchset: https://codereview.chromium.org/1706353002/#ps120001 (title: "Fixed merge issues.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1706353002/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1706353002/120001
Message was sent while issue was closed.
Committed patchset #7 (id:120001)
Message was sent while issue was closed.
Description was changed from ========== Added the selected text marker range to the dictionary that accompanies the text selection change notification. This makes cursor navigation with VoiceOver work in content editables. <textarea> and <input> will need further investigation. BUG=530826 TESTED=Manually with VoiceOver ========== to ========== Added the selected text marker range to the dictionary that accompanies the text selection change notification. This makes cursor navigation with VoiceOver work in content editables. <textarea> and <input> will need further investigation. BUG=530826 TESTED=Manually with VoiceOver Committed: https://crrev.com/4909d921b4be8870e3de62d9eb24467452232900 Cr-Commit-Position: refs/heads/master@{#376515} ==========
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/4909d921b4be8870e3de62d9eb24467452232900 Cr-Commit-Position: refs/heads/master@{#376515} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
