Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2255363002:
TextInputManager::GetTextSelection() should be called on focused widget. (Closed)
|
Created:
4 years, 4 months ago by EhsanK Modified:
4 years, 4 months ago CC:
chromium-reviews, yusukes+watch_chromium.org, shuchen+watch_chromium.org, jam, sievers+watch_chromium.org, jbauman+watch_chromium.org, nona+watch_chromium.org, darin-cc_chromium.org, piman+watch_chromium.org, kalyank, danakj+watch_chromium.org, James Su, site-isolation-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionTextInputManager::GetTextSelection() should be called on focused widget.
Currently, in the following methods in RenderWidgetHostViewAura, we use
the active widget, i.e., the widget with focused text field to get text
selection information:
GetTextRange()
GetSelectionRange()
GetTextFromRange()
Using the active widget as opposed to the focused widget regressed the
long press touch quick menu on Aura. This is due to the call to
GetSelectionRange() which returns empty when the selection is on a
non-editable text. Using the focused widget is correct and fixes the
issue.
The corresponding unit tests are also modified so that we now test the
selection on "focused widget" as opposed to "active widget".
BUG=638898, 602723, 578168
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
Committed: https://crrev.com/9135773062a5f13e51348454c682d6661447c887
Cr-Commit-Position: refs/heads/master@{#413795}
Patch Set 1 #Patch Set 2 : Rebased #Patch Set 3 : Initialize |text_input_manager_| when RWHVCF is created #Patch Set 4 : Rebased #
Total comments: 2
Patch Set 5 : Check for focused widget before using its view #
Messages
Total messages: 27 (21 generated)
Description was changed from ========== TextInputManager::GetTextSelection() should be called on focused widget. Currently, in the following methods in RenderWidgetHostViewAura, we use the active widget, i.e., the widget with focused text field to get text selection information: GetTextRange() GetSelectionRange() GetTextFromRange() Using the active widget as opposed to the focused widget regressed the long press touch quick menu on Aura. This is due to the call to GetSelectionRange() which returns empty when the selection is on a non-editable text. Using the focused widget is correct and fixes the issue. The corresponding unit tests are also modified so that we now test the selection on "focused widget" as opposed to "active widget". BUG=638898, 602723, 578168 ========== to ========== TextInputManager::GetTextSelection() should be called on focused widget. Currently, in the following methods in RenderWidgetHostViewAura, we use the active widget, i.e., the widget with focused text field to get text selection information: GetTextRange() GetSelectionRange() GetTextFromRange() Using the active widget as opposed to the focused widget regressed the long press touch quick menu on Aura. This is due to the call to GetSelectionRange() which returns empty when the selection is on a non-editable text. Using the focused widget is correct and fixes the issue. The corresponding unit tests are also modified so that we now test the selection on "focused widget" as opposed to "active widget". BUG=638898, 602723, 578168 ==========
The CQ bit was checked by ekaramad@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: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Description was changed from ========== TextInputManager::GetTextSelection() should be called on focused widget. Currently, in the following methods in RenderWidgetHostViewAura, we use the active widget, i.e., the widget with focused text field to get text selection information: GetTextRange() GetSelectionRange() GetTextFromRange() Using the active widget as opposed to the focused widget regressed the long press touch quick menu on Aura. This is due to the call to GetSelectionRange() which returns empty when the selection is on a non-editable text. Using the focused widget is correct and fixes the issue. The corresponding unit tests are also modified so that we now test the selection on "focused widget" as opposed to "active widget". BUG=638898, 602723, 578168 ========== to ========== TextInputManager::GetTextSelection() should be called on focused widget. Currently, in the following methods in RenderWidgetHostViewAura, we use the active widget, i.e., the widget with focused text field to get text selection information: GetTextRange() GetSelectionRange() GetTextFromRange() Using the active widget as opposed to the focused widget regressed the long press touch quick menu on Aura. This is due to the call to GetSelectionRange() which returns empty when the selection is on a non-editable text. Using the focused widget is correct and fixes the issue. The corresponding unit tests are also modified so that we now test the selection on "focused widget" as opposed to "active widget". BUG=638898, 602723, 578168 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
The CQ bit was checked by ekaramad@chromium.org to run a CQ dry run
Description was changed from ========== TextInputManager::GetTextSelection() should be called on focused widget. Currently, in the following methods in RenderWidgetHostViewAura, we use the active widget, i.e., the widget with focused text field to get text selection information: GetTextRange() GetSelectionRange() GetTextFromRange() Using the active widget as opposed to the focused widget regressed the long press touch quick menu on Aura. This is due to the call to GetSelectionRange() which returns empty when the selection is on a non-editable text. Using the focused widget is correct and fixes the issue. The corresponding unit tests are also modified so that we now test the selection on "focused widget" as opposed to "active widget". BUG=638898, 602723, 578168 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== TextInputManager::GetTextSelection() should be called on focused widget. Currently, in the following methods in RenderWidgetHostViewAura, we use the active widget, i.e., the widget with focused text field to get text selection information: GetTextRange() GetSelectionRange() GetTextFromRange() Using the active widget as opposed to the focused widget regressed the long press touch quick menu on Aura. This is due to the call to GetSelectionRange() which returns empty when the selection is on a non-editable text. Using the focused widget is correct and fixes the issue. The corresponding unit tests are also modified so that we now test the selection on "focused widget" as opposed to "active widget". BUG=638898, 602723, 578168 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
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 ekaramad@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...
ekaramad@chromium.org changed reviewers: + kenrb@chromium.org, piman@chromium.org
kenrb@: Could you please take a look, esp. the RWHVCF change? piman@: Could you please take a look? https://codereview.chromium.org/2255363002/diff/60001/content/browser/frame_h... File content/browser/frame_host/render_widget_host_view_child_frame.cc (right): https://codereview.chromium.org/2255363002/diff/60001/content/browser/frame_h... content/browser/frame_host/render_widget_host_view_child_frame.cc:59: GetTextInputManager(); This initialization is necessary because InputMethodChromeOS calls GetTextSelection() on the focused view which has not yet initialized its |text_input_manager_|. This was firing the DCHECK in TextInputManager::GetTextSelection. https://codereview.chromium.org/2255363002/diff/60001/content/browser/rendere... File content/browser/renderer_host/render_widget_host_view_aura.cc (right): https://codereview.chromium.org/2255363002/diff/60001/content/browser/rendere... content/browser/renderer_host/render_widget_host_view_aura.cc:1539: bool RenderWidgetHostViewAura::GetSelectionRange(gfx::Range* range) const { This method was being used in TouchSelectionController and was the main cause for the quick menu regression. Any text selection related method should use focused widget since text selection could be from non-editable elements. This being said, the other two methods changed seem to have only IME related uses so GetActiveWidget() should be equal to GetFocusedWidget().
lgtm
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: 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...)
lgtm
The CQ bit was checked by ekaramad@chromium.org to run a CQ dry run
Thanks for the reviews!
The CQ bit was unchecked by ekaramad@chromium.org
The CQ bit was checked by ekaramad@chromium.org
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== TextInputManager::GetTextSelection() should be called on focused widget. Currently, in the following methods in RenderWidgetHostViewAura, we use the active widget, i.e., the widget with focused text field to get text selection information: GetTextRange() GetSelectionRange() GetTextFromRange() Using the active widget as opposed to the focused widget regressed the long press touch quick menu on Aura. This is due to the call to GetSelectionRange() which returns empty when the selection is on a non-editable text. Using the focused widget is correct and fixes the issue. The corresponding unit tests are also modified so that we now test the selection on "focused widget" as opposed to "active widget". BUG=638898, 602723, 578168 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== TextInputManager::GetTextSelection() should be called on focused widget. Currently, in the following methods in RenderWidgetHostViewAura, we use the active widget, i.e., the widget with focused text field to get text selection information: GetTextRange() GetSelectionRange() GetTextFromRange() Using the active widget as opposed to the focused widget regressed the long press touch quick menu on Aura. This is due to the call to GetSelectionRange() which returns empty when the selection is on a non-editable text. Using the focused widget is correct and fixes the issue. The corresponding unit tests are also modified so that we now test the selection on "focused widget" as opposed to "active widget". BUG=638898, 602723, 578168 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== TextInputManager::GetTextSelection() should be called on focused widget. Currently, in the following methods in RenderWidgetHostViewAura, we use the active widget, i.e., the widget with focused text field to get text selection information: GetTextRange() GetSelectionRange() GetTextFromRange() Using the active widget as opposed to the focused widget regressed the long press touch quick menu on Aura. This is due to the call to GetSelectionRange() which returns empty when the selection is on a non-editable text. Using the focused widget is correct and fixes the issue. The corresponding unit tests are also modified so that we now test the selection on "focused widget" as opposed to "active widget". BUG=638898, 602723, 578168 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== TextInputManager::GetTextSelection() should be called on focused widget. Currently, in the following methods in RenderWidgetHostViewAura, we use the active widget, i.e., the widget with focused text field to get text selection information: GetTextRange() GetSelectionRange() GetTextFromRange() Using the active widget as opposed to the focused widget regressed the long press touch quick menu on Aura. This is due to the call to GetSelectionRange() which returns empty when the selection is on a non-editable text. Using the focused widget is correct and fixes the issue. The corresponding unit tests are also modified so that we now test the selection on "focused widget" as opposed to "active widget". BUG=638898, 602723, 578168 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation Committed: https://crrev.com/9135773062a5f13e51348454c682d6661447c887 Cr-Commit-Position: refs/heads/master@{#413795} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/9135773062a5f13e51348454c682d6661447c887 Cr-Commit-Position: refs/heads/master@{#413795}
Message was sent while issue was closed.
Description was changed from ========== TextInputManager::GetTextSelection() should be called on focused widget. Currently, in the following methods in RenderWidgetHostViewAura, we use the active widget, i.e., the widget with focused text field to get text selection information: GetTextRange() GetSelectionRange() GetTextFromRange() Using the active widget as opposed to the focused widget regressed the long press touch quick menu on Aura. This is due to the call to GetSelectionRange() which returns empty when the selection is on a non-editable text. Using the focused widget is correct and fixes the issue. The corresponding unit tests are also modified so that we now test the selection on "focused widget" as opposed to "active widget". BUG=638898, 602723, 578168 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation Committed: https://crrev.com/9135773062a5f13e51348454c682d6661447c887 Cr-Commit-Position: refs/heads/master@{#413795} ========== to ========== TextInputManager::GetTextSelection() should be called on focused widget. Currently, in the following methods in RenderWidgetHostViewAura, we use the active widget, i.e., the widget with focused text field to get text selection information: GetTextRange() GetSelectionRange() GetTextFromRange() Using the active widget as opposed to the focused widget regressed the long press touch quick menu on Aura. This is due to the call to GetSelectionRange() which returns empty when the selection is on a non-editable text. Using the focused widget is correct and fixes the issue. The corresponding unit tests are also modified so that we now test the selection on "focused widget" as opposed to "active widget". BUG=638898, 602723, 578168 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation Committed: https://crrev.com/9135773062a5f13e51348454c682d6661447c887 Cr-Commit-Position: refs/heads/master@{#413795} ========== |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
