Delay in reaching Mouseup event
Reported by
hltat...@gmail.com,
Mar 15 2018
|
|||||||||
Issue descriptionUserAgent: Mozilla/5.0 (Windows NT 10.0; Win64; x64) AppleWebKit/537.36 (KHTML, like Gecko) Chrome/63.0.3239.108 Safari/537.36 Steps to reproduce the problem: 1. Access website http://cwsdemo.hexagonusfederal.com/cwe 2. Click Open 3. Click Choose File 4. Browse to 5672-1.svg (attached) 5. Click open 6. Click Insert Item 7. Click Text 8. Click in map 9. "New Text" should be placed in the map What is the expected behavior? Immediate placement of default text. What went wrong? Depending on the size of the SVG file it takes seconds to minutes to place the text in the map with the last two official builds of Chrome. Chrome version 63.0.3239.108 does not have a delay in reaching the mouse up event. Did this work before? Yes 63.0.3239.108 Chrome version: 65.0.3325.162 Channel: stable OS Version: 10.0 Flash Version:
,
Mar 16 2018
Tested the issue on chrome reported version 65.0.3325.162 using Windows 10 with steps mentioned below: 1) Launched chrome reported version and downloaded the 5672-1.svg file provided in comment#0 2) Navigated to URL: http://cwsdemo.hexagonusfederal.com/cwe (as provided in comment#0), clicked on "Open" > "Choose file" > Browsed the file downloaded in step-1 > Clicked on Insert item > Clicked on Text and clicked on "click in map" 3) Able to see the map in the below field after clicking on "click in map" Observation: Tested the issue on "63.0.3239.108", even seen the same behaviour there. @Reporter: Please find the attached screen cast for your reference and let us know if we missed anything in reproducing the issue, provide your feedback on it which help in triaging it in better way. If possible could you please provide the screen cast of the excepted behaviour which helps us in better understanding. Thanks!
,
Mar 16 2018
,
Mar 16 2018
,
Mar 16 2018
There are no delays in placing the text element in FireFox or Edge or older builds of Chrome.
,
Mar 16 2018
Thank you for providing more feedback. Adding the requester to the cc list. For more details visit https://www.chromium.org/issue-tracking/autotriage - Your friendly Sheriffbot
,
Mar 19 2018
Able to reproduce the issue on reported chrome version 65.0.3325.162 using Windows-10, Mac 10.12.6 & Ubuntu 14.04 hence providing Bisect Info Bisect Info: ================ Good build: 64.0.3241.0 Bad build: 64.0.3242.0 You are probably looking for a change made after 508987 (known good), but no later than 508988 (first known bad). https://chromium.googlesource.com/chromium/src/+log/dcfd0545c42eb980b1769a1aa769b6708b3b53e3..0289e4ac1388e042be0f2258332ffbb6a01ffa2a Reviewed-on: https://chromium-review.googlesource.com/719355 @Xiaocheng Hu: Please confirm the issue and help in re-assigning if it is not related to your change. Thanks!
,
Mar 20 2018
When clicking inside the svg, MostForwardCaretPosition() makes 10k+ calls of EndsOfNodeAreVisuallyDistinctPositions() on the same SVG root. The problem got revealed by my patch, which changed EndsOfNodeAreVisuallyDistinctPositions() to return more consistent result, at the cost of being slower. Still, we shouldn't make so many identical calls in MostForwardCaretPosition().
,
Mar 22 2018
It seems that the best solution is to make PositionIterator remember the value of EndsOfNodeAreVisuallyDistinctPositions() for the ancestor nodes of |anchor_node_|, so that we don't need to call this function repeatedly. yosin, yoichio: what do you think about this idea?
,
Mar 22 2018
Do we call |EndsOfNodeAreVisuallyDistinctPositions()| for same node other than Text node? It seems |EndsOfNodeAreVisuallyDistinctPositions()| is called N times for Text node which length is N, but once only for non-Text node.
,
Mar 22 2018
#10: In the repro test case, we call |EndsOfNodeAreVisuallyDistinctPositions()| for 10k+ times, resulting in a delay of a few seconds. It seems that the issue is especially bad on nodes with very large degrees. Every time PositionIterator traverses from a child node back to such a large-degree parent, we may call |EndsOfNodeAreVisuallyDistinctPositions()| on it. For the repro test case, it contains an <svg> with 35661 child nodes.
,
Mar 23 2018
If there are 35k nodes, cashing algorithm doesn't work, I guess. I think slowness of HasRenderedNonAnonymousDescendantsWithHeight to IsVisuallyEmpty comes from computing bounding box. Could you investigate if we can early return w/o computing bounding box?
,
Mar 23 2018
Thanks for explanation. I think PositionIterator::GetNode() returns a node of
current position instead of anchor/container node.
BTW, I think we should rename GetNode() to GetContainerNode() and OffsetInLeafNode() to OffsetInContainerNode().
Options to solve this issue:
1. Cache EndsOfNodeAreVisuallyDistinctPositions() for current container node
2. Cache EndsOfNodeAreVisuallyDistinctPositions() for inclusive ancestors of current container node
3. Compute EndsOfNodeAreVisuallyDistinctPositions() when updating |last_node|
in caller. Identical to #1 but in caller.
4. Change PositionIterator::GetNode() to return a node at current position instead of container node
I recommend option 3 because we will have other performance sensitive functions to call with |current_node|.
Note: We may want to consider following if-statement:
if (EditingIgnoresContent(*current_node) ||
IsDisplayInsideTable(current_node)) {
if (current_pos.AtEndOfNode())
return PositionTemplate<Strategy>::AfterNode(*current_node);
continue;
}
Both EditingIgnoresContent() and IsDisplayInsideTable() are inexpensive. But it is worth to move it at updating
|last_node|.
,
Mar 23 2018
Discussed offline with yosin@. We can simply skip non-text elements when traversing in SVG. Although the issue might happen as long as there is a node with a lot of children without caret positions, it seems natural only for SVG elements. Hence, we just need to add a fast path to SVG.
,
Mar 23 2018
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/8e793a18ec6358414ffc337b336c93d4f6061c91 commit 8e793a18ec6358414ffc337b336c93d4f6061c91 Author: Xiaocheng Hu <xiaochengh@chromium.org> Date: Fri Mar 23 09:01:52 2018 Skip non-text svg elements in Most{For/Back}wardCaretPosition There is no caret position in non-text svg elements. Hence, this patch skips such elements in the two functions for performance optimization. Bug: 822312 Change-Id: I74321aa328315f6afb5372de53696bf87cef518b Reviewed-on: https://chromium-review.googlesource.com/977578 Commit-Queue: Xiaocheng Hu <xiaochengh@chromium.org> Commit-Queue: Yoshifumi Inoue <yosin@chromium.org> Reviewed-by: Yoshifumi Inoue <yosin@chromium.org> Cr-Commit-Position: refs/heads/master@{#545391} [modify] https://crrev.com/8e793a18ec6358414ffc337b336c93d4f6061c91/third_party/WebKit/Source/core/editing/VisibleUnits.cpp
,
Mar 26 2018
Fixed in M67. Not merging it back as it's there since M63.
,
Mar 26 2018
,
Mar 27 2018
Able to reproduce the issue on chrome reported version 65.0.3325.162 Verified the fix on Mac 10.12.6, Windows-10 & Ubuntu 14.04 on Chrome version #67.0.3381.0 as per the comment#0 Attaching screen cast for reference. Observed "Immediate placement of default text" Hence, the fix is working as expected. Adding the verified label. Thanks!
,
Mar 27 2018
Thanks so much for your quick response in resolving the issue. Will I be able to access this change on the Canary channel in the next few days?
,
Mar 28 2018
#19: Yes, it's already in Canary (67.0.3381.0 and later versions). Please also let us know if you spot any related regression with SVG. Thanks! |
|||||||||
►
Sign in to add a comment |
|||||||||
Comment 1 by krajshree@chromium.org
, Mar 16 2018