Issue metadata
Sign in to add a comment
|
Placeholder text gets selected on performing long tap in empty field.
Reported by
ngo...@etouch.net,
Jul 23
|
||||||||||||||||||||||
Issue descriptionDevice name: Pixel XL 8.1.0/(OPM1.171019.012), Nexus 5X 7.1.2/(N2G47W), Htc Desire 630/6.0.1(1.00.400.3), Panasonic Eluga Turbo/5.1(LMY4&D) WebView version: 69.0.3496.7 Application: Amazon Shopping Application version: 16.12.0.300 Bisect Information: This is a regression issue broken in 'M-69' Per-Version bisect information: Good build: 69.0.3495.0 Bad build: 69.0.3496.0 Regression range: https://chromium.googlesource.com/chromium/src/+log/69.0.3495.0..69.0.3496.0?pretty=fuller&n=10000 Steps to reproduce: 1. Launch Amazon app > Create account > Long tap in empty field > Observe. Actual result: Ghost text gets selected. Expected result: Ghost text should not get selected. Note: Issue is not reproducible on Stable Build (67.0.3396.87)
,
Jul 31
This confuses me, with my local build, it actually crashes on a DCHECK (stack is below), which is inside of |GranularityAdjuster|. This DCHECK violation is the reason why the ghost text gets selected, because end position < start position. However |GranularityAdjuster| is called before |EditingBoundaryAdjuster|, so this bug should not related to my CL. battun@, could you please get per-cl bisect result? Although it could be fixed by checking |selection.IsCaret()| in |ComputeVisibleSelection| (http://crrev/c/1156059), I wonder it is a proper fix. ----------------crash stack-------------------- [FATAL:ephemeral_range.cc(45)] Check failed: start_position_ <= end_position_ (INPUT id="ap_customer_name" (editable) (focused)@afterAnchor vs. DIV (editable)@offsetInAnchor[0]) Stack Trace: RELADDR FUNCTION FILE:LINE 0000000000148277 logging::LogMessage::~LogMessage() /usr/local/google/code/clankium/src/base/logging.cc:592:29 0000000001139077 blink::EphemeralRangeTemplate<blink::EditingAlgorithm<blink::FlatTreeTraversal> >::EphemeralRangeTemplate(blink::PositionTemplate<blink::EditingAlgorithm<blink::FlatTreeTraversal> > const&, blink::PositionTemplate<blink::EditingAlgorithm<blink::FlatTreeTraversal> > const&) /usr/local/google/code/clankium/src/third_party/blink/renderer/core/editing/ephemeral_range.cc:0:5 000000000117080f blink::GranularityAdjuster::AdjustStartAndEnd(blink::PositionTemplate<blink::EditingAlgorithm<blink::FlatTreeTraversal> > const&, blink::PositionTemplate<blink::EditingAlgorithm<blink::FlatTreeTraversal> > const&) /usr/local/google/code/clankium/src/third_party/blink/renderer/core/editing/selection_adjuster.cc:335:12 000000000116f80b blink::SelectionTemplate<blink::EditingAlgorithm<blink::FlatTreeTraversal> > blink::GranularityAdjuster::AdjustSelection<blink::EditingAlgorithm<blink::FlatTreeTraversal> >(blink::SelectionTemplate<blink::EditingAlgorithm<blink::FlatTreeTraversal> > const&, blink::TextGranularity) /usr/local/google/code/clankium/src/third_party/blink/renderer/core/editing/selection_adjuster.cc:303:9 0000000001199ddf blink::VisibleSelectionTemplate<blink::EditingAlgorithm<blink::FlatTreeTraversal> >::Creator::ComputeVisibleSelection(blink::SelectionTemplate<blink::EditingAlgorithm<blink::FlatTreeTraversal> > const&, blink::TextGranularity) /usr/local/google/code/clankium/src/third_party/blink/renderer/core/editing/visible_selection.cc:87:5 0000000001199b7f blink::VisibleSelectionTemplate<blink::EditingAlgorithm<blink::FlatTreeTraversal> >::Creator::CreateWithGranularity(blink::SelectionTemplate<blink::EditingAlgorithm<blink::FlatTreeTraversal> > const&, blink::TextGranularity) /usr/local/google/code/clankium/src/third_party/blink/renderer/core/editing/visible_selection.cc:65:0 00000000011729c3 blink::SelectionController::SelectClosestWordFromHitTestResult(blink::HitTestResult const&, blink::SelectionController::AppendTrailingWhitespace, blink::SelectionController::SelectInputEventType) /usr/local/google/code/clankium/src/third_party/blink/renderer/core/editing/selection_controller.cc:613:25 00000000011744b7 blink::SelectionController::HandleGestureLongPress(blink::HitTestResult const&) /usr/local/google/code/clankium/src/third_party/blink/renderer/core/editing/selection_controller.cc:1132:7 00000000013a8e23 blink::GestureManager::HandleGestureLongPress(blink::EventWithHitTestResults<blink::WebGestureEvent> const&) /usr/local/google/code/clankium/src/third_party/blink/renderer/core/input/gesture_manager.cc:340:34 00000000013a864b blink::GestureManager::HandleGestureEventInFrame(blink::EventWithHitTestResults<blink::WebGestureEvent> const&) /usr/local/google/code/clankium/src/third_party/blink/renderer/core/input/gesture_manager.cc:133:7 v------> std::__ndk1::enable_if<(((!(std::is_same<std::__ndk1::remove_cv<std::__ndk1::remove_reference<base::TimeTicks>::type>::type, base::Optional<base::TimeTicks> >::value)) && (std::is_constructible<base::TimeTicks, base::TimeTicks>::value)) && (std::is_assignable<base::TimeTicks&, base::TimeTicks>::value)) && ((!(std::__ndk1::integral_constant<bool, false>::value)) || (!(std::is_same<std::__ndk1::decay<base::TimeTicks>::type, base::TimeTicks>::value))), base::Optional<base::TimeTicks>&>::type base::Optional<base::TimeTicks>::operator=<base::TimeTicks>(base::TimeTicks&&) /usr/local/google/code/clankium/src/base/optional.h:551:5 00000000013a4dff blink::EventHandler::HandleGestureEvent(blink::EventWithHitTestResults<blink::WebGestureEvent> const&) /usr/local/google/code/clankium/src/third_party/blink/renderer/core/input/event_handler.cc:1364:0 00000000011e5237 blink::WebViewImpl::HandleGestureEvent(blink::WebGestureEvent const&) /usr/local/google/code/clankium/src/third_party/blink/renderer/core/exported/web_view_impl.cc:679:11 000000000161d127 blink::PageWidgetDelegate::HandleInputEvent(blink::PageWidgetEventHandler&, blink::WebCoalescedInputEvent const&, blink::LocalFrame*) /usr/local/google/code/clankium/src/third_party/blink/renderer/core/page/page_widget_delegate.cc:164:3 v------> std::__ndk1::unique_ptr<blink::UserGestureIndicator, std::__ndk1::default_delete<blink::UserGestureIndicator> >::release() /usr/local/google/code/clankium/src/third_party/android_ndk/sources/cxx-stl/llvm-libc++/include/memory:2536:26 v------> std::__ndk1::unique_ptr<blink::UserGestureIndicator, std::__ndk1::default_delete<blink::UserGestureIndicator> >::operator=(std::__ndk1::unique_ptr<blink::UserGestureIndicator, std::__ndk1::default_delete<blink::UserGestureIndicator> >&&) /usr/local/google/code/clankium/src/third_party/android_ndk/sources/cxx-stl/llvm-libc++/include/memory:2412:0 00000000011e92c7 blink::WebViewImpl::HandleInputEvent(blink::WebCoalescedInputEvent const&) /usr/local/google/code/clankium/src/third_party/blink/renderer/core/exported/web_view_impl.cc:1942:0 00000000018ec59f content::RenderWidgetInputHandler::HandleInputEvent(blink::WebCoalescedInputEvent const&, ui::LatencyInfo const&, base::OnceCallback<void (content::InputEventAckState, ui::LatencyInfo const&, std::__ndk1::unique_ptr<ui::DidOverscrollParams, std::__ndk1::default_delete<ui::DidOverscrollParams> >, base::Optional<cc::TouchAction>)>) /usr/local/google/code/clankium/src/content/renderer/input/render_widget_input_handler.cc:370:46 00000000019fe64f content::RenderWidget::HandleInputEvent(blink::WebCoalescedInputEvent const&, ui::LatencyInfo const&, base::OnceCallback<void (content::InputEventAckState, ui::LatencyInfo const&, std::__ndk1::unique_ptr<ui::DidOverscrollParams, std::__ndk1::default_delete<ui::DidOverscrollParams> >, base::Optional<cc::TouchAction>)>) /usr/local/google/code/clankium/src/content/renderer/render_widget.cc:976:19 00000000018eb22b content::MainThreadEventQueue::HandleEventOnMainThread(blink::WebCoalescedInputEvent const&, ui::LatencyInfo const&, base::OnceCallback<void (content::InputEventAckState, ui::LatencyInfo const&, std::__ndk1::unique_ptr<ui::DidOverscrollParams, std::__ndk1::default_delete<ui::DidOverscrollParams> >, base::Optional<cc::TouchAction>)>) /usr/local/google/code/clankium/src/content/renderer/input/main_thread_event_queue.cc:502:14 00000000018eb52f content::QueuedWebInputEvent::Dispatch(content::MainThreadEventQueue*) /usr/local/google/code/clankium/src/content/renderer/input/main_thread_event_queue.cc:104:12 00000000018eaecf content::MainThreadEventQueue::DispatchEvents() /usr/local/google/code/clankium/src/content/renderer/input/main_thread_event_queue.cc:372:11 000000000012682b base::OnceCallback<void ()>::Run() && /usr/local/google/code/clankium/src/base/callback.h:99:12 000000000013483f base::debug::TaskAnnotator::RunTask(char const*, base::PendingTask*) /usr/local/google/code/clankium/src/base/debug/task_annotator.cc:101:33 0000000000199243 base::sequence_manager::internal::ThreadControllerImpl::DoWork(base::sequence_manager::internal::ThreadControllerImpl::WorkType) /usr/local/google/code/clankium/src/base/task/sequence_manager/thread_controller_impl.cc:169:21 000000000012682b base::OnceCallback<void ()>::Run() && /usr/local/google/code/clankium/src/base/callback.h:99:12 000000000013483f base::debug::TaskAnnotator::RunTask(char const*, base::PendingTask*) /usr/local/google/code/clankium/src/base/debug/task_annotator.cc:101:33 0000000000151eeb base::MessageLoop::RunTask(base::PendingTask*) /usr/local/google/code/clankium/src/base/message_loop/message_loop.cc:427:46 00000000001521df base::MessageLoop::DeferOrRunPendingTask(base::PendingTask) /usr/local/google/code/clankium/src/base/message_loop/message_loop.cc:438:5 000000000015232f base::MessageLoop::DoWork() /usr/local/google/code/clankium/src/base/message_loop/message_loop.cc:510:16 000000000015505b base::MessagePumpDefault::Run(base::MessagePump::Delegate*) /usr/local/google/code/clankium/src/base/message_loop/message_pump_default.cc:37:31
,
Jul 31
cc yoichio@ also for better insights.
,
Jul 31
Per-CL bisect information: Good commit:575974 Bad commit:575975 Suspect CL: https://chromium.googlesource.com/chromium/src/+/7bd29404a6ab8d36bdff4123ae522fcd9068344b ctzsm@ It's pointing above suspect CL only. Thank You!
,
Aug 1
Spent more time on this today, check for selection is caret is definitely not the proper fix, since long press/double click should select a word.
I think the previous |EditingBoundaryAdjuster| had a workaround for this case, so my CL exposed the bug in |GranularityAdjuster|.
The tree structure is like this:
DIV class="a-input-text-wrapper auth-required-field accordionFontFamily a-form-focus"
INPUT id="ap_customer_name" (focused)
#shadow-root(UserAgent)
DIV id="placeholder" style="display: block !important;"
#text "Name"
* DIV (editable)
Sometimes the input element will be editable, so |ParentEditingBoundary()| [1] crossed shadow boundary, as a result, the search scope in |PreviousBoundaryAlgorithm| is larger than expected. I'll try to replace |ParentEditingBoundary()| with |RootBoundaryElement()| in my CL to see what happens.
[1] https://cs.chromium.org/chromium/src/third_party/blink/renderer/core/editing/visible_units.cc?rcl=0bf136214445b8018d40cc60ea12ad9e80c8cd53&l=327
,
Aug 2
I think this is not regression. The CL[1] mentioned in #c4 does right thing == selecting word in shadow DOM. It seems previous version of EditingBoundaryAdjust() somehow adjust selection in placeholder text to null selection.
ctzsm@, could you add |user-select: none !important| to "::--webkit-input-placeholder" rule in "core/css/html.css"?
::-webkit-input-placeholder {
-webkit-text-security: none;
color: #757575;
pointer-events: none !important;
}
Once placeholder text has "user-select:none", SelectionController::SelectClosestWordFromHitTestResult() will not select placeholder text.
bool SelectionController::SelectClosestWordFromHitTestResult(
const HitTestResult& result,
AppendTrailingWhitespace append_trailing_whitespace,
SelectInputEventType select_input_event_type) {
Node* const inner_node = result.InnerNode();
if (!inner_node || !inner_node->GetLayoutObject() ||
!inner_node->GetLayoutObject()->IsSelectable())
return false;
...
}
bool LayoutObject::IsSelectable() const {
return !IsInert() && !(Style()->UserSelect() == EUserSelect::kNone &&
Style()->UserModify() == EUserModify::kReadOnly);
}
Note: SelectionController::HandleGestureLongPress() still set |inner_node_is_selectable| true, even if Node::CanStartSelection() which checks user-select:none, but |inner_node| is Text node for placeholder text.
bool SelectionController::HandleGestureLongPress(
const HitTestResult& hit_test_result) {
TRACE_EVENT0("blink", "SelectionController::handleGestureLongPress");
if (!Selection().IsAvailable())
return false;
if (hit_test_result.IsLiveLink())
return false;
Node* inner_node = hit_test_result.InnerNode();
inner_node->GetDocument().UpdateStyleAndLayoutTree();
bool inner_node_is_selectable = HasEditableStyle(*inner_node) ||
inner_node->IsTextNode() ||
inner_node->CanStartSelection();
if (!inner_node_is_selectable)
return false;
...
BTW, we should hoist |UpdateStyleAndLayoutTree| to |GestureManager::HandleGestureLongPress()|
[1] http://crrev.com/c/1102157 Avoid crossing editing boundaries selection.
,
Aug 2
,
Aug 2
yosin@,
Thanks for the suggestion, adding |user-select: none !important| to "::--webkit-input-placeholder" won't resolve this issue though. This is because the |inner_node| in |SelectClosestWordFromHitTestResult()| for this case is not |DIV id="placeholder"| but |DIV (editable)| in shadow tree, see the tree structure below.
INPUT id="ap_customer_name" (editable) (focused)
#shadow-root(UserAgent)
DIV id="placeholder" style="display: block !important;"
#text "Name"
* DIV (editable)
The selection (which is a caret) before |GranularityAdjuster::AdjustSelection()| is:
start: DIV (editable)@offsetInAnchor[0]
end: DIV (editable)@offsetInAnchor[0]
It selects placeholder text because the result from |GranularityAdjuster::AdjustSelection()| is:
start: INPUT id="ap_customer_name" (editable) (focused)@afterAnchor
end: DIV (editable)@offsetInAnchor[0]
Visually it selects placeholder DIV, which I am not very sure why though, but start > end already, so weird thing could happen. You may see the selection bubbles in the video are strange, left bubble is on right, right bubble is on left.
The root reason is as I noted in CL description (http://crrev/c/1156059), |PreviousBoundaryAlgorithm()| received a wrong searching scope from |ParentEditingBoundary()|, because |ParentEditingBoundary()| ignored shadow boundary and treated |INPUT id="ap_customer_name" (editable) (focused)| and |DIV (editable)| as one editing host. |PreviousBoundaryAlgorithm()| figured out a wrong position as the result.
As you may notice that we depended very little on the placeholder DIV here, so adding |user-select: none !important| to placeholder DIV won't resolve it unfortunately.
But since there is another plan for removing |ParentEditingBoundary()|, I think it's better to leave it as is.
A workaround for this bug specifically is to add |-webkit-user-modify: read-only !important;| to
input {
-webkit-appearance: textfield;
padding: 1px;
background-color: white;
border: 2px inset;
-webkit-rtl-ordering: logical;
cursor: text;
}
in "core/css/html.css".
This way will resolve this bug only, my concern is if web developer had a similar shadow tree structure, intuitively they would add |user-select: none;| to the place they don't want to be selected as you suggested before. However, to make it work, they need to add |-webkit-user-modify: read-only;| to the shadow host, which isn't obvious.
I have uploaded the |-webkit-user-modify: read-only !important;| workaround to http://crrev/c/1161173, but feel free to dup this issue to removing |ParentEditingBoundary()| issue. However the fix needs to be cherry-pick back to M69.
,
Aug 3
Thanks ctzsm@ for analysis! BTW, I don't think this is Pri-1 and RB-Stable. Since it doesn't prevent user action. We should ask Amazon not to make <input> as content editable because it has no reasons to do so. It seems this is hit test issue returning DIV@0 for empty editable DIV. Hit test should return position inside placeholder text then web authors can do something for placeholder of their own, e.g. the web author implements <input> like element and clicking placeholder to show candidates list etc. In other words, Blink should provide functionalities allow elements using UA shadow root, <input>, <select>, etc. without hacky code in JavaScript. We found hit testing does higher-level things which should be handled in Selection Controller[1]. Thus, we want to make hit test not to handle user-select, pointer-event, editing behavior etc, and selection controller to handle them. BTW, we don't want to make shadow dom handling smarter at this time. Since Edge and Firefox are working this. Once Blink does smarter thing, other vendors need to follow. We'll work with them and find reasonable solution for both of browser vendors and web developers. [1] http://crbug.com/870555 Hit testing should not handle editing features
,
Aug 7
Lowering the priority sounds good to me. I agree Amazon shouldn't make <input> as contenteditable, which is a weird thing to do. However, http://crrev/c/1156059 (change |PreviousBoundaryAlgorithm()|) isn't the way we want to pursuit, http://crrev/c/1161173 (change edibility of <input>) exposing other issues. Makes the bug kind of annoying...
,
Aug 7
Opening this bug to public for better communication with Amazon app team. It only contains bug and debugging info here.
,
Aug 7
Removing RBS since this is not a recent regression, and as #c9, it doesn't prevent user action.
,
Aug 7
,
Aug 10
It seems the root cause is as same as issue 872443.
,
Aug 17
ctzsm@, could you add a test case to reproduce failures on ChromeOS and Win10-with-touch. Thanks! We attempted to reproduce on ChromsOS and Win10 by using inspector to add "user-select:none", but we could not reproduce.
,
Aug 18
http://jsfiddle.net/0nwy3ho1/3/ should repro this on CrOS, just double click or long press placeholder text will select it; Add "user-select: none" to <input> does preventing the issue. Do we want to add "user-select: none" to <input>? There is another placeholder text get selected with different root cause issue 873999 I am fixing. Doesn't seem fixing that will fix this, just an FYI.
,
Aug 28
So I am trying to add |-webkit-user-modify: read-only| to <input>, turns out this is really hard..
There are few tests failed because |Most{Backward,Forward}CaretPosition()| treat {after,before}_anchor positions to be the same edibility with anchor node. Which is not true apparently, because these positions are actually outside of the anchor node.
So I ended to try to fix |Most{Backward,Forward}CaretPosition()| for this special case [1], but it turns out we will fail on other tests, such as making infinity loop in test fast/events/scoped/editing-commands.html for 'InsertOrderedList' case. After struggling on this for few hours, I still have no clue why:(, looks like we are posting infinite number of 'InsertOrderedList' command from stack trace.
[1] http://crrev/c/1161173/8
,
Nov 22
Issue fixed on latest M71: 71.0.3578.66 and M72: 72.0.3618.0 Tested devices:Pixel XL 8.1.0/(OPM1.171019.012), Nexus 5X 7.1.2/(N2G47W) Thanks!
,
Nov 30
,
Dec 10
|
|||||||||||||||||||||||
►
Sign in to add a comment |
|||||||||||||||||||||||
Comment 1 by battun@chromium.org
, Jul 23Labels: -Pri-3 hasbisect-per-revision ReleaseBlock-Stable M-69 RegressedIn-69 Target-69 FoundIn-69 Pri-1 Type-Bug-Regression
Owner: ctzsm@chromium.org
Status: Assigned (was: Unconfirmed)