Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2748483002:
Making sure ClearHandleAfterTap's tap occurs inside of HTML document (Closed)
DescriptionMaking sure ClearHandleAfterTap's tap occurs inside of HTML document
The last tap in ClearHandleAfterTap was causing the HitTestResult's innerNode to
be null. This change makes it so that the tap occurs on an HTML element thus
making the innerNode not be null. This is change was necessary to for
crrev.com/2616623002.
Review-Url: https://codereview.chromium.org/2748483002
Cr-Commit-Position: refs/heads/master@{#457488}
Committed: https://chromium.googlesource.com/chromium/src/+/7f75cce9905c0fe40761f247a47bfc66ef2f3321
Patch Set 1 #Patch Set 2 : git cl try #Patch Set 3 : tap in actual element #
Total comments: 1
Patch Set 4 : removed unnecessary div #Messages
Total messages: 33 (22 generated)
The CQ bit was checked by amaralp@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_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 amaralp@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_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 amaralp@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.
amaralp@chromium.org changed reviewers: + yosin@chromium.org
yosin@, I made this patch to to fix the failing test in hugoh@opera's CL (https://codereview.chromium.org/2616623002/). As the description says this patch ensures that the tap used to clear the handle actually lands in the page and not in some gutter space. I noticed something unexpected when changing the test. Even though there is a tap in the empty div selection().computeVisibleSelectionInDOMTreeDeprecated().isNone() returns false. The selection is actually a caret selection. Is this expected behavior? I think the problem is that |SelectionController::handleMousePressEventSingleClick()| calls |updateSelectionForMouseDownDispatchingSelectStart()| even if the inner node wasn't editable.
Current test clicks <body>, right? I wonder why tapping <body> gives document().focusedElement() == null. I expected it to be <body> :) This is why I needed the null-check in: https://codereview.chromium.org/2616623002/diff/440001/third_party/WebKit/Sou... ... without it, many many tests failed. > I think the problem is that > |SelectionController::handleMousePressEventSingleClick()| calls >|updateSelectionForMouseDownDispatchingSelectStart()| even if the > inner node wasn't editable. I let yosin@ answer this, but my understanding is that any mouse click on text could be the start of a selection. The element does not have to be editable, <body>some text</body.
On 2017/03/14 at 11:56:15, hugoh wrote: > Current test clicks <body>, right? I wonder why tapping <body> gives document().focusedElement() == null. I expected it to be <body> :) > > This is why I needed the null-check in: > https://codereview.chromium.org/2616623002/diff/440001/third_party/WebKit/Sou... > ... without it, many many tests failed. > > > I think the problem is that > > |SelectionController::handleMousePressEventSingleClick()| calls > >|updateSelectionForMouseDownDispatchingSelectStart()| even if the > > inner node wasn't editable. > I let yosin@ answer this, but my understanding is that any mouse click on text could be the start of a selection. The element does not have to be editable, <body>some text</body. Yes, "tap" sets selection as caret. Event if it is non-editable. I guess this is the reason why this patch removes: ASSERT_TRUE(selection().computeVisibleSelectionInDOMTreeDeprecated().isNone()); Since default window size of content_shell is 800x600, tapping 700, 700 is out side of window then Blink doesn't set new focus element. So, this test do: 1. Tapping in TEXTAREA to show seleciton handle 2. Tapping non-editable to hide selection handle
The CQ bit was checked by amaralp@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/03/15 01:20:11, yosin_UTC9 wrote: > On 2017/03/14 at 11:56:15, hugoh wrote: > > Current test clicks <body>, right? I wonder why tapping <body> gives > document().focusedElement() == null. I expected it to be <body> :) > > > > This is why I needed the null-check in: > > > https://codereview.chromium.org/2616623002/diff/440001/third_party/WebKit/Sou... > > > ... without it, many many tests failed. > > > > > I think the problem is that > > > |SelectionController::handleMousePressEventSingleClick()| calls > > >|updateSelectionForMouseDownDispatchingSelectStart()| even if the > > > inner node wasn't editable. > > I let yosin@ answer this, but my understanding is that any mouse click on text > could be the start of a selection. The element does not have to be editable, > <body>some text</body. > > Yes, "tap" sets selection as caret. Event if it is non-editable. > I guess this is the reason why this patch removes: > > ASSERT_TRUE(selection().computeVisibleSelectionInDOMTreeDeprecated().isNone()); > > Since default window size of content_shell is 800x600, tapping 700, 700 is out > side of window then Blink doesn't set new focus element. > > So, this test do: > 1. Tapping in TEXTAREA to show seleciton handle > 2. Tapping non-editable to hide selection handle Makes sense. Thanks!
hugoh@opera.com changed reviewers: + hugoh@opera.com
https://codereview.chromium.org/2748483002/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/input/EventHandlerTest.cpp (right): https://codereview.chromium.org/2748483002/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/core/input/EventHandlerTest.cpp:373: "<div style='height: 1000px; width: 1000px;'></div>"); I think we should fix that tapping on <body> gives the same test result as tapping on a <div>. Maybe that would be a rather big change, so we don't need to do it right now. @yosin, do you know the reason why tapping <body> gives document().focusedElement() == null (and not HTMLBodyElement)? Feels like Blink doesn't "see" <body> in hit tests. bool FocusController::setFocusedElement(Element* element, Frame* newFocusedFrame, const FocusParams& params) { + LOG(ERROR) << "setFocusedElement: " << element; // null for <body>.
On 2017/03/15 01:48:09, hugoh_UTC9 wrote: > https://codereview.chromium.org/2748483002/diff/40001/third_party/WebKit/Sour... > File third_party/WebKit/Source/core/input/EventHandlerTest.cpp (right): > > https://codereview.chromium.org/2748483002/diff/40001/third_party/WebKit/Sour... > third_party/WebKit/Source/core/input/EventHandlerTest.cpp:373: "<div > style='height: 1000px; width: 1000px;'></div>"); > I think we should fix that tapping on <body> gives the same test result as > tapping on a <div>. Maybe that would be a rather big change, so we don't need to > do it right now. > > @yosin, do you know the reason why tapping <body> gives > document().focusedElement() == null (and not HTMLBodyElement)? Feels like Blink > doesn't "see" <body> in hit tests. > > bool FocusController::setFocusedElement(Element* element, > Frame* newFocusedFrame, > const FocusParams& params) { > + LOG(ERROR) << "setFocusedElement: " << element; // null for <body>. @amaralp, I verified this CL on top of https://codereview.chromium.org/2616623002/ PS 23. Test passes. lgtm :)
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2017/03/15 02:30:53, hugoh_UTC9 wrote: > On 2017/03/15 01:48:09, hugoh_UTC9 wrote: > > > https://codereview.chromium.org/2748483002/diff/40001/third_party/WebKit/Sour... > > File third_party/WebKit/Source/core/input/EventHandlerTest.cpp (right): > > > > > https://codereview.chromium.org/2748483002/diff/40001/third_party/WebKit/Sour... > > third_party/WebKit/Source/core/input/EventHandlerTest.cpp:373: "<div > > style='height: 1000px; width: 1000px;'></div>"); > > I think we should fix that tapping on <body> gives the same test result as > > tapping on a <div>. Maybe that would be a rather big change, so we don't need > to > > do it right now. > > > > @yosin, do you know the reason why tapping <body> gives > > document().focusedElement() == null (and not HTMLBodyElement)? Feels like > Blink > > doesn't "see" <body> in hit tests. > > > > bool FocusController::setFocusedElement(Element* element, > > Frame* newFocusedFrame, > > const FocusParams& params) { > > + LOG(ERROR) << "setFocusedElement: " << element; // null for <body>. > > @amaralp, I verified this CL on top of > https://codereview.chromium.org/2616623002/ PS 23. Test passes. lgtm :) @amaralp, do you wanna land this? BUG=678601 could be used so this CL does not hang loose.
amaralp@chromium.org changed reviewers: + bokan@chromium.org
On 2017/03/16 at 01:21:04, hugoh wrote: > On 2017/03/15 02:30:53, hugoh_UTC9 wrote: > > On 2017/03/15 01:48:09, hugoh_UTC9 wrote: > > > > > https://codereview.chromium.org/2748483002/diff/40001/third_party/WebKit/Sour... > > > File third_party/WebKit/Source/core/input/EventHandlerTest.cpp (right): > > > > > > > > https://codereview.chromium.org/2748483002/diff/40001/third_party/WebKit/Sour... > > > third_party/WebKit/Source/core/input/EventHandlerTest.cpp:373: "<div > > > style='height: 1000px; width: 1000px;'></div>"); > > > I think we should fix that tapping on <body> gives the same test result as > > > tapping on a <div>. Maybe that would be a rather big change, so we don't need > > to > > > do it right now. > > > > > > @yosin, do you know the reason why tapping <body> gives > > > document().focusedElement() == null (and not HTMLBodyElement)? Feels like > > Blink > > > doesn't "see" <body> in hit tests. > > > > > > bool FocusController::setFocusedElement(Element* element, > > > Frame* newFocusedFrame, > > > const FocusParams& params) { > > > + LOG(ERROR) << "setFocusedElement: " << element; // null for <body>. > > > > @amaralp, I verified this CL on top of > > https://codereview.chromium.org/2616623002/ PS 23. Test passes. lgtm :) > > @amaralp, do you wanna land this? BUG=678601 could be used so this CL does not hang loose. Added bokan@ for core/input OWNERS.
lgtm
The CQ bit was checked by amaralp@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 60001, "attempt_start_ts": 1489683006446240,
"parent_rev": "f113bf9dba5b08756a2a6140f73a2cb0bb8e0435", "commit_rev":
"7f75cce9905c0fe40761f247a47bfc66ef2f3321"}
Message was sent while issue was closed.
Description was changed from ========== Making sure ClearHandleAfterTap's tap occurs inside of HTML document The last tap in ClearHandleAfterTap was causing the HitTestResult's innerNode to be null. This change makes it so that the tap occurs on an HTML element thus making the innerNode not be null. This is change was necessary to for crrev.com/2616623002. ========== to ========== Making sure ClearHandleAfterTap's tap occurs inside of HTML document The last tap in ClearHandleAfterTap was causing the HitTestResult's innerNode to be null. This change makes it so that the tap occurs on an HTML element thus making the innerNode not be null. This is change was necessary to for crrev.com/2616623002. Review-Url: https://codereview.chromium.org/2748483002 Cr-Commit-Position: refs/heads/master@{#457488} Committed: https://chromium.googlesource.com/chromium/src/+/7f75cce9905c0fe40761f247a47b... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001) as https://chromium.googlesource.com/chromium/src/+/7f75cce9905c0fe40761f247a47b... |
