Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2931443003:
Add support for Android spellcheck menu in Chrome/WebViews (Closed)
|
Created:
3 years, 6 months ago by rlanday Modified:
3 years, 4 months ago Reviewers:
Theresa, aelias_OOO_until_Jul13, Torne, chongz, boliu, Ted C, Mike West, Xiaocheng, *yosin_UTC9, kouhei (in TOK) CC:
Aaron Boodman, abarth-chromium, agrieve+watch_chromium.org, android-webview-reviews_chromium.org, blink-reviews, blink-reviews-api_chromium.org, blink-reviews-frames_chromium.org, chromium-reviews, darin (slow to review), darin-cc_chromium.org, dglazkov+blink, groby+blinkspell_chromium.org, haraken, jam, nona+watch_chromium.org, qsr+mojo_chromium.org, shuchen+watch_chromium.org, James Su, timvolodine, viettrungluu+watch_chromium.org, yusukes+watch_chromium.org, yzshen+watch_chromium.org Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd support for Android spellcheck menu in Chrome/WebViews
Currently, in Chrome and in WebViews, tapping misspelled words on Android (that
have red underlines) doesn't do anything. In a native text box, tapping a
misspelled word opens a menu that allows the user to pick from a list of
suggested replacements (if any), delete the misspelled word, or add it to the
system dictionary. This CL ports this menu to Chrome and WebViews.
Before this project, the list of spelling suggestions from the Android
spellchecker was being discarded. https://codereview.chromium.org/2905003003
fixes this so the suggestions are passed through to the Spelling
DocumentMarkers. This CL adds code to SelectionController::HandleSingleClick()
to check if the user tapped on a misspelled word, and if so, send a Mojo
message from the renderer to the browser to start a timer to open the
spellcheck menu unless we receive another tap within the double-tap timeout
threshold (note: this CL restricts the spellcheck menu to Android by not
handling this message on any other platform). If the timer expires, a message
is sent back to the renderer to try to open the spellcheck menu (nothing
happens if the spelling marker no longer exists). If the renderer decides the
spellcheck menu should be opened, it highlights the misspelled word with in red
using an ActiveSuggestionMarker (added in
https://codereview.chromium.org/2923033002), hides the caret, and sends another
message back to the browser with a list of spelling suggestions (if any).
The layout files for the menu and the logic for showing it were ported over from
the implementation for the same menu in android.widget.Editor. Some of the XML
files were modified slightly to avoid dependencies on internal Android system
resources or newer Android versions.
Tapping outside the spellcheck menu closes it, and sends a message to Blink
telling it to remove the highlight and show the caret again. There are three
other messages for applying a suggestion, deleting the misspelled word, and
adding the word to the dictionary. For adding the word to the dictionary, I'm
using the same Android intent the native textbox uses, with the same wonky
behavior that the spellcheck marker is removed immediately when the user taps
"Add to dictionary", before the user has gone through the confirmation dialog.
BUG=715365
Review-Url: https://codereview.chromium.org/2931443003
Cr-Commit-Position: refs/heads/master@{#489505}
Committed: https://chromium.googlesource.com/chromium/src/+/e4a6ec83ec32b4f139794f39bf2cbb0b25617b56
Patch Set 1 #
Total comments: 27
Patch Set 2 : Respond to yosin@'s comments #
Total comments: 8
Patch Set 3 : Split off SpellChecker changes #
Total comments: 1
Patch Set 4 : Rebase on moving color to LayoutTheme, do beforeinput stuff #
Total comments: 3
Patch Set 5 : Rebase #Patch Set 6 : Fix PopupZoomerTest biuld #Patch Set 7 : Re-upload with no changes because I screwed something up trying to cancel the last upload when I sa… #
Total comments: 9
Patch Set 8 : Respond to comments #Patch Set 9 : Remove some unused fields in SuggestionsPopupWindow, don't hold reference to mutable array passed in #
Total comments: 2
Patch Set 10 : Rebase #Patch Set 11 : Rebase on DocumentShutdownObserver CL #
Total comments: 5
Patch Set 12 : Respond to xiaochengh's comments #Patch Set 13 : Fix dependency #
Total comments: 8
Patch Set 14 : Respond to aelias@'s comments #
Total comments: 18
Patch Set 15 : Attempt to fix trybots #Patch Set 16 : Respond to yosin@'s and aelias@'s comments #Patch Set 17 : Split off LayoutTheme changes #Patch Set 18 : Actually split off LayoutTheme changes #
Total comments: 48
Patch Set 19 : Make HandlePotentialMisspelledWordTap() more efficient #Patch Set 20 : Use correct base commit #
Total comments: 1
Patch Set 21 : Remove leftover traces #
Total comments: 4
Patch Set 22 : Respond to Android-side feedback #
Total comments: 21
Patch Set 23 : Improve positioning logic, hard code menu item color, improve pre-L appearance #
Total comments: 4
Patch Set 24 : Update TextSuggestionControllerTest #Patch Set 25 : Rebase, respond to comments #Patch Set 26 : Rebase #
Total comments: 23
Patch Set 27 : Respond to comments #Patch Set 28 : Un-comment out event.Event().FromTouch() check in SelectionController #Patch Set 29 : s/override/final/ in TextSuggestionBackendImpl.h #Patch Set 30 : Make sure we clean up the suggestion highlight properly if a delete operation fails #
Total comments: 12
Patch Set 31 : Add support for Android spellcheck menu in Chrome/WebViews #Patch Set 32 : Reduce code duplication in TextSuggestionController #
Total comments: 18
Patch Set 33 : Respond to comments, fix DeleteActiveSuggestionRange() bug #Patch Set 34 : Consolidate mContentView and mContainerView #
Total comments: 8
Patch Set 35 : Respond to latest comments #
Total comments: 13
Patch Set 36 : Respond to UI + editing feedback #
Total comments: 6
Patch Set 37 : Rebase, respond to comments #Patch Set 38 : Simplify Mojo binding based on rockot@'s advice #Depends on Patchset: Messages
Total messages: 285 (146 generated)
rlanday@chromium.org changed reviewers: + aelias@chromium.org, jochen@chromium.org, mkwst@chromium.org, tedchoc@chromium.org, yosin@chromium.org
This is based on my CL to add support for SuggestionSpans on Android (not yet completed, I decided to do the spellcheck menu first because it's simpler and a lot of the code can be reused): https://codereview.chromium.org/2650113004/ I've tried to incorporate all the feedback I got there; one thing I haven't done yet is figure out if I can write a useful Robolectric test case for some of the Android code. I'd love to split this CL up into smaller pieces if we can find good split points, but I think we'd have to be OK with landing pieces of code that aren't really useful by themselves (e.g. TextSuggestionController). This CL has a small dependency on another CL that writes the Android spelling suggestions to the DocumentMarkers: https://codereview.chromium.org/2905003003/ and also on some CLs to finish implementing ActiveSuggestionMarker that should be landing soon. yosin@, adding you as an OWNER of: third_party/WebKit/Source/core/editing/BUILD.gn third_party/WebKit/Source/core/editing/SelectionController.cpp third_party/WebKit/Source/core/editing/TextSuggestionBackendImpl.cpp third_party/WebKit/Source/core/editing/TextSuggestionBackendImpl.h third_party/WebKit/Source/core/editing/TextSuggestionController.cpp third_party/WebKit/Source/core/editing/TextSuggestionController.h third_party/WebKit/Source/core/editing/TextSuggestionControllerTest.cpp third_party/WebKit/Source/core/editing/commands/CompositeEditCommand.cpp third_party/WebKit/Source/core/editing/spellcheck/SpellChecker.cpp third_party/WebKit/Source/core/editing/spellcheck/SpellChecker.h third_party/WebKit/Source/core/editing/spellcheck/SpellCheckerTest.cpp mkwst@, adding you as an OWNER of: third_party/WebKit/Source/core/events/InputEvent.cpp third_party/WebKit/Source/core/events/InputEvent.h third_party/WebKit/Source/core/frame/LocalFrame.cpp third_party/WebKit/Source/core/frame/LocalFrame.h third_party/WebKit/Source/modules/ModulesInitializer.cpp aelias@, adding you as an OWNER of: content/browser/renderer_host/render_widget_host_view_android.cc content/public/android/BUILD.gn content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java content/public/android/java/src/org/chromium/content/browser/input/ImeAdapter.java content/public/android/java/src/org/chromium/content/browser/input/InputMethodManagerWrapper.java content/public/android/java/src/org/chromium/content/browser/input/SuggestionsPopupWindow.java content/public/android/java/strings/android_content_strings.grd tedchoc@, adding you as an OWNER of: content/browser/android/ime_adapter_android.cc content/browser/android/ime_adapter_android.h content/browser/android/text_suggestion_host_impl.cc content/browser/android/text_suggestion_host_impl.h jochen@, adding you as an OWNER of: android_webview/browser/aw_browser_manifest_overlay.json content/browser/BUILD.gn content/browser/DEPS
https://codereview.chromium.org/2931443003/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/events/InputEvent.h (right): https://codereview.chromium.org/2931443003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/events/InputEvent.h:42: kDeleteByComposition, See the Input Events Level 2 working draft: https://www.w3.org/TR/input-events-2/#interface-InputEvent-Attributes My interpretation of the spec is that our use case falls under this: "15. If the user expresses an intention to remove a part of the DOM in order to recompose this part using IME, the inputType must be "deleteByComposition"."
The CQ bit was checked by rlanday@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_arm64_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_arm6...) android_compile_dbg on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_comp...)
Here's a set of screenshots that show how my implementation of the spellcheck menu compares to the native Android spellcheck menu: https://docs.google.com/document/d/1L8SIZpfUebRpFbtr5MkR1d7GwSZ9fJlQ27MBSwBJ1... (the most obvious difference is actually that Chrome uses a squiggly red underline and Android uses a solid red underline; I wonder if there's any interest in fixing this)
It looks like the trybots are failing because I'm including "mojo/public/cpp/bindings/strong_binding.h" from inside core/editing (in TextSuggestionBackendImpl.cpp)...so my Mojo code is probably structured somewhat incorrectly, but I'm not sure how to correct it.
https://codereview.chromium.org/2931443003/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/editing/spellcheck/SpellChecker.h (right): https://codereview.chromium.org/2931443003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/editing/spellcheck/SpellChecker.h:77: GetSpellCheckMarkerTouchingSelection(); This method's eventually going to need to be extended to support suggestion markers too, at which point it clearly doesn't make sense to have this in SpellChecker...yosin@, should I add this as a method on DocumentMarkerController? Something like GetMarkersTouchingRange(const EphemeralRange&, MarkerTypes)?
https://codereview.chromium.org/2931443003/diff/1/content/browser/android/ime... File content/browser/android/ime_adapter_android.cc (right): https://codereview.chromium.org/2931443003/diff/1/content/browser/android/ime... content/browser/android/ime_adapter_android.cc:42: int kDoubleTapTimeoutInMilliseconds = 300; nit: s/int/const int/ https://codereview.chromium.org/2931443003/diff/1/content/browser/android/ime... content/browser/android/ime_adapter_android.cc:303: if (rfh) { nit: early-return style is better as L130. Or change this like L130. Please use same style. https://codereview.chromium.org/2931443003/diff/1/content/browser/android/ime... content/browser/android/ime_adapter_android.cc:360: for (const blink::mojom::SpellCheckSuggestionPtr& suggestion_ptr : I think |const auto&| is enough since we know the element type of |suggestions| from function signature. https://codereview.chromium.org/2931443003/diff/1/content/browser/android/ime... content/browser/android/ime_adapter_android.cc:520: RenderFrameHost* rfh = GetFocusedFrame(); nit: We can put |rfh| in if-statement. if (RenderFrameHost* rfh = GetFocusedFrame() { rfh->... } https://codereview.chromium.org/2931443003/diff/1/content/browser/android/tex... File content/browser/android/text_suggestion_host_impl.h (right): https://codereview.chromium.org/2931443003/diff/1/content/browser/android/tex... content/browser/android/text_suggestion_host_impl.h:18: class TextSuggestionHostImpl : public blink::mojom::TextSuggestionHost { Could you add class level comment? Is this |final| class? https://codereview.chromium.org/2931443003/diff/1/content/browser/android/tex... content/browser/android/text_suggestion_host_impl.h:35: ImeAdapterAndroid* ime_adapter_android_; nit: s/ImeAdapterAndroid*/ImeAdapterAndroid* const/ https://codereview.chromium.org/2931443003/diff/1/content/browser/android/tex... content/browser/android/text_suggestion_host_impl.h:36: }; Please add DISALLOW_COPY_AND_ASSIGN(...) https://codereview.chromium.org/2931443003/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/editing/TextSuggestionBackendImpl.h (right): https://codereview.chromium.org/2931443003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/editing/TextSuggestionBackendImpl.h:1: // Copyright 2017 The Chromium Authors. All rights reserved. Text suggestion is related to spell checking, could you move new file for text suggestions to core/editing/suggetion? https://codereview.chromium.org/2931443003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/editing/TextSuggestionBackendImpl.h:16: class CORE_EXPORT TextSuggestionBackendImpl final Could you add a class comment? https://codereview.chromium.org/2931443003/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/editing/TextSuggestionController.cpp (right): https://codereview.chromium.org/2931443003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/editing/TextSuggestionController.cpp:71: bool delete_next_char = false; Could you move calculation of |delete_next_char| in another function? https://codereview.chromium.org/2931443003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/editing/TextSuggestionController.cpp:99: EphemeralRange range_to_delete = EphemeralRange( nit: s/EphemeralRange/const EphemeralRange/ https://codereview.chromium.org/2931443003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/editing/TextSuggestionController.cpp:101: Position(marker_text_node, marker->EndOffset() + delete_next_char)); At first glance "int + bool" is strange, but studying, it is valid. C++ converts static_cast<int>(false)->0 and static_cast<int>(true)->1. https://codereview.chromium.org/2931443003/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/editing/TextSuggestionController.h (right): https://codereview.chromium.org/2931443003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/editing/TextSuggestionController.h:17: class CORE_EXPORT TextSuggestionController final Could you add a class comment? https://codereview.chromium.org/2931443003/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/editing/spellcheck/SpellChecker.cpp (right): https://codereview.chromium.org/2931443003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/editing/spellcheck/SpellChecker.cpp:822: EphemeralRange range_to_check(selection.Start(), selection.End()); Could you move calculation of |range_to_check| in another function? https://codereview.chromium.org/2931443003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/editing/spellcheck/SpellChecker.cpp:847: Node* start_container = range_to_check.StartPosition().ComputeContainerNode(); nit: s/Node*/Node* const/ https://codereview.chromium.org/2931443003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/editing/spellcheck/SpellChecker.cpp:850: Node* end_container = range_to_check.EndPosition().ComputeContainerNode(); nit: s/Node*/Node* const/ https://codereview.chromium.org/2931443003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/editing/spellcheck/SpellChecker.cpp:859: if (&node == start_container && marker->EndOffset() <= start_offset) nit: s/&node/node/ There is |bool operator==(const Node&, const Node*)| https://codereview.chromium.org/2931443003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/editing/spellcheck/SpellChecker.cpp:861: if (&node == end_container && marker->StartOffset() >= end_offset) nit: s/&node/node/ There is |bool operator==(const Node&, const Node*)| https://codereview.chromium.org/2931443003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/editing/spellcheck/SpellChecker.cpp:873: Optional<std::pair<Node*, SpellCheckMarker*>> node_and_marker = nit: s/Optional<std::pair<Node*, SpellCheckMarker*>>/const Optional<std::pair<Node*, SpellCheckMarker*>>&/ https://codereview.chromium.org/2931443003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/editing/spellcheck/SpellChecker.cpp:878: Node* container_node = node_and_marker.value().first; nit: s/Node*/Node* const/ https://codereview.chromium.org/2931443003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/editing/spellcheck/SpellChecker.cpp:879: SpellCheckMarker* marker = node_and_marker.value().second; nit: s/SpellCheckMakrer*/SpellCheckMakrer* const/ https://codereview.chromium.org/2931443003/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/editing/spellcheck/SpellChecker.cpp:881: EphemeralRange misspelled_range( nit: s/EphemeralRange/const EphemeralRange/
https://codereview.chromium.org/2931443003/diff/1/content/public/android/java... File content/public/android/java/src/org/chromium/content/browser/input/SuggestionsPopupWindow.java (right): https://codereview.chromium.org/2931443003/diff/1/content/public/android/java... content/public/android/java/src/org/chromium/content/browser/input/SuggestionsPopupWindow.java:34: public class SuggestionsPopupWindow implements OnItemClickListener, OnDismissListener { > Here's a set of screenshots that show how my implementation of the spellcheck menu compares to the native Android spellcheck menu: https://docs.google.com/document/d/1L8SIZpfUebRpFbtr5MkR1d7GwSZ9fJlQ27MBSwBJ1... Do we understand why Hangouts and Messages have different appearance? That indicates one or both of them is not using the menu baked into framework EditText. It's unlikely that they would've reinvented the wheel as we are doing -- is it possible Android support library has a copy of this that we could use? > (the most obvious difference is actually that Chrome uses a squiggly red underline and Android uses a solid red underline; I wonder if there's any interest in fixing this) Yeah, I had noticed that, but squiggly underline is the well-known signifier for spelling mistake whereas solid red underline seems weird. I'm not sure if this was a conscious UX decision on Android's part (in which case we should follow) or if it was just implementation laziness (in which case maybe they should fix this on their end).
On 2017/06/07 at 03:02:59, aelias wrote: > Do we understand why Hangouts and Messages have different appearance? That indicates one or both of them is not using the menu baked into framework EditText. It's unlikely that they would've reinvented the wheel as we are doing -- is it possible Android support library has a copy of this that we could use? I think all that's happening is that the list is picking up some sort of style information from the app. There are two different versions of the system menu: text_edit_suggestion_container.xml (Holo-themed; not sure what's using this) and text_edit_suggestion_container_material.xml (Material Design-themed; I think this is what Messages and Hangouts use and it's what I'm basing the Chromium implementation on). I think Messages is somehow overriding the green text to be blue and Hangouts is somehow either hiding the list divider or making it invisible. I can investigate a little more tomorrow.
https://codereview.chromium.org/2931443003/diff/1/content/browser/renderer_ho... File content/browser/renderer_host/render_widget_host_view_android.cc (right): https://codereview.chromium.org/2931443003/diff/1/content/browser/renderer_ho... content/browser/renderer_host/render_widget_host_view_android.cc:941: // Receiving any other touch event before the double-tap timeout expires This matches the behavior of the native Android text box: https://android.googlesource.com/platform/frameworks/base/+/master/core/java/... However, the native Android text box exhibits seriously buggy behavior if you tap on a misspelled word and then keep typing (in most cases the spellcheck marker is cleared from the edit, and it tries to do the highlight + open the menu at the last location the menu was opened at; sometimes a crash even results). So it might be prudent to also clear the timer on a keyboard event. (Note for Googlers: Filed the Android bug as b/62392030)
On 2017/06/07 at 04:01:17, rlanday wrote: > On 2017/06/07 at 03:02:59, aelias wrote: > > Do we understand why Hangouts and Messages have different appearance? That indicates one or both of them is not using the menu baked into framework EditText. It's unlikely that they would've reinvented the wheel as we are doing -- is it possible Android support library has a copy of this that we could use? > > I think all that's happening is that the list is picking up some sort of style information from the app. There are two different versions of the system menu: text_edit_suggestion_container.xml (Holo-themed; not sure what's using this) and text_edit_suggestion_container_material.xml (Material Design-themed; I think this is what Messages and Hangouts use and it's what I'm basing the Chromium implementation on). I think Messages is somehow overriding the green text to be blue and Hangouts is somehow either hiding the list divider or making it invisible. I can investigate a little more tomorrow. "Add to dictionary" and "Delete" in the spellcheck menu use the Widget.Material.SuggestionButton style, which sets the text color as "?attr/colorAccent". The Messages app uses a ContextThemeWrapper to override colorAccent to be blue, making the buttons display in blue. It's unclear to me why the divider's not showing up in Hangouts, but Hangouts is using a subclass of EditText, and space for the divider is being added even though the divider is not visible, so I think it's highly unlikely there's some other hidden implementation of this menu.
https://codereview.chromium.org/2931443003/diff/1/content/browser/BUILD.gn File content/browser/BUILD.gn (right): https://codereview.chromium.org/2931443003/diff/1/content/browser/BUILD.gn#ne... content/browser/BUILD.gn:293: "android/text_suggestion_host_impl.cc", Should these be in the in_android() selection of the BUILD.gn file? I'm confused why there are so many android/ files that are apparently included on all platforms.
The CQ bit was checked by rlanday@chromium.org to run a CQ dry run
Updated in response to yosin@'s comments
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_...)
yosin@chromium.org changed reviewers: + chongz@chromium.org
+chongz@ for introducing "deleteByComposition" and use it in TextSuggestionController. https://codereview.chromium.org/2931443003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/SelectionController.cpp (right): https://codereview.chromium.org/2931443003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/SelectionController.cpp:283: nit: we don't need to have an extra blank line. https://codereview.chromium.org/2931443003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/SelectionController.cpp:285: nit: we don't need to have an extra blank line. https://codereview.chromium.org/2931443003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/spellcheck/SpellChecker.cpp (right): https://codereview.chromium.org/2931443003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/spellcheck/SpellChecker.cpp:814: Could you move spell checker changes into another patch to make patch size smaller? Since these changes have test cases, we can commit itself. https://codereview.chromium.org/2931443003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/suggestion/TextSuggestionController.cpp (right): https://codereview.chromium.org/2931443003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/suggestion/TextSuggestionController.cpp:122: "", false, false, InputEvent::InputType::kDeleteByComposition); Can we use DeleteSelectionCommand instead of ReplaceSelectionWithText? Do we really need to introduce "deleteByComposition" which we don't want to introduce[1]. Is it better to use "deleteContent" or "insertReplacementText" as spellchecker? +chongz@ to discuss about "InputType" [1] http://crbug.com/652397 ⚐ InputEvent: Support 4 IME related events https://codereview.chromium.org/2931443003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/suggestion/TextSuggestionController.cpp:161: Color(SK_ColorRED).CombineWithAlpha(kSuggestionUnderlineAlphaMultiplier)); Could you move color of marker to InlineTextBoxPainter.cpp, see RecordMarker? We should take these colors from "::spelling-error"[1] or "::grammar-error"[1] and taking default value from LayoutTheme, e.g. LayoutObject::SelectionBackgroundColor() which uses LayoutTheme::GetTheme().ActiveSelectionBackgroundColor(). "core/editing" isn't a good place to specify colors. sk_sp<PaintRecord> RecordMarker(DocumentMarker::MarkerType marker_type) { SkColor color = (marker_type == DocumentMarker::kGrammar) ? SkColorSetRGB(0x6B, 0x6B, 0x6B) : SkColorSetRGB(0xFB, 0x2D, 0x1D); [1] https://drafts.csswg.org/css-pseudo/#highlight-selectors
On 2017/06/08 at 01:50:09, yosin wrote: https://codereview.chromium.org/2931443003/diff/20001/third_party/WebKit/Sour... > Is it better to use "deleteContent" or "insertReplacementText" as spellchecker? The spec says deleteContent should be used "If the user expresses an intention to delete the current, non-collapsed selection". I don't think this would be correct since we don't actually select the highlighted range. I think insertReplacementText is also incorrect since we're not actually inserting any replacement text for a delete operation. Note: it doesn't actually matter for the purposes of implementing the spellcheck menu which event we use, I'm just worried we'll eventually confuse someone's code if we use the wrong one. https://codereview.chromium.org/2931443003/diff/20001/third_party/WebKit/Sour... > third_party/WebKit/Source/core/editing/suggestion/TextSuggestionController.cpp:161: Color(SK_ColorRED).CombineWithAlpha(kSuggestionUnderlineAlphaMultiplier)); > Could you move color of marker to InlineTextBoxPainter.cpp, see RecordMarker? > > We should take these colors from "::spelling-error"[1] or "::grammar-error"[1] and > taking default value from LayoutTheme, e.g. LayoutObject::SelectionBackgroundColor() > which uses LayoutTheme::GetTheme().ActiveSelectionBackgroundColor(). > > "core/editing" isn't a good place to specify colors. This marker is eventually going to be used for SuggestionSpans in addition to spellcheck. For SuggestionSpans we'll want to highlight in gray instead of red. So alternative approaches we could take vs. storing the color on the marker are: - Have two marker types (so we can use different colors) for highlighting the active suggestion region, one for spellcheck and one for SuggestionSpan - Have a flag on ActiveSuggestionMarker that says whether it's for spellcheck or a SuggestionSpan, then have InlineTextBoxPainter read the flag I think SuggestionSpan is only ever going to be painted in a single color (maybe two if we add support for FLAG_EASY_CORRECT)? So perhaps we want to backtrack on the StyleableMarker stuff I did and just have InlineTextBoxPainter pick the colors?
On 2017/06/08 at 03:20:43, rlanday wrote: > On 2017/06/08 at 01:50:09, yosin wrote: > https://codereview.chromium.org/2931443003/diff/20001/third_party/WebKit/Sour... > > Is it better to use "deleteContent" or "insertReplacementText" as spellchecker? > > The spec says deleteContent should be used "If the user expresses an intention to delete the current, non-collapsed selection". I don't think this would be correct since we don't actually select the highlighted range. > > I think insertReplacementText is also incorrect since we're not actually inserting any replacement text for a delete operation. > > Note: it doesn't actually matter for the purposes of implementing the spellcheck menu which event we use, I'm just worried we'll eventually confuse someone's code if we use the wrong one. > > https://codereview.chromium.org/2931443003/diff/20001/third_party/WebKit/Sour... > > third_party/WebKit/Source/core/editing/suggestion/TextSuggestionController.cpp:161: Color(SK_ColorRED).CombineWithAlpha(kSuggestionUnderlineAlphaMultiplier)); > > Could you move color of marker to InlineTextBoxPainter.cpp, see RecordMarker? > > > > We should take these colors from "::spelling-error"[1] or "::grammar-error"[1] and > > taking default value from LayoutTheme, e.g. LayoutObject::SelectionBackgroundColor() > > which uses LayoutTheme::GetTheme().ActiveSelectionBackgroundColor(). > > > > "core/editing" isn't a good place to specify colors. > > This marker is eventually going to be used for SuggestionSpans in addition to spellcheck. For SuggestionSpans we'll want to highlight in gray instead of red. So alternative approaches we could take vs. storing the color on the marker are: > - Have two marker types (so we can use different colors) for highlighting the active suggestion region, one for spellcheck and one for SuggestionSpan > - Have a flag on ActiveSuggestionMarker that says whether it's for spellcheck or a SuggestionSpan, then have InlineTextBoxPainter read the flag > > I think SuggestionSpan is only ever going to be painted in a single color (maybe two if we add support for FLAG_EASY_CORRECT)? So perhaps we want to backtrack on the StyleableMarker stuff I did and just have InlineTextBoxPainter pick the colors? Can we specify the color from Android side?
https://codereview.chromium.org/2931443003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/suggestion/TextSuggestionController.cpp (right): https://codereview.chromium.org/2931443003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/suggestion/TextSuggestionController.cpp:153: const String& misspelled_word = marker_range->GetText(); Please use PlaintText() instead of using temporary Range.
On 2017/06/08 at 05:05:14, yosin wrote: > On 2017/06/08 at 03:20:43, rlanday wrote: > > I think SuggestionSpan is only ever going to be painted in a single color (maybe two if we add support for FLAG_EASY_CORRECT)? So perhaps we want to backtrack on the StyleableMarker stuff I did and just have InlineTextBoxPainter pick the colors? > > Can we specify the color from Android side? There's some asymmetry here in that we can specify the color of the SuggestionMarker underline from Android, but spellcheck markers currently do not store a color. We need ActiveSuggestionMarker to support both cases (or else introduce a second type of marker, which I think otherwise would not be necessary).
https://codereview.chromium.org/2931443003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/suggestion/TextSuggestionController.cpp (right): https://codereview.chromium.org/2931443003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/suggestion/TextSuggestionController.cpp:122: "", false, false, InputEvent::InputType::kDeleteByComposition); On 2017/06/08 03:20:43, rlanday wrote: > On 2017/06/08 at 01:50:09, yosin wrote: > https://codereview.chromium.org/2931443003/diff/20001/third_party/WebKit/Sour... > > Is it better to use "deleteContent" or "insertReplacementText" as > spellchecker? > > The spec says deleteContent should be used "If the user expresses an intention > to delete the current, non-collapsed selection". I don't think this would be > correct since we don't actually select the highlighted range. > > I think insertReplacementText is also incorrect since we're not actually > inserting any replacement text for a delete operation. JS can always figure it out from the empty |data| field. > > Note: it doesn't actually matter for the purposes of implementing the spellcheck > menu which event we use, I'm just worried we'll eventually confuse someone's > code if we use the wrong one. IMO we should use "insertReplacementText" here as it carries more information about request source, but you can argue that this is a standalone feature (separated from spellchecker) and should use "deleteContent". (assuming we are not talking about IME). We use "insertReplacementText" for UA/spellchecker initiated requests (e.g. User didn't type these characters), e.g. 1. Chrome desktop's spellchecker in Context Menu 2. Safari's auto-correction (e.g. Safari will replace 'helo' with 'help' automatically) 3. Other Context Menu actions such as Substitution, Make Upper Case, etc. And Web Apps can disable or do something different to these kind of requests. On the other side "deleteContent" was introduced to cover deletions that cannot be classified elsewhere, e.g. 1. macOS "Menu Bar Edit -> Delete": It has no direction/granularity, not from a spellchecker, and user directly issued 'delete' command. We shouldn't use "deleteByComposition" as it has special usage for IME re-composition.
https://codereview.chromium.org/2931443003/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/suggestion/TextSuggestionController.cpp (right): https://codereview.chromium.org/2931443003/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/suggestion/TextSuggestionController.cpp:122: "", false, false, InputEvent::InputType::kDeleteByComposition); On 2017/06/08 at 01:50:09, yosin_UTC9 wrote: > Can we use DeleteSelectionCommand instead of ReplaceSelectionWithText? Do you want me to use DeleteSelectionCommand directly or to call Editor::DeleteWithDirection()/DeleteSelectionWithSmartDelete()? Looking at where DeleteSelectionCommand is currently created, it seems odd to use it directly.
On 2017/06/08 at 01:50:09, yosin wrote: > https://codereview.chromium.org/2931443003/diff/20001/third_party/WebKit/Sour... > third_party/WebKit/Source/core/editing/suggestion/TextSuggestionController.cpp:161: Color(SK_ColorRED).CombineWithAlpha(kSuggestionUnderlineAlphaMultiplier)); > Could you move color of marker to InlineTextBoxPainter.cpp, see RecordMarker? > > We should take these colors from "::spelling-error"[1] or "::grammar-error"[1] and > taking default value from LayoutTheme, e.g. LayoutObject::SelectionBackgroundColor() > which uses LayoutTheme::GetTheme().ActiveSelectionBackgroundColor(). > > "core/editing" isn't a good place to specify colors. > > > sk_sp<PaintRecord> RecordMarker(DocumentMarker::MarkerType marker_type) { > SkColor color = (marker_type == DocumentMarker::kGrammar) > ? SkColorSetRGB(0x6B, 0x6B, 0x6B) > : SkColorSetRGB(0xFB, 0x2D, 0x1D); > > > [1] https://drafts.csswg.org/css-pseudo/#highlight-selectors I think I did just copy the color from InlineTextBoxPainter.cpp...maybe we should do a bit of refactoring so the color is accessible in other pieces of code as well? What sort of support do we currently have for ::spelling-error and ::grammar-error? Based on the painting code (which appears to use hard-coded colors), we don't actually let websites change the underline colors, right? I don't think the highlight color we want here has anything to do with the default selection color (it's not a selection, and in a native Android text box it doesn't use the selection color).
The CQ bit was checked by rlanday@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...
I've split off the SpellChecker changes into https://codereview.chromium.org/2925363002 and addressed the easier comments. It does appear that we don't currently have any support in Chrome for ::spelling-error or ::grammar-error: http://jsbin.com/deliqasopu/edit?html,output but I suppose if we do add support at some point, and let web developers restyle the squiggly underlines, we'd want to paint the ActiveSuggestionMarker based on that color, so I guess it does make sense to keep storing the color on the marker. I think for now we should just stick a helper method somewhere to get the correct misspelling underline color for the current platform.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: 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...)
On 2017/06/08 at 22:58:21, rlanday wrote: > I've split off the SpellChecker changes into https://codereview.chromium.org/2925363002 and addressed the easier comments. > > It does appear that we don't currently have any support in Chrome for ::spelling-error or ::grammar-error: > http://jsbin.com/deliqasopu/edit?html,output > > but I suppose if we do add support at some point, and let web developers restyle the squiggly underlines, we'd want to paint the ActiveSuggestionMarker based on that color, so I guess it does make sense to keep storing the color on the marker. I think for now we should just stick a helper method somewhere to get the correct misspelling underline color for the current platform. ::spelling-error and ::grammar-error aren't implemented yet. They will be implemented in future. Let's use LayoutTheme as selection color/background. It is the place to provide default value for platforms.
https://codereview.chromium.org/2931443003/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/suggestion/TextSuggestionController.cpp (right): https://codereview.chromium.org/2931443003/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/suggestion/TextSuggestionController.cpp:122: "", false, false, InputEvent::InputType::kInsertReplacementText); I can't find where 'beforeinput' was fired, is it possible to do something similar to |SpellChecker::ReplaceMisspelledRange()|? https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/editing/s...
The CQ bit was checked by rlanday@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...
Updated https://codereview.chromium.org/2931443003/diff/60001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/editing/suggestion/TextSuggestionController.cpp (right): https://codereview.chromium.org/2931443003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/suggestion/TextSuggestionController.cpp:141: GetFrame().GetDocument()->UpdateStyleAndLayoutIgnorePendingStylesheets(); Do we need this? SpellChecker::ReplaceMisspelledRange() calls this but I don't understand why. https://codereview.chromium.org/2931443003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/suggestion/TextSuggestionController.cpp:145: GetFrame().GetEditor().ReplaceSelectionWithText( Ok, I've copied all this code from SpellChecker::ReplaceMisspelledRange() as chongz@ requested. Do we still want to use a delete Command/method instead of ReplaceSelectionWithText()? https://codereview.chromium.org/2931443003/diff/60001/third_party/WebKit/Sour... third_party/WebKit/Source/core/editing/suggestion/TextSuggestionController.cpp:183: LayoutTheme::GetTheme().SpellingMarkerUnderlineColor().CombineWithAlpha( I'm moving the spelling underline color to LayoutTheme in https://codereview.chromium.org/2932913002/.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: 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...)
The CQ bit was checked by rlanday@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...)
The CQ bit was checked by rlanday@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_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
https://codereview.chromium.org/2931443003/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/suggestion/TextSuggestionBackendImpl.h (right): https://codereview.chromium.org/2931443003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/suggestion/TextSuggestionBackendImpl.h:24: void ApplySpellCheckSuggestion(const WTF::String& suggestion) override; nit: %s/WTF::String/String/ Since WTFString.h has |using WTF::String;|, we don't need to have namespace prefix for |String|. https://codereview.chromium.org/2931443003/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/suggestion/TextSuggestionController.cpp (right): https://codereview.chromium.org/2931443003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/suggestion/TextSuggestionController.cpp:119: GetFrame().Selection().SetSelection( FrameSelection::SetSelection() can make current frame gone. We should check whether current frame available or not after FS::SetSelection() like L136 for after dispatching "beforeinput" https://codereview.chromium.org/2931443003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/suggestion/TextSuggestionController.cpp:184: .PlatformSpellingMarkerUnderlineColor() Could you introduce |PlatformActiveSuggestionUnderlineColor()|? to move |kSuggestionUnderlineAlphaMultiplier| to platform specific part? Note: Color[1] can have RGBA32. BTW, do we really use alpha != 1.0? I guess painting alpha=1.0 is faster than alpha != 1.0. [1] https://cs.chromium.org/chromium/src/third_party/WebKit/Source/platform/graph...
https://codereview.chromium.org/2931443003/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/suggestion/TextSuggestionController.cpp (right): https://codereview.chromium.org/2931443003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/suggestion/TextSuggestionController.cpp:184: .PlatformSpellingMarkerUnderlineColor() On 2017/06/13 at 06:29:31, yosin_UTC9 wrote: > Could you introduce |PlatformActiveSuggestionUnderlineColor()|? > to move |kSuggestionUnderlineAlphaMultiplier| to platform specific part? > > Note: Color[1] can have RGBA32. > > BTW, do we really use alpha != 1.0? I guess painting alpha=1.0 is faster than alpha != 1.0. > > > [1] https://cs.chromium.org/chromium/src/third_party/WebKit/Source/platform/graph... I'm basically porting over the code Android uses to pick the background color: https://android.googlesource.com/platform/frameworks/base/+/master/core/java/... I think the reason it's set up the way it is (the alpha is added separately) is because the SuggestionSpan underline color can be either gray (for a Voice IME suggestion) or red (for a spellcheck suggestion), and then we just add some transparency to get the highlight color (so we do use alpha != 1.0). So we can either end up with: PlatformSpellingMarkerUnderlineColor() PlatformSpellingMarkerHighlightColor() PlatformActiveSuggestionUnderlineColor() PlatformActiveSuggestionHighlightColor() or we can have: PlatformSpellingMarkerUnderlineColor() PlatformActiveSuggestionUnderlineColor() and stick the 0.4 alpha multiplier constant somewhere (this might be a little awkward). I guess it's probably better to just have the four colors?
https://codereview.chromium.org/2931443003/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/suggestion/TextSuggestionController.cpp (right): https://codereview.chromium.org/2931443003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/suggestion/TextSuggestionController.cpp:184: .PlatformSpellingMarkerUnderlineColor() On 2017/06/13 at 06:59:27, rlanday wrote: > On 2017/06/13 at 06:29:31, yosin_UTC9 wrote: > > Could you introduce |PlatformActiveSuggestionUnderlineColor()|? > > to move |kSuggestionUnderlineAlphaMultiplier| to platform specific part? > > > > Note: Color[1] can have RGBA32. > > > > BTW, do we really use alpha != 1.0? I guess painting alpha=1.0 is faster than alpha != 1.0. > > > > > > [1] https://cs.chromium.org/chromium/src/third_party/WebKit/Source/platform/graph... > > I'm basically porting over the code Android uses to pick the background color: > https://android.googlesource.com/platform/frameworks/base/+/master/core/java/... > > I think the reason it's set up the way it is (the alpha is added separately) is because the SuggestionSpan underline color can be either gray (for a Voice IME suggestion) or red (for a spellcheck suggestion), and then we just add some transparency to get the highlight color (so we do use alpha != 1.0). > > So we can either end up with: > > PlatformSpellingMarkerUnderlineColor() > PlatformSpellingMarkerHighlightColor() > PlatformActiveSuggestionUnderlineColor() > PlatformActiveSuggestionHighlightColor() > > or we can have: > > PlatformSpellingMarkerUnderlineColor() > PlatformActiveSuggestionUnderlineColor() > > and stick the 0.4 alpha multiplier constant somewhere (this might be a little awkward). I guess it's probably better to just have the four colors? Let's provide four colors in LayoutTheme to provide freedom of design to UX designer. Other platform UX designer, may want to use different color scheme.
xiaochengh@chromium.org changed reviewers: + xiaochengh@chromium.org
https://codereview.chromium.org/2931443003/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/suggestion/TextSuggestionController.cpp (right): https://codereview.chromium.org/2931443003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/suggestion/TextSuggestionController.cpp:57: namespace { nit: Shouldn't have two anonymous namespaces. https://codereview.chromium.org/2931443003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/suggestion/TextSuggestionController.cpp:76: TextIteratorBehavior::Builder().SetEmitsSpaceForNbsp(true).Build()); |EmitsSpaceForNbsp| worsens the performance, as it needs to copy and modify the string from LayoutText: https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/editing/i... I think it's better to check space and nbsp explicitly. It does't make code more complicated. https://codereview.chromium.org/2931443003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/suggestion/TextSuggestionController.cpp:95: TextIteratorBehavior::Builder().SetEmitsSpaceForNbsp(true).Build()); Ditto. https://codereview.chromium.org/2931443003/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/suggestion/TextSuggestionController.cpp:207: void TextSuggestionController::SuggestionMenuClosed() { The frame may be already detached when called from ApplySpellCheckSuggestion() and DeleteActiveSuggestionRange(). We should check that before accessing DMC and FrameSelection.
The CQ bit was checked by rlanday@chromium.org to run a CQ dry run
Updated
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_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
The CQ bit was checked by rlanday@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_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...) android_cronet on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...) chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...) mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
The CQ bit was checked by rlanday@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.
https://codereview.chromium.org/2931443003/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/suggestion/TextSuggestionController.cpp (right): https://codereview.chromium.org/2931443003/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/suggestion/TextSuggestionController.cpp:206: if (!frame_) This doesn't work because |frame_| won't be set to nullptr when document is detached. How about making TextSuggestionController a SynchronousMutationObserver, so that we can check document availability by checking SMO::LifecycleContext()?
On 2017/06/14 at 00:44:38, xiaochengh wrote: > https://codereview.chromium.org/2931443003/diff/160001/third_party/WebKit/Sou... > File third_party/WebKit/Source/core/editing/suggestion/TextSuggestionController.cpp (right): > > https://codereview.chromium.org/2931443003/diff/160001/third_party/WebKit/Sou... > third_party/WebKit/Source/core/editing/suggestion/TextSuggestionController.cpp:206: if (!frame_) > This doesn't work because |frame_| won't be set to nullptr when document is detached. > > How about making TextSuggestionController a SynchronousMutationObserver, so that we can check document availability by checking SMO::LifecycleContext()? I think I must've been falling asleep at the wheel when I wrote that...what does it mean that the frame is detached? The LocalFrame itself can't go away since we still have a Member pointing at it, right?
On 2017/06/14 at 00:50:44, rlanday wrote: > On 2017/06/14 at 00:44:38, xiaochengh wrote: > > https://codereview.chromium.org/2931443003/diff/160001/third_party/WebKit/Sou... > > File third_party/WebKit/Source/core/editing/suggestion/TextSuggestionController.cpp (right): > > > > https://codereview.chromium.org/2931443003/diff/160001/third_party/WebKit/Sou... > > third_party/WebKit/Source/core/editing/suggestion/TextSuggestionController.cpp:206: if (!frame_) > > This doesn't work because |frame_| won't be set to nullptr when document is detached. > > > > How about making TextSuggestionController a SynchronousMutationObserver, so that we can check document availability by checking SMO::LifecycleContext()? > > I think I must've been falling asleep at the wheel when I wrote that...what does it mean that the frame is detached? The LocalFrame itself can't go away since we still have a Member pointing at it, right? It means LocalFrame::Detach() is called. Think about the following scenario: 0. Everything happens in an <iframe> 1. TextSuggestionController::ApplySpellCheckSuggestion() is called, which replaces the selection with some other text 2. The above replacement triggers an event handler, which removes the <iframe> from its parent frame 3. If we still try to access DMC or FrameSelection, we crash Step 2 is what we call "frame is detached"
https://codereview.chromium.org/2931443003/diff/160001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/suggestion/TextSuggestionController.cpp (right): https://codereview.chromium.org/2931443003/diff/160001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/suggestion/TextSuggestionController.cpp:206: if (!frame_) On 2017/06/14 at 00:44:38, Xiaocheng wrote: > This doesn't work because |frame_| won't be set to nullptr when document is detached. > > How about making TextSuggestionController a SynchronousMutationObserver, so that we can check document availability by checking SMO::LifecycleContext()? It is good idea to observe Document shutdown as SMO. However, SMO is performance sensitive since SMO is called during Node#appendChild() and removeChild(). It is better to introduce DocumentShutdown{Observer,Notifier}. Note: We can't make SMO to be derived from DocumentShutdownObserver because we can't share observer list in DocumentShutdownNotifier and SynchronousMutationNotifier.
On 2017/06/14 at 01:17:04, yosin wrote: > https://codereview.chromium.org/2931443003/diff/160001/third_party/WebKit/Sou... > File third_party/WebKit/Source/core/editing/suggestion/TextSuggestionController.cpp (right): > > https://codereview.chromium.org/2931443003/diff/160001/third_party/WebKit/Sou... > third_party/WebKit/Source/core/editing/suggestion/TextSuggestionController.cpp:206: if (!frame_) > On 2017/06/14 at 00:44:38, Xiaocheng wrote: > > This doesn't work because |frame_| won't be set to nullptr when document is detached. > > > > How about making TextSuggestionController a SynchronousMutationObserver, so that we can check document availability by checking SMO::LifecycleContext()? > > It is good idea to observe Document shutdown as SMO. However, SMO is performance sensitive since SMO is called during Node#appendChild() and removeChild(). > It is better to introduce DocumentShutdown{Observer,Notifier}. Note: We can't make SMO to be derived from DocumentShutdownObserver because we can't share > observer list in DocumentShutdownNotifier and SynchronousMutationNotifier. To clarify here, we have to explicitly call SetContext() with the Document* at some point, right? Is it safe to do this in the TextSuggestionController constructor? We construct TextSuggestionController while LocalFrame is still being constructed.
On 2017/06/14 at 01:22:39, rlanday wrote: > On 2017/06/14 at 01:17:04, yosin wrote: > > https://codereview.chromium.org/2931443003/diff/160001/third_party/WebKit/Sou... > > File third_party/WebKit/Source/core/editing/suggestion/TextSuggestionController.cpp (right): > > > > https://codereview.chromium.org/2931443003/diff/160001/third_party/WebKit/Sou... > > third_party/WebKit/Source/core/editing/suggestion/TextSuggestionController.cpp:206: if (!frame_) > > On 2017/06/14 at 00:44:38, Xiaocheng wrote: > > > This doesn't work because |frame_| won't be set to nullptr when document is detached. > > > > > > How about making TextSuggestionController a SynchronousMutationObserver, so that we can check document availability by checking SMO::LifecycleContext()? > > > > It is good idea to observe Document shutdown as SMO. However, SMO is performance sensitive since SMO is called during Node#appendChild() and removeChild(). > > It is better to introduce DocumentShutdown{Observer,Notifier}. Note: We can't make SMO to be derived from DocumentShutdownObserver because we can't share > > observer list in DocumentShutdownNotifier and SynchronousMutationNotifier. > > To clarify here, we have to explicitly call SetContext() with the Document* at some point, right? Is it safe to do this in the TextSuggestionController constructor? We construct TextSuggestionController while LocalFrame is still being constructed. Please set document in LocalFrame::DocumentAttached() as FrameSelection, Editor, etc.
The CQ bit was checked by rlanday@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...
Ok, I've added DocumentShutdown{Observer,Notifier} in
https://codereview.chromium.org/2939223003 and rebased this CL on top.
On an unrelated note, I've noticed that the Android spellchecker is putting
zero-width space characters (\u200b) at the end of some suggestions, and this is
causing odd behavior when applying replacements. E.g., composition underlines
that don't go away; if I change CompositionMarkerListImpl to use the
content-dependent ShiftMarkers() method, the behavior is still broken, but in a
different way: if I apply a two-word replacement ending in \u200b, and then try
to tap on the second word, a composition underline doesn't appear.
I suppose the best way to handle this is to filter out these characters when we
get the suggestions from the Android spellcheck?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-device on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...) ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)
It seems fine to me to filter ZWS from suggestion results. Just curious, why does android spellchecker add ZWS? To separate different suggestions? https://codereview.chromium.org/2931443003/diff/200001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/suggestion/TextSuggestionController.cpp (right): https://codereview.chromium.org/2931443003/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/suggestion/TextSuggestionController.cpp:125: if (!LifecycleContext()) s/LifecycleContext/IsAvailable/ since it's easier to understand the meaning of availability. https://codereview.chromium.org/2931443003/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/suggestion/TextSuggestionController.cpp:140: if (!LifecycleContext()) Ditto. https://codereview.chromium.org/2931443003/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/suggestion/TextSuggestionController.cpp:210: if (!LifecycleContext()) Ditto. https://codereview.chromium.org/2931443003/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/suggestion/TextSuggestionController.cpp:221: return *GetFrame().GetDocument(); Should return |*LifecycleContent()| instead. https://codereview.chromium.org/2931443003/diff/200001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/suggestion/TextSuggestionController.cpp:225: return GetFrame().GetDocument(); Should check |LifecycleContent()| instead.
The CQ bit was checked by rlanday@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...
On 2017/06/16 at 00:23:33, xiaochengh wrote: > It seems fine to me to filter ZWS from suggestion results. > > Just curious, why does android spellchecker add ZWS? To separate different suggestions? I put up a CL to do this here: https://codereview.chromium.org/2943033002 I'm not sure why it happens. The spellchecker provides the suggestions in an array, so there's no need to use this character to separate them. > https://codereview.chromium.org/2931443003/diff/200001/third_party/WebKit/Sou... > File third_party/WebKit/Source/core/editing/suggestion/TextSuggestionController.cpp (right): > > https://codereview.chromium.org/2931443003/diff/200001/third_party/WebKit/Sou... > third_party/WebKit/Source/core/editing/suggestion/TextSuggestionController.cpp:125: if (!LifecycleContext()) > s/LifecycleContext/IsAvailable/ since it's easier to understand the meaning of availability. > > https://codereview.chromium.org/2931443003/diff/200001/third_party/WebKit/Sou... > third_party/WebKit/Source/core/editing/suggestion/TextSuggestionController.cpp:140: if (!LifecycleContext()) > Ditto. > > https://codereview.chromium.org/2931443003/diff/200001/third_party/WebKit/Sou... > third_party/WebKit/Source/core/editing/suggestion/TextSuggestionController.cpp:210: if (!LifecycleContext()) > Ditto. > > https://codereview.chromium.org/2931443003/diff/200001/third_party/WebKit/Sou... > third_party/WebKit/Source/core/editing/suggestion/TextSuggestionController.cpp:221: return *GetFrame().GetDocument(); > Should return |*LifecycleContent()| instead. > > https://codereview.chromium.org/2931443003/diff/200001/third_party/WebKit/Sou... > third_party/WebKit/Source/core/editing/suggestion/TextSuggestionController.cpp:225: return GetFrame().GetDocument(); > Should check |LifecycleContent()| instead. Updated.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: 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 rlanday@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_headless_rel 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_...)
Code shape of Blink part seems to be OK. To move forward, please get "looks good to me" for Mojo API. Because Blink parts depend of design of Mojo API, when Mojo API is changed Blink part also needs to be changed. https://codereview.chromium.org/2931443003/diff/240001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/suggestion/TextSuggestionBackendImpl.h (right): https://codereview.chromium.org/2931443003/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/suggestion/TextSuggestionBackendImpl.h:20: explicit TextSuggestionBackendImpl(LocalFrame&); Can we make ctor to private since we have |Create| function? Is this for testing purpose? https://codereview.chromium.org/2931443003/diff/240001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/suggestion/TextSuggestionController.h (right): https://codereview.chromium.org/2931443003/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/suggestion/TextSuggestionController.h:24: WTF_MAKE_NONCOPYABLE(TextSuggestionController); Let's use DISALLOW_COPY_AND_ASSIGN()
https://codereview.chromium.org/2931443003/diff/240001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/input/ImeAdapter.java (right): https://codereview.chromium.org/2931443003/diff/240001/content/public/android... content/public/android/java/src/org/chromium/content/browser/input/ImeAdapter.java:928: public float getContentOffsetYPix() { Please add the offset to caretY when calling "show" instead of making it available here. You can apply density multiplication from here instead of within SuggestionsPopupWindow (it's also available from RenderCoordinates). https://codereview.chromium.org/2931443003/diff/240001/content/public/android... content/public/android/java/src/org/chromium/content/browser/input/ImeAdapter.java:932: public void applySpellCheckSuggestion(String suggestion) { If this goes in a big class, it should look like an interface SuggestionPopupMenuClient that ImeAdapter implements, and pass that instead of making SuggestionPopupMenu directly aware of ImeAdapter. But, looking closely, I don't see any dependencies on ImeAdapter. Can you avoid ImeAdapter entirely and create a Java-side class for SuggestionHost? ImeAdapter will only need to be involved when it comes to plumbing through SuggestionSpans in a later patch (and I think it should still remain pretty isolated). https://codereview.chromium.org/2931443003/diff/240001/content/public/android... content/public/android/java/src/org/chromium/content/browser/input/ImeAdapter.java:944: public void suggestionMenuClosed() { Can we set mSuggestionPopupMenu = null from here or whenever it would be destroyed? I'm concerned about the persistent refs to the Context, View etc. https://codereview.chromium.org/2931443003/diff/240001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/input/SuggestionsPopupWindow.java (right): https://codereview.chromium.org/2931443003/diff/240001/content/public/android... content/public/android/java/src/org/chromium/content/browser/input/SuggestionsPopupWindow.java:62: CursorAnchorInfoController cursorAnchorInfoController) { cursorAnchorInfoController is unused, please delete it.
https://codereview.chromium.org/2931443003/diff/240001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/suggestion/TextSuggestionBackendImpl.h (right): https://codereview.chromium.org/2931443003/diff/240001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/suggestion/TextSuggestionBackendImpl.h:20: explicit TextSuggestionBackendImpl(LocalFrame&); On 2017/06/16 at 01:21:04, yosin_UTC9 wrote: > Can we make ctor to private since we have |Create| function? > Is this for testing purpose? We can't use MakeUnique() inside Create() if we make the constructor private, but maybe we should do it anyway?
https://codereview.chromium.org/2931443003/diff/240001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/input/ImeAdapter.java (right): https://codereview.chromium.org/2931443003/diff/240001/content/public/android... content/public/android/java/src/org/chromium/content/browser/input/ImeAdapter.java:932: public void applySpellCheckSuggestion(String suggestion) { On 2017/06/16 at 01:25:14, aelias wrote: > If this goes in a big class, it should look like an interface SuggestionPopupMenuClient that ImeAdapter implements, and pass that instead of making SuggestionPopupMenu directly aware of ImeAdapter. > > But, looking closely, I don't see any dependencies on ImeAdapter. Can you avoid ImeAdapter entirely and create a Java-side class for SuggestionHost? ImeAdapter will only need to be involved when it comes to plumbing through SuggestionSpans in a later patch (and I think it should still remain pretty isolated). Sure, we can do this; I was thinking about how to do this and I was hoping we could have a single TextSuggestionHost class, but I think we actually need two: one to be constructed from the Java TextSuggestionHost (I think we do need to construct the C++ class from the Java class, since if we go the other way around, I don't know how we would get the necessary information to display the menu), and then a second class that actually implements the Mojo interface (since mojo::MakeStrongBinding() takes a unique_ptr; if we try to use the same class, I think we would have to pass "this" and give Mojo ownership over the TextSuggestionHost class, which is problematic because it means we would leak memory if we never actually actually make the Mojo binding). Maybe a Mojo expert can come up with a better way later, but this is what I'm going to do for now.
The CQ bit was checked by rlanday@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...
Updated with aelias@'s requested changes. Here is an updated list of files for each OWNER reviewer: yosin@: third_party/WebKit/Source/core/editing/BUILD.gn third_party/WebKit/Source/core/editing/SelectionController.cpp third_party/WebKit/Source/core/editing/suggestion/TextSuggestionBackendImpl.cpp third_party/WebKit/Source/core/editing/suggestion/TextSuggestionBackendImpl.h third_party/WebKit/Source/core/editing/suggestion/TextSuggestionController.cpp third_party/WebKit/Source/core/editing/suggestion/TextSuggestionController.h third_party/WebKit/Source/core/editing/suggestion/TextSuggestionControllerTest.cpp mkwst@: chrome/browser/chrome_content_browser_manifest_overlay.json content/public/app/mojo/content_browser_manifest.json content/public/app/mojo/content_renderer_manifest.json third_party/WebKit/Source/core/frame/LocalFrame.cpp third_party/WebKit/Source/core/frame/LocalFrame.h third_party/WebKit/Source/core/layout/LayoutTheme.cpp third_party/WebKit/Source/core/layout/LayoutTheme.h third_party/WebKit/Source/modules/ModulesInitializer.cpp third_party/WebKit/public/platform/input_host.mojom third_party/WebKit/public/platform/input_messages.mojom aelias@: content/browser/renderer_host/render_widget_host_view_android.cc content/browser/renderer_host/render_widget_host_view_android.h) content/public/android/BUILD.gn content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java content/public/android/java/src/org/chromium/content/browser/input/InputMethodManagerWrapper.java content/public/android/java/src/org/chromium/content/browser/input/SuggestionsPopupWindow.java content/public/android/java/src/org/chromium/content/browser/input/TextSuggestionHost.java) content/public/android/java/strings/android_content_strings.grd content/public/android/javatests/src/org/chromium/content/browser/PopupZoomerTest.java tedchoc@: content/browser/android/browser_jni_registrar.cc content/browser/android/text_suggestion_host_impl.cc content/browser/android/text_suggestion_host_impl.h jochen@: android_webview/browser/aw_browser_manifest_overlay.json content/browser/BUILD.gn content/browser/DEPS
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_...)
https://codereview.chromium.org/2931443003/diff/260001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/SelectionController.cpp (right): https://codereview.chromium.org/2931443003/diff/260001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/SelectionController.cpp:283: frame_->GetTextSuggestionController().HandlePotentialMisspelledWordTap(); Do we really want to ask spell checker for every single click? This change makes mouse handling slow down. I think we should stop calling |HandlePotentialMisspelledWordTap()| when "selectstart" is canceled which is checked in |UpdateSelectionForMouseDownDispatchingSelectStart()| just before this call. Can we move this to idle callback like idle time spell checker? https://codereview.chromium.org/2931443003/diff/260001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/suggestion/TextSuggestionBackendImpl.h (right): https://codereview.chromium.org/2931443003/diff/260001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/suggestion/TextSuggestionBackendImpl.h:32: }; Can we have DISALLOW_COPY_AND_ASSIGN()? https://codereview.chromium.org/2931443003/diff/260001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/suggestion/TextSuggestionController.cpp (right): https://codereview.chromium.org/2931443003/diff/260001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/suggestion/TextSuggestionController.cpp:22: bool ShouldDeleteNextCharacter(const Node* marker_text_node, nit: s/const Node*/const Node&/ https://codereview.chromium.org/2931443003/diff/260001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/suggestion/TextSuggestionController.cpp:23: const DocumentMarker* marker) { nit: s/const DocumentMarker*/const DocumentMarker&/ https://codereview.chromium.org/2931443003/diff/260001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/suggestion/TextSuggestionController.cpp:39: UChar next_character = next_character_str[0]; nit: s/UChar/const UChar/ https://codereview.chromium.org/2931443003/diff/260001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/suggestion/TextSuggestionController.cpp:61: UChar prev_character = prev_character_str[0]; nit: s/UChar/const UChar/ https://codereview.chromium.org/2931443003/diff/260001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/suggestion/TextSuggestionController.cpp:135: const bool cancel = DispatchBeforeInputDataTransfer( nit: s/cancel/is_cancel/ See Precede boolean values with words like "is" and "did" in [1] [1] https://docs.google.com/document/d/1ZQLiQKw9hCOyDDhDS4JhckhuiJmLqSbw9FLfySMyx... https://codereview.chromium.org/2931443003/diff/260001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/suggestion/TextSuggestionController.h (right): https://codereview.chromium.org/2931443003/diff/260001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/suggestion/TextSuggestionController.h:47: LocalFrame& GetFrame() const { Could you move this to .cpp file for ease of debugging in Visual Studio on Windows? VS is slow when setting break point at inline functions. https://codereview.chromium.org/2931443003/diff/260001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/suggestion/TextSuggestionController.h:54: bool suggestion_menu_is_open_; nit: s/suggetions_menu_is_open_/is_suggestions_menu_open_/ or s/suggetions_menu_is_open_/is_menu_open_/ See "Precede boolean values with words like "is" and "did"" in [1] [1] https://docs.google.com/document/d/1ZQLiQKw9hCOyDDhDS4JhckhuiJmLqSbw9FLfySMyx... https://codereview.chromium.org/2931443003/diff/260001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/suggestion/TextSuggestionController.h:55: Member<LocalFrame> frame_; nit: s/Member<LocalFrame>/const Member<LocalFrame>/ https://codereview.chromium.org/2931443003/diff/260001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/layout/LayoutTheme.h (right): https://codereview.chromium.org/2931443003/diff/260001/third_party/WebKit/Sou... third_party/WebKit/Source/core/layout/LayoutTheme.h:136: virtual Color PlatformActiveSpellingMarkerHighlightColor() const; It is better to move LayoutTheme.{h,cpp} into another patch to make this patch smaller.
Browser-side lgtm modulo minor comments https://codereview.chromium.org/2931443003/diff/260001/content/browser/androi... File content/browser/android/text_suggestion_host_android.h (right): https://codereview.chromium.org/2931443003/diff/260001/content/browser/androi... content/browser/android/text_suggestion_host_android.h:5: #ifndef TEXT_SUGGESTION_HOST_ANDROID_H_ nit: standard format for this encodes the full path, i.e. CONTENT_BROWSER_ANDROID_TEXT_SUGGESTION_HOST_ANDROID_H_ https://codereview.chromium.org/2931443003/diff/260001/content/browser/androi... File content/browser/android/text_suggestion_host_mojo_impl_android.h (right): https://codereview.chromium.org/2931443003/diff/260001/content/browser/androi... content/browser/android/text_suggestion_host_mojo_impl_android.h:5: #ifndef TEXT_SUGGESTION_HOST_IMPL_ANDROID_H_ nit: standard format for this encodes the full path, i.e. CONTENT_BROWSER_ANDROID_TEXT_SUGGESTION_HOST_MOJO_IMPL_ANDROID_H_ https://codereview.chromium.org/2931443003/diff/260001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/input/InputMethodManagerWrapper.java (right): https://codereview.chromium.org/2931443003/diff/260001/content/public/android... content/public/android/java/src/org/chromium/content/browser/input/InputMethodManagerWrapper.java:40: public Context getContext() { Please delete, now unused https://codereview.chromium.org/2931443003/diff/260001/content/public/android... File content/public/android/java/strings/android_content_strings.grd (right): https://codereview.chromium.org/2931443003/diff/260001/content/public/android... content/public/android/java/strings/android_content_strings.grd:108: <message name="IDS_DELETE_TEXT" desc="Button that deletes the currently highlighted text"> More context is always useful for translators: "IDS_DELETE_SUGGESTION_TEXT" "Button that deletes the currently highlighted word(s) from a spellcheck or voice correction menu"
https://codereview.chromium.org/2931443003/diff/260001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/SelectionController.cpp (right): https://codereview.chromium.org/2931443003/diff/260001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/SelectionController.cpp:283: frame_->GetTextSuggestionController().HandlePotentialMisspelledWordTap(); On 2017/06/20 at 01:46:36, yosin_UTC9 wrote: > Do we really want to ask spell checker for every single click? > This change makes mouse handling slow down. > > I think we should stop calling |HandlePotentialMisspelledWordTap()| when "selectstart" is canceled > which is checked in |UpdateSelectionForMouseDownDispatchingSelectStart()| just before this call. > > Can we move this to idle callback like idle time spell checker? To clarify, we're not calling the spell checker here, we're just checking for the presence of a spelling DocumentMarker. I'm kind of hesitant to make this asynchronous because we want to show the popup menu 300ms (the double-tap threshold) after the tap, and we can't start the timer until after HandlePotentialMisspelledWordTap() runs. So if we make this run in idle-time, the timing will probably get thrown off and the interaction will feel laggy. One tradeoff we can make is, instead of creating the two-character wide selection range on every tap and checking for a DocumentMarker in that range (which requires using TextIterator, which is a somewhat complex operation), we can start the timer on every tap, and only check for the spelling marker if the timer expires. That way, if we get a bunch of touch events with less than 300ms between them, we only have to check for the spelling marker after the last one. What do you think of this idea? The tradeoff would be that we have to reset the timer more often.
https://codereview.chromium.org/2931443003/diff/260001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/SelectionController.cpp (right): https://codereview.chromium.org/2931443003/diff/260001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/SelectionController.cpp:283: frame_->GetTextSuggestionController().HandlePotentialMisspelledWordTap(); On 2017/06/20 at 05:34:21, rlanday wrote: > On 2017/06/20 at 01:46:36, yosin_UTC9 wrote: > > Do we really want to ask spell checker for every single click? > > This change makes mouse handling slow down. > > > > I think we should stop calling |HandlePotentialMisspelledWordTap()| when "selectstart" is canceled > > which is checked in |UpdateSelectionForMouseDownDispatchingSelectStart()| just before this call. > > > > Can we move this to idle callback like idle time spell checker? > > To clarify, we're not calling the spell checker here, we're just checking for the presence of a spelling DocumentMarker. > > I'm kind of hesitant to make this asynchronous because we want to show the popup menu 300ms (the double-tap threshold) after the tap, and we can't start the timer until after HandlePotentialMisspelledWordTap() runs. So if we make this run in idle-time, the timing will probably get thrown off and the interaction will feel laggy. > > One tradeoff we can make is, instead of creating the two-character wide selection range on every tap and checking for a DocumentMarker in that range (which requires using TextIterator, which is a somewhat complex operation), we can start the timer on every tap, and only check for the spelling marker if the timer expires. That way, if we get a bunch of touch events with less than 300ms between them, we only have to check for the spelling marker after the last one. What do you think of this idea? The tradeoff would be that we have to reset the timer more often. If we move the triggering of spellcheck menu to somewhere asynchronous, is it going to introduce weird interaction? For example, after tapping on a misspelled word, the normal selection menu appears, and then (after 300ms) gets replaced by the spellcheck menu? I think we should do some experiments to see the performance impact. It's more convincing to talk about performance with real data. Those long wikipedia articles' editing pages are good benchmarks.
On 2017/06/20 at 05:55:04, xiaochengh wrote: > If we move the triggering of spellcheck menu to somewhere asynchronous, is it going to introduce weird interaction? For example, after tapping on a misspelled word, the normal selection menu appears, and then (after 300ms) gets replaced by the spellcheck menu? > > I think we should do some experiments to see the performance impact. It's more convincing to talk about performance with real data. Those long wikipedia articles' editing pages are good benchmarks. I don't *think* this is an issue, I think because we only show the spellcheck menu after we set a caret selection, in which case we don't show a selection menu. I think making it asynchronous would effectively just make the 300ms longer rather than fundamentally changing the behavior. Actually, I think making it asynchronous doesn't really work well because if we receive a second touch event that should cancel the menu opening before the idle callback runs, we have to cancel the callback from trying to open the menu, and there could be some lag between when the browser code gets the input event and when the message gets passed to the renderer. I think we should do what I suggested in my previous comment (I'll have to double-check if there's any impact on behavior from that change).
On 2017/06/20 at 05:34:22, rlanday wrote: > https://codereview.chromium.org/2931443003/diff/260001/third_party/WebKit/Sou... > File third_party/WebKit/Source/core/editing/SelectionController.cpp (right): > > https://codereview.chromium.org/2931443003/diff/260001/third_party/WebKit/Sou... > third_party/WebKit/Source/core/editing/SelectionController.cpp:283: frame_->GetTextSuggestionController().HandlePotentialMisspelledWordTap(); > On 2017/06/20 at 01:46:36, yosin_UTC9 wrote: > > Do we really want to ask spell checker for every single click? > > This change makes mouse handling slow down. > > > > I think we should stop calling |HandlePotentialMisspelledWordTap()| when "selectstart" is canceled > > which is checked in |UpdateSelectionForMouseDownDispatchingSelectStart()| just before this call. > > > > Can we move this to idle callback like idle time spell checker? > > To clarify, we're not calling the spell checker here, we're just checking for the presence of a spelling DocumentMarker. > > I'm kind of hesitant to make this asynchronous because we want to show the popup menu 300ms (the double-tap threshold) after the tap, and we can't start the timer until after HandlePotentialMisspelledWordTap() runs. So if we make this run in idle-time, the timing will probably get thrown off and the interaction will feel laggy. > > One tradeoff we can make is, instead of creating the two-character wide selection range on every tap and checking for a DocumentMarker in that range (which requires using TextIterator, which is a somewhat complex operation), we can start the timer on every tap, and only check for the spelling marker if the timer expires. That way, if we get a bunch of touch events with less than 300ms between them, we only have to check for the spelling marker after the last one. What do you think of this idea? The tradeoff would be that we have to reset the timer more often. 300ms is large duration for idle-time callback, which is called every 16ms if the page is not busy. We can request idle time callback when we have spelling marker and stop when we have no spelling marker. For detecting tap, SelectionControler can have tap_count_ then idle time callback checks it and does something. Since, idle time callback is called during Blink isn't busy, you can do non-light weight tasks. Because of chance to have spelling marker is less than number of tapping, we should spend time for spelling menu as minimum as we can.
https://codereview.chromium.org/2931443003/diff/260001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/SelectionController.cpp (right): https://codereview.chromium.org/2931443003/diff/260001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/SelectionController.cpp:283: frame_->GetTextSuggestionController().HandlePotentialMisspelledWordTap(); On 2017/06/20 at 05:55:04, Xiaocheng wrote: > On 2017/06/20 at 05:34:21, rlanday wrote: > > On 2017/06/20 at 01:46:36, yosin_UTC9 wrote: > > > Do we really want to ask spell checker for every single click? > > > This change makes mouse handling slow down. > > > > > > I think we should stop calling |HandlePotentialMisspelledWordTap()| when "selectstart" is canceled > > > which is checked in |UpdateSelectionForMouseDownDispatchingSelectStart()| just before this call. > > > > > > Can we move this to idle callback like idle time spell checker? > > > > To clarify, we're not calling the spell checker here, we're just checking for the presence of a spelling DocumentMarker. > > > > I'm kind of hesitant to make this asynchronous because we want to show the popup menu 300ms (the double-tap threshold) after the tap, and we can't start the timer until after HandlePotentialMisspelledWordTap() runs. So if we make this run in idle-time, the timing will probably get thrown off and the interaction will feel laggy. > > > > One tradeoff we can make is, instead of creating the two-character wide selection range on every tap and checking for a DocumentMarker in that range (which requires using TextIterator, which is a somewhat complex operation), we can start the timer on every tap, and only check for the spelling marker if the timer expires. That way, if we get a bunch of touch events with less than 300ms between them, we only have to check for the spelling marker after the last one. What do you think of this idea? The tradeoff would be that we have to reset the timer more often. > > If we move the triggering of spellcheck menu to somewhere asynchronous, is it going to introduce weird interaction? For example, after tapping on a misspelled word, the normal selection menu appears, and then (after 300ms) gets replaced by the spellcheck menu? > > I think we should do some experiments to see the performance impact. It's more convincing to talk about performance with real data. Those long wikipedia articles' editing pages are good benchmarks. Can we tell spelling marker information to selection menu popup menu? It seems popup menu queries state of Copy, Paste, etc, we can tell marker information at that time. BTW, I withdraw idle-time-callback idea, it doesn't solve inconsistent menu issue.
which part would you like me to review?
On 2017/06/20 at 06:47:43, jochen wrote: > which part would you like me to review? I need your review as OWNER for these files: android_webview/browser/aw_browser_manifest_overlay.json content/browser/BUILD.gn content/browser/DEPS
On 2017/06/20 at 06:47:21, yosin wrote: > Can we tell spelling marker information to selection menu popup menu? It seems popup menu queries state of Copy, Paste, etc, we can tell > marker information at that time. I'm not sure these menus are related, I don't think I have any special logic to only show one or the other. > BTW, I withdraw idle-time-callback idea, it doesn't solve inconsistent menu issue. You're referring to the issue with the selection popup menu we might not actually have?
On 2017/06/20 at 09:08:47, rlanday wrote: > On 2017/06/20 at 06:47:21, yosin wrote: > > Can we tell spelling marker information to selection menu popup menu? It seems popup menu queries state of Copy, Paste, etc, we can tell > > marker information at that time. > > I'm not sure these menus are related, I don't think I have any special logic to only show one or the other. > > > BTW, I withdraw idle-time-callback idea, it doesn't solve inconsistent menu issue. > > You're referring to the issue with the selection popup menu we might not actually have? I tested again on my Android device, I don't think this is an issue. The selection popup appears on double-tap and long press, the spellcheck menu appears on single-tap. One caveat with my idea of starting the timer regardless of whether the tap was on a spelling marker or not is that this causes a slight change behavior for the case where a user taps somewhere, and then causes the word to become misspelled (e.g. by typing) less than 300ms later. I think we can largely solve this problem (aside perhaps from oddball cases where the text is being changed by a script) by making keyboard events also reset the timer (my current version of the code replicates the behavior used by the Android EditText widget...which produces buggy behavior and even crashes sometimes if you start typing after tapping on a spellcheck marker).
The CQ bit was checked by rlanday@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 rlanday@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...
I've updated this CL again in response to the most recent round of comments. I talked to aelias@ about the performance concerns: - He thinks the performance impact of looking for a spellcheck marker is "negligible," but suggested adding filtering so we only do this in response to taps, not mouse clicks, which I have done. Note: the Android EditText widget shows the spellcheck menu for both taps and clicks, but we're not sure if this is intentional or not. - He also thinks trying to show the menu as an idle-time callback isn't a good idea, since it's an operation that's supposed to occur in response to user input, vs. something less time-sensitive. I added code to RenderWidgetHostViewAndroid::SendKeyEvent() to cancel the timer when we send key events to avoid bugs if you type something immediately after tapping on a marker (is this the best place for this code?). With this change, I think we probably could make the change to start the timer on every touch event and only check the marker when it fires with hardly any impact on correctness, but I haven't made that change (do we want to?).
On 2017/06/20 at 21:58:08, rlanday wrote: > I've updated this CL again in response to the most recent round of comments. > > I talked to aelias@ about the performance concerns: > > - He thinks the performance impact of looking for a spellcheck marker is "negligible," but suggested adding filtering so we only do this in response to taps, not mouse clicks, which I have done. Note: the Android EditText widget shows the spellcheck menu for both taps and clicks, but we're not sure if this is intentional or not. > - He also thinks trying to show the menu as an idle-time callback isn't a good idea, since it's an operation that's supposed to occur in response to user input, vs. something less time-sensitive. Right. Personally how many CPU-milliseconds this computation eats is low on my list of potential problems that call for careful performance benchmarking on Android (e.g. because we typically have small text fields, because single-tap gestures have a quite weak performance contract, and because the Blink thread being busy with random JS is a much larger preexisting problem anyway) and I'd prefer not to devote too much time on that worry. Hopefully making the code touch-specific addresses the concern about mouse click perf regressions on desktop. For idle time callback, that is an abstraction intended for batch work that can potentially be put off semi-indefinitely (for example: tracking pingbacks, prepopulating caches, and inherently optional+unreliable helper features such as the initial creation of the spellcheck underlines), but creating a popup menu on idle callback would be a misapplication of the concept. We have never before tied a mandatory UI response to input to an idle time callback.
I tried getting a trace of tapping/clicking around while editing Wikipedia, and I tested on both Android and Linux (since the mobile Wikipedia editor seems to only edit a section at a time?) and SelectionController::HandleSingleClick() comes up but HandlePotentialMisspelledWordTap() is apparently quick enough that it's not showing up in the trace.
On 2017/06/20 at 23:15:09, rlanday wrote: > I tried getting a trace of tapping/clicking around while editing Wikipedia, and I tested on both Android and Linux (since the mobile Wikipedia editor seems to only edit a section at a time?) and SelectionController::HandleSingleClick() comes up but HandlePotentialMisspelledWordTap() is apparently quick enough that it's not showing up in the trace. Sorry, there was some operator error on my part. I set the method up to be included in the trace and tried editing "List of Australian treaties" Wikipedia page (apparently the longest one, roughly a megabyte of text) on Chrome for Linux (with the touch-only gating commented out): https://drive.google.com/file/d/0B7bl8XNCwp3Db0tFMjR5ZjZnVTQ/view?ths=true The run time of TextSuggestionController::HandlePotentialMisspelledWordTap() appears to be fairly small compared to SelectionController::HandleSingleClick(), but it did get up to just over 2 ms here. I tried again on a random, shorter Wikipedia page (2006 European Athletics Championships – Men's 400 metres) and this method ran in 0.476ms of wall time out of 9.665 for HandleSingleClick(). Note that this is a debug build, which feels glacially slow to me compared to the release build.
On 2017/06/20 at 23:42:13, rlanday wrote: > On 2017/06/20 at 23:15:09, rlanday wrote: > > I tried getting a trace of tapping/clicking around while editing Wikipedia, and I tested on both Android and Linux (since the mobile Wikipedia editor seems to only edit a section at a time?) and SelectionController::HandleSingleClick() comes up but HandlePotentialMisspelledWordTap() is apparently quick enough that it's not showing up in the trace. > > Sorry, there was some operator error on my part. I set the method up to be included in the trace and tried editing "List of Australian treaties" Wikipedia page (apparently the longest one, roughly a megabyte of text) on Chrome for Linux (with the touch-only gating commented out): > https://drive.google.com/file/d/0B7bl8XNCwp3Db0tFMjR5ZjZnVTQ/view?ths=true > > The run time of TextSuggestionController::HandlePotentialMisspelledWordTap() appears to be fairly small compared to SelectionController::HandleSingleClick(), but it did get up to just over 2 ms here. > > I tried again on a random, shorter Wikipedia page (2006 European Athletics Championships – Men's 400 metres) and this method ran in 0.476ms of wall time out of 9.665 for HandleSingleClick(). > > Note that this is a debug build, which feels glacially slow to me compared to the release build. For a release build (not official build, just release), the worst case with the giant article is still about the same, about 2 to 3 ms, but down to about ~0.075 ms on the smaller article.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2017/06/21 at 00:23:58, rlanday wrote: > On 2017/06/20 at 23:42:13, rlanday wrote: > > On 2017/06/20 at 23:15:09, rlanday wrote: > > > I tried getting a trace of tapping/clicking around while editing Wikipedia, and I tested on both Android and Linux (since the mobile Wikipedia editor seems to only edit a section at a time?) and SelectionController::HandleSingleClick() comes up but HandlePotentialMisspelledWordTap() is apparently quick enough that it's not showing up in the trace. > > > > Sorry, there was some operator error on my part. I set the method up to be included in the trace and tried editing "List of Australian treaties" Wikipedia page (apparently the longest one, roughly a megabyte of text) on Chrome for Linux (with the touch-only gating commented out): > > https://drive.google.com/file/d/0B7bl8XNCwp3Db0tFMjR5ZjZnVTQ/view?ths=true > > > > The run time of TextSuggestionController::HandlePotentialMisspelledWordTap() appears to be fairly small compared to SelectionController::HandleSingleClick(), but it did get up to just over 2 ms here. > > > > I tried again on a random, shorter Wikipedia page (2006 European Athletics Championships – Men's 400 metres) and this method ran in 0.476ms of wall time out of 9.665 for HandleSingleClick(). > > > > Note that this is a debug build, which feels glacially slow to me compared to the release build. > > For a release build (not official build, just release), the worst case with the giant article is still about the same, about 2 to 3 ms, but down to about ~0.075 ms on the smaller article. 2 to 3 ms seems to be large. Should we another cheap function to check potential markers instead of GetSpellCheckMarkerTouchingSelection()? Following functions in GetSpellCheckMarkerTouchingSelection() can be slow: - ComputeVisibleSelectionInDOM() - ExpandSelectionRangeIfNecessary() However, we don't need to have exact info about markers since it will be changed after 300ms. Because of we have a VisiblePosition, |visible_pos|, at click, it might be Text node if there are Text node thanks by canonicalization. Thus, we can do SelectionController::HandleSingleClick() { ... if (MayHaveMarkers(*visible_pos.GetPosition())) TextSuggestionController()->StartTimer() // Is it better to implement in DMC? // Use Position for offset in Text node in case of huge Text node. bool MayHaveMarkers(const Position& position) { if (!text node) return false; if (!editable) return false; return ask DMC whether |position| in spelling marker? }
On 2017/06/21 at 01:38:15, yosin wrote: > On 2017/06/21 at 00:23:58, rlanday wrote: > > On 2017/06/20 at 23:42:13, rlanday wrote: > > > On 2017/06/20 at 23:15:09, rlanday wrote: > > > > I tried getting a trace of tapping/clicking around while editing Wikipedia, and I tested on both Android and Linux (since the mobile Wikipedia editor seems to only edit a section at a time?) and SelectionController::HandleSingleClick() comes up but HandlePotentialMisspelledWordTap() is apparently quick enough that it's not showing up in the trace. > > > > > > Sorry, there was some operator error on my part. I set the method up to be included in the trace and tried editing "List of Australian treaties" Wikipedia page (apparently the longest one, roughly a megabyte of text) on Chrome for Linux (with the touch-only gating commented out): > > > https://drive.google.com/file/d/0B7bl8XNCwp3Db0tFMjR5ZjZnVTQ/view?ths=true > > > > > > The run time of TextSuggestionController::HandlePotentialMisspelledWordTap() appears to be fairly small compared to SelectionController::HandleSingleClick(), but it did get up to just over 2 ms here. > > > > > > I tried again on a random, shorter Wikipedia page (2006 European Athletics Championships – Men's 400 metres) and this method ran in 0.476ms of wall time out of 9.665 for HandleSingleClick(). > > > > > > Note that this is a debug build, which feels glacially slow to me compared to the release build. > > > > For a release build (not official build, just release), the worst case with the giant article is still about the same, about 2 to 3 ms, but down to about ~0.075 ms on the smaller article. > > 2 to 3 ms seems to be large. > Should we another cheap function to check potential markers instead of GetSpellCheckMarkerTouchingSelection()? > > Following functions in GetSpellCheckMarkerTouchingSelection() can be slow: > - ComputeVisibleSelectionInDOM() > - ExpandSelectionRangeIfNecessary() > > However, we don't need to have exact info about markers since it will be changed after 300ms. > > Because of we have a VisiblePosition, |visible_pos|, at click, it might be Text node if there are Text node thanks by canonicalization. > Thus, we can do > > SelectionController::HandleSingleClick() { > ... > if (MayHaveMarkers(*visible_pos.GetPosition())) > TextSuggestionController()->StartTimer() > > // Is it better to implement in DMC? > // Use Position for offset in Text node in case of huge Text node. > bool MayHaveMarkers(const Position& position) { > if (!text node) > return false; > if (!editable) > return false; > return ask DMC whether |position| in spelling marker? > } I think it makes sense to move GetSpellCheckMarkerTouchingSelection() into DocumentMarkerController since I'm eventually going to want to do the same thing with suggestion markers. It seems the optimizations you're suggesting are basically: 1. Hang onto the visible_pos we pass to UpdateSelectionForMouseDownDispatchingSelectStart() instead of recomputing it 2. Don't expand the selection unless we're at the boundary of a text node, and we need to, to get correct results? The implementation you're suggesting for MayHaveMarkers() seems to not properly handle the case where the tap position is bordered by one text node on the left and a different one on the right. But I think we can do optimization #2 to get the same benefit, aside from the boundary case where we have to do the slower check.
mkwst@chromium.org changed required reviewers: + yosin@chromium.org
I'm going to defer to yosin@ on //third_party/WebKit/Source/core/editing/, as I don't know that code very well. The rest of the Blink changes and the mojo bits LGTM.
tedchoc@chromium.org changed reviewers: + twellington@chromium.org
+twellington to take a glance at SuggestionsPopupWindow.java as they've dealt with some popup window quirks as of late. Are you working with anyone on UX about this? It would be good to get a quick OK on the design before we submit this (in general, we are trying to reduce the colors/text styles and this might be adding new ones). Maybe ping hannahs@chromium.org and confirm. https://codereview.chromium.org/2931443003/diff/340001/content/browser/androi... File content/browser/android/text_suggestion_host_android.cc (right): https://codereview.chromium.org/2931443003/diff/340001/content/browser/androi... content/browser/android/text_suggestion_host_android.cc:27: const int kDoubleTapTimeoutInMilliseconds = 300; Is this supposed to match https://developer.android.com/reference/android/view/ViewConfiguration.html#g... Should we be pulling from there? gfx::ViewConfiguration::GetDoubleTapTimeoutInMs(); ? https://codereview.chromium.org/2931443003/diff/340001/content/browser/androi... content/browser/android/text_suggestion_host_android.cc:57: RenderFrameHost* rfh = GetFocusedFrame(); should we call FocusedNodeChanged() here instead of these lines? https://codereview.chromium.org/2931443003/diff/340001/content/browser/androi... content/browser/android/text_suggestion_host_android.cc:116: void TextSuggestionHostAndroid::SuggestionMenuClosed( add blank line above this https://codereview.chromium.org/2931443003/diff/340001/content/browser/androi... content/browser/android/text_suggestion_host_android.cc:135: ScopedJavaLocalRef<jobject> obj = java_text_suggestion_host_.get(env); should you check that obj is non-null here? https://codereview.chromium.org/2931443003/diff/340001/content/browser/androi... content/browser/android/text_suggestion_host_android.cc:146: rfh->GetInterfaceRegistry()->AddInterface(base::Bind( does there need to be a corresponding RemoveInterface on the previous rfh? https://codereview.chromium.org/2931443003/diff/340001/content/browser/androi... File content/browser/android/text_suggestion_host_android.h (right): https://codereview.chromium.org/2931443003/diff/340001/content/browser/androi... content/browser/android/text_suggestion_host_android.h:15: class TextSuggestionHostAndroid : public RenderWidgetHostConnector { can you add a class level comment about this file. https://codereview.chromium.org/2931443003/diff/340001/content/browser/androi... content/browser/android/text_suggestion_host_android.h:27: // Called from Java -> native I don't think these comments (and the two other divider comments) are too terribly useful. Since they have JNIEnv params, I think that is a clear indicator they are called from java. In general, I think they comments should describe the behavior or not be here. https://codereview.chromium.org/2931443003/diff/340001/content/public/android... File content/public/android/java/res/layout/text_edit_suggestion_container.xml (right): https://codereview.chromium.org/2931443003/diff/340001/content/public/android... content/public/android/java/res/layout/text_edit_suggestion_container.xml:6: <RelativeLayout xmlns:android="http://schemas.android.com/apk/res/android" Why do you need this outer relativelayout? https://codereview.chromium.org/2931443003/diff/340001/content/public/android... content/public/android/java/res/layout/text_edit_suggestion_container.xml:15: android:elevation="2dp" elevation only works on newer versions of android, what happens if you run this on KK? Does it look ok? https://codereview.chromium.org/2931443003/diff/340001/content/public/android... content/public/android/java/res/layout/text_edit_suggestion_container.xml:27: <LinearLayout Why do you need this LinearLayout? Is it to have the divider between the two sections? Could you get the divider by doing: <View android:background="?android:attr/listDivider" /> Then adding the TextViews to this linearlayout? Ideally, you keep your views as shallow as possible. https://codereview.chromium.org/2931443003/diff/340001/content/public/android... File content/public/android/java/res/values-v17/styles.xml (right): https://codereview.chromium.org/2931443003/diff/340001/content/public/android... content/public/android/java/res/values-v17/styles.xml:20: <item name="android:drawablePadding">8dip</item> the rest of this file is indented 4, so I think you need to indent +2 https://codereview.chromium.org/2931443003/diff/340001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/input/SuggestionsPopupWindow.java (right): https://codereview.chromium.org/2931443003/diff/340001/content/public/android... content/public/android/java/src/org/chromium/content/browser/input/SuggestionsPopupWindow.java:31: * suggestions. you can wrap comments to 100chars in java, so this might fit https://codereview.chromium.org/2931443003/diff/340001/content/public/android... content/public/android/java/src/org/chromium/content/browser/input/SuggestionsPopupWindow.java:33: remove blank line https://codereview.chromium.org/2931443003/diff/340001/content/public/android... content/public/android/java/src/org/chromium/content/browser/input/SuggestionsPopupWindow.java:34: public class SuggestionsPopupWindow implements OnItemClickListener, OnDismissListener { Did you look at extending DropdownPopupWindow? That does similar things and would be good to make sure we're not rebuilding the wheel here. https://codereview.chromium.org/2931443003/diff/340001/content/public/android... content/public/android/java/src/org/chromium/content/browser/input/SuggestionsPopupWindow.java:69: mPopupWindow.setWidth(ViewGroup.LayoutParams.WRAP_CONTENT); any reason these aren't in createPopupWindow? https://codereview.chromium.org/2931443003/diff/340001/content/public/android... content/public/android/java/src/org/chromium/content/browser/input/SuggestionsPopupWindow.java:83: mPopupWindow.setBackgroundDrawable(new ColorDrawable(Color.TRANSPARENT)); would null work here or does that make it upset? https://codereview.chromium.org/2931443003/diff/340001/content/public/android... content/public/android/java/src/org/chromium/content/browser/input/SuggestionsPopupWindow.java:114: public void onClick(View v) { @Override here and below It is often better to combine OnClickListeners by implementing the interface at the class level and then doing: if (v == mDeleteButton) { } else ... These create additional classes, but I don't know how relevant that is in the proguard world to see if they can combine them (although I think it would be hard to do) https://codereview.chromium.org/2931443003/diff/340001/content/public/android... content/public/android/java/src/org/chromium/content/browser/input/SuggestionsPopupWindow.java:132: public void dismiss() { all non-private methods should have javadoc https://codereview.chromium.org/2931443003/diff/340001/content/public/android... content/public/android/java/src/org/chromium/content/browser/input/SuggestionsPopupWindow.java:141: final Intent intent = new Intent(ACTION_USER_DICTIONARY_INSERT); are we guaranteed that this action will exist on the device? Should we first verify that we can resolve this intent before showing the button at all and then again here. You might also want to catch exceptions like IntentUtils#safeStartActivity https://codereview.chromium.org/2931443003/diff/340001/content/public/android... content/public/android/java/src/org/chromium/content/browser/input/SuggestionsPopupWindow.java:180: final DisplayMetrics displayMetrics = mContext.getResources().getDisplayMetrics(); in multi-window, does this represent the full screen or our window? https://codereview.chromium.org/2931443003/diff/340001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/input/TextSuggestionHost.java (right): https://codereview.chromium.org/2931443003/diff/340001/content/public/android... content/public/android/java/src/org/chromium/content/browser/input/TextSuggestionHost.java:55: public void hidePopups() { same thing about javadoc
https://codereview.chromium.org/2931443003/diff/340001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/input/SuggestionsPopupWindow.java (right): https://codereview.chromium.org/2931443003/diff/340001/content/public/android... content/public/android/java/src/org/chromium/content/browser/input/SuggestionsPopupWindow.java:86: mPopupWindow.setClippingEnabled(false); This will allow the popup to draw off screen. Is that desired? https://codereview.chromium.org/2931443003/diff/340001/content/public/android... content/public/android/java/src/org/chromium/content/browser/input/SuggestionsPopupWindow.java:206: width += mTempRect.left + mTempRect.right; The background is a transparent color drawable, right? If so, I wouldn't expect it to have any padding. https://codereview.chromium.org/2931443003/diff/340001/content/public/android... content/public/android/java/src/org/chromium/content/browser/input/SuggestionsPopupWindow.java:250: DropdownPopupWindow may be well suited for your use case and would eliminate the need for custom positioning logic and would eliminate some of the duplication in measureContent(). While that class does use ListPopupWindow (which has some framework issues), the impact is likely minimal for this use case and we do have an outstanding bug tracking fixing up uses of ListPopupWindow (crbug.com/709522). If possible, we should make this class simpler and rely on the broader ListPopupWindow clean up to fix in minor issues.
I can't review android_webview, sorry
torne@chromium.org changed reviewers: + torne@chromium.org
[speeds past to note that android_webview LGTM] :)
rlanday@chromium.org changed reviewers: - jochen@chromium.org
rlanday@chromium.org changed reviewers: + boliu@chromium.org
On 2017/06/22 at 16:55:38, torne wrote: > [speeds past to note that android_webview LGTM] :) Thanks! boliu@, can you please review these two files? content/browser/BUILD.gn content/browser/DEPS (Feedback on the other files is welcome as well, but I think these are the only two I don't already have an OWNER looking at)
yosin@chromium.org changed reviewers: + kouhei@chromium.org
On 2017/06/22 20:48:12, rlanday wrote: > On 2017/06/22 at 16:55:38, torne wrote: > > [speeds past to note that android_webview LGTM] :) > > Thanks! > > boliu@, can you please review these two files? > content/browser/BUILD.gn > content/browser/DEPS rs lgtm I did not look at any other files, so wait for approval from ted as well > > (Feedback on the other files is welcome as well, but I think these are the only > two I don't already have an OWNER looking at)
also wanted to add.. integration tests?
On 2017/06/23 at 01:59:13, boliu wrote: > also wanted to add.. integration tests? Do you mean like adding tests for the Java code? Like Robolectric or something?
On 2017/06/23 02:09:56, rlanday wrote: > On 2017/06/23 at 01:59:13, boliu wrote: > > also wanted to add.. integration tests? > > Do you mean like adding tests for the Java code? Like Robolectric or something? I mean instrumentation test, that actually simulates clicking on highlighted words, and make sure the dialog appears and whatnot
I've attempted to address the efficiency concerns with HandlePotentialMisspelledWordTap(). It is quite efficient now. I have not yet addressed any of the Android-side feedback.
The CQ bit was checked by rlanday@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...
https://codereview.chromium.org/2931443003/diff/380001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/SelectionController.cpp (right): https://codereview.chromium.org/2931443003/diff/380001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/SelectionController.cpp:285: frame_->GetTextSuggestionController().HandlePotentialMisspelledWordTap( I think this should probably be gated to only run when we're in an editable region
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...) 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 rlanday@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_cronet on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...) linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...) ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...) ios-simulator-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator-xco...)
The CQ bit was checked by rlanday@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...
I've tried to address most of the feedback received so far. Things I still need to do: - Talk to twellington@ about DropdownPopupWindow, determine if it makes sense + how to use it - Fix some bugs in multi-window mode (incorrect clipping, make typing close menu, close menu on switching to another window instead of switching back; some of these may be potentially addressed by using DropdownPopupWindow?) - Talk to hannahs@ about UX; potentially add a border around the popup window for pre-Lollipop devices (where the elevation shadow isn't supported) - Write the integration test suggested by boliu@ - Should probably test on Jellybean since testing on KitKat exposed a crash https://codereview.chromium.org/2931443003/diff/340001/content/browser/androi... File content/browser/android/text_suggestion_host_android.cc (right): https://codereview.chromium.org/2931443003/diff/340001/content/browser/androi... content/browser/android/text_suggestion_host_android.cc:27: const int kDoubleTapTimeoutInMilliseconds = 300; On 2017/06/21 at 17:59:21, Ted C wrote: > Is this supposed to match https://developer.android.com/reference/android/view/ViewConfiguration.html#g... Should we be pulling from there? > > gfx::ViewConfiguration::GetDoubleTapTimeoutInMs(); ? Ok, I'll do that https://codereview.chromium.org/2931443003/diff/340001/content/browser/androi... content/browser/android/text_suggestion_host_android.cc:55: java_text_suggestion_host_ = JavaObjectWeakGlobalRef(env, obj); This can go in the initializer list https://codereview.chromium.org/2931443003/diff/340001/content/browser/androi... content/browser/android/text_suggestion_host_android.cc:57: RenderFrameHost* rfh = GetFocusedFrame(); On 2017/06/21 at 17:59:21, Ted C wrote: > should we call FocusedNodeChanged() here instead of these lines? I guess it avoids some code duplication, I'll do that https://codereview.chromium.org/2931443003/diff/340001/content/browser/androi... content/browser/android/text_suggestion_host_android.cc:57: RenderFrameHost* rfh = GetFocusedFrame(); On 2017/06/21 at 17:59:21, Ted C wrote: > should we call FocusedNodeChanged() here instead of these lines? I guess it avoids some code duplication, I'll do that https://codereview.chromium.org/2931443003/diff/340001/content/browser/androi... content/browser/android/text_suggestion_host_android.cc:135: ScopedJavaLocalRef<jobject> obj = java_text_suggestion_host_.get(env); On 2017/06/21 at 17:59:20, Ted C wrote: > should you check that obj is non-null here? I think so...this class is set up pretty similarly to ImeAdapterAndroid, and I see that class is full of "if (obj.is_null())" checks https://codereview.chromium.org/2931443003/diff/340001/content/browser/androi... content/browser/android/text_suggestion_host_android.cc:146: rfh->GetInterfaceRegistry()->AddInterface(base::Bind( On 2017/06/21 at 17:59:21, Ted C wrote: > does there need to be a corresponding RemoveInterface on the previous rfh? I don't think so, I think this object is shared between RenderFrameHosts. Looking at other code, this seems to be a common pattern. E.g.: https://chromium.googlesource.com/chromium/src/+/e13b5cad035abf6c2d0d75f26303... "AutofillAgent is guaranteed to outlive |render_frame|." seems to be a justification for not ever calling RemoveInterface(). If we did want to call RemoveInterface(), would it actually be safe to hold onto a pointer to the RenderFrameHost or service_manager::BinderRegistry? I'm not sure how else we would do this. https://codereview.chromium.org/2931443003/diff/340001/content/browser/androi... File content/browser/android/text_suggestion_host_android.h (right): https://codereview.chromium.org/2931443003/diff/340001/content/browser/androi... content/browser/android/text_suggestion_host_android.h:15: class TextSuggestionHostAndroid : public RenderWidgetHostConnector { On 2017/06/21 at 17:59:21, Ted C wrote: > can you add a class level comment about this file. Ok https://codereview.chromium.org/2931443003/diff/340001/content/browser/androi... content/browser/android/text_suggestion_host_android.h:27: // Called from Java -> native On 2017/06/21 at 17:59:21, Ted C wrote: > I don't think these comments (and the two other divider comments) are too terribly useful. Since they have JNIEnv params, I think that is a clear indicator they are called from java. In general, I think they comments should describe the behavior or not be here. Ok https://codereview.chromium.org/2931443003/diff/340001/content/public/android... File content/public/android/java/res/layout/text_edit_suggestion_container.xml (right): https://codereview.chromium.org/2931443003/diff/340001/content/public/android... content/public/android/java/res/layout/text_edit_suggestion_container.xml:6: <RelativeLayout xmlns:android="http://schemas.android.com/apk/res/android" On 2017/06/21 at 17:59:21, Ted C wrote: > Why do you need this outer relativelayout? I was copying from this file: https://android.googlesource.com/platform/frameworks/base/+/refs/heads/master... Seems if I try to remove the outer RelativeLayout, mContainerView.getLayoutParams() returns a null reference. Does LinearLayout.getLayoutParams() not work unless it's inside a RelativeLayout? https://codereview.chromium.org/2931443003/diff/340001/content/public/android... content/public/android/java/res/layout/text_edit_suggestion_container.xml:15: android:elevation="2dp" On 2017/06/21 at 17:59:21, Ted C wrote: > elevation only works on newer versions of android, what happens if you run this on KK? Does it look ok? It looks flat. We might want to add a border so the menu doesn't look invisible on a white background. I guess I'll talk to hannahs@ about this https://codereview.chromium.org/2931443003/diff/340001/content/public/android... content/public/android/java/res/layout/text_edit_suggestion_container.xml:27: <LinearLayout On 2017/06/21 at 17:59:21, Ted C wrote: > Why do you need this LinearLayout? Is it to have the divider between the two sections? > > Could you get the divider by doing: > <View android:background="?android:attr/listDivider" /> > > Then adding the TextViews to this linearlayout? Ideally, you keep your views as shallow as possible. It's so we don't get a divider between "Add to Dictionary" and "Delete". And yeah, removing the extra LinearLayout and adding the divider like that works. https://codereview.chromium.org/2931443003/diff/340001/content/public/android... File content/public/android/java/res/values-v17/styles.xml (right): https://codereview.chromium.org/2931443003/diff/340001/content/public/android... content/public/android/java/res/values-v17/styles.xml:20: <item name="android:drawablePadding">8dip</item> On 2017/06/21 at 17:59:21, Ted C wrote: > the rest of this file is indented 4, so I think you need to indent +2 Ok https://codereview.chromium.org/2931443003/diff/340001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/input/SuggestionsPopupWindow.java (right): https://codereview.chromium.org/2931443003/diff/340001/content/public/android... content/public/android/java/src/org/chromium/content/browser/input/SuggestionsPopupWindow.java:31: * suggestions. On 2017/06/21 at 17:59:21, Ted C wrote: > you can wrap comments to 100chars in java, so this might fit It does :) https://codereview.chromium.org/2931443003/diff/340001/content/public/android... content/public/android/java/src/org/chromium/content/browser/input/SuggestionsPopupWindow.java:33: On 2017/06/21 at 17:59:22, Ted C wrote: > remove blank line Ok https://codereview.chromium.org/2931443003/diff/340001/content/public/android... content/public/android/java/src/org/chromium/content/browser/input/SuggestionsPopupWindow.java:69: mPopupWindow.setWidth(ViewGroup.LayoutParams.WRAP_CONTENT); On 2017/06/21 at 17:59:21, Ted C wrote: > any reason these aren't in createPopupWindow? I was copying from Android's Editor.java where there are multiple related subclasses and createPopupWindow() is a protected method that can be overridden by different subclasses and setWidth()/setHeight() is shared code that doesn't need to be overridden: https://android.googlesource.com/platform/frameworks/base/+/master/core/java/... But for our purposes, we can put them in createPopupWindow(). https://codereview.chromium.org/2931443003/diff/340001/content/public/android... content/public/android/java/src/org/chromium/content/browser/input/SuggestionsPopupWindow.java:83: mPopupWindow.setBackgroundDrawable(new ColorDrawable(Color.TRANSPARENT)); On 2017/06/21 at 17:59:21, Ted C wrote: > would null work here or does that make it upset? Yes, null works https://codereview.chromium.org/2931443003/diff/340001/content/public/android... content/public/android/java/src/org/chromium/content/browser/input/SuggestionsPopupWindow.java:86: mPopupWindow.setClippingEnabled(false); On 2017/06/21 at 23:44:53, Theresa wrote: > This will allow the popup to draw off screen. Is that desired? I think so, I copied this from the native Android Editor.java and it probably makes sense to have the same behavior: https://android.googlesource.com/platform/frameworks/base/+/master/core/java/... The alternative is to show the popup with a scrollbar? I think this only really comes into play on really small windows (like turning your phone sideways with two windows open and the keyboard taking up the bottom half of the screen) https://codereview.chromium.org/2931443003/diff/340001/content/public/android... content/public/android/java/src/org/chromium/content/browser/input/SuggestionsPopupWindow.java:114: public void onClick(View v) { On 2017/06/21 at 17:59:21, Ted C wrote: > @Override here and below > > It is often better to combine OnClickListeners by implementing the interface at the class level and then doing: > > if (v == mDeleteButton) { > } else ... > > These create additional classes, but I don't know how relevant that is in the proguard world to see if they can combine them (although I think it would be hard to do) Ok, I'll do that https://codereview.chromium.org/2931443003/diff/340001/content/public/android... content/public/android/java/src/org/chromium/content/browser/input/SuggestionsPopupWindow.java:132: public void dismiss() { On 2017/06/21 at 17:59:21, Ted C wrote: > all non-private methods should have javadoc Ok https://codereview.chromium.org/2931443003/diff/340001/content/public/android... content/public/android/java/src/org/chromium/content/browser/input/SuggestionsPopupWindow.java:141: final Intent intent = new Intent(ACTION_USER_DICTIONARY_INSERT); On 2017/06/21 at 17:59:21, Ted C wrote: > are we guaranteed that this action will exist on the device? Should we first verify that we can resolve this intent before showing the button at all and then again here. You might also want to catch exceptions like IntentUtils#safeStartActivity So, I've gotten multiple opinions about this; I talked to Svet on the Android team about getting this intent added as part of the official interface, which he said sounded reasonable; he advised me to query the package manager to see if the intent exists. However, I talked to aelias@, and he said we can just assume it exists (it appears to pre-date Jellybean); I think his argument was that the only reason it wouldn't exist was because an OEM removed it for some reason, and it's OK to fail as a result of an OEM doing something weird like that. https://codereview.chromium.org/2931443003/diff/340001/content/public/android... content/public/android/java/src/org/chromium/content/browser/input/SuggestionsPopupWindow.java:180: final DisplayMetrics displayMetrics = mContext.getResources().getDisplayMetrics(); On 2017/06/21 at 17:59:21, Ted C wrote: > in multi-window, does this represent the full screen or our window? I think it's probably the display...I just tested multi-window, and the clipping's not right. I'm going to hold off on addressing this until I decide if this should be using DropdownPopupWindow or not. Also, we should probably close the menu if the user changes windows; right now it stays open but closes when you switch back, how can I do this? I tried closing the menu in ContentViewCore.onWindowFocusChanged() but then the menu just closes immediately (I guess opening the popup counts as a focus change). Another issue: in multi-window mode, if you try to start typing with the popup menu open, instead of dismissing the popup, it actually keeps the popup open and forwards the typing to Blink. How can I fix this behavior? https://codereview.chromium.org/2931443003/diff/340001/content/public/android... content/public/android/java/src/org/chromium/content/browser/input/SuggestionsPopupWindow.java:206: width += mTempRect.left + mTempRect.right; On 2017/06/21 at 23:44:53, Theresa wrote: > The background is a transparent color drawable, right? If so, I wouldn't expect it to have any padding. Yeah, plus tedchoc@ told me to set it to null, so then we don't even enter this block. I'll just remove it (more stuff copied from Android's Editor.java) https://codereview.chromium.org/2931443003/diff/340001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/input/TextSuggestionHost.java (right): https://codereview.chromium.org/2931443003/diff/340001/content/public/android... content/public/android/java/src/org/chromium/content/browser/input/TextSuggestionHost.java:55: public void hidePopups() { On 2017/06/21 at 17:59:22, Ted C wrote: > same thing about javadoc Ok https://codereview.chromium.org/2931443003/diff/400001/content/public/android... File content/public/android/java/res/values-v17/styles.xml (right): https://codereview.chromium.org/2931443003/diff/400001/content/public/android... content/public/android/java/res/values-v17/styles.xml:21: <item name="android:fontFamily">sans-serif-medium</item> I think sans-serif-medium was added in Lollipop so the text looks thinner on Kit Kat. Not sure if there's anything we can do about this. I suppose I should use sans-serif in the v17 file just so we're not using an invalid option (it still looks thin) https://codereview.chromium.org/2931443003/diff/400001/content/public/android... content/public/android/java/res/values-v17/styles.xml:31: <item name="android:textColor">?android:attr/colorAccent</item> It turns out that android:attr/colorAccent was added in API level 21 so this crashes on Kit Kat. I could hard code a color here but right now e.g. we have different colors in Chrome (green, which I think is the default) and Gmail (blue), so it'd be nice to still support this. I guess I'll provide a separate version of this file for v21? https://codereview.chromium.org/2931443003/diff/400001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java (right): https://codereview.chromium.org/2931443003/diff/400001/content/public/android... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:988: mPopupZoomer.hide(false); should probably call mTextSuggestionHost.hidePopups() here too https://codereview.chromium.org/2931443003/diff/400001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/suggestion/TextSuggestionController.cpp (right): https://codereview.chromium.org/2931443003/diff/400001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/suggestion/TextSuggestionController.cpp:89: caret_visible_position.DeepEquivalent(); Oops, I was refactoring and lost the PreviousPositionOf() here and the NextPositionOf() in the next one...will re-add
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Re: KitKat and Jellybean, if they involve extra challenges to support, one reasonable option is to simply disabling spellcheck and SuggestionSpan support below L. Market share of those older versions is low and this is an helper feature we aren't obliged to support for 100% of our userbase.
https://codereview.chromium.org/2931443003/diff/340001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/input/SuggestionsPopupWindow.java (right): https://codereview.chromium.org/2931443003/diff/340001/content/public/android... content/public/android/java/src/org/chromium/content/browser/input/SuggestionsPopupWindow.java:86: mPopupWindow.setClippingEnabled(false); On 2017/06/28 01:35:34, rlanday wrote: > On 2017/06/21 at 23:44:53, Theresa wrote: > > This will allow the popup to draw off screen. Is that desired? > > I think so, I copied this from the native Android Editor.java and it probably > makes sense to have the same behavior: > https://android.googlesource.com/platform/frameworks/base/+/master/core/java/... > > The alternative is to show the popup with a scrollbar? I think this only really > comes into play on really small windows (like turning your phone sideways with > two windows open and the keyboard taking up the bottom half of the screen) Unfortunately copying Android's framework code without digging into what it's doing (and why) is ill-advised. The framework code is often quite complex and past framework versions have had numerous bugs related popup windows. The alternative is to make sure the popup window is appropriately sized so that it's not cut-off, which could mean scrolling the content if displaying all of it without scrolling would make the popup too tall. With multi-window on Android N+ and small phones, having a working UI on small screen-heights is important. How many suggestions are expected in the suggestions popupwindow?
On 2017/06/28 at 15:43:49, twellington wrote: > https://codereview.chromium.org/2931443003/diff/340001/content/public/android... > File content/public/android/java/src/org/chromium/content/browser/input/SuggestionsPopupWindow.java (right): > > https://codereview.chromium.org/2931443003/diff/340001/content/public/android... > content/public/android/java/src/org/chromium/content/browser/input/SuggestionsPopupWindow.java:86: mPopupWindow.setClippingEnabled(false); > On 2017/06/28 01:35:34, rlanday wrote: > > On 2017/06/21 at 23:44:53, Theresa wrote: > > > This will allow the popup to draw off screen. Is that desired? > > > > I think so, I copied this from the native Android Editor.java and it probably > > makes sense to have the same behavior: > > https://android.googlesource.com/platform/frameworks/base/+/master/core/java/... > > > > The alternative is to show the popup with a scrollbar? I think this only really > > comes into play on really small windows (like turning your phone sideways with > > two windows open and the keyboard taking up the bottom half of the screen) > > Unfortunately copying Android's framework code without digging into what it's doing (and why) is ill-advised. The framework code is often quite complex and past framework versions have had numerous bugs related popup windows. > > The alternative is to make sure the popup window is appropriately sized so that it's not cut-off, which could mean scrolling the content if displaying all of it without scrolling would make the popup too tall. With multi-window on Android N+ and small phones, having a working UI on small screen-heights is important. > > How many suggestions are expected in the suggestions popupwindow? We show at most five suggestions, plus the "Add to dictionary" and "Delete" options. I think those two options are the highest-priority; if we don't have enough space to show them, we shouldn't show the menu at all. Otherwise, we should probably show those two options plus however many suggestions fit on the screen (with whatever padding we currently use). Note: the native Android spell check menu is unsurprisingly (since I copied most of the code) also somewhat broken in multi-window mode...filed as (Google-internal) b/63099186
I talked to hannahs@ about the design; she had some minor comments (remove the divider when there are no suggestions, make the buttons blue in Chrome, check that the spacing above "Add to dictionary" and below "Delete" is correct). I asked what to do about the drop shadow pre-Lollipop (where elevation isn't supported), and she said we don't have any designs for showing a drop-down menu without a drop shadow, so I don't think that's a good idea. Pre-Lollipop, the native Android text box spell check menu used a different, non-Material, design. I think we have the following choices: 1. Ship the Material design spell check menu on pre-Lollipop devices, where it won't match the native text box menu, and it will look wonky, since it won't have a drop shadow (unless we create a background image to add the shadow) 2. Implement the pre-Material design, which requires a few extra XML files and two PNG files (in as many resolutions as we care to support). This doesn't seem like much work, but I'm not sure how we feel about adding extra PNG files to support this menu on old Android versions (due to app weight concerns). 3. Disable the menu pre-Lollipop (probably by TextSuggestionHostAndroid not actually start the timer). Perhaps we should go with option #3 for now to avoid adding even more complexity to this CL? If someone complains about not having this on Jelly Bean/KitKit, we can come back and add it later.
Yes, I suggest going with #3. Failing that #1. In general, we have never attempted to match pre-Material styling nor OEM styling.
On 2017/06/28 23:16:06, aelias wrote: > Yes, I suggest going with #3. Failing that #1. In general, we have never > attempted to match pre-Material styling nor OEM styling. rlanday@ and I discussed using the same 9-patch background as autofill suggestions (dropdown_popup_background). That allows us to ship on all Chrome supported Android versions and keeps the styling consistent across OS-versions, which is typical of Chrome.
hannahs@ said we should be using #4285F4 (a blue color we use elsewhere in Chrome) for the "Add to dictionary" and "Delete" buttons. However, I think we need to pull from the colorAccent setting to get the right color in apps embedding us as a WebView. If we use colorAccent, the buttons are green in Chrome. I think this is because we don't set colorAccent in our MainTheme. Should we do this? Or should I just hardcode the buttons to blue for all apps?
I started trying to see if it's possible to use DropdownPopupWindow and I'm not feeling too optimistic. One problem (which may be solvable if I spend some more time on it) is that I'm having trouble getting the menu to match the Android spell check menu in appearance. It seems there's some background being drawn on it which I'm having trouble removing (by calling getListView() and trying to reset the background) that adds a shadow on the top and a border on the bottom. More importantly, there's no guarantee that it will actually behave the same way as the native Android spell check menu (one of the design goals of the feature is to match that menu as closely as possible). For example, I noticed that DropdownPopupWindow is currently set up to never show on top of the keyboard (whereas the Android spell check menu does): https://chromium.googlesource.com/chromium/src/+/9ee8865d84c2156728f1b9f78fb8... DropdownPopupWindow wants to be anchored to a view, whereas for my use case, I have a caret location I want to center the menu at as closely as possible without going off-screen. So I think it's unlikely that it's going to produce the behavior we want. Critically, once I add support for SuggestionSpan, I'm going to need to support menu items where some of the text is bolded and some isn't (we need to show a regular/bold/regular sandwich). DropdownItem and DropdownAdapter don't appear to support this use case, and I'm not sure if it's possible to extend them in a general way to support it. We don't really have to use DropdownAdapter to use DropdownPopupWindow, we could write our own adapter class, but I'm not too thrilled about that since DropdownAdapter alone is already nearly as long as my SuggestionsPopupWindow class, and we'd still end up with a bunch of code we can't re-use. So I don't think this is likely to work without extensive modifications to DropdownPopupWindow, and I think it would be fairly challenging to get it to behave the way we want it to for this use case.
On 2017/06/30 02:24:30, rlanday wrote: > I started trying to see if it's possible to use DropdownPopupWindow and I'm not > feeling too optimistic. One problem (which may be solvable if I spend some more > time on it) is that I'm having trouble getting the menu to match the Android > spell check menu in appearance. It seems there's some background being drawn on > it which I'm having trouble removing (by calling getListView() and trying to > reset the background) that adds a shadow on the top and a border on the bottom. DropDownPopupWindow's style is defined here: https://cs.chromium.org/chromium/src/ui/android/java/res/values-v17/styles.xm... If you trace through the code, you can find the Android styles.xml definition here: https://cs.chromium.org/chromium/src/third_party/android_tools/sdk/platforms/... One of those attributes is likely responsible. You can define a custom style that overrides the culprit attribute. > More importantly, there's no guarantee that it will actually behave the same way > as the native Android spell check menu (one of the design goals of the feature > is to match that menu as closely as possible). For example, I noticed that > DropdownPopupWindow is currently set up to never show on top of the keyboard > (whereas the Android spell check menu does): > https://chromium.googlesource.com/chromium/src/+/9ee8865d84c2156728f1b9f78fb8... For this specific issue, you could move DropDownPopupWindow.java's call to its constructor, then set a different input method mode in SuggestionsPopupWindow. Or you could set the input mode in a method in DropDownPopupWindow and override it in SuggestionsPopupWindow. If the primary UI goal is match Android, then we should consider working with the Android team to provide a widget in the support library. > DropdownPopupWindow wants to be anchored to a view, whereas for my use case, I > have a caret location I want to center the menu at as closely as possible > without going off-screen. So I think it's unlikely that it's going to produce > the behavior we want. It looks view_android.h exposes a SetAnchorRect that can be utilized to configure the view returned by based on the caret location. If that doesn't work, you could try manipulating the View returned by ::AcquireAnchorView() in your Java SuggestionsPopupWindow. > Critically, once I add support for SuggestionSpan, I'm going to need to support > menu items where some of the text is bolded and some isn't (we need to show a > regular/bold/regular sandwich). DropdownItem and DropdownAdapter don't appear to > support this use case, and I'm not sure if it's possible to extend them in a > general way to support it. We don't really have to use DropdownAdapter to use > DropdownPopupWindow, we could write our own adapter class, but I'm not too > thrilled about that since DropdownAdapter alone is already nearly as long as my > SuggestionsPopupWindow class, and we'd still end up with a bunch of code we > can't re-use. I think you'll have to write your own adapter class regardless. If you don't use DropdownPopupWindow, what adapter would you use instead? BaseAdapter implements ListAdapter, so you can use the SuggestionAdapter you're currently defining with DropdownPopupWindow. > So I don't think this is likely to work without extensive modifications to > DropdownPopupWindow, and I think it would be fairly challenging to get it to > behave the way we want it to for this use case.
I got all the positioning code wired up to verify how DropdownPopupWindow works; it doesn't really do what I want. It takes a view, and then anchors the menu relative to that view (I think we can force it to be anchored under the view like I want, so that's probably not an issue). If the anchor view is narrower than the minimum width to fit the menu contents, the menu is sized to fit its contents (assuming there's space on the screen, I believe). If the anchor view is wider, the menu is sized to match the anchor view. I need my menu *centered* on the caret location. DropdownPopupWindow seems to always left-align (or maybe right-align in RTL mode?) the menu to the left edge of the anchor view. So the only way I can get my menu centered on the caret seems to be if I pass an anchor view with coordinates centered on the caret. But then we need to do all the clipping logic in C++ before we set the anchor view bounds, and we also need to know the size of the menu contents, which we don't have yet (the Android spell check menu's width is sized to fit its contents, we don't know it in advance). So I don't think there's any way to get around rewriting the positioning logic in DropdownPopupWindow if we want to match the behavior on Android (which I think is important to avoid making both Chrome and Android look bad). As for getting the Android team to provide us a widget to use, I would certainly be interested if they wanted to provide us one, but for context, I asked in May if they could make the ACTION_USER_DICTIONARY_INSERT intent part of the public API going forward, and they've decided that the earliest they can do this is in Android P (Google-internal b/38128847). I'm not sure what the release cycle for the support library is like, but that's a bigger request, and I can't imagine they'll be too interested if I already have the menu working without them doing that. So I think we should go ahead with the cloned implementation I have already.
On 2017/06/30 20:04:23, rlanday wrote: > I got all the positioning code wired up to verify how DropdownPopupWindow works; > it doesn't really do what I want. It takes a view, and then anchors the menu > relative to that view (I think we can force it to be anchored under the view > like I want, so that's probably not an issue). > > If the anchor view is narrower than the minimum width to fit the menu contents, > the menu is sized to fit its contents (assuming there's space on the screen, I > believe). If the anchor view is wider, the menu is sized to match the anchor > view. This could be solved with a "minimum width" concept to DropdownPopupWindow. By default it could use the anchor view width, but subclasses could provide a different value. > I need my menu *centered* on the caret location. DropdownPopupWindow seems to > always left-align (or maybe right-align in RTL mode?) the menu to the left edge > of the anchor view. Typically anchored PopupWindows are aligned with the start or end of the anchor view, but you can use horizontal offsets to adjust as needed. > So the only way I can get my menu centered on the caret > seems to be if I pass an anchor view with coordinates centered on the caret. But > then we need to do all the clipping logic in C++ before we set the anchor view > bounds, and we also need to know the size of the menu contents, which we don't > have yet (the Android spell check menu's width is sized to fit its contents, we > don't know it in advance). So I don't think there's any way to get around > rewriting the positioning logic in DropdownPopupWindow if we want to match the > behavior on Android (which I think is important to avoid making both Chrome and > Android look bad). When shown inside of Chrome, we strive for consistency with other Chrome UI over consistency with the framework UI (especially since OEM's can customize). As long as the popup we're showing looks good and functions well, I don't think it will make either Android or Chrome look bad. > As for getting the Android team to provide us a widget to use, I would certainly > be interested if they wanted to provide us one, but for context, I asked in May > if they could make the ACTION_USER_DICTIONARY_INSERT intent part of the public > API going forward, and they've decided that the earliest they can do this is in > Android P (Google-internal b/38128847). I'm not sure what the release cycle for > the support library is like, but that's a bigger request, and I can't imagine > they'll be too interested if I already have the menu working without them doing > that. Changes to the framework API have to be made with the major Android version releases. The support library is on its own schedule and ships more frequently. > So I think we should go ahead with the cloned implementation I have already.
https://codereview.chromium.org/2931443003/diff/420001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/input/SuggestionsPopupWindow.java (right): https://codereview.chromium.org/2931443003/diff/420001/content/public/android... content/public/android/java/src/org/chromium/content/browser/input/SuggestionsPopupWindow.java:58: Context context, TextSuggestionHost textSuggestionHost, View attachedView) { nit: JavaDocs for this public method https://codereview.chromium.org/2931443003/diff/420001/content/public/android... content/public/android/java/src/org/chromium/content/browser/input/SuggestionsPopupWindow.java:61: mAttachedView = attachedView; I think mParentView is a better name for this View. https://codereview.chromium.org/2931443003/diff/420001/content/public/android... content/public/android/java/src/org/chromium/content/browser/input/SuggestionsPopupWindow.java:77: mPopupWindow.setBackgroundDrawable(null); As discussed offline, if you use dropdown_popup_background_down as the background you'll get a consistent look on pre-L and post-L without adding new assets to the APK. https://codereview.chromium.org/2931443003/diff/420001/content/public/android... content/public/android/java/src/org/chromium/content/browser/input/SuggestionsPopupWindow.java:93: mClippingLimitRight = lp.rightMargin; Does the container view need margins or can we simplify the code a bit by dropping them? https://codereview.chromium.org/2931443003/diff/420001/content/public/android... content/public/android/java/src/org/chromium/content/browser/input/SuggestionsPopupWindow.java:166: final DisplayMetrics displayMetrics = mContext.getResources().getDisplayMetrics(); If all of the items have already been added to the content view, you may be able to significantly simplify this. I made a quick prototype of a PoupWindow that contains a LinearLayout with a bunch of text fields all set to wrap_content and the framework is able to get the correct width: int widthMeasureSpec = View.MeasureSpec.makeMeasureSpec(displayMetrics.heightPixels, View.MeasureSpec.UNSPECIFIED); int heightMeasureSpec = View.MeasureSpec.makeMeasureSpec(displayMetrics.widthPixels, View.MeasureSpec.UNSPECIFIED); mContentView.measure(widthMeasureSpec, heightMeasureSpec); https://codereview.chromium.org/2931443003/diff/420001/content/public/android... content/public/android/java/src/org/chromium/content/browser/input/SuggestionsPopupWindow.java:215: mAttachedView.getLocationOnScreen(locationOnScreen); This doesn't appear to be used. The framework's popup window positioning is based on the current window, so if it is intended for use, you probably want getLocationInWindow() instead. https://codereview.chromium.org/2931443003/diff/420001/content/public/android... content/public/android/java/src/org/chromium/content/browser/input/SuggestionsPopupWindow.java:240: positionY = Math.min(positionY, displayMetrics.heightPixels - height); Will this place the popup above the caret location if there's more space above the caret than below? https://codereview.chromium.org/2931443003/diff/420001/content/public/android... content/public/android/java/src/org/chromium/content/browser/input/SuggestionsPopupWindow.java:243: mPopupWindow.update(positionX, positionY, -1, -1); In what scenarios is show() called more than once?
I'm still trying to get the clipping logic to work properly in multi-window mode on Android (the method I'm using to get the window size is including part of the keyboard in portrait mode, and the whole keyboard in landscape mode, when I don't want it to include the keyboard at all). I sent out a message to the android-chatty-eng list and I'm waiting for a response. https://codereview.chromium.org/2931443003/diff/420001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/input/SuggestionsPopupWindow.java (right): https://codereview.chromium.org/2931443003/diff/420001/content/public/android... content/public/android/java/src/org/chromium/content/browser/input/SuggestionsPopupWindow.java:58: Context context, TextSuggestionHost textSuggestionHost, View attachedView) { On 2017/06/30 at 21:00:33, Theresa wrote: > nit: JavaDocs for this public method Ok https://codereview.chromium.org/2931443003/diff/420001/content/public/android... content/public/android/java/src/org/chromium/content/browser/input/SuggestionsPopupWindow.java:61: mAttachedView = attachedView; On 2017/06/30 at 21:00:33, Theresa wrote: > I think mParentView is a better name for this View. Ok https://codereview.chromium.org/2931443003/diff/420001/content/public/android... content/public/android/java/src/org/chromium/content/browser/input/SuggestionsPopupWindow.java:77: mPopupWindow.setBackgroundDrawable(null); On 2017/06/30 at 21:00:33, Theresa wrote: > As discussed offline, if you use dropdown_popup_background_down as the background you'll get a consistent look on pre-L and post-L without adding new assets to the APK. I'm going to use it pre-L; it doesn't look quite the same as the Material Design Android spell check menu (e.g. the corners aren't rounded at the top) so I'm going to use elevation on L and later. https://codereview.chromium.org/2931443003/diff/420001/content/public/android... content/public/android/java/src/org/chromium/content/browser/input/SuggestionsPopupWindow.java:166: final DisplayMetrics displayMetrics = mContext.getResources().getDisplayMetrics(); On 2017/06/30 at 21:00:33, Theresa wrote: > If all of the items have already been added to the content view, you may be able to significantly simplify this. I made a quick prototype of a PoupWindow that contains a LinearLayout with a bunch of text fields all set to wrap_content and the framework is able to get the correct width: > > int widthMeasureSpec = View.MeasureSpec.makeMeasureSpec(displayMetrics.heightPixels, View.MeasureSpec.UNSPECIFIED); > int heightMeasureSpec = View.MeasureSpec.makeMeasureSpec(displayMetrics.widthPixels, View.MeasureSpec.UNSPECIFIED); > mContentView.measure(widthMeasureSpec, heightMeasureSpec); I tried this and it doesn't seem to work properly (even after changing all the layout_widths in the xml files to "wrap_content"). I'm not 100% sure why, but apparently ListView doesn't support wrap_content for width? https://stackoverflow.com/questions/6547154/wrap-content-for-a-listviews-width https://codereview.chromium.org/2931443003/diff/420001/content/public/android... content/public/android/java/src/org/chromium/content/browser/input/SuggestionsPopupWindow.java:215: mAttachedView.getLocationOnScreen(locationOnScreen); On 2017/06/30 at 21:00:33, Theresa wrote: > This doesn't appear to be used. > > The framework's popup window positioning is based on the current window, so if it is intended for use, you probably want getLocationInWindow() instead. Oops; I do use getLocationInWindow() later, this should be removed https://codereview.chromium.org/2931443003/diff/420001/content/public/android... content/public/android/java/src/org/chromium/content/browser/input/SuggestionsPopupWindow.java:240: positionY = Math.min(positionY, displayMetrics.heightPixels - height); On 2017/06/30 at 21:00:33, Theresa wrote: > Will this place the popup above the caret location if there's more space above the caret than below? Yes; it won't anchor it above the text in any specific way (e.g. lining up the bottom of the menu with the top of the text), it will just shift the menu up however much is necessary to make it fit on-screen. https://codereview.chromium.org/2931443003/diff/420001/content/public/android... content/public/android/java/src/org/chromium/content/browser/input/SuggestionsPopupWindow.java:243: mPopupWindow.update(positionX, positionY, -1, -1); On 2017/06/30 at 21:00:33, Theresa wrote: > In what scenarios is show() called more than once? I don't think it ever is (in the original Editor.java it might be, but I don't think it is here). I will remove this.
https://codereview.chromium.org/2931443003/diff/420001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/input/SuggestionsPopupWindow.java (right): https://codereview.chromium.org/2931443003/diff/420001/content/public/android... content/public/android/java/src/org/chromium/content/browser/input/SuggestionsPopupWindow.java:166: final DisplayMetrics displayMetrics = mContext.getResources().getDisplayMetrics(); On 2017/07/05 22:52:02, rlanday wrote: > On 2017/06/30 at 21:00:33, Theresa wrote: > > If all of the items have already been added to the content view, you may be > able to significantly simplify this. I made a quick prototype of a PoupWindow > that contains a LinearLayout with a bunch of text fields all set to wrap_content > and the framework is able to get the correct width: > > > > int widthMeasureSpec = > View.MeasureSpec.makeMeasureSpec(displayMetrics.heightPixels, > View.MeasureSpec.UNSPECIFIED); > > int heightMeasureSpec = > View.MeasureSpec.makeMeasureSpec(displayMetrics.widthPixels, > View.MeasureSpec.UNSPECIFIED); > > mContentView.measure(widthMeasureSpec, heightMeasureSpec); > > I tried this and it doesn't seem to work properly (even after changing all the > layout_widths in the xml files to "wrap_content"). I'm not 100% sure why, but > apparently ListView doesn't support wrap_content for width? > https://stackoverflow.com/questions/6547154/wrap-content-for-a-listviews-width I changed my example app to use a ListView, and I'm seeing truncated text now. My main goal in this suggestion is to find a simpler way to layout this PopupWindow. I'm okay with either of these approaches: 1) Using DropdownPopupWindow with the modifications necessary to make it work better for this use case). This is my preference. 2) A simpler custom implementation. This is my second choice. One way to achieve #2 may be extracting the measureContentWidth() code in DropdownPopupWindow somewhere that can be shared so that the code measuring the list view contents doesn't need to be duplicated.
On 2017/07/06 at 23:48:01, twellington wrote: > My main goal in this suggestion is to find a simpler way to layout this PopupWindow. I'm okay with either of these approaches: > 1) Using DropdownPopupWindow with the modifications necessary to make it work better for this use case). This is my preference. > 2) A simpler custom implementation. This is my second choice. > > One way to achieve #2 may be extracting the measureContentWidth() code in DropdownPopupWindow somewhere that can be shared so that the code measuring the list view contents doesn't need to be duplicated. I'm starting to worry that there's some disagreement around exactly what we're trying to achieve and what our priorities are here that's making it difficult to achieve something that everyone's going to be happy with. I think my and aelias's top priority is: - Make the menu implement the same behaviors as the text view spell check menu, while matching the UI as close as possible, except for obvious bugs. I think I did have a version of the menu that matched the existing Android menu pretty closely at one point, so we had basically achieved this. hannahs, our UX designer, has this as her top priority (similar but not quite the same): - Make the menu match Material Design specs as closely as possible, even if the margins are slightly off on the existing Android text widget menu. After talking to her, I made some minor changes to what I had, which let us satisfy this goal at the expense of the first one. Your and tedchoc's top priority seems to be: - Avoid re-inventing the wheel, re-use existing code where possible. This seems to be tough to achieve together with the first goal. Trying to match the existing Android spell check menu as closely as possible limits the amount of code we can re-use to some extent. I'll try to see if we can re-use some of the content measurement code, I don't think DropdownPopupWindow seems like a good fit though. Then we started adding on some more goals: - Make the menu fit on screen vs. overflowing off the screen, even if the native Android menu doesn't handle this properly. - Make the menu clip properly in multi-window mode, even if the native Android menu doesn't handle this properly (note: seems to not be possible due to Google-internal b/63405914 causing DisplayMetrics.heightPixels to return height values that include part/all of the keyboard). Now we're starting to invent UI behaviors that may not exist either in the Android menu we're copying, nor in existing Chromium code, so we're moving away both from minimizing code complexity and matching the existing Android menu. I'm going to update this CL tomorrow with some code that I *think* makes the menu clip properly (except for the multi-window case, which is difficult because of the Android bug), and I'll try to see if we can refactor DropdownPopupWindow.measureContentWidth() or something, but I'm not sure how much other code we can re-use while trying to keep the UI matching the native Android menu. I'm also starting to wonder if we should maybe punt on some of the clipping behavior for now and decide either to just match the native Android menu, or come back and revise it in another CL to make review easier.
https://codereview.chromium.org/2931443003/diff/420001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/input/SuggestionsPopupWindow.java (right): https://codereview.chromium.org/2931443003/diff/420001/content/public/android... content/public/android/java/src/org/chromium/content/browser/input/SuggestionsPopupWindow.java:93: mClippingLimitRight = lp.rightMargin; On 2017/06/30 at 21:00:33, Theresa wrote: > Does the container view need margins or can we simplify the code a bit by dropping them? I think the margins are necessary for the drop shadow to display properly.
The CQ bit was checked by rlanday@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...
https://codereview.chromium.org/2931443003/diff/440001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/input/SuggestionsPopupWindow.java (right): https://codereview.chromium.org/2931443003/diff/440001/content/public/android... content/public/android/java/src/org/chromium/content/browser/input/SuggestionsPopupWindow.java:192: // Hack: specify AT_MOST with large vertical height because UNSPECIFIED makes I don't understand why it's not working with UNSPECIFIED, is there something I'm doing wrong? https://codereview.chromium.org/2931443003/diff/440001/content/public/android... content/public/android/java/src/org/chromium/content/browser/input/SuggestionsPopupWindow.java:260: int statusBarHeight = 0; I copied this from LegacyPastePopupMenu, does this seem legit?
I've added an integration test (TextSuggestionMenuTest, it tests opening the menu and picking the "Delete" option), added some code to try to guarantee that the menu fits on-screen, tried to work as well as possible in multi-window mode, and hard coded the menu items as blue since they were showing up as green using Chrome's colorAccent problem, which hannahs@ didn't like. I'm going to try to see if we can share some measuring code with DropdownPopupWindow.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_compile_dbg_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
On 2017/07/07 at 19:36:55, rlanday wrote: > I've added an integration test (TextSuggestionMenuTest, it tests opening the menu and picking the "Delete" option), added some code to try to guarantee that the menu fits on-screen, tried to work as well as possible in multi-window mode, and hard coded the menu items as blue since they were showing up as green using Chrome's colorAccent problem, which hannahs@ didn't like. I'm going to try to see if we can share some measuring code with DropdownPopupWindow. It looks like there are maybe about 10 lines of code in DropdownPopupWindow.measureContentWidth() we could potentially re-use, to get the widest item returned by a ListAdapter? We'd still have to also check the "Add to dictionary" and "Delete" buttons (these are currently fixed children of the menu's view rather than being returned by the ListAdapter; the ListAdapter is only used for suggestions). Do we want to add a method somewhere to get the widest item in a ListAdapter? That actually might be a somewhat dangerous method to have for general use for the same reason ListView doesn't generally expose this functionality (the performance implications of going through a potentially large number of children views).
The CQ bit was checked by rlanday@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_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
On 2017/07/07 19:52:52, rlanday wrote: > On 2017/07/07 at 19:36:55, rlanday wrote: > > I've added an integration test (TextSuggestionMenuTest, it tests opening the > menu and picking the "Delete" option), added some code to try to guarantee that > the menu fits on-screen, tried to work as well as possible in multi-window mode, > and hard coded the menu items as blue since they were showing up as green using > Chrome's colorAccent problem, which hannahs@ didn't like. I'm going to try to > see if we can share some measuring code with DropdownPopupWindow. > > It looks like there are maybe about 10 lines of code in > DropdownPopupWindow.measureContentWidth() we could potentially re-use, to get > the widest item returned by a ListAdapter? We'd still have to also check the > "Add to dictionary" and "Delete" buttons (these are currently fixed children of > the menu's view rather than being returned by the ListAdapter; the ListAdapter > is only used for suggestions). ListView has an addFooterView() method that you may be able to utilize to add "Add to dictionary" and "Delete" as fixed items at the end of the list. https://developer.android.com/reference/android/widget/ListView.html#addFoote... > Do we want to add a method somewhere to get the > widest item in a ListAdapter? That actually might be a somewhat dangerous method > to have for general use for the same reason ListView doesn't generally expose > this functionality (the performance implications of going through a potentially > large number of children views). JavaDocs explaining the performance implications could mitigate that concern.
https://codereview.chromium.org/2931443003/diff/440001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/input/SuggestionsPopupWindow.java (right): https://codereview.chromium.org/2931443003/diff/440001/content/public/android... content/public/android/java/src/org/chromium/content/browser/input/SuggestionsPopupWindow.java:192: // Hack: specify AT_MOST with large vertical height because UNSPECIFIED makes On 2017/07/07 19:36:15, rlanday wrote: > I don't understand why it's not working with UNSPECIFIED, is there something I'm > doing wrong? I suspect that since the popup can draw offscreen unspecified doesn't take into account the available height. https://codereview.chromium.org/2931443003/diff/440001/content/public/android... content/public/android/java/src/org/chromium/content/browser/input/SuggestionsPopupWindow.java:260: int statusBarHeight = 0; On 2017/07/07 19:36:15, rlanday wrote: > I copied this from LegacyPastePopupMenu, does this seem legit? We should be able to get display metrics that don't include the decor views in the height. In addition to the status bar, Android's decor view also includes the bottom bar with back, home, and recents buttons.
On 2017/07/10 at 17:54:12, twellington wrote: > On 2017/07/07 19:52:52, rlanday wrote: > > On 2017/07/07 at 19:36:55, rlanday wrote: > > > I've added an integration test (TextSuggestionMenuTest, it tests opening the > > menu and picking the "Delete" option), added some code to try to guarantee that > > the menu fits on-screen, tried to work as well as possible in multi-window mode, > > and hard coded the menu items as blue since they were showing up as green using > > Chrome's colorAccent problem, which hannahs@ didn't like. I'm going to try to > > see if we can share some measuring code with DropdownPopupWindow. > > > > It looks like there are maybe about 10 lines of code in > > DropdownPopupWindow.measureContentWidth() we could potentially re-use, to get > > the widest item returned by a ListAdapter? We'd still have to also check the > > "Add to dictionary" and "Delete" buttons (these are currently fixed children of > > the menu's view rather than being returned by the ListAdapter; the ListAdapter > > is only used for suggestions). > > ListView has an addFooterView() method that you may be able to utilize to add "Add to dictionary" and "Delete" as fixed items at the end of the list. I tried this; I don't think it's going to work because we want to show a divider between the suggestions and the "Add to dictionary" button. If we try to add the divider ourselves, we either have to attach it as its own footer view, or in the same list item as the "Add to dictionary" button. Either of these options produces weird UX if the user taps the divider (the divider, or the whole "Add to dictionary" button together with the divider, gets highlighted). If we want to use a divider provided by ListView, I think there are three settings we have: - Disable all dividers in the ListView (doesn't work, we don't get one between the suggestions and "Add to dictionary" - Enable all dividers and do setFooterDividersEnabled(true) (doesn't work, we get dividers between all menu items, but we only the one before "Add to dictionary") - Enable all dividers and do setFooterDividersEnabled(false) (doesn't work, we get dividers between all menu items *except* the one we want, and between "Add to dictionary" and "Delete") I think the only thing that sort of works is to just disable all the dividers (so we don't get one between the suggestions and "Add to dictionary"). This is how the native EditText menu appears in the Hangouts app (vs. e.g. Messages, which does show the divider), but it doesn't really look right to me. If we can't have "Add to dictionary" and "Delete" as part of the ListView, can we still re-use the measuring code?
On 2017/07/10 at 17:59:35, twellington wrote: > I suspect that since the popup can draw offscreen unspecified doesn't take into account the available height. > > https://codereview.chromium.org/2931443003/diff/440001/content/public/android... > content/public/android/java/src/org/chromium/content/browser/input/SuggestionsPopupWindow.java:260: int statusBarHeight = 0; > On 2017/07/07 19:36:15, rlanday wrote: > > I copied this from LegacyPastePopupMenu, does this seem legit? > > We should be able to get display metrics that don't include the decor views in the height. In addition to the status bar, Android's decor view also includes the bottom bar with back, home, and recents buttons. I'm unsure how to do this. When I get DisplayMetrics (either doing mWindowAndroidProvider.getWindowAndroid().getActivity().get().getResources().getDisplayMetrics() to get correct values for multi-window mode (as correct as we're going to get, at least), or doing mContext.getResources().getDisplayMetrics()), I get these values for the window size on my Nexus 6P: Portrait mode: Height: 2392px Width: 1440px Landscape mode: Height: 1440px Width: 2392px The Nexus 6P's display is 1440px by 2560px. So getDisplayMetrics() doesn't seem to be including the bottom bar, but it *is* including the status bar (this is at the top of the screen even in landscape mode, but the height reported there is the full device height).
On 2017/07/10 21:40:12, rlanday wrote: > On 2017/07/10 at 17:54:12, twellington wrote: > > On 2017/07/07 19:52:52, rlanday wrote: > > > On 2017/07/07 at 19:36:55, rlanday wrote: > > > > I've added an integration test (TextSuggestionMenuTest, it tests opening > the > > > menu and picking the "Delete" option), added some code to try to guarantee > that > > > the menu fits on-screen, tried to work as well as possible in multi-window > mode, > > > and hard coded the menu items as blue since they were showing up as green > using > > > Chrome's colorAccent problem, which hannahs@ didn't like. I'm going to try > to > > > see if we can share some measuring code with DropdownPopupWindow. > > > > > > It looks like there are maybe about 10 lines of code in > > > DropdownPopupWindow.measureContentWidth() we could potentially re-use, to > get > > > the widest item returned by a ListAdapter? We'd still have to also check the > > > "Add to dictionary" and "Delete" buttons (these are currently fixed children > of > > > the menu's view rather than being returned by the ListAdapter; the > ListAdapter > > > is only used for suggestions). > > > > ListView has an addFooterView() method that you may be able to utilize to add > "Add to dictionary" and "Delete" as fixed items at the end of the list. > > I tried this; I don't think it's going to work because we want to show a divider > between the suggestions and the "Add to dictionary" button. > > If we try to add the divider ourselves, we either have to attach it as its own > footer view, or in the same list item as the "Add to dictionary" button. Either > of these options produces weird UX if the user taps the divider (the divider, or > the whole "Add to dictionary" button together with the divider, gets > highlighted). > > If we want to use a divider provided by ListView, I think there are three > settings we have: > - Disable all dividers in the ListView (doesn't work, we don't get one between > the suggestions and "Add to dictionary" > - Enable all dividers and do setFooterDividersEnabled(true) (doesn't work, we > get dividers between all menu items, but we only the one before "Add to > dictionary") > - Enable all dividers and do setFooterDividersEnabled(false) (doesn't work, we > get dividers between all menu items *except* the one we want, and between "Add > to dictionary" and "Delete") > > I think the only thing that sort of works is to just disable all the dividers > (so we don't get one between the suggestions and "Add to dictionary"). This is > how the native EditText menu appears in the Hangouts app (vs. e.g. Messages, > which does show the divider), but it doesn't really look right to me. > > > If we can't have "Add to dictionary" and "Delete" as part of the ListView, can > we still re-use the measuring code? Since addFooterView() just takes a View, maybe there could be one one footer with both buttons (and optionally a divider line, or you could use #setFooterDividersEnabled())?
On 2017/07/10 22:13:58, rlanday wrote: > On 2017/07/10 at 17:59:35, twellington wrote: > > I suspect that since the popup can draw offscreen unspecified doesn't take > into account the available height. > > > > > https://codereview.chromium.org/2931443003/diff/440001/content/public/android... > > > content/public/android/java/src/org/chromium/content/browser/input/SuggestionsPopupWindow.java:260: > int statusBarHeight = 0; > > On 2017/07/07 19:36:15, rlanday wrote: > > > I copied this from LegacyPastePopupMenu, does this seem legit? > > > > We should be able to get display metrics that don't include the decor views in > the height. In addition to the status bar, Android's decor view also includes > the bottom bar with back, home, and recents buttons. > > I'm unsure how to do this. When I get DisplayMetrics (either doing > mWindowAndroidProvider.getWindowAndroid().getActivity().get().getResources().getDisplayMetrics() > to get correct values for multi-window mode (as correct as we're going to get, > at least), or doing mContext.getResources().getDisplayMetrics()), I get these > values for the window size on my Nexus 6P: > > Portrait mode: > Height: 2392px > Width: 1440px > > Landscape mode: > Height: 1440px > Width: 2392px > > The Nexus 6P's display is 1440px by 2560px. So getDisplayMetrics() doesn't seem > to be including the bottom bar, but it *is* including the status bar (this is at > the top of the screen even in landscape mode, but the height reported there is > the full device height). For the AppMenu we use mActivity#getWindow()#getDecorView()#getWindowVisibleDisplayFrame() to determine the status bar height. Does something like that work?
On 2017/07/10 at 23:02:20, twellington wrote: > Since addFooterView() just takes a View, maybe there could be one one footer with both buttons (and optionally a divider line, or you could use #setFooterDividersEnabled())? You can call addFooterView() multiple times to add multiple views. The issue is that each view you add is a separate tappable item in the ListView. You can add click handlers on "Add to dictionary" and "Delete" to handle those buttons separately, but then if someone taps on the divider, the divider gets highlighted as its own menu item (or if combined with the buttons, the whole thing highlights together, which looks super weird). setFooterDividersEnabled() doesn't really work because I'm already disabling the divider on the ListView to not show dividers between the suggestions. I don't know of a way to show a divider before the footer, but not between the other items.
On 2017/07/10 23:23:44, rlanday wrote:
> On 2017/07/10 at 23:02:20, twellington wrote:
> > Since addFooterView() just takes a View, maybe there could be one one footer
> with both buttons (and optionally a divider line, or you could use
> #setFooterDividersEnabled())?
>
> You can call addFooterView() multiple times to add multiple views. The issue
is
> that each view you add is a separate tappable item in the ListView. You can
add
> click handlers on "Add to dictionary" and "Delete" to handle those buttons
> separately, but then if someone taps on the divider, the divider gets
> highlighted as its own menu item (or if combined with the buttons, the whole
> thing highlights together, which looks super weird).
>
> setFooterDividersEnabled() doesn't really work because I'm already disabling
the
> divider on the ListView to not show dividers between the suggestions. I don't
> know of a way to show a divider before the footer, but not between the other
> items.
I'm suggesting adding one footer view rather than multiple views. In my
prototype this appears to work well. If I tap directly on the divider line
(which is hard since it's only 1dp), nothing happens.
e.g.
<?xml version="1.0" encoding="utf-8"?>
<LinearLayout xmlns:android="http://schemas.android.com/apk/res/android"
android:orientation="vertical" android:layout_width="match_parent"
android:layout_height="match_parent">
<View
android:background="?android:attr/listDivider"
android:layout_height="1dp"
android:layout_width="match_parent" />
<Button
android:layout_width="match_parent"
android:layout_height="wrap_content"
android:text="Add to dictionary"
style="?android:attr/borderlessButtonStyle" />
<Button
android:layout_width="match_parent"
android:layout_height="wrap_content"
android:text="Delete"
style="?android:attr/borderlessButtonStyle" />
</LinearLayout>
On 2017/07/10 at 23:28:47, twellington wrote: > I'm suggesting adding one footer view rather than multiple views. In my prototype this appears to work well. If I tap directly on the divider line (which is hard since it's only 1dp), nothing happens. The difference may be that I added 8dp of padding around the divider since hannahs@ thought we matched the Material Design spec better that way, so then the divider is both easier to tap on, and also visibly highlights when tapped. But I'm open to getting rid of this, since the native Android spell check menu doesn't do this (and aelias@ therefore wasn't really a fan of this suggestion). Then I think we can do what you want with the sizing logic. If we undo the 8dp of padding here, I think we should also remove it from the top and bottom of the menu and just clone the existing (but maybe not 100% correct) Android layout.
On 2017/07/10 at 23:06:08, twellington wrote: > On 2017/07/10 22:13:58, rlanday wrote: > > I'm unsure how to do this. When I get DisplayMetrics (either doing > > mWindowAndroidProvider.getWindowAndroid().getActivity().get().getResources().getDisplayMetrics() > > to get correct values for multi-window mode (as correct as we're going to get, > > at least), or doing mContext.getResources().getDisplayMetrics()), I get these > > values for the window size on my Nexus 6P: > > > > Portrait mode: > > Height: 2392px > > Width: 1440px > > > > Landscape mode: > > Height: 1440px > > Width: 2392px > > > > The Nexus 6P's display is 1440px by 2560px. So getDisplayMetrics() doesn't seem > > to be including the bottom bar, but it *is* including the status bar (this is at > > the top of the screen even in landscape mode, but the height reported there is > > the full device height). > > For the AppMenu we use mActivity#getWindow()#getDecorView()#getWindowVisibleDisplayFrame() to determine the status bar height. Does something like that work? That doesn't really do the right thing either because it doesn't include the area taken up by the keyboard (which is OK to draw over in single-window mode). I'm getting: Portrait mode: Height: 1321px Width: 1440px Landscape mode: Height: 610px Width: 2392px
On 2017/07/11 00:15:37, rlanday wrote: > On 2017/07/10 at 23:06:08, twellington wrote: > > On 2017/07/10 22:13:58, rlanday wrote: > > > I'm unsure how to do this. When I get DisplayMetrics (either doing > > > > mWindowAndroidProvider.getWindowAndroid().getActivity().get().getResources().getDisplayMetrics() > > > to get correct values for multi-window mode (as correct as we're going to > get, > > > at least), or doing mContext.getResources().getDisplayMetrics()), I get > these > > > values for the window size on my Nexus 6P: > > > > > > Portrait mode: > > > Height: 2392px > > > Width: 1440px > > > > > > Landscape mode: > > > Height: 1440px > > > Width: 2392px > > > > > > The Nexus 6P's display is 1440px by 2560px. So getDisplayMetrics() doesn't > seem > > > to be including the bottom bar, but it *is* including the status bar (this > is at > > > the top of the screen even in landscape mode, but the height reported there > is > > > the full device height). > > > > For the AppMenu we use > mActivity#getWindow()#getDecorView()#getWindowVisibleDisplayFrame() to determine > the status bar height. Does something like that work? > > That doesn't really do the right thing either because it doesn't include the > area taken up by the keyboard (which is OK to draw over in single-window mode). > I'm getting: > > Portrait mode: > Height: 1321px > Width: 1440px > > Landscape mode: > Height: 610px > Width: 2392px If you look at AppMenu.java (linked below), we're using the visible display frame to get the status bar height, but using getDisplayMetrics().heightPixels to determine the overall height https://cs.chromium.org/chromium/src/chrome/android/java/src/org/chromium/chr... I suspect this may be not-quite-right in MW mode if the app is docked to the bottom of the screen since AppMenu was implemented before Android added MW. But you could detect this by seeing if the container view's top location on screen is equal to the visible display frame top (or possibly looking at the difference between the two to figure out status bar height).
On 2017/07/10 23:48:58, rlanday wrote: > On 2017/07/10 at 23:28:47, twellington wrote: > > I'm suggesting adding one footer view rather than multiple views. In my > prototype this appears to work well. If I tap directly on the divider line > (which is hard since it's only 1dp), nothing happens. > > The difference may be that I added 8dp of padding around the divider since > hannahs@ thought we matched the Material Design spec better that way, so then > the divider is both easier to tap on, and also visibly highlights when tapped. > But I'm open to getting rid of this, since the native Android spell check menu > doesn't do this (and aelias@ therefore wasn't really a fan of this suggestion). > Then I think we can do what you want with the sizing logic. If we undo the 8dp > of padding here, I think we should also remove it from the top and bottom of the > menu and just clone the existing (but maybe not 100% correct) Android layout. I think the buttons as a listview footer instead of a custom layout makes more sense, and it allows the measuring code to be simplified. I'm fine with removing the 8dp of padding if that's necessary, but I'll test tomorrow in my prototype app to see if there's a combination of XML attributes that allows you to keep the padding (e.g. setting clickable=false or selectable=false to the divider view).
On 2017/07/11 at 00:51:55, twellington wrote: > I'm fine with removing the 8dp of padding if that's necessary, but I'll test tomorrow in my prototype app to see if there's a combination of XML attributes that allows you to keep the padding (e.g. setting clickable=false or selectable=false to the divider view). I'm just going to go ahead with doing this; I think if I don't, eventually someone's likely to complain that we're not matching the native Android menu. I do feel more comfortable using your footer idea knowing that there's (maybe) a way we could add the padding back if the EditText menu is ever updated/fixed.
I've updated this CL with several changes: - SuggestionsPopupWindow now uses a method I pulled out from DropdownPopupWindow into UiUtils to determine the width of the popup (note: the UiUtils changes were reviewed in https://chromium-review.googlesource.com/c/566594/, so I'll remove them from this CL when that one lands) - I removed the 8dp of padding at the top/bottom of the menu and around the divider (hannahs@ requested this, but aelias@ thinks it's better to match the existing menu, and I also think it's important to match that menu) - The divider and the "Add to dictionary" and "Delete" buttons are now included in ListView as a footer (so they can use the measuring method I'm adding to UiUtils) - I changed how we're getting the status bar height to match AppMenu - I'm now using ?android:attr/colorAccent for the button color in the menu on Lollipop and later, so this menu should pick up the correct button colors when running in a WebView (I set colorAccent to be blue for Chrome in https://chromium-review.googlesource.com/c/566031/) - I added a method TextSuggestionController::FirstMarkerIntersectingRange() so we don't need to add DocumentMarkerController::MarkersIntersectingRange() (https://codereview.chromium.org/2948133004) until we support SuggestionSpans and actually need to get multiple markers (yosin@ didn't want to add a method to DocumentMarkerController that we don't need yet) Note that this CL still depends on a series of editing patches that need to be removed.
On 2017/07/12 at 21:47:09, rlanday wrote: > I've updated this CL with several changes: > > - SuggestionsPopupWindow now uses a method I pulled out from DropdownPopupWindow into UiUtils to determine the width of the popup (note: the UiUtils changes were reviewed in https://chromium-review.googlesource.com/c/566594/, so I'll remove them from this CL when that one lands) > - I removed the 8dp of padding at the top/bottom of the menu and around the divider (hannahs@ requested this, but aelias@ thinks it's better to match the existing menu, and I also think it's important to match that menu) > - The divider and the "Add to dictionary" and "Delete" buttons are now included in ListView as a footer (so they can use the measuring method I'm adding to UiUtils) > - I changed how we're getting the status bar height to match AppMenu > - I'm now using ?android:attr/colorAccent for the button color in the menu on Lollipop and later, so this menu should pick up the correct button colors when running in a WebView (I set colorAccent to be blue for Chrome in https://chromium-review.googlesource.com/c/566031/) > - I added a method TextSuggestionController::FirstMarkerIntersectingRange() so we don't need to add DocumentMarkerController::MarkersIntersectingRange() (https://codereview.chromium.org/2948133004) until we support SuggestionSpans and actually need to get multiple markers (yosin@ didn't want to add a method to DocumentMarkerController that we don't need yet) > > Note that this CL still depends on a series of editing patches that need to be removed. s/need to be removed/need to be reviewed/
The CQ bit was checked by rlanday@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 rlanday@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_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
https://codereview.chromium.org/2931443003/diff/420001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/input/SuggestionsPopupWindow.java (right): https://codereview.chromium.org/2931443003/diff/420001/content/public/android... content/public/android/java/src/org/chromium/content/browser/input/SuggestionsPopupWindow.java:93: mClippingLimitRight = lp.rightMargin; On 2017/07/07 03:18:28, rlanday wrote: > On 2017/06/30 at 21:00:33, Theresa wrote: > > Does the container view need margins or can we simplify the code a bit by > dropping them? > > I think the margins are necessary for the drop shadow to display properly. When a 9-patch is used, the view's width should include the necessary space for the background/shadows. Is this different when android:elevation is used? https://codereview.chromium.org/2931443003/diff/500001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/input/SuggestionsPopupWindow.java (right): https://codereview.chromium.org/2931443003/diff/500001/content/public/android... content/public/android/java/src/org/chromium/content/browser/input/SuggestionsPopupWindow.java:203: 10 * displayMetrics.heightPixels, View.MeasureSpec.AT_MOST); Does this work? final int verticalMeasure = View.MeasureSpec.makeMeasureSpec(displayMetrics.heightPixels, View.MeasureSpec.AT_MOST); That doesn't seem like a hack, since the display height is the real maximum height of the menu. We may also want to subtract some of the padding/margin from the max height. https://codereview.chromium.org/2931443003/diff/500001/content/public/android... content/public/android/java/src/org/chromium/content/browser/input/SuggestionsPopupWindow.java:255: final int statusBarHeight = rect.top; If Chrome is docked to the bottom of the screen in multi-window mode, I think this value won't represent the status bar height. https://codereview.chromium.org/2931443003/diff/500001/content/public/android... content/public/android/java/src/org/chromium/content/browser/input/SuggestionsPopupWindow.java:263: height = mContentView.getMeasuredHeight(); Is this dropping suggestions until the measured height fits on screen? That seems rather inefficient. If we know the available screen space, we can calculate the number of items that will fit. You can get the item height programatically, checkout AppMenuHandler for an example. This may be another good candidate to pull out into UiUtils. https://codereview.chromium.org/2931443003/diff/500001/content/public/android... content/public/android/java/src/org/chromium/content/browser/input/SuggestionsPopupWindow.java:270: // We need to render the popup relative to the window nit: period at the end of this sentence https://codereview.chromium.org/2931443003/diff/500001/content/public/android... content/public/android/java/src/org/chromium/content/browser/input/SuggestionsPopupWindow.java:272: mParentView.getLocationInWindow(positionInWindow); Remind me, is the parent the CompositorViewHolder? If so, the position in window Y is either 0 or the toolbar height depending on whether the toolbar is showing, right? Is the X position always 0? https://codereview.chromium.org/2931443003/diff/500001/content/public/android... content/public/android/java/src/org/chromium/content/browser/input/SuggestionsPopupWindow.java:278: positionY -= mContainerView.getPaddingTop(); Why are we subtracting the margin and padding here? If it's okay for the margin and padding not to be visible on screen, can we just remove them entirely? https://codereview.chromium.org/2931443003/diff/500001/content/public/android... content/public/android/java/src/org/chromium/content/browser/input/SuggestionsPopupWindow.java:281: positionX = Math.min(displayMetrics.widthPixels - width + mClippingLimitRight, positionX); Will you please define a variable for (displayMetrics.widthPixels - width + mClippingLimitRight) so that it's obvious what the equation represents? https://codereview.chromium.org/2931443003/diff/500001/content/public/android... content/public/android/java/src/org/chromium/content/browser/input/SuggestionsPopupWindow.java:282: positionX = Math.max(-mClippingLimitLeft, positionX); mClippingLimitLeft = left margin of suggestionWindowContainer. So this will be Math.max(-20, positionX), right? Why do we need a 20dp margin if we allow the Popup to draw -20dp off screen?
https://codereview.chromium.org/2931443003/diff/420001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/input/SuggestionsPopupWindow.java (right): https://codereview.chromium.org/2931443003/diff/420001/content/public/android... content/public/android/java/src/org/chromium/content/browser/input/SuggestionsPopupWindow.java:93: mClippingLimitRight = lp.rightMargin; On 2017/07/13 at 16:47:39, Theresa wrote: > On 2017/07/07 03:18:28, rlanday wrote: > > On 2017/06/30 at 21:00:33, Theresa wrote: > > > Does the container view need margins or can we simplify the code a bit by > > dropping them? > > > > I think the margins are necessary for the drop shadow to display properly. > > When a 9-patch is used, the view's width should include the necessary space for the background/shadows. Is this different when android:elevation is used? All I know is that it doesn't work without the margin. I think the shadow might be getting clipped to the parent view (RelativeLayout)'s bounds or something. https://codereview.chromium.org/2931443003/diff/500001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/input/SuggestionsPopupWindow.java (right): https://codereview.chromium.org/2931443003/diff/500001/content/public/android... content/public/android/java/src/org/chromium/content/browser/input/SuggestionsPopupWindow.java:203: 10 * displayMetrics.heightPixels, View.MeasureSpec.AT_MOST); On 2017/07/13 at 16:47:40, Theresa wrote: > Does this work? > > final int verticalMeasure = View.MeasureSpec.makeMeasureSpec(displayMetrics.heightPixels, View.MeasureSpec.AT_MOST); > > > That doesn't seem like a hack, since the display height is the real maximum height of the menu. We may also want to subtract some of the padding/margin from the max height. If I rewrite the code to do some math to figure out how many items we can fit on-screen before drawing the menu, this will probably work. As written, we need to be able to tell if the menu will fit on screen or not, which requires being able to potentially size it taller than the screen (to know that it's too tall). https://codereview.chromium.org/2931443003/diff/500001/content/public/android... content/public/android/java/src/org/chromium/content/browser/input/SuggestionsPopupWindow.java:255: final int statusBarHeight = rect.top; On 2017/07/13 at 16:47:40, Theresa wrote: > If Chrome is docked to the bottom of the screen in multi-window mode, I think this value won't represent the status bar height. I copied this from AppMenuHandler like I think you suggested: https://cs.chromium.org/chromium/src/chrome/android/java/src/org/chromium/chr... Should I change it back to what I had before? https://codereview.chromium.org/2931443003/diff/500001/content/public/android... content/public/android/java/src/org/chromium/content/browser/input/SuggestionsPopupWindow.java:263: height = mContentView.getMeasuredHeight(); On 2017/07/13 at 16:47:40, Theresa wrote: > Is this dropping suggestions until the measured height fits on screen? > > That seems rather inefficient. If we know the available screen space, we can calculate the number of items that will fit. You can get the item height programatically, checkout AppMenuHandler for an example. > > This may be another good candidate to pull out into UiUtils. Yes, that's what it's doing. I will try to do the computation directly. I don't think it will be re-usable by other code unless we add a bunch of complexity like checking all views vs. using our fixed height, accounting for dividers, etc. https://codereview.chromium.org/2931443003/diff/500001/content/public/android... content/public/android/java/src/org/chromium/content/browser/input/SuggestionsPopupWindow.java:270: // We need to render the popup relative to the window On 2017/07/13 at 16:47:40, Theresa wrote: > nit: period at the end of this sentence Ok https://codereview.chromium.org/2931443003/diff/500001/content/public/android... content/public/android/java/src/org/chromium/content/browser/input/SuggestionsPopupWindow.java:272: mParentView.getLocationInWindow(positionInWindow); On 2017/07/13 at 16:47:40, Theresa wrote: > Remind me, is the parent the CompositorViewHolder? > > If so, the position in window Y is either 0 or the toolbar height depending on whether the toolbar is showing, right? Is the X position always 0? I believe the parent is the ContentView. So for example, in Chrome with the toolbar on top of the web page, I get X = 0, Y = 84. In Gmail, when composing an email, I get X = 56, Y = 880 (I guess this is where the WebView starts in the Gmail window). So no, X is not always 0. Note that it's still possible for the popup to draw against the left edge of the window in this case, even though the WebView starts 56px over (this seems like correct behavior to me). https://codereview.chromium.org/2931443003/diff/500001/content/public/android... content/public/android/java/src/org/chromium/content/browser/input/SuggestionsPopupWindow.java:278: positionY -= mContainerView.getPaddingTop(); On 2017/07/13 at 16:47:40, Theresa wrote: > Why are we subtracting the margin and padding here? If it's okay for the margin and padding not to be visible on screen, can we just remove them entirely? They are being subtracted so the menu lines up properly with the text. Note that there isn't actually any explicit padding (so there's nothing to remove), but the background image we're using pre-L means we have to subtract the top padding in that case so the menu lines up properly. The top margin can't be removed because then the drop shadow wouldn't appear properly on L and later. Also, it's not really ok for the top/bottom margins to not be visible on screen; the native Android spell check menu is fine appearing right up against the left or right edge of the screen, but will shift its vertical position above the baseline of the text to maintain a margin at the bottom of the screen. It doesn't really handle the case where there's no space at the top of the screen either (this is the case where we have to remove items to get the menu to fit, which the native Android one doesn't handle), but my assumption is that the margin behavior should be the same at the top and bottom of the screen. https://codereview.chromium.org/2931443003/diff/500001/content/public/android... content/public/android/java/src/org/chromium/content/browser/input/SuggestionsPopupWindow.java:281: positionX = Math.min(displayMetrics.widthPixels - width + mClippingLimitRight, positionX); On 2017/07/13 at 16:47:40, Theresa wrote: > Will you please define a variable for (displayMetrics.widthPixels - width + mClippingLimitRight) so that it's obvious what the equation represents? This is the furthest right we can draw the menu without it falling off the right edge of the window. I will try to come up with a variable name... https://codereview.chromium.org/2931443003/diff/500001/content/public/android... content/public/android/java/src/org/chromium/content/browser/input/SuggestionsPopupWindow.java:282: positionX = Math.max(-mClippingLimitLeft, positionX); On 2017/07/13 at 16:47:40, Theresa wrote: > mClippingLimitLeft = left margin of suggestionWindowContainer. So this will be Math.max(-20, positionX), right? Why do we need a 20dp margin if we allow the Popup to draw -20dp off screen? The margin is apparently necessary for the drop shadow to display properly (for the case where it doesn't fall off-screen).
https://codereview.chromium.org/2931443003/diff/420001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/input/SuggestionsPopupWindow.java (right): https://codereview.chromium.org/2931443003/diff/420001/content/public/android... content/public/android/java/src/org/chromium/content/browser/input/SuggestionsPopupWindow.java:93: mClippingLimitRight = lp.rightMargin; On 2017/07/13 21:10:29, rlanday wrote: > On 2017/07/13 at 16:47:39, Theresa wrote: > > On 2017/07/07 03:18:28, rlanday wrote: > > > On 2017/06/30 at 21:00:33, Theresa wrote: > > > > Does the container view need margins or can we simplify the code a bit by > > > dropping them? > > > > > > I think the margins are necessary for the drop shadow to display properly. > > > > When a 9-patch is used, the view's width should include the necessary space > for the background/shadows. Is this different when android:elevation is used? > > All I know is that it doesn't work without the margin. I think the shadow might > be getting clipped to the parent view (RelativeLayout)'s bounds or something. On L+ or pre-L? The shadow not overdrawing its parents bounds makes sense, but let's confirm. We should always seek to understand what's going on and why code/XML attributes are needed so that we can be sure the code is technically correct. https://codereview.chromium.org/2931443003/diff/500001/content/public/android... File content/public/android/java/res/layout-v21/text_edit_suggestion_container.xml (right): https://codereview.chromium.org/2931443003/diff/500001/content/public/android... content/public/android/java/res/layout-v21/text_edit_suggestion_container.xml:12: of setting elevation. --> It looks like this comment and the one in your other text_edit_suggestion_container.xml got flipped. Please extract common styles for the two XML files into styles.xml to avoid drift. Or you could use one layout file and create two drawable files with the same name (one in drawable and one in drawable-v21). https://codereview.chromium.org/2931443003/diff/500001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/input/SuggestionsPopupWindow.java (right): https://codereview.chromium.org/2931443003/diff/500001/content/public/android... content/public/android/java/src/org/chromium/content/browser/input/SuggestionsPopupWindow.java:110: mClippingLimitRight = lp.rightMargin; Let's define something in dimens.xml with a meaningful name (e.g. mContainerMargin) and retrieve from resources directly rather than pulling MarginLayoutParams from the view itself. Also, you can probably combine mClippingLimitLeft mClippingLimitRight and mContainerMarginTop into one param since you're using 20dp for all of them. https://codereview.chromium.org/2931443003/diff/500001/content/public/android... content/public/android/java/src/org/chromium/content/browser/input/SuggestionsPopupWindow.java:203: 10 * displayMetrics.heightPixels, View.MeasureSpec.AT_MOST); On 2017/07/13 21:10:29, rlanday wrote: > On 2017/07/13 at 16:47:40, Theresa wrote: > > Does this work? > > > > final int verticalMeasure = > View.MeasureSpec.makeMeasureSpec(displayMetrics.heightPixels, > View.MeasureSpec.AT_MOST); > > > > > > That doesn't seem like a hack, since the display height is the real maximum > height of the menu. We may also want to subtract some of the padding/margin from > the max height. > > If I rewrite the code to do some math to figure out how many items we can fit > on-screen before drawing the menu, this will probably work. Yes please, that's a computationally less expensive way to figure out how many items to display. > As written, we need to be able to tell if the menu will fit on screen or not, > which requires being able to potentially size it taller than the screen (to know > that it's too tall). https://codereview.chromium.org/2931443003/diff/500001/content/public/android... content/public/android/java/src/org/chromium/content/browser/input/SuggestionsPopupWindow.java:255: final int statusBarHeight = rect.top; On 2017/07/13 21:10:29, rlanday wrote: > On 2017/07/13 at 16:47:40, Theresa wrote: > > If Chrome is docked to the bottom of the screen in multi-window mode, I think > this value won't represent the status bar height. > > I copied this from AppMenuHandler like I think you suggested: > https://cs.chromium.org/chromium/src/chrome/android/java/src/org/chromium/chr... > > Should I change it back to what I had before? I suggested that you look at AppMenuHandler with the caveat that it probably wasn't right when Chrome is docked to the bottom of the screen in Android N multi-window since the code was written years go. You can use this code, possibly as is or possibly with modifications. You should flash a device to Android N+ and test that this work in multi-window mode reasonably well. If it doesn't, look for a way to detect whether we're docked to the bottom e.g. maybe you could look at the difference between the app rect and the parent container rect. https://codereview.chromium.org/2931443003/diff/500001/content/public/android... content/public/android/java/src/org/chromium/content/browser/input/SuggestionsPopupWindow.java:263: height = mContentView.getMeasuredHeight(); On 2017/07/13 21:10:29, rlanday wrote: > On 2017/07/13 at 16:47:40, Theresa wrote: > > Is this dropping suggestions until the measured height fits on screen? > > > > That seems rather inefficient. If we know the available screen space, we can > calculate the number of items that will fit. You can get the item height > programatically, checkout AppMenuHandler for an example. > > > > This may be another good candidate to pull out into UiUtils. > > Yes, that's what it's doing. > > I will try to do the computation directly. I don't think it will be re-usable by > other code unless we add a bunch of complexity like checking all views vs. using > our fixed height, accounting for dividers, etc. My sharing suggesting wasn't very clear - I meant sharing the code to get the item height, but as we discussed you use a set 48dp, so you can get it from dimens.xml. https://codereview.chromium.org/2931443003/diff/500001/content/public/android... content/public/android/java/src/org/chromium/content/browser/input/SuggestionsPopupWindow.java:272: mParentView.getLocationInWindow(positionInWindow); On 2017/07/13 21:10:29, rlanday wrote: > On 2017/07/13 at 16:47:40, Theresa wrote: > > Remind me, is the parent the CompositorViewHolder? > > > > If so, the position in window Y is either 0 or the toolbar height depending on > whether the toolbar is showing, right? Is the X position always 0? > > I believe the parent is the ContentView. > > So for example, in Chrome with the toolbar on top of the web page, I get X = 0, > Y = 84. > > In Gmail, when composing an email, I get X = 56, Y = 880 (I guess this is where > the WebView starts in the Gmail window). So no, X is not always 0. Note that > it's still possible for the popup to draw against the left edge of the window in > this case, even though the WebView starts 56px over (this seems like correct > behavior to me). Acknowledged. https://codereview.chromium.org/2931443003/diff/500001/content/public/android... content/public/android/java/src/org/chromium/content/browser/input/SuggestionsPopupWindow.java:278: positionY -= mContainerView.getPaddingTop(); On 2017/07/13 21:10:29, rlanday wrote: > On 2017/07/13 at 16:47:40, Theresa wrote: > > Why are we subtracting the margin and padding here? If it's okay for the > margin and padding not to be visible on screen, can we just remove them > entirely? > > They are being subtracted so the menu lines up properly with the text. Note that > there isn't actually any explicit padding (so there's nothing to remove), but > the background image we're using pre-L means we have to subtract the top padding > in that case so the menu lines up properly. > > The top margin can't be removed because then the drop shadow wouldn't appear > properly on L and later. > > Also, it's not really ok for the top/bottom margins to not be visible on screen; > the native Android spell check menu is fine appearing right up against the left > or right edge of the screen, but will shift its vertical position above the > baseline of the text to maintain a margin at the bottom of the screen. It > doesn't really handle the case where there's no space at the top of the screen > either (this is the case where we have to remove items to get the menu to fit, > which the native Android one doesn't handle), but my assumption is that the > margin behavior should be the same at the top and bottom of the screen. The L+ and pre-L split is making the code a lot more complicated and difficult to reason about. Some code comments explaining what this positioning code is doing and why would be helpful.
The CQ bit was checked by rlanday@chromium.org to run a CQ dry run
Updated https://codereview.chromium.org/2931443003/diff/420001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/input/SuggestionsPopupWindow.java (right): https://codereview.chromium.org/2931443003/diff/420001/content/public/android... content/public/android/java/src/org/chromium/content/browser/input/SuggestionsPopupWindow.java:93: mClippingLimitRight = lp.rightMargin; On 2017/07/13 at 21:41:22, Theresa wrote: > On 2017/07/13 21:10:29, rlanday wrote: > > On 2017/07/13 at 16:47:39, Theresa wrote: > > > On 2017/07/07 03:18:28, rlanday wrote: > > > > On 2017/06/30 at 21:00:33, Theresa wrote: > > > > > Does the container view need margins or can we simplify the code a bit by > > > > dropping them? > > > > > > > > I think the margins are necessary for the drop shadow to display properly. > > > > > > When a 9-patch is used, the view's width should include the necessary space > > for the background/shadows. Is this different when android:elevation is used? > > > > All I know is that it doesn't work without the margin. I think the shadow might > > be getting clipped to the parent view (RelativeLayout)'s bounds or something. > > On L+ or pre-L? The shadow not overdrawing its parents bounds makes sense, but let's confirm. We should always seek to understand what's going on and why code/XML attributes are needed so that we can be sure the code is technically correct. On L+. I've verified that if I make the parent view wider, and remove all the Java logic messing with the view widths, we do get the drop shadow on the right side (I guess the view is left-aligned, so there's still no room on the left, and I didn't try changing the parent's vertical size). So I'm pretty sure now that's what's going on. Having the margin on all sides lets us use wrap_content for some of the view sizes while still giving us room to show the shadow.
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 rlanday@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_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 rlanday@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_tsan_rel_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 rlanday@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: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
On 2017/07/14 22:11:14, rlanday wrote: > On L+. I've verified that if I make the parent view wider, and remove all the > Java logic messing with the view widths, we do get the drop shadow on the right > side (I guess the view is left-aligned, so there's still no room on the left, > and I didn't try changing the parent's vertical size). So I'm pretty sure now > that's what's going on. Having the margin on all sides lets us use wrap_content > for some of the view sizes while still giving us room to show the shadow. That makes sense, thanks for investigating.
https://codereview.chromium.org/2931443003/diff/580001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/input/SuggestionsPopupWindow.java (right): https://codereview.chromium.org/2931443003/diff/580001/content/public/android... content/public/android/java/src/org/chromium/content/browser/input/SuggestionsPopupWindow.java:49: private Rect mTempRect; Findbugs noticed this is not used. https://codereview.chromium.org/2931443003/diff/580001/content/public/android... content/public/android/java/src/org/chromium/content/browser/input/SuggestionsPopupWindow.java:104: ViewGroup.MarginLayoutParams lp = Findbugs also identified this as not used. https://codereview.chromium.org/2931443003/diff/580001/content/public/android... content/public/android/java/src/org/chromium/content/browser/input/SuggestionsPopupWindow.java:187: if (activity == null) { When might the activity be null? https://codereview.chromium.org/2931443003/diff/580001/content/public/android... content/public/android/java/src/org/chromium/content/browser/input/SuggestionsPopupWindow.java:191: // Make the menu wide enough to fit its widest item nit: Add a period at the end of this code comment, as well as inline comments and JavaDocs below. https://codereview.chromium.org/2931443003/diff/580001/content/public/android... content/public/android/java/src/org/chromium/content/browser/input/SuggestionsPopupWindow.java:201: mPopupWindow.setWidth(width); I like how much simpler this method is now, thanks for creating the utility method! https://codereview.chromium.org/2931443003/diff/580001/content/public/android... content/public/android/java/src/org/chromium/content/browser/input/SuggestionsPopupWindow.java:269: updateDividerVisibility(); Is this needed now that we're calling updateDividerVisibility() below on 283? https://codereview.chromium.org/2931443003/diff/580001/content/public/android... content/public/android/java/src/org/chromium/content/browser/input/SuggestionsPopupWindow.java:280: mNumberOfSuggestionsToUse = Math.min(mNumberOfSuggestionsToUse, maxItemsToShow); It seems very unlikely, but it's technically possible for maxItemsToShow to be negative if verticalSpaceAvailableForSuggetionsToUse is negative (e.g. the footer is larger than the current heightPixels). Can we guard against that by setting maxItemsToShow = 0 if verticalSpaceAvailableForSuggetsions < 1?
https://codereview.chromium.org/2931443003/diff/580001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/input/SuggestionsPopupWindow.java (right): https://codereview.chromium.org/2931443003/diff/580001/content/public/android... content/public/android/java/src/org/chromium/content/browser/input/SuggestionsPopupWindow.java:187: if (activity == null) { On 2017/07/17 at 23:13:05, Theresa wrote: > When might the activity be null? The Javadoc for WindowAndroid.getActivity() says: > The returned WeakReference will never be null, but the contained Activity can be null (either if it has been garbage collected or if this is in the context of a WebView that was not created using an Activity). https://chromium.googlesource.com/chromium/src/+/da737c56165f6fae6e81c488cf58... In what circumstances can a WebView be created without using an Activity? Do we need to handle this case? https://codereview.chromium.org/2931443003/diff/580001/content/public/android... content/public/android/java/src/org/chromium/content/browser/input/SuggestionsPopupWindow.java:269: updateDividerVisibility(); On 2017/07/17 at 23:13:05, Theresa wrote: > Is this needed now that we're calling updateDividerVisibility() below on 283? I was thinking that if the menu gets re-used, we'd want to make sure the divider gets reset back to visible here (if we were previously showing the menu with zero suggestions) before measuring the size of the footer. But if we make sure to always reset mSuggestionsPopupWindow = null in TextSuggestionHost on dismiss, I don't think this is necessary (see my other comment for a location where I think we're not currently doing this but should be). https://codereview.chromium.org/2931443003/diff/580001/content/public/android... content/public/android/java/src/org/chromium/content/browser/input/SuggestionsPopupWindow.java:280: mNumberOfSuggestionsToUse = Math.min(mNumberOfSuggestionsToUse, maxItemsToShow); On 2017/07/17 at 23:13:05, Theresa wrote: > It seems very unlikely, but it's technically possible for maxItemsToShow to be negative if verticalSpaceAvailableForSuggetionsToUse is negative (e.g. the footer is larger than the current heightPixels). > > Can we guard against that by setting maxItemsToShow = 0 if verticalSpaceAvailableForSuggetsions < 1? Ok https://codereview.chromium.org/2931443003/diff/580001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/input/TextSuggestionHost.java (right): https://codereview.chromium.org/2931443003/diff/580001/content/public/android... content/public/android/java/src/org/chromium/content/browser/input/TextSuggestionHost.java:66: mSuggestionsPopupWindow.dismiss(); I think we should probably set mSuggestionsPopupWindow = null here?
The CQ bit was checked by rlanday@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...
Updated
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)
The CQ bit was checked by rlanday@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_...)
XML and Java look good to me overall. I have a few more small comments/nits. https://codereview.chromium.org/2931443003/diff/580001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/input/SuggestionsPopupWindow.java (right): https://codereview.chromium.org/2931443003/diff/580001/content/public/android... content/public/android/java/src/org/chromium/content/browser/input/SuggestionsPopupWindow.java:187: if (activity == null) { On 2017/07/17 23:47:51, rlanday wrote: > On 2017/07/17 at 23:13:05, Theresa wrote: > > When might the activity be null? > > The Javadoc for WindowAndroid.getActivity() says: > > > The returned WeakReference will never be null, but the contained Activity can > be null (either if it has been garbage collected or if this is in the context of > a WebView that was not created using an Activity). > > https://chromium.googlesource.com/chromium/src/+/da737c56165f6fae6e81c488cf58... > > > In what circumstances can a WebView be created without using an Activity? Do we > need to handle this case? I don't think a WebView can be created without an activity, but that's a good question for the WebView team. It's probably better to null check it and avoid crashing than to assume the activity is always non-null, so I'm okay with the code as-is. https://codereview.chromium.org/2931443003/diff/620001/content/public/android... File content/public/android/java/res/layout-v21/text_edit_suggestion_container.xml (right): https://codereview.chromium.org/2931443003/diff/620001/content/public/android... content/public/android/java/res/layout-v21/text_edit_suggestion_container.xml:19: tools:ignore="UselessParent"> Can you please try removing ReleativeLayout instead of suppressing the "UselessParent" warning? https://codereview.chromium.org/2931443003/diff/620001/content/public/android... File content/public/android/java/res/layout/text_edit_suggestion_container.xml (right): https://codereview.chromium.org/2931443003/diff/620001/content/public/android... content/public/android/java/res/layout/text_edit_suggestion_container.xml:19: tools:ignore="UselessParent"> Same comment here. https://codereview.chromium.org/2931443003/diff/620001/content/public/android... File content/public/android/java/res/layout/text_edit_suggestion_item.xml (right): https://codereview.chromium.org/2931443003/diff/620001/content/public/android... content/public/android/java/res/layout/text_edit_suggestion_item.xml:16: android:paddingBottom="8dip" nit: While dip and dp are interchangeable, dp is more common in our code base. https://codereview.chromium.org/2931443003/diff/620001/content/public/android... File content/public/android/java/res/values-v17/styles.xml (right): https://codereview.chromium.org/2931443003/diff/620001/content/public/android... content/public/android/java/res/values-v17/styles.xml:21: <!-- v21 uses sans-serif-medium --> If v21 uses sans-serif-medium then you should probably be using RobotStyleMedium to achieve a similar visual effect pre and post L. https://codereview.chromium.org/2931443003/diff/620001/content/public/android... content/public/android/java/res/values-v17/styles.xml:25: <item name="android:layout_height">48dip</item> nit: Same comment about dip here https://codereview.chromium.org/2931443003/diff/620001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java (right): https://codereview.chromium.org/2931443003/diff/620001/content/public/android... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:1006: mTextSuggestionHost.hidePopups(); PopupZoomerTest#testHidePopupOnLosingFocus is failing with this stack trace: java.lang.NullPointerException at org.chromium.content.browser.ContentViewCore.hidePopupsAndClearSelection(ContentViewCore.java:1006) at org.chromium.content.browser.ContentViewCore.onFocusChanged(ContentViewCore.java:1220) at org.chromium.content.browser.PopupZoomerTest.testHidePopupOnLosingFocus(PopupZoomerTest.java:230) at java.lang.reflect.Method.invokeNative(Native Method) ..... https://luci-logdog.appspot.com/v/?s=chromium%2Fbb%2Ftryserver.chromium.andro... https://codereview.chromium.org/2931443003/diff/620001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/input/SuggestionsPopupWindow.java (right): https://codereview.chromium.org/2931443003/diff/620001/content/public/android... content/public/android/java/src/org/chromium/content/browser/input/SuggestionsPopupWindow.java:257: if (Build.VERSION.SDK_INT < Build.VERSION_CODES.N || !activity.isInMultiWindowMode()) { Let's use MultiWindowUtils#isInMultiWindowMode(activity) here. https://codereview.chromium.org/2931443003/diff/620001/content/public/android... content/public/android/java/src/org/chromium/content/browser/input/SuggestionsPopupWindow.java:318: positionY, displayMetrics.heightPixels - height - mContainerView.getPaddingTop()); nit: Should this consider the container margin as well?
On 2017/07/18 15:37:52, Theresa wrote: > XML and Java look good to me overall. I have a few more small comments/nits. > > https://codereview.chromium.org/2931443003/diff/580001/content/public/android... > File > content/public/android/java/src/org/chromium/content/browser/input/SuggestionsPopupWindow.java > (right): > > https://codereview.chromium.org/2931443003/diff/580001/content/public/android... > content/public/android/java/src/org/chromium/content/browser/input/SuggestionsPopupWindow.java:187: > if (activity == null) { > On 2017/07/17 23:47:51, rlanday wrote: > > On 2017/07/17 at 23:13:05, Theresa wrote: > > > When might the activity be null? > > > > The Javadoc for WindowAndroid.getActivity() says: > > > > > The returned WeakReference will never be null, but the contained Activity > can > > be null (either if it has been garbage collected or if this is in the context > of > > a WebView that was not created using an Activity). > > > > > https://chromium.googlesource.com/chromium/src/+/da737c56165f6fae6e81c488cf58... > > > > > > In what circumstances can a WebView be created without using an Activity? Do > we > > need to handle this case? > > I don't think a WebView can be created without an activity, but that's a good > question for the WebView team. It's probably better to null check it and avoid > crashing than to assume the activity is always non-null, so I'm okay with the > code as-is. WebViews can be created without an activity and apps rely on this even though they shouldn't ever do it. We don't support all features in this case, but we do need to not crash. So yes, null check it and do something sensible (often: nothing) rather than assuming it's non-null. > > https://codereview.chromium.org/2931443003/diff/620001/content/public/android... > File > content/public/android/java/res/layout-v21/text_edit_suggestion_container.xml > (right): > > https://codereview.chromium.org/2931443003/diff/620001/content/public/android... > content/public/android/java/res/layout-v21/text_edit_suggestion_container.xml:19: > tools:ignore="UselessParent"> > Can you please try removing ReleativeLayout instead of suppressing the > "UselessParent" warning? > > https://codereview.chromium.org/2931443003/diff/620001/content/public/android... > File content/public/android/java/res/layout/text_edit_suggestion_container.xml > (right): > > https://codereview.chromium.org/2931443003/diff/620001/content/public/android... > content/public/android/java/res/layout/text_edit_suggestion_container.xml:19: > tools:ignore="UselessParent"> > Same comment here. > > https://codereview.chromium.org/2931443003/diff/620001/content/public/android... > File content/public/android/java/res/layout/text_edit_suggestion_item.xml > (right): > > https://codereview.chromium.org/2931443003/diff/620001/content/public/android... > content/public/android/java/res/layout/text_edit_suggestion_item.xml:16: > android:paddingBottom="8dip" > nit: While dip and dp are interchangeable, dp is more common in our code base. > > https://codereview.chromium.org/2931443003/diff/620001/content/public/android... > File content/public/android/java/res/values-v17/styles.xml (right): > > https://codereview.chromium.org/2931443003/diff/620001/content/public/android... > content/public/android/java/res/values-v17/styles.xml:21: <!-- v21 uses > sans-serif-medium --> > If v21 uses sans-serif-medium then you should probably be using RobotStyleMedium > to achieve a similar visual effect pre and post L. > > https://codereview.chromium.org/2931443003/diff/620001/content/public/android... > content/public/android/java/res/values-v17/styles.xml:25: <item > name="android:layout_height">48dip</item> > nit: Same comment about dip here > > https://codereview.chromium.org/2931443003/diff/620001/content/public/android... > File > content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java > (right): > > https://codereview.chromium.org/2931443003/diff/620001/content/public/android... > content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:1006: > mTextSuggestionHost.hidePopups(); > PopupZoomerTest#testHidePopupOnLosingFocus is failing with this stack trace: > > java.lang.NullPointerException > at > org.chromium.content.browser.ContentViewCore.hidePopupsAndClearSelection(ContentViewCore.java:1006) > at > org.chromium.content.browser.ContentViewCore.onFocusChanged(ContentViewCore.java:1220) > at > org.chromium.content.browser.PopupZoomerTest.testHidePopupOnLosingFocus(PopupZoomerTest.java:230) > at java.lang.reflect.Method.invokeNative(Native Method) > ..... > > > > https://luci-logdog.appspot.com/v/?s=chromium%2Fbb%2Ftryserver.chromium.andro... > > https://codereview.chromium.org/2931443003/diff/620001/content/public/android... > File > content/public/android/java/src/org/chromium/content/browser/input/SuggestionsPopupWindow.java > (right): > > https://codereview.chromium.org/2931443003/diff/620001/content/public/android... > content/public/android/java/src/org/chromium/content/browser/input/SuggestionsPopupWindow.java:257: > if (Build.VERSION.SDK_INT < Build.VERSION_CODES.N || > !activity.isInMultiWindowMode()) { > Let's use MultiWindowUtils#isInMultiWindowMode(activity) here. > > https://codereview.chromium.org/2931443003/diff/620001/content/public/android... > content/public/android/java/src/org/chromium/content/browser/input/SuggestionsPopupWindow.java:318: > positionY, displayMetrics.heightPixels - height - > mContainerView.getPaddingTop()); > nit: Should this consider the container margin as well?
On 2017/07/18 at 16:31:22, torne wrote: > On 2017/07/18 15:37:52, Theresa wrote: https://codereview.chromium.org/2931443003/diff/580001/content/public/android... > > content/public/android/java/src/org/chromium/content/browser/input/SuggestionsPopupWindow.java:187: > > if (activity == null) { > > On 2017/07/17 23:47:51, rlanday wrote: > > > On 2017/07/17 at 23:13:05, Theresa wrote: > > > > When might the activity be null? > > > > > > The Javadoc for WindowAndroid.getActivity() says: > > > > > > > The returned WeakReference will never be null, but the contained Activity > > can > > > be null (either if it has been garbage collected or if this is in the context > > of > > > a WebView that was not created using an Activity). > > > > > > > > https://chromium.googlesource.com/chromium/src/+/da737c56165f6fae6e81c488cf58... > > > > > > > > > In what circumstances can a WebView be created without using an Activity? Do > > we > > > need to handle this case? > > > > I don't think a WebView can be created without an activity, but that's a good > > question for the WebView team. It's probably better to null check it and avoid > > crashing than to assume the activity is always non-null, so I'm okay with the > > code as-is. > > WebViews can be created without an activity and apps rely on this even though they shouldn't ever do it. We don't support all features in this case, but we do need to not crash. So yes, null check it and do something sensible (often: nothing) rather than assuming it's non-null. I'm going to get the window size from the passed-in mContext in this case, instead of getting the window size from the Activity. The positioning/sizing won't work quite as well for the multi-window case, but I think it's better than nothing. The other two choices are: - immediately tell Blink to unhighlight the suggestion range if it tries to open the suggestions menu (this means that when you tap on a misspelled word, it will sometimes visibly highlight in red and quickly unhighlight) - write a bunch of code so the C++ text suggestion code knows that the Java code doesn't have access to an activity, then don't ever send Blink the "timer fired" event in this case (this is a lot of code, and seems inferior to having the menu actually appear)
The CQ bit was checked by rlanday@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...
Updated https://codereview.chromium.org/2931443003/diff/620001/content/public/android... File content/public/android/java/res/layout-v21/text_edit_suggestion_container.xml (right): https://codereview.chromium.org/2931443003/diff/620001/content/public/android... content/public/android/java/res/layout-v21/text_edit_suggestion_container.xml:19: tools:ignore="UselessParent"> On 2017/07/18 at 15:37:51, Theresa wrote: > Can you please try removing ReleativeLayout instead of suppressing the "UselessParent" warning? Ok, I experimented a bit and found a way to do this. I was also able to get rid of the margin you didn't like (I still have it as a dimension so we can keep the menu from appearing too close to the top/bottom of the window, to match the existing Android menu's behavior). This lets us consolidate mContentView and mContainerView. https://codereview.chromium.org/2931443003/diff/620001/content/public/android... File content/public/android/java/res/layout/text_edit_suggestion_item.xml (right): https://codereview.chromium.org/2931443003/diff/620001/content/public/android... content/public/android/java/res/layout/text_edit_suggestion_item.xml:16: android:paddingBottom="8dip" On 2017/07/18 at 15:37:51, Theresa wrote: > nit: While dip and dp are interchangeable, dp is more common in our code base. Ok, I've changed all my code to dp https://codereview.chromium.org/2931443003/diff/620001/content/public/android... File content/public/android/java/res/values-v17/styles.xml (right): https://codereview.chromium.org/2931443003/diff/620001/content/public/android... content/public/android/java/res/values-v17/styles.xml:21: <!-- v21 uses sans-serif-medium --> On 2017/07/18 at 15:37:51, Theresa wrote: > If v21 uses sans-serif-medium then you should probably be using RobotStyleMedium to achieve a similar visual effect pre and post L. I'd love to get this to match, but your suggestion didn't seem to change the appearance at all. https://codereview.chromium.org/2931443003/diff/620001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java (right): https://codereview.chromium.org/2931443003/diff/620001/content/public/android... content/public/android/java/src/org/chromium/content/browser/ContentViewCore.java:1006: mTextSuggestionHost.hidePopups(); On 2017/07/18 at 15:37:51, Theresa wrote: > PopupZoomerTest#testHidePopupOnLosingFocus is failing with this stack trace: > > java.lang.NullPointerException > at org.chromium.content.browser.ContentViewCore.hidePopupsAndClearSelection(ContentViewCore.java:1006) > at org.chromium.content.browser.ContentViewCore.onFocusChanged(ContentViewCore.java:1220) > at org.chromium.content.browser.PopupZoomerTest.testHidePopupOnLosingFocus(PopupZoomerTest.java:230) > at java.lang.reflect.Method.invokeNative(Native Method) > ..... > > > > https://luci-logdog.appspot.com/v/?s=chromium%2Fbb%2Ftryserver.chromium.andro... Fixed (by constructing and setting a TextSuggestionHost in the test, as it does with the other popup stuff) https://codereview.chromium.org/2931443003/diff/620001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/input/SuggestionsPopupWindow.java (right): https://codereview.chromium.org/2931443003/diff/620001/content/public/android... content/public/android/java/src/org/chromium/content/browser/input/SuggestionsPopupWindow.java:257: if (Build.VERSION.SDK_INT < Build.VERSION_CODES.N || !activity.isInMultiWindowMode()) { On 2017/07/18 at 15:37:51, Theresa wrote: > Let's use MultiWindowUtils#isInMultiWindowMode(activity) here. I don't think this works because MultiWindowUtils is in Chrome code (org.chromium.chrome), and my code here also runs in a WebView. Should I move MultiWindowUtils into org.chromium.content? https://codereview.chromium.org/2931443003/diff/620001/content/public/android... content/public/android/java/src/org/chromium/content/browser/input/SuggestionsPopupWindow.java:318: positionY, displayMetrics.heightPixels - height - mContainerView.getPaddingTop()); On 2017/07/18 at 15:37:51, Theresa wrote: > nit: Should this consider the container margin as well? In this version, no (height is the height of mContentView, which has mContainerView as a child, and therefore already includes mContainerView's margins). In the version I have up now, we do need to include the margin (to keep the menu from appearing too close to the bottom edge of the window).
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2931443003/diff/620001/content/public/android... File content/public/android/java/res/values-v17/styles.xml (right): https://codereview.chromium.org/2931443003/diff/620001/content/public/android... content/public/android/java/res/values-v17/styles.xml:21: <!-- v21 uses sans-serif-medium --> On 2017/07/19 00:23:55, rlanday wrote: > On 2017/07/18 at 15:37:51, Theresa wrote: > > If v21 uses sans-serif-medium then you should probably be using > RobotStyleMedium to achieve a similar visual effect pre and post L. > > I'd love to get this to match, but your suggestion didn't seem to change the > appearance at all. It would only change the appearance on Pre-L devices. Will you grab updated screenshots on pre-L and L+ devices? RobotoMediumStyle is used to emulate sans-serif-medium on pre-L devices using sans-serif + bold text style. https://codereview.chromium.org/2931443003/diff/620001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/input/SuggestionsPopupWindow.java (right): https://codereview.chromium.org/2931443003/diff/620001/content/public/android... content/public/android/java/src/org/chromium/content/browser/input/SuggestionsPopupWindow.java:257: if (Build.VERSION.SDK_INT < Build.VERSION_CODES.N || !activity.isInMultiWindowMode()) { On 2017/07/19 00:23:56, rlanday wrote: > On 2017/07/18 at 15:37:51, Theresa wrote: > > Let's use MultiWindowUtils#isInMultiWindowMode(activity) here. > > I don't think this works because MultiWindowUtils is in Chrome code > (org.chromium.chrome), and my code here also runs in a WebView. Should I move > MultiWindowUtils into org.chromium.content? You're correct that it won't work since it's in Chrome code. ApiCompatibilityUtils.java is the appropriate place for an isInMultiWindow() helper method. Will you please create a helper method in there for this code to use? No need to convert all of the MultiWindowUtils#.. references (I can do that at some point in the future). https://codereview.chromium.org/2931443003/diff/660001/content/public/android... File content/public/android/java/res/layout-v21/text_edit_suggestion_container.xml (right): https://codereview.chromium.org/2931443003/diff/660001/content/public/android... content/public/android/java/res/layout-v21/text_edit_suggestion_container.xml:15: android:background="@drawable/floating_popup_background_light"> Since the only difference in the two files is now the background, and we have to check Lollipop or not to set the elevation programatically, let's set the background programatically as well and remove one of this XML file. Once they are combined, I would drop in-line the attributes in @style/TextSuggestionWindowContainer rather then defining them in styles.xml. https://codereview.chromium.org/2931443003/diff/660001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/input/SuggestionsPopupWindow.java (right): https://codereview.chromium.org/2931443003/diff/660001/content/public/android... content/public/android/java/src/org/chromium/content/browser/input/SuggestionsPopupWindow.java:87: mPopupWindow.setHeight(ViewGroup.LayoutParams.WRAP_CONTENT); I stumbled upon a note on AppMenu.java that may be relevant to this code as well: "// Some OEMs don't actually let us change the background... but they still return the // padding of the new background, which breaks the menu height. If we still have a // drawable here even though our style says @null we should use this padding instead..." I'm not sure if that will be an issue for this PopupWindow or if it was a specific issue for our AppMenu when we were using ListPopupWindow, but it would be good to test on a device that exhibited the original issue e.g. Samsung Note 3. Code/bug references: https://cs.chromium.org/chromium/src/chrome/android/java/src/org/chromium/chr... crbug.com/398837 https://codereview.chromium.org/2931443003/diff/660001/content/public/android... content/public/android/java/src/org/chromium/content/browser/input/SuggestionsPopupWindow.java:93: // This line is needed for the elevation drop shadow to show. Is this code comment for the line above or left-over from something?
The CQ bit was checked by rlanday@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...
Updated https://codereview.chromium.org/2931443003/diff/620001/content/public/android... File content/public/android/java/res/values-v17/styles.xml (right): https://codereview.chromium.org/2931443003/diff/620001/content/public/android... content/public/android/java/res/values-v17/styles.xml:21: <!-- v21 uses sans-serif-medium --> On 2017/07/19 at 15:10:50, Theresa wrote: > It would only change the appearance on Pre-L devices. Will you grab updated screenshots on pre-L and L+ devices? > > RobotoMediumStyle is used to emulate sans-serif-medium on pre-L devices using sans-serif + bold text style. Ok, done. Updated screenshots here: https://docs.google.com/document/d/1_HPiXt2ufXT8Qm5yWXKK0zgPkQNZbCSVzt-_N0LFj... I have a CL up to move RobotoMediumStyle out of chrome/android so we can use it in content: https://chromium-review.googlesource.com/c/578477/ I've also included these changes in this CL so the try bots build cleanly, I will remove them once that CL lands. https://codereview.chromium.org/2931443003/diff/620001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/input/SuggestionsPopupWindow.java (right): https://codereview.chromium.org/2931443003/diff/620001/content/public/android... content/public/android/java/src/org/chromium/content/browser/input/SuggestionsPopupWindow.java:257: if (Build.VERSION.SDK_INT < Build.VERSION_CODES.N || !activity.isInMultiWindowMode()) { On 2017/07/19 at 15:10:50, Theresa wrote: > You're correct that it won't work since it's in Chrome code. ApiCompatibilityUtils.java is the appropriate place for an isInMultiWindow() helper method. Will you please create a helper method in there for this code to use? No need to convert all of the MultiWindowUtils#.. references (I can do that at some point in the future). I'm landing a CL to do that here: https://chromium-review.googlesource.com/c/578390/ I've also included these changes in this CL for now (until that lands) so the try bots can build. https://codereview.chromium.org/2931443003/diff/660001/content/public/android... File content/public/android/java/res/layout-v21/text_edit_suggestion_container.xml (right): https://codereview.chromium.org/2931443003/diff/660001/content/public/android... content/public/android/java/res/layout-v21/text_edit_suggestion_container.xml:15: android:background="@drawable/floating_popup_background_light"> On 2017/07/19 at 15:10:50, Theresa wrote: > Since the only difference in the two files is now the background, and we have to check Lollipop or not to set the elevation programatically, let's set the background programatically as well and remove one of this XML file. > > > Once they are combined, I would drop in-line the attributes in @style/TextSuggestionWindowContainer rather then defining them in styles.xml. Done https://codereview.chromium.org/2931443003/diff/660001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/input/SuggestionsPopupWindow.java (right): https://codereview.chromium.org/2931443003/diff/660001/content/public/android... content/public/android/java/src/org/chromium/content/browser/input/SuggestionsPopupWindow.java:87: mPopupWindow.setHeight(ViewGroup.LayoutParams.WRAP_CONTENT); On 2017/07/19 at 15:10:50, Theresa wrote: > I stumbled upon a note on AppMenu.java that may be relevant to this code as well: > > "// Some OEMs don't actually let us change the background... but they still return the > // padding of the new background, which breaks the menu height. If we still have a > // drawable here even though our style says @null we should use this padding instead..." > > I'm not sure if that will be an issue for this PopupWindow or if it was a specific issue for our AppMenu when we were using ListPopupWindow, but it would be good to test on a device that exhibited the original issue e.g. Samsung Note 3. > > Code/bug references: > https://cs.chromium.org/chromium/src/chrome/android/java/src/org/chromium/chr... > > crbug.com/398837 I grabbed a Note 3 running 4.3 to test this; I couldn't actually get any spell check underlines to appear for some reason, but I forced the menu to display and it works fine. https://codereview.chromium.org/2931443003/diff/660001/content/public/android... content/public/android/java/src/org/chromium/content/browser/input/SuggestionsPopupWindow.java:93: // This line is needed for the elevation drop shadow to show. On 2017/07/19 at 15:10:50, Theresa wrote: > Is this code comment for the line above or left-over from something? When I was refactoring to remove the "useless parent" in the layout file, I thought I needed to set the background to white here. But that actually produces incorrect behavior (it makes the corners no longer rounded). The real problem was I had another line afterward setting the background to transparent, so I moved that into the pre-L block.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: 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...)
https://codereview.chromium.org/2931443003/diff/660001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/input/SuggestionsPopupWindow.java (right): https://codereview.chromium.org/2931443003/diff/660001/content/public/android... content/public/android/java/src/org/chromium/content/browser/input/SuggestionsPopupWindow.java:87: mPopupWindow.setHeight(ViewGroup.LayoutParams.WRAP_CONTENT); On 2017/07/19 22:03:44, rlanday wrote: > On 2017/07/19 at 15:10:50, Theresa wrote: > > I stumbled upon a note on AppMenu.java that may be relevant to this code as > well: > > > > "// Some OEMs don't actually let us change the background... but they still > return the > > // padding of the new background, which breaks the menu height. If we still > have a > > // drawable here even though our style says @null we should use this padding > instead..." > > > > I'm not sure if that will be an issue for this PopupWindow or if it was a > specific issue for our AppMenu when we were using ListPopupWindow, but it would > be good to test on a device that exhibited the original issue e.g. Samsung Note > 3. > > > > Code/bug references: > > > https://cs.chromium.org/chromium/src/chrome/android/java/src/org/chromium/chr... > > > > crbug.com/398837 > > I grabbed a Note 3 running 4.3 to test this; I couldn't actually get any spell > check underlines to appear for some reason, but I forced the menu to display and > it works fine. Thanks for testing this. Was it using our background 9-patch or one set by the OEM? https://codereview.chromium.org/2931443003/diff/680001/chrome/android/javates... File chrome/android/javatests/src/org/chromium/chrome/browser/input/TextSuggestionMenuTest.java (right): https://codereview.chromium.org/2931443003/diff/680001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/input/TextSuggestionMenuTest.java:43: Thread.sleep(1000); I just noticed this test file (sorry!). What is the Thread.sleep() for? Ideally we would wait for a callback, or, if a specific callback isn't available, we would wait for some criteria to be met. Checkout CallbackHelper and CriteriaHelper. https://codereview.chromium.org/2931443003/diff/680001/content/public/android... File content/public/android/java/res/layout/text_edit_suggestion_container.xml (right): https://codereview.chromium.org/2931443003/diff/680001/content/public/android... content/public/android/java/res/layout/text_edit_suggestion_container.xml:6: <!-- Based on Android's text_edit_suggestion_container_material.xml --> nit: This comment probably no longer applies https://codereview.chromium.org/2931443003/diff/680001/content/public/android... File content/public/android/java/res/layout/text_edit_suggestion_item.xml (right): https://codereview.chromium.org/2931443003/diff/680001/content/public/android... content/public/android/java/res/layout/text_edit_suggestion_item.xml:6: <!-- Based on Android's text_edit_suggestion_item.xml --> nit: is this comment still valid? https://codereview.chromium.org/2931443003/diff/680001/content/public/android... File content/public/android/java/res/layout/text_edit_suggestion_list_footer.xml (right): https://codereview.chromium.org/2931443003/diff/680001/content/public/android... content/public/android/java/res/layout/text_edit_suggestion_list_footer.xml:6: <!-- Based on Android's text_edit_suggestion_container_material.xml --> nit: I think this can be removed as well. https://codereview.chromium.org/2931443003/diff/680001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/input/SuggestionsPopupWindow.java (right): https://codereview.chromium.org/2931443003/diff/680001/content/public/android... content/public/android/java/src/org/chromium/content/browser/input/SuggestionsPopupWindow.java:301: final int width = mContentView.getMeasuredWidth(); Thanks providing screenshots! It looks like on pre-L the background 9-patch padding (where the shadow is drawn) is included in the measured width. So while on L we allow the popup to be flush with the left side of the screen, on pre-L the shadow on the left side will always be visible. If you want pre-L to behave exactly the same as L+, you could subtract the background padding from this width.
Could you move to Gerrit? 35 patches and 120 messages make Rietveld slower and a preparation of Rietveld shutdown. Thanks in advance. https://codereview.chromium.org/2931443003/diff/680001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/suggestion/TextSuggestionController.h (right): https://codereview.chromium.org/2931443003/diff/680001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/suggestion/TextSuggestionController.h:37: const VisiblePositionInFlatTree& caret_visible_position); Please use PositionInFlatTree instead of VisiblePositoinInFlatTree. VisiblePostion* and VisibleSelection* are being deprecated and not suitable for parameters. https://codereview.chromium.org/2931443003/diff/680001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/suggestion/TextSuggestionController.h:39: void ApplySpellCheckSuggestion(const WTF::String& suggestion); nit: s/WTF::String/String/ L59 uses |WTF::String| as |String|. https://codereview.chromium.org/2931443003/diff/680001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/suggestion/TextSuggestionController.h:48: Document& GetDocument() const; nit: s/WTF::String/String/ L59 uses |WTF::String| as |String|. https://codereview.chromium.org/2931443003/diff/680001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/suggestion/TextSuggestionController.h:52: Optional<std::pair<const Node*, const DocumentMarker*>> I think it is better to use |pair.first == nullptr| as no result for simplification. When using Option<std::<const Node*, const DocumetnMarker*>>, we can read maybe_data.first can hold nullptr. Hence, we need to check |pair.first| as non-null event if |Optional| has value.
https://codereview.chromium.org/2931443003/diff/680001/chrome/android/javates... File chrome/android/javatests/src/org/chromium/chrome/browser/input/TextSuggestionMenuTest.java (right): https://codereview.chromium.org/2931443003/diff/680001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/input/TextSuggestionMenuTest.java:43: Thread.sleep(1000); On 2017/07/20 at 00:50:33, Theresa wrote: > I just noticed this test file (sorry!). What is the Thread.sleep() for? Ideally we would wait for a callback, or, if a specific callback isn't available, we would wait for some criteria to be met. Checkout CallbackHelper and CriteriaHelper. This is to wait for the popup menu to show (there's a 300ms delay). There's no callback we can use but using CriteriaHelper to wait for getSuggestionsPopupWindowForTesting() to be non-null works. https://codereview.chromium.org/2931443003/diff/680001/chrome/android/javates... chrome/android/javatests/src/org/chromium/chrome/browser/input/TextSuggestionMenuTest.java:43: Thread.sleep(1000); On 2017/07/20 at 00:50:33, Theresa wrote: > I just noticed this test file (sorry!). What is the Thread.sleep() for? Ideally we would wait for a callback, or, if a specific callback isn't available, we would wait for some criteria to be met. Checkout CallbackHelper and CriteriaHelper. This is to wait for the popup menu to show (there's a 300ms delay). There's no callback we can use but using CriteriaHelper to wait for getSuggestionsPopupWindowForTesting() to be non-null works. https://codereview.chromium.org/2931443003/diff/680001/content/public/android... File content/public/android/java/res/layout/text_edit_suggestion_container.xml (right): https://codereview.chromium.org/2931443003/diff/680001/content/public/android... content/public/android/java/res/layout/text_edit_suggestion_container.xml:6: <!-- Based on Android's text_edit_suggestion_container_material.xml --> On 2017/07/20 at 00:50:33, Theresa wrote: > nit: This comment probably no longer applies Ok, I'll remove all these "Based on..." comments. https://codereview.chromium.org/2931443003/diff/680001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/input/SuggestionsPopupWindow.java (right): https://codereview.chromium.org/2931443003/diff/680001/content/public/android... content/public/android/java/src/org/chromium/content/browser/input/SuggestionsPopupWindow.java:301: final int width = mContentView.getMeasuredWidth(); On 2017/07/20 at 00:50:33, Theresa wrote: > Thanks providing screenshots! It looks like on pre-L the background 9-patch padding (where the shadow is drawn) is included in the measured width. So while on L we allow the popup to be flush with the left side of the screen, on pre-L the shadow on the left side will always be visible. > > If you want pre-L to behave exactly the same as L+, you could subtract the background padding from this width. Good eye! I wasn't too concerned since the native pre-L menu looks totally different (non-Material Design), but I guess it makes sense to do this for consistency. I think we need to adjust the horizontal clipping bounds rather than changing the width.
yosin@: I can't move this to Gerrit without also either moving or landing all the editing CLs I have this stacked on top of (I don't think you can stack a Gerrit CL on top of a Rietveld CL and have the try bots work). I can move it once the dependencies are landed. https://codereview.chromium.org/2931443003/diff/660001/content/public/android... File content/public/android/java/src/org/chromium/content/browser/input/SuggestionsPopupWindow.java (right): https://codereview.chromium.org/2931443003/diff/660001/content/public/android... content/public/android/java/src/org/chromium/content/browser/input/SuggestionsPopupWindow.java:87: mPopupWindow.setHeight(ViewGroup.LayoutParams.WRAP_CONTENT); On 2017/07/20 at 00:50:33, Theresa wrote: > On 2017/07/19 22:03:44, rlanday wrote: > > On 2017/07/19 at 15:10:50, Theresa wrote: > > > I stumbled upon a note on AppMenu.java that may be relevant to this code as > > well: > > > > > > "// Some OEMs don't actually let us change the background... but they still > > return the > > > // padding of the new background, which breaks the menu height. If we still > > have a > > > // drawable here even though our style says @null we should use this padding > > instead..." > > > > > > I'm not sure if that will be an issue for this PopupWindow or if it was a > > specific issue for our AppMenu when we were using ListPopupWindow, but it would > > be good to test on a device that exhibited the original issue e.g. Samsung Note > > 3. > > > > > > Code/bug references: > > > > > https://cs.chromium.org/chromium/src/chrome/android/java/src/org/chromium/chr... > > > > > > crbug.com/398837 > > > > I grabbed a Note 3 running 4.3 to test this; I couldn't actually get any spell > > check underlines to appear for some reason, but I forced the menu to display and > > it works fine. > > Thanks for testing this. > > Was it using our background 9-patch or one set by the OEM? I believe it was our background 9-patch (I don't know what the OEM one looks like but it looked exactly the same as it did on my other test device).
On 2017/07/20 at 01:41:40, rlanday wrote: > yosin@: I can't move this to Gerrit without also either moving or landing all the editing CLs I have this stacked on top of (I don't think you can stack a Gerrit CL on top of a Rietveld CL and have the try bots work). I can move it once the dependencies are landed. Does following steps work? git checkout -b all-in-one git cl patch patch1 git cl patch patch2 ... git cl patch patch9 git cl patch this-patch I have no data about number of splitting CL in average, but I feel splitting into 9 is large. And having 120 message in one patch is too long. BTW, can we split Android UI change logic changes? If we follow MVC model, it is easy to split view, model and control.
On 2017/07/20 02:04:24, yosin_UTC9 wrote: > On 2017/07/20 at 01:41:40, rlanday wrote: > > yosin@: I can't move this to Gerrit without also either moving or landing all > the editing CLs I have this stacked on top of (I don't think you can stack a > Gerrit CL on top of a Rietveld CL and have the try bots work). I can move it > once the dependencies are landed. > > Does following steps work? > > git checkout -b all-in-one > > git cl patch patch1 > git cl patch patch2 > ... > git cl patch patch9 > git cl patch this-patch > > I have no data about number of splitting CL in average, but I feel splitting > into 9 is large. > And having 120 message in one patch is too long. > > BTW, can we split Android UI change logic changes? > If we follow MVC model, it is easy to split view, model and control. This CL is very close to being done with review of Android UI changes, so I would prefer to keep all of the review history together and give my +1 on this patchset. In the future, I agree that the UI changes could/should be split from backend work.
The CQ bit was checked by rlanday@chromium.org to run a CQ dry run
Updated (both UI and editing)
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
lgtm w/nits for core/editing https://codereview.chromium.org/2931443003/diff/700001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/editing/suggestion/TextSuggestionController.cpp (right): https://codereview.chromium.org/2931443003/diff/700001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/suggestion/TextSuggestionController.cpp:72: const unsigned position_offset_in_node = nit: s/unsigned/int/ Position::ComputeOffsetInContainerNode() returns |int|. https://codereview.chromium.org/2931443003/diff/700001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/suggestion/TextSuggestionController.cpp:230: const unsigned range_start_offset = nit: s/unsigned/int/ https://codereview.chromium.org/2931443003/diff/700001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/suggestion/TextSuggestionController.cpp:234: const unsigned range_end_offset = nit: s/unsigned/int/ https://codereview.chromium.org/2931443003/diff/700001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/suggestion/TextSuggestionController.cpp:241: const unsigned start_offset = nit: s/unsigned/int/ https://codereview.chromium.org/2931443003/diff/700001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/suggestion/TextSuggestionController.cpp:243: const unsigned end_offset = node == range_end_container nit: s/unsigned/int/ CharacterData::MaxCharacterOffset() returns |int|. https://codereview.chromium.org/2931443003/diff/700001/third_party/WebKit/Sou... third_party/WebKit/Source/core/editing/suggestion/TextSuggestionController.cpp:266: selection.IsRange() ? selection.ToNormalizedEphemeralRange() Could you use |EphemeralRangeInFlatTree(selection.Start(), selection.End())|? |VS::ToNormalizedEphemeralRange()| does lots of unwanted things and we would like to avoid to use it.
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...)
On 2017/07/21 01:27:39, Theresa wrote: > lgtm TextSuggestionMenuTest - lgtm
The CQ bit was checked by rlanday@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: chromeos_amd64-generic_chromium_compile_only_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromeos_amd64-...) 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 rlanday@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: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
The CQ bit was checked by rlanday@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...
I think there are a couple more files that may have slipped through the cracks; tedchoc@, can you please also review the following files? content/browser/android/text_suggestion_host_android.cc content/browser/android/text_suggestion_host_android.h content/browser/android/text_suggestion_host_mojo_impl_android.cc content/browser/android/text_suggestion_host_mojo_impl_android.h
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: 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...)
On 2017/07/25 20:37:55, rlanday wrote: > I think there are a couple more files that may have slipped through the cracks; > tedchoc@, can you please also review the following files? > > content/browser/android/text_suggestion_host_android.cc > content/browser/android/text_suggestion_host_android.h > content/browser/android/text_suggestion_host_mojo_impl_android.cc > content/browser/android/text_suggestion_host_mojo_impl_android.h text_suggestion_host_* - lgtm
The CQ bit was checked by rlanday@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from aelias@chromium.org, torne@chromium.org, boliu@chromium.org, mkwst@chromium.org, twellington@chromium.org, yosin@chromium.org Link to the patchset: https://codereview.chromium.org/2931443003/#ps740001 (title: "Simplify Mojo binding based on rockot@'s advice")
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": 740001, "attempt_start_ts": 1501022604567500,
"parent_rev": "eb7572a1c04926d4eb5e41b7d7de8233530c4844", "commit_rev":
"31d7050864bb334a4bcfbc938d6b160f15918617"}
CQ is committing da patch.
Bot data: {"patchset_id": 740001, "attempt_start_ts": 1501022604567500,
"parent_rev": "677bd3e5a3f2d39a0acaca9cf9513f68a9b4d6b9", "commit_rev":
"e4a6ec83ec32b4f139794f39bf2cbb0b25617b56"}
Message was sent while issue was closed.
Description was changed from ========== Add support for Android spellcheck menu in Chrome/WebViews Currently, in Chrome and in WebViews, tapping misspelled words on Android (that have red underlines) doesn't do anything. In a native text box, tapping a misspelled word opens a menu that allows the user to pick from a list of suggested replacements (if any), delete the misspelled word, or add it to the system dictionary. This CL ports this menu to Chrome and WebViews. Before this project, the list of spelling suggestions from the Android spellchecker was being discarded. https://codereview.chromium.org/2905003003 fixes this so the suggestions are passed through to the Spelling DocumentMarkers. This CL adds code to SelectionController::HandleSingleClick() to check if the user tapped on a misspelled word, and if so, send a Mojo message from the renderer to the browser to start a timer to open the spellcheck menu unless we receive another tap within the double-tap timeout threshold (note: this CL restricts the spellcheck menu to Android by not handling this message on any other platform). If the timer expires, a message is sent back to the renderer to try to open the spellcheck menu (nothing happens if the spelling marker no longer exists). If the renderer decides the spellcheck menu should be opened, it highlights the misspelled word with in red using an ActiveSuggestionMarker (added in https://codereview.chromium.org/2923033002), hides the caret, and sends another message back to the browser with a list of spelling suggestions (if any). The layout files for the menu and the logic for showing it were ported over from the implementation for the same menu in android.widget.Editor. Some of the XML files were modified slightly to avoid dependencies on internal Android system resources or newer Android versions. Tapping outside the spellcheck menu closes it, and sends a message to Blink telling it to remove the highlight and show the caret again. There are three other messages for applying a suggestion, deleting the misspelled word, and adding the word to the dictionary. For adding the word to the dictionary, I'm using the same Android intent the native textbox uses, with the same wonky behavior that the spellcheck marker is removed immediately when the user taps "Add to dictionary", before the user has gone through the confirmation dialog. BUG=715365 ========== to ========== Add support for Android spellcheck menu in Chrome/WebViews Currently, in Chrome and in WebViews, tapping misspelled words on Android (that have red underlines) doesn't do anything. In a native text box, tapping a misspelled word opens a menu that allows the user to pick from a list of suggested replacements (if any), delete the misspelled word, or add it to the system dictionary. This CL ports this menu to Chrome and WebViews. Before this project, the list of spelling suggestions from the Android spellchecker was being discarded. https://codereview.chromium.org/2905003003 fixes this so the suggestions are passed through to the Spelling DocumentMarkers. This CL adds code to SelectionController::HandleSingleClick() to check if the user tapped on a misspelled word, and if so, send a Mojo message from the renderer to the browser to start a timer to open the spellcheck menu unless we receive another tap within the double-tap timeout threshold (note: this CL restricts the spellcheck menu to Android by not handling this message on any other platform). If the timer expires, a message is sent back to the renderer to try to open the spellcheck menu (nothing happens if the spelling marker no longer exists). If the renderer decides the spellcheck menu should be opened, it highlights the misspelled word with in red using an ActiveSuggestionMarker (added in https://codereview.chromium.org/2923033002), hides the caret, and sends another message back to the browser with a list of spelling suggestions (if any). The layout files for the menu and the logic for showing it were ported over from the implementation for the same menu in android.widget.Editor. Some of the XML files were modified slightly to avoid dependencies on internal Android system resources or newer Android versions. Tapping outside the spellcheck menu closes it, and sends a message to Blink telling it to remove the highlight and show the caret again. There are three other messages for applying a suggestion, deleting the misspelled word, and adding the word to the dictionary. For adding the word to the dictionary, I'm using the same Android intent the native textbox uses, with the same wonky behavior that the spellcheck marker is removed immediately when the user taps "Add to dictionary", before the user has gone through the confirmation dialog. BUG=715365 Review-Url: https://codereview.chromium.org/2931443003 Cr-Commit-Position: refs/heads/master@{#489505} Committed: https://chromium.googlesource.com/chromium/src/+/e4a6ec83ec32b4f139794f39bf2c... ==========
Message was sent while issue was closed.
Committed patchset #38 (id:740001) as https://chromium.googlesource.com/chromium/src/+/e4a6ec83ec32b4f139794f39bf2c... |
