Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2812613004:
Fire a click event even when a clicked text node is removed in mouseup (Closed)
DescriptionFire a click event even when a clicked text node is removed in mouseup
This is a tentative fix for bug 708394.
See also https://codereview.chromium.org/2807123002/ for details, which is trying
to fix the root cause. Since fixing the root cause might change the other
behaviors as well as fixing bug 708394, let me land the tentative fix to prevent
a regression in M58.
Blink does not fire a click event if a user removes a clicked text node in mouseup
event listener because of the followings:
1. User presses a mouse button
2. UA did a hittest. The result's innerNode() is a text node.
3. MouseEventManager#click_node_ is set to the parent element of the clicked
text node
4. Dispatch mouse press events (or anything)
5. User releases a mouse button
6. UA did another hittest because UA would dispatch a click event at the common
ancestor of:
- A location of mouse pressed; MouseEventManager#click_node_ is used in this case.
- A location of mouse released; The 2nd hit test's result is used in this case.
7. Users removed a text node, which is the result of 2nd hit test.
8. Since there is no common ancestor of MouseEventManager#click_node_ and the
2nd hit test's result, a click event is not dispatched.
This CL keeps the parent element, if necessary, of 2nd hit test's result before
dispatching a mouseup event, and use it to decide whether to dispatch a click
event or not.
BUG=708394
Review-Url: https://codereview.chromium.org/2812613004
Cr-Commit-Position: refs/heads/master@{#463950}
Committed: https://chromium.googlesource.com/chromium/src/+/a971919aa2691e65eaf9a085b949ed6effc143a3
Patch Set 1 #
Total comments: 10
Messages
Total messages: 25 (15 generated)
The CQ bit was checked by hayato@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...
Description was changed from ========== wip BUG= ========== to ========== Fire a click event even when a clicked text node is removed in mouseup This is a tentative fix for bug 708394. See also https://codereview.chromium.org/2807123002/ for details, which is trying to fix the root cause. Since fixing the root cause might change the other behaviors as well as fixing bug 708394, let me land the tentative fix to prevent a regression in M58. Blink does not fire a click event if a user removes a clicked text node in mouseup event listener. The followings explains why this happens: 1. User presses a mouse button 2. UA did a hittest. The result's innerNode() is a text node. 3. MouseEventManager#click_node_ is set to the parent element of the clicked text node 4. Dispatch mouse press events (or anything) 5. User releases a mouse button 6. UA did another hittest because UA would dispatch a click event at the commo n ancestor of: - A location of mouse pressed; MouseEventManager#click_node_ is used in this case. - and A location of mouse released; The 2nd hit test's result is used in this case. 7. Users removed a text node, which is the result of 2nd hit test. 8. Since there is no common ancestor of MouseEventManager#click_node_ and the 2nd hit test's result, a click event is not dispatched. BUG=708394 ==========
Description was changed from ========== Fire a click event even when a clicked text node is removed in mouseup This is a tentative fix for bug 708394. See also https://codereview.chromium.org/2807123002/ for details, which is trying to fix the root cause. Since fixing the root cause might change the other behaviors as well as fixing bug 708394, let me land the tentative fix to prevent a regression in M58. Blink does not fire a click event if a user removes a clicked text node in mouseup event listener. The followings explains why this happens: 1. User presses a mouse button 2. UA did a hittest. The result's innerNode() is a text node. 3. MouseEventManager#click_node_ is set to the parent element of the clicked text node 4. Dispatch mouse press events (or anything) 5. User releases a mouse button 6. UA did another hittest because UA would dispatch a click event at the commo n ancestor of: - A location of mouse pressed; MouseEventManager#click_node_ is used in this case. - and A location of mouse released; The 2nd hit test's result is used in this case. 7. Users removed a text node, which is the result of 2nd hit test. 8. Since there is no common ancestor of MouseEventManager#click_node_ and the 2nd hit test's result, a click event is not dispatched. BUG=708394 ========== to ========== Fire a click event even when a clicked text node is removed in mouseup This is a tentative fix for bug 708394. See also https://codereview.chromium.org/2807123002/ for details, which is trying to fix the root cause. Since fixing the root cause might change the other behaviors as well as fixing bug 708394, let me land the tentative fix to prevent a regression in M58. Blink does not fire a click event if a user removes a clicked text node in mouseup event listener because of the followings: 1. User presses a mouse button 2. UA did a hittest. The result's innerNode() is a text node. 3. MouseEventManager#click_node_ is set to the parent element of the clicked text node 4. Dispatch mouse press events (or anything) 5. User releases a mouse button 6. UA did another hittest because UA would dispatch a click event at the commo n ancestor of: - A location of mouse pressed; MouseEventManager#click_node_ is used in this case. - and A location of mouse released; The 2nd hit test's result is used in this case. 7. Users removed a text node, which is the result of 2nd hit test. 8. Since there is no common ancestor of MouseEventManager#click_node_ and the 2nd hit test's result, a click event is not dispatched. This CL preserves the parent element, if necessary, of a clicked text before dispatching a mouse up, and use it to decide whether to dispatch a click event or not. BUG=708394 ==========
Description was changed from ========== Fire a click event even when a clicked text node is removed in mouseup This is a tentative fix for bug 708394. See also https://codereview.chromium.org/2807123002/ for details, which is trying to fix the root cause. Since fixing the root cause might change the other behaviors as well as fixing bug 708394, let me land the tentative fix to prevent a regression in M58. Blink does not fire a click event if a user removes a clicked text node in mouseup event listener because of the followings: 1. User presses a mouse button 2. UA did a hittest. The result's innerNode() is a text node. 3. MouseEventManager#click_node_ is set to the parent element of the clicked text node 4. Dispatch mouse press events (or anything) 5. User releases a mouse button 6. UA did another hittest because UA would dispatch a click event at the commo n ancestor of: - A location of mouse pressed; MouseEventManager#click_node_ is used in this case. - and A location of mouse released; The 2nd hit test's result is used in this case. 7. Users removed a text node, which is the result of 2nd hit test. 8. Since there is no common ancestor of MouseEventManager#click_node_ and the 2nd hit test's result, a click event is not dispatched. This CL preserves the parent element, if necessary, of a clicked text before dispatching a mouse up, and use it to decide whether to dispatch a click event or not. BUG=708394 ========== to ========== Fire a click event even when a clicked text node is removed in mouseup This is a tentative fix for bug 708394. See also https://codereview.chromium.org/2807123002/ for details, which is trying to fix the root cause. Since fixing the root cause might change the other behaviors as well as fixing bug 708394, let me land the tentative fix to prevent a regression in M58. Blink does not fire a click event if a user removes a clicked text node in mouseup event listener because of the followings: 1. User presses a mouse button 2. UA did a hittest. The result's innerNode() is a text node. 3. MouseEventManager#click_node_ is set to the parent element of the clicked text node 4. Dispatch mouse press events (or anything) 5. User releases a mouse button 6. UA did another hittest because UA would dispatch a click event at the commo n ancestor of: - A location of mouse pressed; MouseEventManager#click_node_ is used in this case. - and A location of mouse released; The 2nd hit test's result is used in this case. 7. Users removed a text node, which is the result of 2nd hit test. 8. Since there is no common ancestor of MouseEventManager#click_node_ and the 2nd hit test's result, a click event is not dispatched. This CL keeps the parent element, if necessary, of a clicked text before dispatching a mouseup event, and use it to decide whether to dispatch a click event or not. BUG=708394 ==========
Description was changed from ========== Fire a click event even when a clicked text node is removed in mouseup This is a tentative fix for bug 708394. See also https://codereview.chromium.org/2807123002/ for details, which is trying to fix the root cause. Since fixing the root cause might change the other behaviors as well as fixing bug 708394, let me land the tentative fix to prevent a regression in M58. Blink does not fire a click event if a user removes a clicked text node in mouseup event listener because of the followings: 1. User presses a mouse button 2. UA did a hittest. The result's innerNode() is a text node. 3. MouseEventManager#click_node_ is set to the parent element of the clicked text node 4. Dispatch mouse press events (or anything) 5. User releases a mouse button 6. UA did another hittest because UA would dispatch a click event at the commo n ancestor of: - A location of mouse pressed; MouseEventManager#click_node_ is used in this case. - and A location of mouse released; The 2nd hit test's result is used in this case. 7. Users removed a text node, which is the result of 2nd hit test. 8. Since there is no common ancestor of MouseEventManager#click_node_ and the 2nd hit test's result, a click event is not dispatched. This CL keeps the parent element, if necessary, of a clicked text before dispatching a mouseup event, and use it to decide whether to dispatch a click event or not. BUG=708394 ========== to ========== Fire a click event even when a clicked text node is removed in mouseup This is a tentative fix for bug 708394. See also https://codereview.chromium.org/2807123002/ for details, which is trying to fix the root cause. Since fixing the root cause might change the other behaviors as well as fixing bug 708394, let me land the tentative fix to prevent a regression in M58. Blink does not fire a click event if a user removes a clicked text node in mouseup event listener because of the followings: 1. User presses a mouse button 2. UA did a hittest. The result's innerNode() is a text node. 3. MouseEventManager#click_node_ is set to the parent element of the clicked text node 4. Dispatch mouse press events (or anything) 5. User releases a mouse button 6. UA did another hittest because UA would dispatch a click event at the common ancestor of: - A location of mouse pressed; MouseEventManager#click_node_ is used in this case. - A location of mouse released; The 2nd hit test's result is used in this case. 7. Users removed a text node, which is the result of 2nd hit test. 8. Since there is no common ancestor of MouseEventManager#click_node_ and the 2nd hit test's result, a click event is not dispatched. This CL keeps the parent element, if necessary, of a clicked text before dispatching a mouseup event, and use it to decide whether to dispatch a click event or not. BUG=708394 ==========
Description was changed from ========== Fire a click event even when a clicked text node is removed in mouseup This is a tentative fix for bug 708394. See also https://codereview.chromium.org/2807123002/ for details, which is trying to fix the root cause. Since fixing the root cause might change the other behaviors as well as fixing bug 708394, let me land the tentative fix to prevent a regression in M58. Blink does not fire a click event if a user removes a clicked text node in mouseup event listener because of the followings: 1. User presses a mouse button 2. UA did a hittest. The result's innerNode() is a text node. 3. MouseEventManager#click_node_ is set to the parent element of the clicked text node 4. Dispatch mouse press events (or anything) 5. User releases a mouse button 6. UA did another hittest because UA would dispatch a click event at the common ancestor of: - A location of mouse pressed; MouseEventManager#click_node_ is used in this case. - A location of mouse released; The 2nd hit test's result is used in this case. 7. Users removed a text node, which is the result of 2nd hit test. 8. Since there is no common ancestor of MouseEventManager#click_node_ and the 2nd hit test's result, a click event is not dispatched. This CL keeps the parent element, if necessary, of a clicked text before dispatching a mouseup event, and use it to decide whether to dispatch a click event or not. BUG=708394 ========== to ========== Fire a click event even when a clicked text node is removed in mouseup This is a tentative fix for bug 708394. See also https://codereview.chromium.org/2807123002/ for details, which is trying to fix the root cause. Since fixing the root cause might change the other behaviors as well as fixing bug 708394, let me land the tentative fix to prevent a regression in M58. Blink does not fire a click event if a user removes a clicked text node in mouseup event listener because of the followings: 1. User presses a mouse button 2. UA did a hittest. The result's innerNode() is a text node. 3. MouseEventManager#click_node_ is set to the parent element of the clicked text node 4. Dispatch mouse press events (or anything) 5. User releases a mouse button 6. UA did another hittest because UA would dispatch a click event at the common ancestor of: - A location of mouse pressed; MouseEventManager#click_node_ is used in this case. - A location of mouse released; The 2nd hit test's result is used in this case. 7. Users removed a text node, which is the result of 2nd hit test. 8. Since there is no common ancestor of MouseEventManager#click_node_ and the 2nd hit test's result, a click event is not dispatched. This CL keeps the parent element, if necessary, of a clicked text before dispatching a mouseup event, and use it to decide whether to dispatch a click event or not. BUG=708394 ==========
Description was changed from ========== Fire a click event even when a clicked text node is removed in mouseup This is a tentative fix for bug 708394. See also https://codereview.chromium.org/2807123002/ for details, which is trying to fix the root cause. Since fixing the root cause might change the other behaviors as well as fixing bug 708394, let me land the tentative fix to prevent a regression in M58. Blink does not fire a click event if a user removes a clicked text node in mouseup event listener because of the followings: 1. User presses a mouse button 2. UA did a hittest. The result's innerNode() is a text node. 3. MouseEventManager#click_node_ is set to the parent element of the clicked text node 4. Dispatch mouse press events (or anything) 5. User releases a mouse button 6. UA did another hittest because UA would dispatch a click event at the common ancestor of: - A location of mouse pressed; MouseEventManager#click_node_ is used in this case. - A location of mouse released; The 2nd hit test's result is used in this case. 7. Users removed a text node, which is the result of 2nd hit test. 8. Since there is no common ancestor of MouseEventManager#click_node_ and the 2nd hit test's result, a click event is not dispatched. This CL keeps the parent element, if necessary, of a clicked text before dispatching a mouseup event, and use it to decide whether to dispatch a click event or not. BUG=708394 ========== to ========== Fire a click event even when a clicked text node is removed in mouseup This is a tentative fix for bug 708394. See also https://codereview.chromium.org/2807123002/ for details, which is trying to fix the root cause. Since fixing the root cause might change the other behaviors as well as fixing bug 708394, let me land the tentative fix to prevent a regression in M58. Blink does not fire a click event if a user removes a clicked text node in mouseup event listener because of the followings: 1. User presses a mouse button 2. UA did a hittest. The result's innerNode() is a text node. 3. MouseEventManager#click_node_ is set to the parent element of the clicked text node 4. Dispatch mouse press events (or anything) 5. User releases a mouse button 6. UA did another hittest because UA would dispatch a click event at the common ancestor of: - A location of mouse pressed; MouseEventManager#click_node_ is used in this case. - A location of mouse released; The 2nd hit test's result is used in this case. 7. Users removed a text node, which is the result of 2nd hit test. 8. Since there is no common ancestor of MouseEventManager#click_node_ and the 2nd hit test's result, a click event is not dispatched. This CL keeps the parent element, if necessary, of 2nd hit test's result before dispatching a mouseup event, and use it to decide whether to dispatch a click event or not. BUG=708394 ==========
hayato@chromium.org changed reviewers: + nzolghadr@chromium.org, tkent@chromium.org
PTAL
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
hayato@chromium.org changed reviewers: + kochi@chromium.org
kochi@, could you take a look? tkent@ looks OOO today.
https://codereview.chromium.org/2812613004/diff/1/third_party/WebKit/LayoutTe... File third_party/WebKit/LayoutTests/fast/events/remove-text-node-in-mouseup.html (right): https://codereview.chromium.org/2812613004/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/fast/events/remove-text-node-in-mouseup.html:4: <div id="test"></div> It is better to use w3c test handler to avoid having expected.txt https://codereview.chromium.org/2812613004/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/input/EventHandler.cpp (right): https://codereview.chromium.org/2812613004/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/input/EventHandler.cpp:964: ? FlatTreeTraversal::Parent(*mev.InnerNode()) Do we need to use flat tree parent? Can we distribute Text node directly into shadow tree?
https://codereview.chromium.org/2812613004/diff/1/third_party/WebKit/LayoutTe... File third_party/WebKit/LayoutTests/fast/events/remove-text-node-in-mouseup.html (right): https://codereview.chromium.org/2812613004/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/fast/events/remove-text-node-in-mouseup.html:4: <div id="test"></div> We can't because the test needs eventSender. https://codereview.chromium.org/2812613004/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/input/EventHandler.cpp (right): https://codereview.chromium.org/2812613004/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/input/EventHandler.cpp:964: ? FlatTreeTraversal::Parent(*mev.InnerNode()) Yes. Text node can be distributed.
lgtm https://codereview.chromium.org/2812613004/diff/1/third_party/WebKit/LayoutTe... File third_party/WebKit/LayoutTests/fast/events/remove-text-node-in-mouseup.html (right): https://codereview.chromium.org/2812613004/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/fast/events/remove-text-node-in-mouseup.html:4: <div id="test"></div> On 2017/04/12 06:16:06, hayato wrote: > We can't because the test needs eventSender. > I think it's okay to use testharness.js for tests that cannot be upstreamed to WPT. https://codereview.chromium.org/2812613004/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/input/EventHandler.cpp (right): https://codereview.chromium.org/2812613004/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/input/EventHandler.cpp:963: Node* release_node = (mev.InnerNode() && mev.InnerNode()->IsTextNode()) How about adding a TODO comment on this?
The CQ bit was checked by hayato@chromium.org
https://codereview.chromium.org/2812613004/diff/1/third_party/WebKit/LayoutTe... File third_party/WebKit/LayoutTests/fast/events/remove-text-node-in-mouseup.html (right): https://codereview.chromium.org/2812613004/diff/1/third_party/WebKit/LayoutTe... third_party/WebKit/LayoutTests/fast/events/remove-text-node-in-mouseup.html:4: <div id="test"></div> It would be, but I would prefer consistency with other tests in this directory. https://codereview.chromium.org/2812613004/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/input/EventHandler.cpp (right): https://codereview.chromium.org/2812613004/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/input/EventHandler.cpp:963: Node* release_node = (mev.InnerNode() && mev.InnerNode()->IsTextNode()) What kind of TODO comment?
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
still lgtm https://codereview.chromium.org/2812613004/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/input/EventHandler.cpp (right): https://codereview.chromium.org/2812613004/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/input/EventHandler.cpp:963: Node* release_node = (mev.InnerNode() && mev.InnerNode()->IsTextNode()) On 2017/04/12 06:52:22, hayato wrote: > What kind of TODO comment? If this is a tentative solution to the problem, have a pointer to crbug and why. Probably this will be overridden by the original CL https://codereview.chromium.org/2807123002/ soon, so I do not bother you for adding comment here much.
https://codereview.chromium.org/2812613004/diff/1/third_party/WebKit/Source/c... File third_party/WebKit/Source/core/input/EventHandler.cpp (right): https://codereview.chromium.org/2812613004/diff/1/third_party/WebKit/Source/c... third_party/WebKit/Source/core/input/EventHandler.cpp:963: Node* release_node = (mev.InnerNode() && mev.InnerNode()->IsTextNode()) On 2017/04/12 at 07:03:19, kochi wrote: > On 2017/04/12 06:52:22, hayato wrote: > > What kind of TODO comment? > > If this is a tentative solution to the problem, have a pointer > to crbug and why. Probably this will be overridden by the > original CL https://codereview.chromium.org/2807123002/ soon, > so I do not bother you for adding comment here much. That kind of TODO comment should be a file level TODO comment. That should not be added on this particular line. That is rather confusing, I think.
CQ is committing da patch.
Bot data: {"patchset_id": 1, "attempt_start_ts": 1491979936686210, "parent_rev":
"d4ac95b13ea78688e1a8dfd6dbceac097e7fc968", "commit_rev":
"a971919aa2691e65eaf9a085b949ed6effc143a3"}
Message was sent while issue was closed.
Description was changed from ========== Fire a click event even when a clicked text node is removed in mouseup This is a tentative fix for bug 708394. See also https://codereview.chromium.org/2807123002/ for details, which is trying to fix the root cause. Since fixing the root cause might change the other behaviors as well as fixing bug 708394, let me land the tentative fix to prevent a regression in M58. Blink does not fire a click event if a user removes a clicked text node in mouseup event listener because of the followings: 1. User presses a mouse button 2. UA did a hittest. The result's innerNode() is a text node. 3. MouseEventManager#click_node_ is set to the parent element of the clicked text node 4. Dispatch mouse press events (or anything) 5. User releases a mouse button 6. UA did another hittest because UA would dispatch a click event at the common ancestor of: - A location of mouse pressed; MouseEventManager#click_node_ is used in this case. - A location of mouse released; The 2nd hit test's result is used in this case. 7. Users removed a text node, which is the result of 2nd hit test. 8. Since there is no common ancestor of MouseEventManager#click_node_ and the 2nd hit test's result, a click event is not dispatched. This CL keeps the parent element, if necessary, of 2nd hit test's result before dispatching a mouseup event, and use it to decide whether to dispatch a click event or not. BUG=708394 ========== to ========== Fire a click event even when a clicked text node is removed in mouseup This is a tentative fix for bug 708394. See also https://codereview.chromium.org/2807123002/ for details, which is trying to fix the root cause. Since fixing the root cause might change the other behaviors as well as fixing bug 708394, let me land the tentative fix to prevent a regression in M58. Blink does not fire a click event if a user removes a clicked text node in mouseup event listener because of the followings: 1. User presses a mouse button 2. UA did a hittest. The result's innerNode() is a text node. 3. MouseEventManager#click_node_ is set to the parent element of the clicked text node 4. Dispatch mouse press events (or anything) 5. User releases a mouse button 6. UA did another hittest because UA would dispatch a click event at the common ancestor of: - A location of mouse pressed; MouseEventManager#click_node_ is used in this case. - A location of mouse released; The 2nd hit test's result is used in this case. 7. Users removed a text node, which is the result of 2nd hit test. 8. Since there is no common ancestor of MouseEventManager#click_node_ and the 2nd hit test's result, a click event is not dispatched. This CL keeps the parent element, if necessary, of 2nd hit test's result before dispatching a mouseup event, and use it to decide whether to dispatch a click event or not. BUG=708394 Review-Url: https://codereview.chromium.org/2812613004 Cr-Commit-Position: refs/heads/master@{#463950} Committed: https://chromium.googlesource.com/chromium/src/+/a971919aa2691e65eaf9a085b949... ==========
Message was sent while issue was closed.
Committed patchset #1 (id:1) as https://chromium.googlesource.com/chromium/src/+/a971919aa2691e65eaf9a085b949... |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
