New issue
Advanced search Search tips

Issue 822312 link

Starred by 2 users

Issue metadata

Status: Fixed
Owner:
Closed: Mar 2018
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Linux , Windows , Mac
Pri: 1
Type: Bug-Regression



Sign in to add a comment

Delay in reaching Mouseup event

Reported by hltat...@gmail.com, Mar 15 2018

Issue description

UserAgent: 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:
 
5672-1.svg
9.8 MB Download
chrome_issue_readmen.txt
1.8 KB View Download
Labels: Needs-Bisect Needs-Triage-M65
Labels: Triaged-ET Needs-Feedback
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!
822312.mp4
2.8 MB View Download

Comment 4 by hltat...@gmail.com, Mar 16 2018

Version65_0_3325_62.mp4
871 KB View Download
Version63_0_3239_108.mp4
750 KB View Download

Comment 5 by hltat...@gmail.com, Mar 16 2018

There are no delays in placing the text element in FireFox or Edge or older builds of Chrome.
Project Member

Comment 6 by sheriffbot@chromium.org, Mar 16 2018

Cc: viswa.karala@chromium.org
Labels: -Needs-Feedback
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
Components: -Blink Blink>Editing Blink>Layout
Labels: -Pri-2 -Needs-Bisect hasbisect-per-revision Target-67 RegressedIn-64 Target-66 M-67 FoundIn-66 FoundIn-67 Target-65 FoundIn-65 OS-Linux OS-Mac Pri-1
Owner: xiaoche...@chromium.org
Status: Assigned (was: Unconfirmed)
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!
Components: -Blink>Layout
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().
Cc: yoichio@chromium.org yosin@chromium.org
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?

Comment 10 by yosin@chromium.org, 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.

#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.
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?

Comment 13 by yosin@chromium.org, Mar 23 2018

Cc: -yoichio@chromium.org -yosin@chromium.org
Components: -Blink>Editing Blink>Editing>Selection
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|.











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.
Project Member

Comment 15 by bugdroid1@chromium.org, 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

Fixed in M67.

Not merging it back as it's there since M63.
Status: Fixed (was: Assigned)
Labels: TE-Verified-M67 TE-Verified-67.0.3381.0
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!
822312.ogv
2.2 MB View Download

Comment 19 by hltat...@gmail.com, 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?
#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